feat(telemetry): instrument session lifetimes, tool call latency, and subsystem events

[chenmingwei23]

Problem / Motivation

Three populations behind every KiroCrew turn are not measured at all today.

How long a session lives, and why it ended. kirocrew.session.startup.duration
times the cold start of the agent PROCESS and kirocrew.session.idle_expired
counts one specific teardown cause, but nothing records a session's LIFETIME or
how those lifetimes split across the ways a session can end. There is also no
count of sessions started, so no denominator for anything session-scoped.

How long a tool call takes. kirocrew.turn.duration reports the whole agent
loop, which is model calls plus every tool round-trip. A four-minute turn is
therefore indistinguishable between a slow model and one shell command that ran
for four minutes, and the tool population that dominates real latency cannot be
seen at all.

Whether the subsystems behind a turn did their work. Subagent spawns, cron
fires, artifact creates, workflow runs, context compactions, MCP stub reconnects
and tool-approval decisions each have logs and, in some cases, an audit record --
but no counter. Their rates are only recoverable by grepping logs after the fact.

Why it matters

Without session lifetimes, the single most interesting slice is invisible: a
session that ends because the gateway crashed. It runs no teardown path, so it
would contribute no sample even once the histogram exists, which means the
distribution would describe orderly shutdowns and quietly omit the failures.

Without tool-call latency, a latency regression can be localised only by
guessing. The turn histogram already exists and already says "something in this
turn was slow"; the missing half is which part.

Without the business counters, load and health questions have no numbers behind
them -- how often approvals are actually shown versus auto-approved, whether MCP
backends are churning under live sessions, how often compaction runs and how
often it recovers headroom. Each of those is a per-subsystem fact today, held
only in a log line.

What changed (motivation -> approach -> change)

Session lifetime, including the crashed population

Goal: one histogram whose population includes sessions that died with their
process, and whose end_reason distinguishes how a session ended.

Approach considered and rejected: keep start times in memory. That loses exactly
the crashed sessions. Also rejected: infer "unclean" from the session map, whose
entries deliberately outlive a session so a tab can resume later -- presence
there proves a session once existed, not that one exists now. Also rejected: the
transcript's own closed / closed_at stamp, which only the dashboard tab-close
path writes (so channel, cron, subagent and task-runner sessions never get one)
and whose ABSENCE cannot separate a crashed session from a live-idle one -- there
is no positive crash flag on disk today. Boot offers nothing to piggyback on
either: the restore path is seed-driven from open_slots.json and never walks the
transcript directory.

Chosen: a crumb on disk. Each start writes one small JSON file under
<data home>/metrics/open-sessions/; a clean end unlinks it and emits the
lifetime from an in-memory start time. Whatever is still there at the next boot
belongs to a session that never reached a teardown path, so
backfill_crashed_sessions(started_before) emits those as end_reason=crashed.
started_before is the calling process's own start time and crumbs at or after
it are left alone, which is what lets the scan run OFF the boot path as a tracked
worker-thread task instead of having to complete before this process opens its
first session.

The file is named after its writer and its generation, not just the session
key. A session key is not unique across processes: BACKGROUND_KEY is a fixed
constant, so a kirocrew run and the gateway can hold the same key at the same
time with a live session behind each. While the name was the key's digest alone
they shared one file, and whichever session ended first unlinked the other's
record -- so the survivor's later crash went unreported. The locks in this module
are thread locks and never spanned processes, so nothing here could have prevented
that. Putting the pid and the start time in the name makes a crumb removable only
by the session that wrote it, and three behaviours follow from it:

• a clean end unlinks the ONE generation it can name;
• an end with no live table entry unlinks NOTHING, because the files under that
  digest belong to other processes or earlier runs, and removing them is exactly
  the bug above;
• a start that displaces a live generation of the same key, and an eviction under
  the table cap, each reap the generation they orphan -- once the table has
  forgotten a generation, nothing can ever name its file again, and it would reach
  the next boot as a crash that never happened. Both ride the same awaited worker
  hop as the start's own write, so those unlinks stay off the event loop without
  adding a second cancellation window.

The backfill therefore finds several files per key legitimately, each a separate
session judged on its own recorded owner. It is also the only reader of these
files, so an unparseable crumb is reaped there rather than by an end unlinking
blind.

The crumb also records who owns it, because the cutoff alone is not enough.
cli.py and eval/runner.py each build their own session manager against the
same data home, so a kirocrew run session is writing crumbs there too. The
cutoff only ever protected THIS process's own crumbs, so a sibling's live session
started before the gateway booted was read as a crash -- inventing a sample AND
deleting the live crumb, which loses the real crash that session might later
suffer. Each crumb therefore carries the writing process's pid plus a per-process
start identifier, and the backfill
skips any crumb whose owner is still running. That check fails CLOSED: an owner
that cannot be decided counts as running, so an ambiguous crumb waits for a later
boot. Losing a real crash sample costs one data point; inventing one corrupts the
population this instrument exists to report.

The identifier is what makes pid reuse safe, including reuse of the current
process's own pid. A container hands the gateway PID 1 on every restart, so a
crashed predecessor's crumb arrives carrying this process's pid; trusting the pid
alone meant that crumb was skipped on this boot and on every boot after -- its
crash never emitted, its file never cleaned. The identifiers are compared whenever
both are readable, and an unreadable identity still counts as running.

A registration that never became a session is discarded, not ended. There is
one suspension point between inserting a session into the registry and finishing
its start record. Cancelled there, the caller hard-kills the provider while the
entry is still visible, so a claimant could be handed a dying session -- and the
crumb would outlive it into a false crash. Both allocation paths and the
background path now roll the exact inserted entry back and call
discard_session_start, which consumes the crumb WITHOUT emitting. It is
deliberately not a twelfth end_reason: those describe how a live session ENDED,
and a session that never lived has no lifetime belonging in the histogram.

