Repository navigation
fix(orcad): publish the headless graph and start the agent hook server - #17735
klaidliadon wants to merge 1 commit into
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 3dd8f20a117d3cf0d406bc91df731ffc475910f6 and b65bf8c. 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Orcad startup path now imports Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The headless graph is ready before RPC requests are served, so the reported startup readiness issue is addressed. No merge-blocking risk remains in this change. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation [
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: de82cbfb-52e1-4425-9964-bb82486979d7
📥 Commits
Reviewing files that changed from the base of the PR and between aa658d2 and 491f2dfaf49918829af645dcf990c3812d6a858f.
📒 Files selected for processing (1)
src/main/orcad/orcad-entry.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The agent-hook server starts, but the runtime is never wired to read hook status back, so hook-reported status — including the providerSession.transcriptPath native chat addresses transcripts by — still never reaches the paired client.
Reviewed changes
- Publish the headless graph —
runtime.syncWindowGraph(HEADLESS_RUNTIME_WINDOW_ID, { tabs: [], leaves: [] })mirrors the--servesequence (src/main/index.ts:3480), socaptureReadyGraphEpoch()stops throwingruntime_unavailableand the tab inventory epoch is established. - Start the agent hook server —
agentHookServer.start({ env: 'production', userDataPath })runs before the daemon, gated onisAgentStatusHooksEnabled, fail-open, with a pairedstop()in the shutdown closure.
⚠️ Hook status is written but never read back — agent chat still has no transcript
The syncWindowGraph half is complete and correct, but the hook-server half is only the write path. Starting the server means hooks can now POST and buildPtyEnv() returns real coordinates (the verified 6/6 ORCA_AGENT_HOOK_*). But OrcaRuntimeService in orcad-entry.ts is constructed with only getLocalProvider/getSshProvider/canRecoverPersistentLocalPtys/getDesktopWindowStatus. None of the agent-hook read seams are wired — no getAgentProviderSessionSnapshot, getAgentProviderSessionRowsForPane, getAgentStatusSnapshot, or onTerminalAgentStatus.
Consequently toMobileSessionTabsResult (src/main/runtime/orca-runtime.ts:36844) falls back to an empty getAgentProviderSessionSnapshotFn, so providerSession is never attached to session.tabs.listAll results, and the mobile chat still has no transcript address. The comment at orca-runtime.ts:37253 states this exact dependency: "only the hook payload does, and headless serve has no renderer to publish tab.agentStatus." The verification list in the PR body confirms the write path (env vars, endpoint file, terminal creation) but does not actually verify that a transcript reaches the chat view.
Technical details
# Wire the agent-hook read path into orcad's runtime
## Affected sites
- `src/main/orcad/orcad-entry.ts:203` — `new OrcaRuntimeService(store, undefined, { ... })` omits the agent-hook deps.
- `src/main/index.ts:2884-2929` — the `--serve` construction wires `onTerminalAgentStatus` → `agentHookServer.ingestTerminalStatus`, `getAgentStatusSnapshot`, `getAgentProviderSessionSnapshot`, `getAgentProviderSessionRowsForPane`, plus `attestAgentHookCompatibilityAuthority` / `retireAgentHookCompatibilityAuthority` / `reconcileAgentStatusForEndedProcess` / `buildAgentHookPtyEnv`.
- `src/main/runtime/orca-runtime.ts:36844-36862` — `getHookRowsForPane` reads only those two injected fns; both are null in orcad.
- `src/main/runtime/orca-runtime.ts:37253-37257` — "only the hook payload" carries `providerSession`, so native chat has no transcript without it.
## Required outcome
- `session.tabs.listAll` (and `terminal.agentStatus`/`worktree.ps` where relevant) must surface hook-reported status and `providerSession.transcriptPath` on an orcad host, so a paired client can address the transcript.
## Suggested approach (optional)
- Mirror `src/main/index.ts:2884-2929` in the orcad runtime construction: at minimum `getAgentProviderSessionSnapshot: () => agentHookServer.getStatusSnapshot()` and `getAgentProviderSessionRowsForPane: (paneKey) => agentHookServer.getStatusSnapshotForPane(paneKey)`; the OSC bridge (`onTerminalAgentStatus`) and compatibility-authority seams should follow if they are needed on this host.
- Extend the orcad verification to assert a `providerSession.transcriptPath` is present in `session.tabs.listAll` for a pane that has POSTed a hook.
## Open questions for the human (optional)
- Is hook status supposed to reach clients through a different orcad-specific channel this PR intentionally leaves for a follow-up?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The read path from the prior review is now wired, matching the desktop host byte-for-byte.
Reviewed changes
- Wired the agent-hook read seams —
orcad-entry.tsnow constructsOrcaRuntimeServicewithonTerminalAgentStatus,getAgentStatusSnapshot(filtered onproviderSessionOnly),getAgentProviderSessionSnapshot(unfiltered),getAgentProviderSessionRowsForPane,attestAgentHookCompatibilityAuthority,retireAgentHookCompatibilityAuthority, andreconcileAgentStatusForEndedProcess, a 1:1 copy ofsrc/main/index.ts:2884-2909. - Omitting
buildAgentHookPtyEnvis justified —src/main/ipc/pty/host-env/assembly.ts:121reads theagentHookServersingleton directly, so wiring a second source would be redundant. - Hardened the launch-failure path — armed
stopOrcadAgentHookServerimmediately after a successfulstart()so a later startup throw leaves the listener unbound; corrected the shutdown closure'sawait agentHookServer.stop()to the now-voidagentHookServer.stop(). - Pinned the wiring in a source-scan test —
orcad-headless-startup-wiring.test.tsfollows the existingsrc/main/startup/*.test.tspattern and asserts graph-sentinel ordering, hook-server-before-daemon ordering, presence of all seven read seams, and the teardown/failure stop wiring.
ℹ️ Transcript delivery is pinned statically, not behaviorally verified
The read seams are correct, and the source-pin test will catch a regression that removes them, but nothing yet exercises the end-to-end claim — a POSTed hook's providerSession.transcriptPath surfacing in session.tabs.listAll (or terminal.agentStatus). The PR body's Verification section predates the read-path commits and covers only the write path (env vars, endpoint file, terminal creation). A follow-up that POSTs a hook in a scratch data root and asserts a transcript address in session.tabs.listAll would close the loop; not blocking, since the wiring that enables it is verified correct.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 62a33194-cd88-415e-9459-a094ca674eb0
📥 Commits
Reviewing files that changed from the base of the PR and between 491f2dfaf49918829af645dcf990c3812d6a858f and 6328d76e1be199831ec7e6e567c2c3fe1a741692.
📒 Files selected for processing (2)
src/main/orcad/orcad-entry.tssrc/main/orcad/orcad-headless-startup-wiring.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
|
Pushed Taken — CodeRabbit: "Stop the hook server when daemon teardown fails"Correct, and the mechanism checks out. I fixed it at the source rather than guarding the one line, because the hook server is not the worst thing the flat sequence drops. The steps after A daemon rejection skipped all five, and the last one is Second await, same class: The launch-failure path in Pinned in Declined — Greptile P1: "Retained PTYs Lack Hook Endpoint"The mechanism is right. But there is no fix, and no regression:
So: accurate observation, one-time transient, unfixable in this direction. Pullfrog: "pinned statically, not behaviorally verified"Agreed, and the PR body already says something stronger — see the " That section is deliberate and is the next person's starting point — please don't trim it in review. The wiring here is correct and required either way; I'd rather land it and say so plainly than imply chat is fixed. Gates
|
There was a problem hiding this comment.
ℹ️ No critical issues — one minor comment-accuracy note inline.
Reviewed changes
- Reworked the launch-failure path —
browserProvider.stop()instartOrcad's catch is now guarded, so a teardown rejection can't skipinstanceLock.release()or mask the original startup error. - Nested the shutdown teardown —
stop()now runs each teardown step even when a priorawaitrejects, guaranteeing the instance lock is released. - Pinned the structure in a test — the new test asserts the
try/finallynesting and the guarded failure path.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Pushed Taken — Pullfrog: the "first failure" claim is invertedCorrect, and I got this wrong in both the code comment and my previous reply here. A throw from a try { throw new Error('A-daemon') }
finally {
try { throw new Error('B-browser') }
finally { console.log('lock released: yes') }
}
// lock released: yes
// propagated: B-browserSo if the daemon and the browser provider both reject, the caller sees the browser's error and the daemon's is lost. I've reworded the comment to state the guarantee the code actually provides — every step runs — and to say plainly which error survives. I did not add error aggregation to preserve both: the lock is released either way, and a lost second error costs a log line where a skipped release costs the next launch. That is the trade worth making, but it should be written down rather than papered over with a wrong claim, which is what the comment was doing. Comment-only change; no behaviour difference from Declined — CodeRabbit "Linked Issues check": fix the provider-session merge in this PRThe warning reads: "the description confirms that session tabs still lack provider-session data, so the agent chat objective is not yet fully met", resolving to "investigate and fix the downstream provider-session merge... then add behavioral coverage." Not in this PR, for three reasons:
The behavioral coverage the check asks for would currently fail — that is the point of the caveat. Adding a test that asserts a transcript reaches the chat view would be asserting a thing that does not happen yet. Not addressed — Docstring Coverage (33.33%)Unchanged by my commits: it read 33.33% across the same 3 functions before them. This file's convention is dense inline Still open — Greptile: retained PTYsGreptile's updated summary now states the mechanism itself: "daemon adoption reuses existing processes whose environments cannot be mutated." That is the reason it cannot be fixed from this direction, and it is the same point I answered above — one-time transient affecting only PTYs spawned before the feature existed, draining as terminals are recreated, against a baseline where no PTY had hook coordinates. |
|
Independent reproduction of #17846 on current code, and confirmation that the graph publication in this PR is the fix for the mobile terminal path. Setup: orcad built from the Unpatched Same build plus only the graph line from this PR ( A terminal created before two orcad restarts still round-tripped afterwards (daemon adopted, same pid), so the graph publication does not disturb daemon adoption. Two notes that may help this land:
|
3dd8f20 to
1a18599
Compare
orcad never publishes its window layout. The runtime treats that layout as the thing it places work into, so until something publishes one the runtime reports itself unavailable and refuses everything that depends on it. Creating a terminal is refused before it reaches any terminal code, and listing tabs waits on a publication that can never arrive. The daemon is healthy throughout, it is simply never asked. This publishes the same empty placeholder the Electron serve path publishes, after terminal recovery so the layout is adopted against a settled runtime, and before the transport binds so no client can observe the unavailable state. Fixes stablyai#17846 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PHbhxM6H9evtLR95vUGs4G
1a18599 to
b65bf8c
Compare
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Published the headless graph —
runtime.syncWindowGraph(HEADLESS_RUNTIME_WINDOW_ID, { tabs: [], leaves: [] })now runs afterreconcileLegacyWorkerTerminalsand before the RPC server binds (orcad-entry.ts:297), mirroringmain-process-runtime-launch.ts:160sographStatusreaches'ready'and the graph-gated terminal-create / tab-inventory RPCs unblock. - Pinned the ordering in a source-scan test —
headless-pty-hydration-ordering.test.ts:56-68asserts the sentinel sits afterregisterHeadlessPtyRuntimeand beforenew OrcaRuntimeRpcServer(.
ℹ️ The description no longer matches the diff
The PR body narrates five commits — the agent-hook server start, the seven read seams, the failure/teardown hardening — but the diff is now a single commit adding only the graph publication. That other work is already on main (verified against origin/main's orcad-entry.ts), so nothing is missing from the host; it is the description that overstates the scope. Worth narrowing before merge so a reviewer does not approve expecting the hook-server wiring to land here.
Technical details
# Align the PR description with the diff
## Affected sites
- `src/main/orcad/orcad-entry.ts:295-297` — the syncWindowGraph call is the only production change in the diff.
- `src/main/startup/headless-pty-hydration-ordering.test.ts:56-68` — the only test change.
- `origin/main:src/main/orcad/orcad-entry.ts` — already contains `agentHookServer.start(...)`, all seven read seams, `buildAgentHookPtyEnv`, and the nested teardown.
## Required outcome
- The description's "What Changed" and "Known limitations" reflect what this PR actually ships (the graph publication), or the broader work is explicitly marked as already merged to `main`.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
3 participants
ELI5
orcadskips three startup steps thatorca serveperforms. Because of that itcannot create a terminal at all, and its agent chat view renders nothing. Each
fix is one small piece of wiring that
--servealready had.What Changed
Five commits in
src/main/orcad/orcad-entry.ts, plus a test that pins the wiring.1. Publish the headless graph
orcad never called
runtime.syncWindowGraph(HEADLESS_RUNTIME_WINDOW_ID, …).--servedoes, with a comment naming this exact hazard — "headless servers haveno renderer graph publisher; publish an explicit empty graph so status clients see
a ready server." orcad copies the surrounding sequence
(
rehydrateClientHostedBrowserPages,refreshRestoredOrchestrationAuthority,reconcileLegacyWorkerTerminals) and omits that one line, sographStatusneverleaves
'unavailable'.Two failures, one cause, and neither names the graph:
session.tabs.createTerminalreturnsruntime_unavailable. The throw isassertGraphReady()viacaptureReadyGraphEpoch(), the first statement ofrunCreateMobileSessionTerminal— above the authoritative-window check and farabove any PTY code. A live daemon and an installed
ptyController.spawnmake nodifference because neither is reached.
session.tabs.listAllhangs.getAuthoritativeSessionTabsInventoryEpoch()returns non-null only when
graphStatus === 'ready', so thewhile (true)inlistAllMobileSessionTabsInventorywaits on a publication thatmarkSessionTabsInventoryPublishedearly-returns out of once the epochs match.No timeout, no error, no log — an RPC that never answers.
One call opens both gates: it adopts
HEADLESS_RUNTIME_WINDOW_IDasauthoritative, marks the graph ready, and publishes the tab inventory.
2. Start the agent hook server
Native chat reads the agent's transcript file, and
session-file-resolver.tsnamesthe source: "Authoritative transcript path reported by the agent hook
(
providerSession.transcriptPath)".The spawn path was already correct —
host-env/assembly.tsfoldsagentHookServer.buildPtyEnv()into every PTY's environment — butbuildPtyEnv()opens with
if (this.port <= 0 || !this.token) return {}, and with no server theport is 0. Measured on the live agent process before the change: eight
ORCA_*vars present, and zero
ORCA_AGENT_HOOK_*. Soclaude-hook.shreturned at itsfirst line on every invocation.
userDataPathis load-bearing, not incidental:ORCA_AGENT_HOOK_ENDPOINTis setonly when the endpoint file was written, and it is written only when
start()received that path. It matters more here than on the desktop, because
start()regenerates token and port every launch while orcad's terminal daemon deliberately
outlives the runtime — so a surviving PTY carries stale coordinates that the hook
repairs only by sourcing the endpoint file at invocation time.
3. Wire the hook read seams
Starting the server only opened the write path. The runtime was constructed
with none of the agent-hook getters the desktop host passes, so every row the
server collected reached nothing:
getHookRowsForPaneanswered[]for each pane,published tabs carried no
providerSession.transcriptPath, andhasProviderSessionObservationSource()answered false — which makes structuredClaude resume refuse for want of a fresh provider session.
The filter asymmetry is deliberate, not cosmetic:
getAgentStatusSnapshothidesresume-identity rows from live-agent views while
getAgentProviderSessionSnapshotkeeps them, because those rows carry the provider session native chat addresses
transcripts by. Taking one without the other fixes chat and breaks the live-agent
rows.
buildAgentHookPtyEnvstays unwired on purpose —host-env/assembly.tsreads thesame singleton directly, so passing it here would add a second source for env this
host already gets.
Also stops the server when a launch fails:
startOrcad's failure path undoes thebrowser provider and the instance lock but never imported the hook server, so a
step throwing after
start()left the listener bound — andstart()returns earlywhile
this.serveris set, handing the next attempt in the same process a staletoken and endpoint file.
4. Pin the wiring in a test
So a future refactor cannot drop these steps silently, which is how all three went
missing in the first place.
5. Finish teardown when a step rejects
Added in review. The shutdown closure ran its teardown steps as a flat sequence, so
the first rejection skipped every later one — and both awaits in it genuinely
reject.
stopOrcadDaemon()reachesdisconnectOnly(), which fans out withPromise.allover every adapter and awaits an unguarded checkpoint write;browserProvider.stop()ends inrm(..., { force: true }), andforceforgivesonly ENOENT.
The step furthest down that sequence is
instanceLock.release(). Leaking the lockfile makes the next orcad launch refuse to start until someone deletes it by
hand, so a transient teardown fault became a dead host.
agentHookServer.stop()was dropped the same way, which is what review caught; the leaked lock is the same
bug with a worse blast radius, so both are fixed at the source.
startOrcad's launch-failure path had a third form: a rejectingbrowserProvider.stop()there replaced the error explaining why startup failed andskipped the lock release, so a launch that failed once could not be retried. It now
catches and logs that, and rethrows the original.
This is stated deliberately rather than left for the next person to rediscover.
With this deployed on a Synology NAS, the hook server holds rows for both live
panes with
transcriptPathpopulated — one received 22:25:03, one 22:35:53, bothafter the restart — and
session.tabs.listAllstill reports noproviderSessionon any tab. So the input is wired and something downstream ofit still declines.
The prime suspect is the merge's own recency gate:
While the accessors were missing, this was unreachable — the
&&short-circuitedon an undefined
hookAgentStatus. Wiring them makes it live for the firsttime, and agent status updates arrive from terminal parsing independently of hook
delivery, so a status update landing after the hook inverts that comparison
permanently.
That is a hypothesis from reading the condition, not a measured result, and it
wants an instrumented build to settle. I'd rather land the wiring — which is
correct and required either way — and flag this honestly than imply chat is fixed.
There is also a diagnosis worth recording: this third one is not a missing startup
step but a missing constructor dependency, so a shared startup sequence would
not have caught it. A shared factory that builds the runtime with its full deps
would.
Known limitations
A PTY retained across the upgrade that introduces this commit stays hookless.
ORCA_AGENT_HOOK_*reaches a terminal only through its spawn-time environment, anda running process's environment is frozen. So a PTY the daemon carries across this
upgrade has no vars at all —
claude-hook.shreturns at its first line, with noendpoint path to source. It self-heals when that terminal is recreated.
This is distinct from the stale coordinates case the endpoint file does solve: a
PTY spawned by a previous orcad that already had the hook server has wrong port and
token in its frozen env, and recovers because the hook re-reads the endpoint file at
invocation. That is why
userDataPathis load-bearing here.Closing the first case would need the hook to locate the endpoint without an env
pointer — a fixed well-known path — which is a different trust model and wants its
own design rather than a rider on this commit. Raised by Greptile in review.
Linked Issue
Fixes #17846
Visual Proof
N/A— headless runtime, no UI in this process. The client-visible effect is anRPC that answers instead of hanging; before/after RPC output is in Testing below.
Testing
Rebased onto
mainand rebuilt before submitting, rather than assuming the branchit was developed on still applied. On a scratch data root:
The daemon line is the one that matters for the first commit: before it, the
terminal daemon never received a create request at all, from any client, on any
attempt. It was healthy throughout and simply never asked.
Platforms: Linux (Synology DSM), as a real system-scope systemd service with a
paired iOS client. orcad is a headless Linux/Unix target; no macOS or Windows
surface.
Not run in this environment:
tscandoxlint— neither is installable on thehost this was developed and verified on. CI is the authority for those.
AI Disclosure
Claude Opus 5 wrote the commits and this description. I reviewed them and ran all
the NAS verification above.
Review
additions with no platform branch. File modes on the agent-hooks directory (0700
/ 0600) follow the existing serve-path behaviour.
reached over an SSH port-forward by a paired client. Verified in exactly that
configuration.
filter asymmetry is preserved exactly as the desktop host has it, so no provider
is treated specially here.
accessors are pass-through reads over an in-memory snapshot.
in a 0700 directory, matching
--serve.endpointNamespaceis deliberately notset — it is dev-worktree only, and namespacing would break endpoint-file
stability across the restarts this host exists to survive. Fail-open on start,
matching
--serve.cannot create a terminal, so there is no working behaviour to regress.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
The caveat above is the next person's starting point; please don't let it get lost
in the merge.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)