Repository navigation
refactor(agent-status): publish structured sessions into the hook server store - #19683
Conversation
…ver store Structured (native chat) sessions have no PTY and no hook script, so their status never reached the hook server's store; #19217 gave `worktree ps` its own adapter over the structured feed instead. The feed now writes every projection into that store through a status sink the runtime wires, drops the row when the host closes the session, and `worktree ps` reads the one snapshot like every other agent. Rows carry a `structuredHost` marker and the journal clock; they are never persisted to last-status.json, and the main process does not forward them to the renderer yet, whose feed bridge still owns them until it is retired. Design and the two follow-ups: docs/reference/agent-status-store.md.
|
Review status: no proven in-scope defects found after independent review and readiness scans. Validation: 33 targeted Vitest tests passed; tsc -p config/tsconfig.node.json --noEmit passed. Readiness review found no P0/P1/P2 issues. Reference comparison was completed privately. Electron QA was attempted with ORCA_BACKGROUND_LAUNCH=1 and CDP, but the launched identity resolved to an unrelated QA19259 worktree, so the screenshot is incidental and does not provide PR-specific merge confidence. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughStructured native-chat sessions now publish status summaries into the hook server’s shared store. The store maps session state, preserves journal timestamps, tracks host ownership, and excludes structured rows from persistence. Runtime wiring sends feed updates and removals to the store. Worktree projections consume hook snapshots instead of separate structured summaries. IPC and renderer paths filter structured-host rows to avoid duplicate updates. Tests cover ingestion, lifecycle events, persistence, freshness, projection, and sink failures. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This change documents the existing headless status-store wiring in its producer census test. No actionable current-head merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: d635eca3-8a3a-4e3c-bfa0-781b874655b8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.gitignoreAGENTS.mddocs/reference/agent-status-store.mdsrc/main/agent-hooks/server-ingest-structured-status.test.tssrc/main/agent-hooks/server/server-ingest-remote.tssrc/main/agent-hooks/server/server-ingest-structured.tssrc/main/agent-hooks/server/server-persistence-validation.tssrc/main/agent-hooks/server/server-persistence.tssrc/main/agent-hooks/server/server-state.tssrc/main/agent-hooks/server/server-status-application.tssrc/main/agent-hooks/server/server-status-identity.tssrc/main/agent-hooks/server/server-status-update.tssrc/main/ipc/agent-hooks.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-types.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-status-feed.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-status-feed.tssrc/main/runtime/orca-runtime-get-worktree-ps.tssrc/main/runtime/orca-runtime-preserved-branch-cleanup.tssrc/main/runtime/orca-runtime-state-fields.tssrc/main/runtime/orca-runtime-structured-status-sink-wiring.test.tssrc/main/runtime/orca-runtime-tests/worktree-ps-structured-host.spec.tssrc/main/runtime/runtime-worktree-agent-rows-structured.test.tssrc/main/runtime/runtime-worktree-agent-rows.tssrc/main/runtime/runtime-worktree-agent-source.tssrc/main/runtime/runtime-worktree-agent-sources.tssrc/main/runtime/runtime-worktree-pty-agent-sources.tssrc/main/runtime/runtime-worktree-structured-agent-rows-liveness.test.tssrc/main/runtime/runtime-worktree-structured-agent-rows.tssrc/main/runtime/structured-agent-session-runtime.tssrc/main/startup/main-process-runtime-service.tssrc/main/startup/main-window-agent-status.tssrc/shared/agent-hook-listener/listener-event.tssrc/shared/agent-status-ipc-payload.tssrc/shared/agent-status-observation.tssrc/shared/runtime-worktree-contracts.ts
💤 Files with no reviewable changes (1)
- src/main/runtime/runtime-worktree-structured-agent-rows.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
|
|
||
| Orca shows "what is this agent doing" in four places: the desktop sidebar, the | ||
| `orca worktree ps` command, the mobile app, and the agent dashboard. Before | ||
| #19217 those readers did not even share their inputs. After #19217 they share |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Markdownlint warning.
Line 18 starts with #19217. markdownlint-cli2 reports MD018 for this line. Wrap the issue reference in code formatting or rewrite the sentence so validation has no warning.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 18-18: No space after hash on atx style heading
(MD018, no-missing-space-atx)
Source: Linters/SAST tools
An unrelated local pnpm run added @pnpm/exe as a packageManagerDependency with no package.json change, so CI's --frozen-lockfile install failed before any job ran.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — the refactor is sound and well-characterized by the new tests. Two doc/consistency notes below.
Reviewed changes
- Structured ingest into the hook server —
ingestStructuredStatus/dropStructuredStatusproject a structured session's summary into a normal status row under the renderer's pane key, stamped with the journal clock and astructuredHost: 'held' | 'owned'marker. - Status feed sink —
StructuredAgentSessionStatusFeedgains astatusSink(publish/forget), invoked on every publish, on ownership revocation, and on the newclosepath, with subscribers kept safe when the sink throws. worktree psreads the store — the #19217 structured adapter andliveSessionStatusSummariesare deleted; the store snapshot becomes the single source,structuredHostskips the connected-process gate, andstructuredHostOwnedfeeds the shared freshness rule.- Persistence hygiene — structured rows are skipped by the serializer and dropped on hydrate in both directions.
- Renderer deferral —
agentStatus:set/agentStatus:getSnapshotfilter structured rows so the renderer's feed bridge stays the sole writer for now. - Docs + wire —
docs/reference/agent-status-store.mdand the optionalstructuredHost/structuredHostOwnedfields under rule 1 of the wire doc.
ℹ️ Nitpicks
pnpm-lock.yamladds a@pnpm/exe@12.0.0packageManagerDependenciesentry in the root importer, unrelated to this refactor. Fine to land, but if it was just localpnpmself-registration it could be split out.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ Delta is clean — the new commit only removes the stray
@pnpm/exelockfile entry flagged in the prior review. No further issues on the incremental change.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
The design record claimed PR 1 deletes RuntimeAgentRowStore, drops the retained-versus-hook reconciliation, stamps terminalHandle on OSC rows, and tags rows with a source field of 'structured-host'. None of that is true of the shipped code: the retained store and its reconciliation are still in place, and the row field is structuredHost: 'held' | 'owned'. AGENTS.md points every future contributor here before they touch agent status, so split the roadmap into the 1a that landed and the 1b that has not, and name the fields the code actually writes.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor doc nit inline.
Reviewed changes
- Doc re-scoped to steps 1a/1b —
docs/reference/agent-status-store.mdnow splits step 1 into PR 1a (this PR) and a not-yet-landed PR 1b (retained-store deletion), moving theRuntimeAgentRowStore/terminalHandle/ reader-rewrite content under PR 1b and correcting the retained-versus-hook reconciliation claim that previously over-scoped the doc.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| what the chat shows. When the host revokes live ownership the row is re-set | ||
| without the flag; when the host closes or evicts the session the row is | ||
| dropped. Both already exist as feed events (`revokeLive` and the roster | ||
| filter in `liveSessionSummaries`); PR 1 turns them into store writes. |
There was a problem hiding this comment.
Two stale references after the 1a/1b split: PR 1 should be PR 1a (this section), and liveSessionSummaries is the feed method this PR deletes — the close/evict edge is now the new close/forget sink, not a roster filter.
A session dropped from the host's map without an explicit forget left its row in the store forever: `structuredHostOwned` bypasses the staleness check, so a failed re-attach (the Claude rewind path reaches one) stranded a permanently working agent in `worktree ps` and on mobile with no UI able to clear it. Deletion and forget are now one operation both callers route through.
`orcad` constructed its runtime with neither `getAgentStatusSnapshot` nor `structuredAgentStatusSink`, so once `worktree ps` sourced rows only from that snapshot the headless host published nowhere and listed nothing. The hook server's store is a module singleton whose import tree never reaches Electron, and its file paths come from `start()`, which orcad never calls.
`dropStructuredStatus` went through `clearPaneState`, which fans a pane clear out to the renderer for a pane key the renderer's own feed bridge still writes - so 'exactly one writer per pane key' held for writes and not for deletes. `dropStatusEntry` routes through the status-drop tap instead, and skips the resume-identity remnant: a structured session has no pane to resume into, and every null-status publish would otherwise re-mint one.
Neither the `agentStatus:getSnapshot` filter nor the main-window listener's had a single assertion, so deleting either — the first step of PR 2 — was green everywhere. Also covers the perf skip and the drop's lack of a renderer clear.
The sink JSDoc claimed only tests construct a host without one; `orcad` did. The doc argued a structured row needs no tab mirror 'because headless serve has no renderer', reasoning about exactly the topology the wiring had not reached. The deleted runtime adapter's warning that the pane key must be the DERIVED one - never a bearer handle or minted worker key - was lost with it.
Wiring the hook store into the orcad runtime added a production site that hands hook rows to a consumer, which the census ratchet pins deliberately.
There was a problem hiding this comment.
ℹ️ No new issues in the fixes — one doc nit the latest doc pass still left behind, inline.
Reviewed changes
- Paired session removal with the status-row forget —
forgetStructuredAgentSessionmakes deleting a session from the host's map and dropping its store row one operation, wired into both the failed-attach cleanup and the evictionforget-sessionstep. - Wired
orcadinto the store —orcad-entry.tsnow imports the in-memoryagentHookServerand passesgetAgentStatusSnapshot/structuredAgentStatusSink, so a headless host lists structured sessions inworktree pslike the desktop. - Dropped the row without a renderer clear —
dropStructuredStatusroutes throughdropStatusEntry(preserveResumeIdentity: false) instead ofclearPaneState, resolving the earlieragentStatus:clearasymmetry. - Pinned both half-migration filters — new tests cover
agentStatus:getSnapshot, the main-windowagentStatus:setlistener, and a source-scan wiring test across both entry points. - Corrected the doc —
agent-status-store.mdre-explains the persist skip, the fan-outs, and the orcad wiring obligation.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| what the chat shows. When the host revokes live ownership the row is re-set | ||
| without the flag; when the host closes or evicts the session the row is | ||
| dropped. Both already exist as feed events (`revokeLive` and the roster | ||
| filter in `liveSessionSummaries`); PR 1 turns them into store writes. |
There was a problem hiding this comment.
The 3a37982 doc pass still leaves two stale references on this line: PR 1 should be PR 1a (this doc ships with PR 1a; PR 1 spans the not-yet-landed 1b), and liveSessionSummaries is the accessor this PR deletes — the roster edge is now the sink's forget (forgetStructuredAgentSession), documented two lines below.
There was a problem hiding this comment.
ℹ️ No new issues in the merge — one doc nit from the prior reviews is still outstanding.
Reviewed changes
- Merged
origin/main— a clean merge pulling 57 unrelated files from other PRs (orchestration, Claude/Codex dispatch, native-chat task lists); none overlap this PR's 46 files. - No behavioral change — the structured ingest, status-feed sink,
worktree psread path, and persistence filters are unchanged from thee791513state already reviewed.pnpm tc:nodeand the focused structured-status suites pass.
The docs/reference/agent-status-store.md:108 nit flagged in the prior two reviews remains unaddressed: the sentence still reads PR 1 where the doc elsewhere uses PR 1a, and it still references liveSessionSummaries, the accessor this PR deletes (the close/evict edge is now forgetStructuredAgentSession, documented two lines below).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready to merge — CI green on
|
| # | Question | Verdict |
|---|---|---|
| 1 | Structured session shows a sidebar agent row | PASS |
| 2 | Settles to done, no stuck "working" | PASS |
| 3 | PTY agent row unaffected | PASS |
| 4 | Exactly one row per session | PASS |
| 5 | worktree ps lists it live, drops it after close |
PASS |
The decisive check is #4: probed live against both APIs while a structured session was open, agentStatus.getSnapshot() returned 0 structured rows while the renderer store held exactly 1 — the deliberate filter working as designed.
Baseline — idle workspace, no agent rows:
Q1 — structured Codex session mid-turn, one row, amber working indicator:
Q2 — same session settled to done:
Q3 — PTY Claude row live and updating through its turn:
Q4 — one structured + one PTY agent, exactly two rows, no duplicate:
Q5 — worktree ps --json with the structured row live (structuredHostOwned: true), then after closing the chat:
Validation performed
- Typecheck clean on node, cli and web at the merged head — and the first real typecheck this branch had at all, since CI never got past install.
orca-runtime.test.tsaggregator plus every new and touched suite, on the merged tree: 1303 passed, 1 skipped, 0 failed.check:code-quality:changed: 0 findings across 41 changed files, all three gates. Full CI: 23 passed, 0 failed.- Ablations, not assertions. Every new test was verified by reverting its production change and confirming the test fails — including multi-file arms so one lever cannot mask another. No pins. The phantom-row test was re-verified independently: with the fix reverted, the session leaves the roster while its row stays in the snapshot.
Architecture
Upheld, with one honest qualifier. The PR removes the failure mechanism rather than guarding it: a session-id-keyed broadcast cache that never reached the store is why worktree ps grew a private adapter and a hand-rolled freshness bypass, and both are now gone. No supersede warranted.
The qualifier: for structured rows this relocates a derivation rather than collapsing one — main and the renderer still derive the row independently from the same summary. The real win is that the CLI and mobile now read the same store, with the single writer in place for the follow-up. That is sound as staging, not as a steady state, so the follow-up should land close behind.
Known gaps, not blockers
- Validation was a dev build on macOS, local execution host, folder workspace, Codex-only structured lane. No packaged build, no git-worktree path, no interrupt/failure/restart paths.
worktree pshuman-readable output never prints agent rows (pre-existingformatWorktreePsbehavior, not a regression) — the JSON form is the real surface.- Mobile still ignores
structuredHostOwned, so an owned row decays to idle there. Pre-existing; an incomplete improvement rather than a regression. - Two follow-up items for the next PR: main and the renderer key structured panes differently (the renderer's tab id takes a
:history-Nsuffix on collision), which would yield two rows once the filter comes off; and a refused ingest never retries, because the feed's dedupe cache has already recorded the summary.
…atus-store # Conflicts: # src/main/native-chat/agent-session-wire/structured-agent-session-host.ts # src/main/native-chat/agent-session-wire/structured-agent-session-status-feed.test.ts # src/main/native-chat/agent-session-wire/structured-agent-session-status-feed.ts # src/main/runtime/orca-runtime-tests/worktree-ps-structured-host.spec.ts
There was a problem hiding this comment.
ℹ️ No new issues in this merge — the reconciliation against
origin/mainis clean.
Reviewed changes
- Merged
origin/main— the branch now sits on a newer main whosecreateStructuredAgentSessionHostStatusFeedtakes adepsthunk (threadingreadBackgroundTasks/fence/hasProviderChild); the PR'sstatusSinkis re-wired inside that thunk asstatusSink: () => args.deps().statusSink, preserving the same publish/forget behavior. - Updated the status-feed test helper —
feedForthreadsstatusSinkalongside main's newreadBackgroundTasksparam, keeping the sink-dedup and sink-throws assertions; the status-feed and forget-status suites pass (25 tests).
The docs/reference/agent-status-store.md:108 nit from the prior reviews is unchanged (stale PR 1 and the deleted liveSessionSummaries reference); it remains the sole item before a clean approval.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
ELI5
Orca kept three copies of "what is this agent doing" inside the main process. The hook server's copy is the one with authority and persistence, but structured (native chat) sessions never wrote to it, so
worktree psgrew its own adapter in #19217. Now the structured host writes into the hook server like every other agent, andworktree psjust reads the store.What Changed
ingestStructuredStatus/dropStructuredStatus. A structured session's projection lands as a normal status row under the pane key the renderer already derives, stamped with the journal clock and astructuredHost: 'held' | 'owned'marker.statusSinkand calls it on every publish, on ownership revocation, and (new) onforgetwhen the host closes the session. The runtime wires that sink to the hook server.worktree psreads structured rows from the hook snapshot. The separate structured adapter and the host'sliveSessionStatusSummariesaccessor are deleted. Rows carrystructuredHostOwnedso the shared freshness rule handles the aged-but-owned case; the hand-rolled bypass is gone.last-status.json, and one found on disk is dropped on hydrate.agentStatus:set/getSnapshot; the renderer's feed bridge still owns those rows until the next PR retires it. This keeps one writer per pane key.docs/reference/agent-status-store.mdrecords the single-store design and the two follow-up PRs. Linked from AGENTS.md.Why
The execution host should own agent status in one store, and every reader should subscribe to it. An audit found the hook server already is that store for every PTY-based agent on local, WSL, and SSH hosts: hook HTTP, the relays, and main's own OSC parse all converge on it, and it alone holds pane authority and persistence. The structured feed was the one producer bypassing it. Routing it through the store removes a reader-side adapter and makes the CLI, mobile, and (after PR 2) the sidebar read the same row.
Wire compatibility:
AgentStatusIpcPayload.structuredHostandRuntimeWorktreeAgentRow.structuredHostOwnedare new optional fields (rule 1).worktree pskeeps its shape and vocabulary.Linked Issue
Follow-up to #19217; design in
docs/reference/agent-status-store.md.Visual Proof
N/A. Main-process only; the desktop sidebar's rendering path is unchanged in this PR.
Testing
tscfor node, web, and CLI projects.Focused suites: hook-server structured ingest and persistence, status feed sink, worktree ps source admission and structured rows, the closed-chat liveness chain, the runtime call-site spec, and a wiring test that pins the sink through the
@ts-nocheckinstall path. Broader hook-server, runtime worktree, and structured-session suites re-run.pnpm run check:code-quality:changed.Automated tests added/updated
I manually tested these changes locally
Review