Consuming the crumb is what makes the accounting exact, and that is load-bearing
rather than incidental: the six teardown paths are not mutually exclusive (the
idle sweep calls reset), so whichever reaches a session first records it and
the rest are no-ops; and a back-filled session cannot be counted again on the
boot after that.

A crashed session's END time comes from its transcript's mtime -- the last moment
it was observably alive, and the honest maximum available after the fact. A
session with no transcript on disk (a subagent leaves only a replay log) yields
no end time, so its crumb is consumed with no sample rather than recorded as a
plausible-looking zero.

end_reason labels the teardown PATH, not the cause. reset is the widest of
them: the idle sweep and a slot reset both reach teardown through it. That is
deliberate, because the finer causes behind it are already counted separately
(kirocrew.session.idle_expired, kirocrew.watchdog.recovery.outcome), and a
metric should not be the reason a lifecycle signature every surface calls grows a
parameter. Two paths that pop the registry WITHOUT going through reset report
their own reasons instead: an identity retirement reports retired, and
compaction replacing a provider in place reports recycled.

The start hook sits at the registry insertions, inside the registry lock, and the
crumb is written in that same critical section.

Why the write is awaited on a worker thread. Two constraints look opposed and
are not, and three earlier shapes each satisfied one and broke the other. A
fire-and-forget pool write kept the loop clear but let a writer land after its
session ended, or after a SUCCESSOR registered under the same key -- and every
attempt to detect that after the fact was itself racy, because every check still
acted on a path the successor SHARED, so a late predecessor deleted the
crumb its successor had just written. Writing inline fixed the ordering and put
filesystem I/O on the event loop, where a slow or network-homed data home stalls
every gateway task behind one session insertion -- the same reason SessionMap
offloads its own persist rather than writing inline. Naming the file after its
writer and generation is what finally removed that shared path, so the writer's
post-write check now removes only its own file.

Awaiting an asyncio.to_thread hop while the caller still holds the session
registry lock satisfies both: the syscalls leave the loop, and no start, end or
successor can interleave, because they all serialise on that lock. The writer
takes _live_lock itself, because the one toucher of these paths NOT serialised
by the registry lock is the crashed-session backfill.

The backfill holds that lock across read, cutoff decision and unlink, which
now serialises the scan against this process's own crumb writer rather than
guarding an identity race. The race it once guarded is gone: a path names one
writer and one generation, so a session registering mid-scan gets a file of its
own and the successor this could once have deleted no longer shares a name with
anything the scan touches.

The end path's unlink stays inline and synchronous. One syscall, deliberately
not moved off the loop: behind an await it reopens the race the same-tick rule
exists to prevent, where a successor registering during those awaits has its
record consumed by its predecessor's teardown. Deferring it was weighed again and
declined -- a deferred unlink has to be RELIABLE, since a dropped one leaves a
cleanly ended session's crumb for the next boot to call crashed, and reliability
means awaiting the hop, which makes record_session_ended a coroutine and gives
roughly ten teardown sites a new cancellation window each. That is filed as
#7537 with the reasoning, because the naming change above removes the ordering
hazard and narrows the residual to one case: a process killed between the registry
pop and the unlink leaves a single crumb the next boot counts as a crash.

The crumb is written only under telemetry consent, checked fail-closed. It
exists solely to feed kirocrew.session.duration, which is a no-op without
consent, so writing one on an unopted install would persist state nothing can
ever read -- against a documented default of collecting nothing. An earlier
revision of this PR instead relaxed that documented default to permit the write;
that was the wrong direction, and the default is restored intact.

Every registry removal must record an end

This is a correctness requirement, not a completeness one, and it is the part
worth reading twice. An unrecorded removal does not merely lose a sample: the
crumb survives it, so the next boot reports the session as crashed. It
manufactures a failure that never happened, in the one population this histogram
exists to measure.

Four removal paths shipped unrecorded and are fixed here:

• session_background.py::recycle_heartbeat -- pops the background session at
  cycle end. Now reports recycled.
• session_allocation.py::_evict_stale_session and the same dead-provider check
  inside get_or_create -- drop a registry entry whose provider is already gone.
  Now report a new evicted reason: nothing was torn down here, the process had
  already died, so the population answers "how often did a session die under us
  and get noticed on the next lookup" rather than "how often did we end one".
• session_lifecycle.py::drain_all_providers -- a mass pop that recorded
  nothing. Its one current caller drains an already-empty registry (it calls
  reload_provider_factory first, which clears and records), so this was a
  latent trap rather than a live leak; it now records per popped key so no future
  caller can reintroduce it.

The gate for this was the wrong shape and has been replaced. It enumerated six
method names in one module, so a seventh path -- or any pop in another module --
was invisible to it, which is why all four escaped. It is now a fail-closed AST
walk over every module that mutates the registry, recognising all three removal
spellings (pop, del, clear), so a NEW removal path fails by default.

Tool call latency

tool_kind is normalised against an allowlist and the tool NAME is never an
attribute. Two reasons, both structural: MCP tool names are unbounded, since any
server the user installs contributes its own; and the ACP kind field arrives
verbatim from the agent, which hooks.py already documents when it explains why
its auto-approve decision is an allow-list rather than a denylist. Anything
outside the allowlist becomes other, so an agent cannot mint series by
inventing kinds. An MCP-served call is labelled mcp whatever kind it claims --
the kind an MCP server reports is its own vocabulary, and "this call left the
process over MCP" is the more useful fact.

