Repository navigation
fix(agents): one derivation of "last active", shared by the footer and the MCP - #1080
Conversation
…d the MCP
Two surfaces answered "when was this agent last active" from the same runtime
fields with different rules, so they could report different times for the same
agent:
- the TLDR peek footer took max(lastJsonlEntryAt, phaseChangedAt,
turnStartedAt, submittedAt), with a transcript-tail fallback that accepted
ANY entry;
- the `agent_management` MCP inventory left `lastJsonlEntryAt` out entirely,
and its tail fallback accepted only user/assistant entries carrying
non-empty text.
They diverge exactly when the newest transcript entry is a tool or system
record and the ingest watermark is null: the footer reports the tool record's
time, the inventory skips it and reports an older turn, or nothing.
Cosmetic in the UI — but the inventory is what an ORCHESTRATING AGENT reads to
decide whether a child is idle, so two answers to "when was this last active"
is two answers to "is this safe to close".
`sessionActivity` is now the single rule, in a neutral module both surfaces
import. The footer's rule is the one kept, as #915 recommends, because it
includes the ingest watermark: `lastJsonlEntryAt` is the newest thing the
transcript reader has actually seen, and ignoring it means calling an agent
idle while its transcript is still growing. A tool record counts too — an
agent mid-tool-call is not idle.
THE MAIN BRIDGE ALSO HAD TO CHANGE, which the issue does not mention: it
recombined the two raw renderer fields itself before producing
`lastActivityAt`, so publishing a unified field without touching it would have
left the divergence exactly where it mattered. It now takes
`descriptor.lastActiveAt` as the renderer's answer. The transcript file's
mtime and the backend's own activity remain candidates because they are
evidence the RENDERER cannot see — not a second opinion about the same facts.
The two raw fields are still published, documented as the INPUTS rather than
answers. Removing them would be a breaking change for an existing caller, and
the issue anticipated that cost without requiring it; an agent deciding
whether a child is idle should read `lastActiveAt`, because recombining the
raw fields is precisely how this happened.
One test asserts the two exports are literally the same function. The surfaces
agreeing today is worth less than their being unable to drift tomorrow.
Mutation-checked, 1093 renderer/main tests:
the tail fallback skips tool entries 2 failed
the ingest watermark is ignored 5 failed
the inventory answers on its own again 2 failed
lastActiveAt is not published 2 failed
the raw fields stop being published 1 failed
the bridge recombines the raw fields 1 failed
restored 1093 passed
Fixes #915
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e dead evidence fields Review findings on #1080. FINDING 1 (high): `lastActivitySource` silently flipped `transcript` to `runtime`. The JSONL watermark used to reach the bridge as `descriptor.transcriptActivityAt` and was labelled `'transcript'`; it now arrives inside the renderer's unified answer, which the bridge hardcoded as `'runtime'`. The value was right and the citation was wrong — and the new test PINNED the wrong label. `lastActivitySource` is what an auditing agent quotes back, and the design doc ranks a real record above a clock a screen repaint can move. The bridge cannot fix this on its own: it sees one number and can only say where it arrived from. So `sessionActivity` now returns `{ active, timestamp, source }`, the descriptor carries `lastActiveSource`, and the bridge uses it. A tie goes to the transcript, the stronger evidence. FINDING 2 (high): the comment justifying `transcriptActivityAt` and `runtimeActivityAt` — "kept because an existing caller may read them" — was false. `ManagedAgentRendererDescriptor` is renderer-private and never serialised to an MCP caller; the JSDoc three lines above says so. Once the bridge stopped recombining them, nothing read them at all. They are deleted, along with `latestVisibleTimestamp`, the inventory's old tail scan that `sessionActivity` superseded. FINDING 4 (medium): the regression fixture was shaped like no entry that exists — `{ type: 'tool_use' }` is not an entry type, and a `user` entry with a bare `text` property has no text as far as `entryTextContent` is concerned, so the test passed without reproducing anything. Replaced with the two REAL shapes, read off the corpus: an `assistant` row whose content is a single `tool_use` block, and a `user` row whose content is a single `tool_result` block. The second is the more common one (20 of the 24 no-visible-text tails in 500 local transcripts) and was missing entirely. FINDING 6 (medium): "some history-loading paths predate the ingest watermark" was too vague to check. The path is OLDER-HISTORY PAGING: the initial load is the only writer of `lastJsonlEntryAt`, so a first chunk of non-renderable metadata leaves it null, and the prepend in history.ts never touches it. Now asserted against the real reducer in history.renderer.test.tsx, where it is a fact rather than an assumption. FINDING 5/7 (medium): four WHY claims were false and are corrected in place, each saying what it used to say: - the inventory did NOT leave `lastJsonlEntryAt` out entirely; only `runtimeActivityAt` did, and the transcript mtime usually masked the rest; - there is no "system record" case — the mapper drops every system/meta record and compact boundaries carry no timestamp; - `backendActivityAt` is NOT evidence the renderer cannot see. It is `markActivity`, a wall clock fired from pty-data and screen frames (before the frame gate, at that). It stays because it is the only signal for a pane that was never hydrated, and the comment now says that instead; - "the inventory is what an orchestrating agent reads" overstated it. An orchestrating parent reads `orchestration_list_agents`, a different cascade this does not touch. `agent_management` is the project-scoped audit surface. CloseOldAgentsModal's comment told the next reader that #915 was still open and that this rule should become its consumer. It is now the honest version: that rule keeps two signals `sessionActivity` does not have (semantic turn boundaries, and refusing to answer while bootstrapping) because its question is destructive and `null` is a real answer to it. FINDING 3 (medium): all seven surviving mutants are now killed — dropping `turnStartedAt` or `submittedAt` from the evidence, either read path dropping the forwarded answer, `enrichOutput` dropping it, the bridge dropping the backend candidate, and the bridge hardcoding the source. Two more: flipping the tie to runtime, and a tail fallback that refuses tool-only entries. Verified in a clean worktree, since this one reproduces a local vitest module-resolution failure ("Cannot find module '<root>/string_decoder'") that the same commit does not show anywhere else: 350 files / 2510 tests passing across unit, renderer and system. `npx tsc -b` clean on Node 24. Refs #915 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Adversarial review returned REQUEST CHANGES with nine sections. Point by point. 1.
|
| mutant | now |
|---|---|
M5 drop turnStartedAt |
2 failed |
M6 drop submittedAt |
2 failed |
| M15/M16 read paths drop the forwarded answer | 1 failed |
M21 bridge drops the backendActivityAt candidate |
1 failed |
M22 enrichOutput drops lastActiveAt |
2 failed |
M22b enrichOutput drops lastActiveSource |
2 failed |
M24 bridge hardcodes 'runtime' |
1 failed |
M25 re-add ?? descriptor.transcriptActivityAt |
unrepresentable — the field is gone |
Two more of my own: flipping the tie to runtime (1 failed), and a tail fallback that refuses tool-only entries (4 failed).
M22 needed the bulk fixture to send an empty agents array — with the census populated the bulk path reuses that record and never reaches enrichOutput, so the obvious test would have passed vacuously.
4. The fixture is shaped like no entry that exists (MEDIUM) — valid, and the sharpest finding
{ type: 'tool_use' } is not an entry type and { type: 'user', text } has no text as far as entryTextContent is concerned. The test passed without reproducing anything — my own "stated separately so it can't pass if both regress" guard did not save it, because the premise never held.
Replaced with the two real shapes, read off the corpus: an assistant row whose content is a single tool_use block, and a user row whose content is a single tool_result block. Your count is now in the file: 24 of 500 tails carry no visible text, 20 of them the tool_result shape — the more common direction, and the one the PR missed entirely.
5. False WHY claims (MEDIUM) — valid, all four corrected in place
Each correction says what it used to say, so the next reader learns the error rather than inheriting a silently-edited comment:
- the inventory did not leave
lastJsonlEntryAtout entirely — onlyruntimeActivityAtdid, andtranscriptLastModifiedAt ?? transcriptActivityAtusually masked it; - there is no "system record" case;
backendActivityAtis not evidence the renderer cannot see. It ismarkActivity, a wall clock fired frompty-dataandscreen— before the frame gate, as you note. The comment now says that plainly, says it stays because it is the only signal for a pane that was never hydrated, and says that narrowing it to the signals the design doc counts is its own change.
6. The reachability argument (MEDIUM) — valid, and asserted rather than argued
Older-history paging is the path. It is now a test against the real reducer in history.renderer.test.tsx: an older page populates entries while lastJsonlEntryAt stays null, and sessionActivity then reports the tail with source: 'transcript'. Asserted there rather than in the agreement test because it is a fact about that reducer — a hand-built runtime would only restate the assumption. Your 351/500 cost-state figure is in the comment.
7. "One derivation" overclaims (MEDIUM) — valid, scope corrected in words not code
The framing is fixed: agent_management is the project-scoped audit surface and requires user authorisation before a close; an orchestrating parent reads orchestration_list_agents, a cascade this does not touch. The stake is an audit reading a mid-tool-call agent as idle, not an automatic close.
CloseOldAgentsModal was telling the next reader that #915 is open and that this rule should become its consumer. It is now the honest version: that rule keeps two signals sessionActivity does not have — semantic turn boundaries, and refusing to answer while bootstrapping — because its question is destructive and null is a real answer to it. latestAgentActivityAt >= sessionActivity().timestamp by construction, and the comment says to keep it that way.
orchestrationMcp.ts's cascade stays untouched. Thank you for tracing isIdleOrchestrationAgent — confirming #960/#962 reads orchestrationChildLifecycle rather than these fields is the reason this could stay a display-layer change.
8. Not-a-defect section — noted, and the monotonicity claim is now stated
The 5,000-runtime fuzz and the 495/495 mtime check are exactly the evidence the PR should have carried. The "new value is always ≥ the old one, so an idle agent can only look more active" property is the safe direction for a destructive filter and is now written down.
sessionActivity().active being unified in name only is stated in the doc comment.
9. Build claims — accepted
The tsc -b note was wrong in spirit: two TS2742 errors in paneActionsHarness.tsx reproduce identically at the base, so the delta is zero, but "clean" was the wrong word for it. My worktree does not show them; yours does; the symlinked node_modules is the difference.
On the string_decoder failure you could not reproduce: it reproduces deterministically in this worktree and nowhere else. The same commit passes in three other checkouts of byte-identical content, and copying the whole diff into a fresh worktree gives 350 files / 2510 tests passing across unit, renderer and system. It is a local vitest module-resolution artifact, not a property of the change; that is where the numbers below come from.
npx tsc -b clean on Node 24. Verified in a clean worktree: 2510 passed.
…tage 1's findings The first cut of this decomposition said sessionDisplayTitle resolves 'explicit title -> spoken agent name -> first prompt -> cwd'. It does not: there is no spoken-name term and no first-prompt term, only title, the terminal's live cwd, the spawn cwd and the raw path. That is not a footnote. The document's headline finding -- rows are unidentifiable -- was written believing a better source existed and was merely being reached for last. With 3 titles and 0 spoken names across 33 sessions there is nothing to fall back to, so Stage 2 cannot fix identity by reordering a chain; it has to introduce a source. Which one is now an owner call in the unknowns. Also recorded, from reading the substrate rather than assuming it: TLDR, Goal and Goal Loop have no renderer store and are deliberately subscribed only while a preview is visible, so the fleet-wide batched reader they need is a stage this document did not have (now 2b); the conditions capability lookup throws for terminals; limitHit never self-clears; terminalProviderFailure is inert without meta; Grok can never read failed; a session can hold more than one lane; and Stage 6's 'computed once' is reachable in the renderer but not across the MCP surfaces, by #1080's own design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #915.
The bug
Two surfaces answered "when was this agent last active" from the same runtime fields with different rules:
max(lastJsonlEntryAt, phaseChangedAt, turnStartedAt, submittedAt), tail fallback accepts any entryagent_managementMCP inventorylastJsonlEntryAt, tail fallback accepts onlyuser/assistantentries with non-empty textThey diverge exactly when the newest transcript entry is a tool or system record and the ingest watermark is null: the footer reports the tool record's time, the inventory skips it and reports an older turn — or nothing.
Cosmetic in the UI, but the inventory is what an orchestrating agent reads to decide whether a child is idle, so two answers to "when was this last active" is two answers to "is this safe to close".
The fix
sessionActivityis the single rule now, in a neutralsession-runtime/module both surfaces import. The footer's rule is the one kept — as the issue recommends — because it includes the ingest watermark:lastJsonlEntryAtis the newest thing the transcript reader has actually seen, and ignoring it means calling an agent idle while its transcript is still growing. A tool record counts too; an agent mid-tool-call is not idle.The main bridge also had to change
The issue does not mention this, and it is where the fix would otherwise have failed:
AgentManagementBridgerecombined the two raw renderer fields itself before producinglastActivityAt. Publishing a unified field without touching it would have left the divergence exactly where it mattered.It now takes
descriptor.lastActiveAtas the renderer's answer. The transcript file's mtime and the backend's own activity remain candidates because they are evidence the renderer cannot see — not a second opinion about the same facts.The raw fields stay
transcriptActivityAtandruntimeActivityAtare still published, documented as the inputs rather than answers. Removing them is a breaking change for an existing caller, and the issue anticipated that cost without requiring it. An agent deciding whether a child is idle should readlastActiveAt— recombining the raw fields is precisely how this happened.Tests
One assertion is that the two exports are literally the same function. The surfaces agreeing today is worth less than their being unable to drift tomorrow.
lastActiveAtis not publishednpx tsc -bclean. The wider renderer run shows two suites failing to load withCannot find module '…/string_decoder'— a worktreenode_modulessymlink artifact that reproduces identically with my changes stashed.🤖 Generated with Claude Code