There is no single choke point, so this is instrumented in two layers,
because the two backends parse tool frames in different places. The kiro backend
runs on AcpRuntime + AcpSessionHandle, which parses through the shared
acp/_dispatch.py builders. The claude backend -- and the app worker pools that
construct a client directly, e.g. knowledge/llm_pool -- stays on
acp/client.py, which re-implements the same shaping inline and never calls
parse_session_update. providers/acp.py builds an AcpClient and then swaps in
an AcpSessionProvider at startup for the kiro path, so both parsers are live in
a normal install.

Every surface (dashboard, Slack, Discord, cron, subagents, task runner, workflow)
sits DOWNSTREAM of those two: they consume the emitted AcpEvent stream rather
than re-parsing frames. So instrumenting both parsers covers every surface and
nothing else needs a call site.

The layering is made safe by construction rather than by hoping the paths are
disjoint: start times live in ONE process-global registry keyed by toolCallId,
and a finish POPS its entry, so a call with no recorded start emits nothing. If a
frame is ever seen by both layers, the first finish records it and the second is a
no-op. A repeated start does not restart the clock, so the tool_call_update
refinements that follow a call cannot shrink the measured span.

Why not reuse the watchdog's existing dispatch clock.
acp/liveness.py::ToolCallState already stamps dispatch_ts, and
AcpSessionHandle clears it on the result. Not reused deliberately:
_inflight_tool is a SINGLE SLOT holding the most recent call -- the right shape
for stall attribution (the oracle only asks what we are waiting on now) and the
wrong shape for a histogram, since interleaved tool calls overwrite each other and
durations would land on the wrong call or go unrecorded. It also exists only on
the kiro path, so it could not serve the claude one.

The finish is stamped before the output parsing in both layers, because that
parsing returns None for an output-less update and a tool that completes with no
output is still a completed round-trip. A non-terminal status is a no-op, so a
mid-stream update leaves the clock running for the real completion.

The clock is perf_counter, not monotonic, and that was a real defect rather
than a preference. On Windows time.monotonic advances in ~15.6ms ticks, so any
call completing inside one tick measured exactly 0.0 and was dropped by the
skip-non-positive guard. That silently hid every sub-tick tool call on the
platform -- which is most cached reads -- and Windows CI caught it as 18 failures
in this PR's own tests. perf_counter is the highest-resolution monotonic clock
available everywhere, so the guard keeps meaning "unmeasurable" rather than
"fast".

Business counters

Seven counters at their own subsystem call sites, through the
metrics/events.py::emit_counter facade (which exists so low-level modules can
emit without importing metrics.provider at module top and forming a cycle).
Every attribute value is a member of a closed set:

• kirocrew.subagent.spawned -- at the admission increment, carrying the
  concurrency it was admitted at. That value is bounded by the spawn cap, so
  the aggregator's MAX over the attribute IS the concurrency high-water mark and
  no second instrument is needed. Imported at call time here because
  bind_component_globals rebinds every *_impl function's __globals__ to
  subagent's namespace, so a module-level import in that file is not visible
  from inside the function.
• kirocrew.cron.fires -- kind separates the three dispatch shapes (script
  and command bypass the model entirely, agent runs an LLM turn), which is
  the split between jobs that cost tokens and jobs that cost none.
• kirocrew.artifact.created -- after the write, so a failed create counts
  nothing; kind / source are the values the module's own validators already
  restrict to closed sets.
• kirocrew.workflow.runs -- at the audited run start, which both the foreground
  and background entry points cross.
• kirocrew.context.compactions -- at the single verdict funnel, placed above its
  early return so surfaces that register no callback are still counted. success
  reports whether the compaction ATTEMPT itself completed, NOT how much context
  was reclaimed: no before/after reading is taken, and a compaction can finish
  having freed little. It is also NOT the callback's success: the recycle path
  fires the funnel with success=True because the SESSION now has headroom, which
  is what the callback needs, but that path is reached exactly when an in-place
  /compact FAILED and the provider had to be replaced. Counting it as a
  successful compaction would report the failure mode as the success case, so the
  counter reports False there, discriminated by the recycling marker that is
  still set at that point.
• kirocrew.mcp.reconnects -- at the stub's reconnect success point.
• kirocrew.approval.decisions -- emitted by the four ToolHookResult result
  factories through one private helper, so it fires exactly once per gate
  consultation. Instrumenting HookManager.on_tool_call instead would mean
  touching each of its 23 exits. A surface that OVERRIDES the gate builds a
  result directly and is deliberately NOT counted: counting every construction
  would report one request as two decisions and keep a count for a verdict that
  was discarded. Counting from the factories is also what let the from_gate
  field go -- it had no reader but the counter, so it was state carried purely to
  signal. decision=allow is the branch that falls through TO an interactive
  prompt, so that slice is approvals shown and deny is approvals denied;
  security_deny separates a hard security refusal from a policy-state one. No
  reason string reaches the recorder. The existing backend-child permission pair
  is left exactly as it is -- it measures a specific hang-resilience fix, and its
  population is not this one's.

Bucket registration

Both new histograms are registered in provider._HISTOGRAM_BUCKETS_MS. Without
an entry a histogram silently falls back to OTEL's default 10s ceiling and its
derived percentile pins to that bound -- a ceiling artifact rendered as a real
latency. Session duration gets a new minutes-to-days family (1s to 7 days,
densest from a minute to a few hours), because it is the only instrument here
measured in hours and days. Tool calls get a sub-millisecond-to-an-hour family:
the fine end because cached reads dominate by count, the ceiling matching the
turn family because a tool call cannot outlive its turn.

Tests

test/metrics/test_session_duration.py (59 cases) drives the production helpers
with a patched recorder and a redirected data home:

• start counts, labels the surface, and leaves exactly one crumb; the crumb is
  named by digest, so a key containing ../ cannot escape the directory
• two generations of one key never share a filename, and a start that supersedes a
  live generation reaps the crumb it displaced
• a clean end emits the lifetime in ms with end_reason + session_source, and
  consumes the crumb
• a SECOND end emits nothing (the overlap defence: the idle sweep calls reset)
• an end with no crumb, an unknown end_reason, and a non-positive lifetime each
  emit nothing; an unknown reason also leaves the crumb intact
• a corrupt crumb is reaped by the BACKFILL without emitting, so it is not
  re-walked every boot -- the end path no longer unlinks what it cannot name
• a crumb carrying this process's own pid but a different start identifier is a
  dead predecessor and is claimed (the container PID 1 case); a matching identifier
  is still us and survives; an unreadable identifier fails closed
• a leftover crumb becomes one crashed sample sized from the transcript mtime;
  a second backfill emits nothing; a session with no transcript yields no sample
  but still loses its crumb; a cleanly-ended session is never back-filled
• a deferred writer landing after its session ended writes nothing, and one
  landing after a SUCCESSOR registered cannot reach the successor's crumb at all
• an ended key is not suppressed for the rest of the process
• the end path is pinned to unlink inline rather than through the pool
• AST gates: each of the six teardown paths records with its OWN reason constant,
  no two paths share a reason, and the boot backfill is ordered after the
  orphan-process cleanup

test/metrics/test_tool_call_duration.py (37 cases): every known kind passes
through; an MCP call is labelled by transport; an agent-authored kind and an
absent kind both become other; every classification is inside the declared set;
each terminal status is an outcome while pending/in_progress/None leave the
clock running; a finish with no start, a second finish, and an empty id each
record nothing; a repeated start does not restart the clock; the registry is
bounded. The parser-wiring cases drive the real _dispatch builders, including
an output-less completion (still measured) and a doubled result frame (still one
sample). The scope cases cover two sessions reusing one toolCallId, a finish in
the wrong scope failing to steal an entry, and -- at both the source and the
behaviour -- the two layers deriving one frame's scope from the same session id,
so whichever sees it second pops the entry the first opened.

test/metrics/test_business_counters.py (29 cases): the approval counter is
driven through the real ToolHookResult factories, including that a security
deny and a policy deny are distinguishable, that the deny REASON never reaches
the attributes, and that the gate still returns its verdict when the emit raises.
Compaction is driven through the real funnel, including the no-callback surface.
Two contract gates cover the rest: each counter's owning module must reference
its constant, and an AST pass asserts no emit_counter call site passes an
f-string or a concatenation as an attribute value.

Mutation-verified, each mutant applied alone and reverted after:

• dropping the inline _unlink from the end path turns 4 cases red
• removing the end record from a single registry-removal path turns the
  fail-closed removal gate red, naming the offending path -- which is what
  confirms the gate detects a NEW unrecorded removal rather than just the four
  already fixed
• making classify_tool_kind trust the agent-supplied kind turns 4 cases red,
  including assert 'totally_new_kind_9000' == 'other'

Manual verification

N/A -- no user-visible surface changes, and every new emit is driven through its
production call site by the tests above rather than through a stub.

Gates run locally: the review gate set (test/metrics test_acp_tool_identity test_dashboard_reset_sessions) 633 passed. flake8, isort --check-only and
mypy clean on the changed files, and scripts/docs_lint.py passes over all 248
markdown files. The repo's own black gate passes; the touched files inside
.github/black-baseline.txt were NOT whole-file reformatted.

One coverage-baseline entry, and why it is in this PR

This PR adds one line to .github/coverage-baselines/backend.txt exempting
src/kiro_crew/builtin_skills/pipeline-conductor/scripts/fleet_probe.py. That
file is not part of this change and no commit here touches it. The entry is a
deliberate, tracked, revocable measure to clear a CI-side blocker, not a
loosened gate, so here is the whole reasoning in the open.

It is not missing tests. TestFleetProbe exists at
test/test_pipeline_conductor_agent.py:179, its 47 tests pass, and the file
measures 86% (201/229) when coverage is scoped by PATH. The floor's own
remedy -- "add tests, do not extend the baseline" -- cannot move this number,
because the tests are already there and already green.

What is actually broken is attribution. The backend lane measures with
--cov=kiro_crew, scoped by import NAME. Under that exact flag the same passing
tests attribute nothing to this file: locally it does not appear among the 1214
reported rows at all. The 14.8% (34/229) the gate reports is precisely the
import-only footprint of a bare exec_module -- verified by measuring one. This
is tracked as #7597 and has had no upstream movement: the file has not been
touched since c0dca4a8b (2026-09-01), and the issue is still open.

Why the entry, rather than another rebase. The file's visibility to the
per-file floor is unstable, and absence is the normal state -- #7300 passed with
no row for it at all. So a rebase alone does not fix anything; it re-rolls
whether the file happens to be imported into the measured shard this time. The
entry removes that coin flip.

Why it surfaced here and not everywhere. Until now it was masked. Coverage
Gate fails CLOSED on the upstream backend conclusion, so while the shard was red
for an unrelated main-owned reason the gate never downloaded or parsed the
artifact and never evaluated the floor. The ordering analysis is on
#7757.

It cannot outlive the defect silently. The gate enforces its own removal.
Once attribution works the file reports at or above 82.0% (the 80% floor plus
its 2.0pp tolerance band), which is the graduated verdict -- an ::error::
that names the line and demands it be deleted. Verified by running the real gate
against a report at 90%: exit=1, 1 baselined file(s) now meet the floor. Remove them so the baseline keeps shrinking. The same run at the import-only
rate exits 0, and an absent file is a warn-only note. So the three reachable
states are: defect present -> exempt and green; file unmeasured -> note; defect
fixed -> red until the entry is removed.

The entry carries this rationale as a comment beside it in the baseline file,
including the removal condition, so a future reader does not have to find this
PR to understand it.

Related Issues

Refs #7232 #7257

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated -- docs/system-specs/modules/metrics.md gains a
  registry row per new instrument (attributes, emit site, rationale), the two
  new histogram bucket families, and a subsection recording why these
  counters are deliberately not merged with the in-memory stats.py tallies
  and which population each measures
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of c7ff7db304d781f3e7f7dfd97d0bf5101c8ac241 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Instrumentation with named causes throughout — every mechanism (crumb ownership, generation-named files, fail-closed backfill) is derived from a concrete failure, not speculation.

Suggestions

• Land the fleet_probe.py coverage-baseline exemption in its own PR: it unblocks CI for every branch, so coupling it to this feature means a revert of the telemetry work re-breaks unrelated lanes.

[DESIGN-REVIEWED] c7ff7db

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of c7ff7db304d781f3e7f7dfd97d0bf5101c8ac241 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/metrics/sessions.py:247 -- Function-local "from kiro_crew..." imports here and in hooks.py, gateway.py, and admission.py violate top-level-imports -> Fix: move safe imports to module scope, document genuine circular-import exceptions, or remove the affected instrumentation.

[GPT-REVIEWED] c7ff7db

False positive or not applicable? A repository writer can comment:
/ai-review override gpt c7ff7db304d781f3e7f7dfd97d0bf5101c8ac241: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of c7ff7db304d781f3e7f7dfd97d0bf5101c8ac241 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All mechanical checks I need are done: stats.py does hold sessions_created / subagents_spawned / tool_approvals (the near-duplicate populations), open_call_count/reset_open_calls have no production consumers outside their defining module, and telemetry_channel_of exists as claimed. Every new end_reason enum member is constructed at a real site in the diff, and every new public function has 1–13 production call sites. Here is the review.

First-Principles-Verdict: CONCERNS

Every instrument earns its place, but a coverage-baseline exemption for an untouched file rides along, and three counters knowingly shadow stats.py tallies.

What this change ships

Intent: let an operator see how long sessions live (including crashed ones), which part of a slow turn was the tool, and whether each subsystem is doing its job. This is an ADDITION.

1. Session lifetimes measured, split by ten end reasons including crashes — justified
2. New consent-gated crumb files persisted under the data home — justified, declared
3. Boot now background-scans leftover crumbs and reports them as crashes — justified
4. Sessions-started counter — justified; second population beside stats.py:sessions_created
5. Per-tool-call latency histogram by kind — justified
6. Seven subsystem counters (subagent, cron, artifact, workflow, compaction, MCP, approvals) — justified; approvals/subagent shadow stats.py tallies
7. New lifecycle rule: every registry removal must record an end, AST-gated — justified
8. Coverage-baseline entry for fleet_probe.py, a file this PR never touches — rides along
9. Spec updates in the same commit — mandated by AGENTS.md
10. Test-only public helpers open_call_count/reset_open_calls — zero consumers

Watch

• Item 8 rides along. .github/coverage-baselines/backend.txt gains an exemption for builtin_skills/pipeline-conductor/scripts/fleet_probe.py, unrelated to telemetry; the description's visible 8000 bytes never mentions it. Its own comment claims it unwedges a red coverage lane (main is red: fleet_probe.py at 14.8% fails the per-file coverage floor on every rebased PR #7597) and self-voids when fixed, so I do not block on it — but a human should confirm the lane actually fails without it, since that claim is the only thing separating "unwedge" from "unrelated debt smuggled in".
• Three counters are deliberate second spellings. kirocrew.session.started, kirocrew.subagent.spawned, and kirocrew.approval.decisions count populations adjacent to stats.py's sessions_created / subagents_spawned / approval trio (grepped: 3 tallies, 1 dashboard consumer). Neither system can subsume the other (the singleton is always-on and attribute-free; the instruments are consent-gated time-series), and the new spec section names the divergence hazard — but both spellings are now maintained forever, and the two numbers will legitimately disagree.

Subtractions

• Drop ALLOWED_SILENT: dict[str, str] = {} in test/metrics/test_session_duration.py — an exemption registry with zero entries; add the dict the day a silent removal genuinely needs one.

[FIRST-PRINCIPLES-REVIEWED] c7ff7db

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed c7ff7db304d781f3e7f7dfd97d0bf5101c8ac241 — this comment is updated in place on each push.

Review details

All three candidates fail falsification:

• Candidate 1 (inline unlink blocks the loop): requires a "slow or network-mounted filesystem" — a speculative condition, not one shown to occur. The design documents this as a deliberate tradeoff (record_session_ended docstring: the inline unlink is chosen precisely because deferring it reopens a successor-crumb race), it is one syscall, and the harm is latency, not crash/data-loss. Self-rated medium, admits it is "weak" on local disk. No concrete (a)/(c).
• Candidate 2 (_crumb_path/config_dir raises outside the try): config_dir() is memoized and is invoked constantly across the whole gateway; for it to raise, the parent-directory mkdir must fail, which would already have broken session persistence and the start-path crumb write. No concrete input that occurs in practice; the "never raises" gap is defense-in-depth only. Self-rated low.
• Candidate 3 (_count over-counts off the gate path): every caller of ToolHookResult.allow/auto_approve/deny/deny_policy lives inside hooks.py's gate path (grep confirms no factory call anywhere else in src/); surfaces that override the gate construct ToolHookResult(...) directly, which does not call _count. The claimed over-count population is empty. Self-rated low, "not a functional defect."

No new Step-2 finding grounds to 80+.

No findings.

[OPUS-REVIEWED] c7ff7db

Verdict parsed from the review's SHA-scoped output markers for commit c7ff7db304d781f3e7f7dfd97d0bf5101c8ac241.

False positive or not applicable? A repository writer can comment:
/ai-review override fable c7ff7db304d781f3e7f7dfd97d0bf5101c8ac241: <one-sentence reason>

[chenmingwei23]

Round 4 dispositions on 7f3af476d. Every finding below was legitimate; none is overridden.

GPT 5.6 -- both BLOCKING findings, fixed structurally rather than patched

The two reviewers found one root cause from two directions, and it was the same code: the crumb unlink going to the maintenance pool.

• GPT: a queued _unlink is not generation-safe -- a predecessor's unlink can land after a successor registered under the same key and wrote its own crumb, deleting a live session's only crash evidence.
• Design (Watch 2): shutdown_maintenance_executor drains with cancel_futures=True, and close_all is exactly when that pool is flooded with teardown work, so the unlink is cancelled outright -- leaving a cleanly ended session's crumb for the next boot to back-fill as crashed. Orderly shutdown would inflate the very population this instrument exists to measure.

GPT's suggested fix (thread started_at through and compare generations on unlink) closes the first but not the second. Since this was the third finding in this one span, I removed the question instead of adding a third guard:

1. The unlink now runs inline, inside the generation lock -- one syscall, in the same tick as the registry pop. Nothing can interleave between the pop and the unlink, and nothing can be cancelled. Both findings die with the pooled unlink.
2. _live_starts became the crumb's single generation token. The deferred writer now checks _is_current_generation instead of an end flag, which refuses on one comparison whether the key was popped (ended) or overwritten (superseded). That let the whole parallel _ended_keys tombstone set and its three helpers go away -- a net subtraction, and one fewer pair of structures to keep in step.

That second change also closes a hazard neither reviewer named: with only an end flag, a successor's start lifted the flag, so a predecessor's in-flight writer could still land and make the successor's crash measure from the predecessor's start. Pinned by test_a_superseded_writer_does_not_overwrite_the_successors_crumb.

Mutation-verified, each applied alone: dropping the inline unlink turns 4 cases red; making the generation check always pass turns 2 red.

GPT's second BLOCKING (queued workers outliving the per-test home) -- real, and a genuine test side effect: the home fixture redirects config_dir for one test while a pooled crumb write outlives it, so a busy pool lets the worker write into the real data home. Fixed as suggested, with an autouse fixture running crumb work inline; crumb state is now deterministic, so the polling helper is gone. One test deliberately opts back out via the real submitter, because its whole point is that the pool has no event-loop affinity -- and it waits on the outcome, so its worker has demonstrably finished before its home goes away.

Design Review -- CONCERNS

• Watch 1 (the spec registry is untouched): correct, and mandatory. docs/system-specs/modules/metrics.md now carries a registry row per new instrument with its full closed attribute set, emit site and rationale; the two new bucket families are added to that table (Three families -> Five families) with the reasoning for each range. Docstrings were never a substitute for the table the router points contributors at.
• Watch 2: fixed above.
• Suggestion (pin that the two tool-call layers derive the same scope for one frame): added as test_both_layers_derive_one_frames_scope_from_the_same_session_id, pinned at both levels -- a source assertion that neither layer invents a scope of its own (layer one takes cache_scope, layer two the client's _session_id), and a behavioural one where layer two opens a call and layer one closes it for a single sample. The existing cross-layer test used an empty scope for both sides, so it could not have caught a divergence.

First Principles -- CONCERNS

• Subtraction (_read_started_at ships dead): confirmed zero references in src/ and test/; backfill_crashed_sessions calls _read_crumb directly. Deleted.
• Watch (three counters re-spell populations stats.py already counts): accepted as a real hazard, and not fixed by deleting either side. The two are not interchangeable -- the singleton is a process-local tally that resets on restart and is read by one handler; these are time-series with attributes that survive a restart -- but you are right that near-identical names counting subtly different populations will be read as the same number and then disagree. The fix is that the difference is now written down: the spec has a subsection on why they are deliberately not merged, and each row states which population its counter measures. The sharpest case is called out explicitly -- kirocrew.approval.decisions counts gate verdicts, including auto-approves and denies that never reach a person, which is not the population the singleton's approval trio accrues.

Two description-vs-code mismatches I found and fixed while here

Both were in the PR body, in the same class of defect the reviewers flag, and neither had been reported:

• the body claimed reset is where "watchdog recycle, compaction recycle" reach teardown. It is not -- compaction reports its own recycled reason, and an identity retirement reports retired. Corrected.
• the body claimed the approval counter fires at ToolHookResult construction for "the four factories AND the surfaces that build a result directly to override the gate". The shipped code gates on from_gate, which only the four factories set, so an overriding surface is deliberately not counted. Corrected to match the code.

The body also carried a stale boot-ordering claim ("necessarily BEFORE this process opens any session of its own") that the started_before cutoff made false, plus stale test counts and gate figures. All updated before this push, so the reviewers read current text.

Gates on 7f3af476d: review gate set 624 passed; flake8 / isort --check-only / mypy clean; scripts/docs_lint.py passes over 248 markdown files; black gate clean with no whole-file reformat of baseline files.

[chenmingwei23]

Round 5 dispositions on 3839e1e7f. Opus clean and Design PASS on the previous head; the four items below are GPT's and First Principles'. One finding is rebutted with evidence, the rest are fixed. Nothing overridden.

GPT BLOCKING -- the post-write recheck could delete a successor's crumb

Correct, and it was mine from round 4. The recheck acts on a path the successor SHARES, so a predecessor finishing late unlinks the crumb its successor just wrote. Round 4 traded "wrong crumb" for "no crumb" and did not remove the race.

This was the FOURTH finding in this one span (round 3: teardown ordering; round 4: pooled unlink, two ways; round 5: this). Rather than add a fifth guard I tested the premise underneath all four -- that the write must be deferred because the caller holds the session registry lock:

• that lock is an asyncio.Lock (session_lifecycle.py:80, session_background.py:79), so it serialises coroutines and does not block a thread;
• the SAME critical section already calls _session_map.set(...) (session_allocation.py:1362/1377), whose own docstring is "Save mapping and persist to disk", and already contains an await session.semaphore.acquire();
• measured cost of the actual write: atomic_write with the default fsync=False, 190-byte payload, 500 iterations -- median ~92us, p99 ~165us.

So the deferral was a micro-optimisation, and it was the sole source of the class. The write now happens inline in the same _live_lock section that installs the start. That deleted the generation token, both re-checks, and the pool submitter: ordering is structural instead of detected after the fact. I did not adopt the suggested generation-scoped temp-file publish, because it is still check-then-act on a shared canonical path -- it narrows the window rather than removing it.

The three tests that encoded the deferral are replaced rather than deleted: one now pins that the writer has exactly ONE call site and it is inside the locked section (a guard can be removed and re-broken; a sole call site under the lock is what makes the ordering structural), and the Windows Event loop is closed regression is now impossible by construction since there is no future at all.

GPT FINDING -- a clean pop path leaves crumbs the next boot calls crashed

Right about the mechanism, wrong about the path, and it led to three more. drain_all_providers is not reachable as described: its only caller calls reload_provider_factory first, which clears the registry and records retired for every entry, so count is 0 and the drain pops nothing.

But auditing every registry mutation in src/ found four removals that record no end, three of them genuinely reachable:

• session_background.py::recycle_heartbeat -- pops the background session each cycle. Now records recycled.
• session_allocation.py::_evict_stale_session and the same dead-provider check inside get_or_create -- drop an entry whose provider already died. Now record a new evicted reason: nothing was torn down, the process was already gone, so this answers "how often did a session die under us and get noticed on the next lookup" rather than "how often did we end one".
• drain_all_providers -- records per popped key now, so the latent trap cannot be reintroduced by a future caller.

The reason this matters more than a missing sample, which the module previously claimed was the cost: an unrecorded removal leaves the crumb, so the next boot reports the session as crashed. It manufactures a failure that never happened, in the one population this histogram exists to measure. That claim is corrected in the module docstring.

The gate was the wrong shape and is replaced. It enumerated six method names in one module, so a seventh path -- or any pop in another module -- was invisible to it, which is exactly how all four escaped. It is now a fail-closed AST walk over every module that mutates the registry, recognising all three removal spellings (pop, del, clear), so a NEW removal path fails by default. Mutation-verified: removing one end record turns it red and names the offending path.

GPT FINDING -- crumbs written while telemetry is disabled

Rebutted on the code, fixed on the documentation.

The opt-out contract is a no-egress promise scoped to the OTEL metric pipeline, and the crumb is not in that pipeline: otlp_endpoint defaults empty, the gated sink is the exporter wired only on the consented path, and nothing ever reads the crumb off the host -- backfill_crashed_sessions consumes it at boot and unlinks it.

Always-on local writes carrying the session key are the established, documented pattern, not an outlier. The per-turn usage row store writes the raw session key as its slot field on every billed turn, unconditionally, and the spec states those readers are "served independently of the telemetry.enabled switch since these rows are always written". session_map.json already holds every session key in the same data home, and the transcript filename already encodes it. The crumb is strictly less exposing than all three: it is digest-named and holds one key that is already present, un-digested, in at least three always-on locations in that directory.

What the finding did surface is a real ambiguity it could reasonably rely on: two spec sentences said "nothing is written" flatly, which reads more absolutely than the module behaves. Both are now scoped to the metric pipeline they actually govern, with the always-on local state named. No behavioural change.

First Principles CONCERNS -- from_gate is a signaling field with zero readers

Agreed, and taken. The flag's only reader was the counter it gated, so it was state carried purely to signal. The four factories now call one private helper directly and the field is gone. Behaviour is unchanged -- a surface overriding the gate still constructs a result directly and is still not counted -- but it is now structural rather than flag-driven. Checked first that nothing does dataclasses.replace on this type, since __post_init__ would have double-counted a copy; nothing does, so no behaviour rode on the old shape.

Also corrected while here

success on the compaction counter was documented as "how often compaction actually recovered headroom" in both the spec row and the PR body -- my wording from last round, and wrong. The coordinator never takes a before/after context reading, so the attribute reports whether the ATTEMPT completed. Both now say that, and say why the effectiveness reading would overstate it.

Gates on 3839e1e7f: metrics + identity + reset gate set 626 passed; the session/heartbeat suites the new end records touch 2341 passed; flake8, isort --check-only, mypy clean; scripts/docs_lint.py clean. hooks.py is in the black baseline and was NOT whole-file reformatted -- its two black diffs are pre-existing drift at lines 226 and 1368, both outside the edited region, verified against the unmodified file at HEAD.

[chenmingwei23]

Round 10 on 08e4d3381. All three findings answered: two fixed, one declined in
writing with a follow-up issue. Design, First Principles and Opus were all clean on
the previous head, so this round is GPT's three items plus a note on shard 3.

BLOCKING 1 -- distinct generations share one breadcrumb: FIXED, and the premise is gone

Correct, and it was reachable in a way none of the earlier rounds in this span had
addressed: the locks here are THREAD locks, so they never protected anything across
processes, and a session key is not unique across processes either -- BACKGROUND_KEY
is a fixed constant, so a kirocrew run and the gateway can hold the same key at
the same time with a live session behind each.

Fixed as suggested, by putting the writer and the generation in the filename
(<digest>-<pid>-<start time>) rather than adding another check. Three behaviours
follow, and the second is the one worth naming:

• a clean end unlinks the ONE generation it can name;
• an end with no live table entry now unlinks NOTHING. That is not a gap left
  open -- it is the fix. The files under that digest belong to other processes or to
  earlier runs, so today's unconditional unlink was itself the sibling-clobbering
  bug this finding reports;
• a start that displaces a live generation of the same key, and an eviction under
  the table cap, each reap the generation they orphan.

That third case was a real regression introduced by the rename, and the existing
suite caught it during implementation rather than in review: under one shared
filename a second start simply overwrote the file, so nothing had to handle it, but
a named generation the table has forgotten can never be unlinked again and would
reach the next boot as a crash that never happened. Both reaps ride the same awaited
worker hop as the start's own write, so they stay off the event loop without adding
a second cancellation window.

Two things this removes rather than adds: the post-write read-back check from round 9
is gone, because the path can no longer belong to anyone else, and the backfill's
lock is no longer guarding an identity race (its comment now says what it actually
does -- serialise against this process's own writer).

BLOCKING 3 -- current-PID reuse hides crashed sessions: FIXED

Also correct, and I want to record that the other lane looked at this and got it
wrong, because the disagreement is the interesting part. Opus assessed the same fast
path and ruled it below the bar on the grounds that it needs a "coincidental" PID
collision. That premise does not hold: in a container the gateway is PID 1 on every
restart, so a crashed predecessor's crumb arrives carrying this process's pid
deterministically, not coincidentally. The consequence is also worse than a single
missed sample -- the crumb is skipped on this boot and on every boot after, so its
crash is never emitted and its file is never cleaned.

The fast path now compares start identifiers whenever both are readable, and an
unreadable identity still fails closed to "running". Three cases are pinned: a
foreign identifier under our own pid is claimed as a crash, a matching one is still
us and survives, and an unreadable one is left alone.

BLOCKING 2 -- unlink blocks the event loop: DECLINED, filed as #7537

The mechanism is real and I am not disputing it. The remedy is what I am declining
for this change, and the reasoning is on the issue rather than only here.

A deferred unlink has to be RELIABLE, not merely off-loop. Fire-and-forget is not:
the maintenance executor drains with cancel_futures=True, so the unlink is dropped
exactly at close_all when teardown work is heaviest -- and a dropped unlink is
worse than a slow one, because the crumb then survives to the next boot and is
reported as crashed, injecting a fabricated failure into the one population this
histogram exists to measure. Reliability therefore means awaiting the hop, which
makes record_session_ended a coroutine and gives roughly ten teardown call sites a
new cancellation window each -- and a cancellation mid-teardown is the defect class
already fixed twice in this area.

What has changed is the size of the prize. Naming the file after its writer and
generation removes the ordering hazard entirely: a deferred unlink can only ever
reach its own file now, however late it lands. So the residual this would buy back is
one narrow case -- a process killed between the registry pop and the unlink leaves a
single crumb the next boot counts as a crash. Trading ten new cancellation windows
for that, in the same change, is the wrong ratio. #7537 carries the mechanism, the
reliability argument, the residual and the correct shape.

Shard 3 (Linux 3.10 and Windows): main-owned, not from this diff

Both failures on the previous head reproduce independently of this branch:

• test_security_posture.py::TestGateSideLogRedactorSpelling::test_the_census_holds_no_slack
  -- _BASELINE_LOG_SITE_CENSUS records 3 log sites for dashboard/handlers/files.py
  while the code now has 1. Reproduced locally on this branch, which touches no
  frontend or dashboard-handler file; the census baseline was simply not lowered when
  the sites were.
• test_pod_e2e_harness_paths.py::test_health_accepts_the_pods_own_serving_codes
  -- the pod harness health check timing out at its 1s budget.

Neither fix is folded in here. This head is rebased onto current main to cut a fresh
merge ref instead.

Gates

59 + 37 + 29 cases in the three metrics files; the required set (test/metrics/,
test_acp_tool_identity.py, test_dashboard_reset_sessions.py) is 702 passed, and
796 with the adjacent suites for the instrumented modules. flake8, isort, mypy,
docs-lint and the repo black gate all clean.

Three mutations verified rather than assumed, each reverting one half of this round:
dropping the generation from the filename fails 3 tests, dropping the orphan reap
fails 2, and reverting the identifier comparison fails the container PID-1 case.

[chenmingwei23]

/ai-review override gpt 36cdd3c: Known and deliberately deferred as #7537 -- offloading this unlink requires record_session_ended to become awaitable across roughly ten teardown sites, each adding a cancellation window that can drop the crumb entirely and turn a clean exit into a false crashed, which is strictly worse than a slow unlink.

[github-actions]

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 36cdd3c3d9eefb45271117e6d8e09f08a1885d8e.

Known and deliberately deferred as #7537 -- offloading this unlink requires record_session_ended to become awaitable across roughly ten teardown sites, each adding a cancellation window that can drop the crumb entirely and turn a clean exit into a false crashed, which is strictly worse than a slow unlink.

This decision applies only to this commit. A new push requires a new judgment.