Repository navigation
fix(zcode): wait for composer before first worker dispatch - #23374
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughFresh ZCode workers without an explicit terminal now wait for a composer marker before dispatch. Reused terminals continue to use the Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Fresh ZCode workers can receive their first task while setup remains recorded as running. Settle the setup outcome before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to A fresh ZCode worker may receive its task while a required worktree setup is still recorded as running. Terminal identity checks remain in place, but the setup transition needs resolution before the changed startup path can be treated as equivalent to the prior path. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: Advanced
Run ID: f7c2a267-cf93-449f-8a47-cde8a5735474
📒 Files selected for processing (6)
src/main/runtime/orca-runtime-activate-managed-worktree.tssrc/main/runtime/rpc/methods/orchestration/worker/local-worker-start.tssrc/main/runtime/rpc/methods/orchestration/worker/zcode-worker-readiness.test.tssrc/main/runtime/runtime-worktree-startup-readiness.test.tssrc/main/runtime/runtime-worktree-startup-readiness.tssrc/main/runtime/zcode-readiness-transcript.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| ? await runtime.waitForFreshWorkerComposer( | ||
| terminalHandle, | ||
| agent, | ||
| params.timeoutMs ?? 60_000 | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,140p' src/main/runtime/rpc/methods/orchestration/worker/worker-setup-gate.ts
sed -n '140,235p' src/main/runtime/rpc/methods/orchestration/worker/local-worker-start.ts
rg -n 'composer_ready|type TerminalWait|interface TerminalWait|wait-for-setup|setup_settled' src/main/runtime/rpc/methods/orchestration/worker src/sharedRepository: stablyai/orca
Length of output: 9125
🏁 Script executed:
sed -n '1,190p' src/main/runtime/rpc/methods/orchestration/worker/worker-topology.ts
sed -n '1,120p' src/main/runtime/rpc/methods/orchestration/worker/worker-start-structured-setup-gate.ts
sed -n '180,235p' src/main/runtime/orca-runtime-activate-managed-worktree.ts
rg -n -C 5 'waitForFreshWorkerComposer|composer_ready|applyWaitForSetupOutcome|WaitResult|TerminalWait|status:' src/main/runtime src/shared
sed -n '340,470p' src/main/runtime/rpc/methods/orchestration/worker/workers-new-worktree.test.tsRepository: stablyai/orca
Length of output: 45658
🏁 Script executed:
rg -n -C 8 'waitForWorktreeStartupDraft|setup runner|agent-first|startupPolicy|agent === .zcode.|requireComposerMarker|SessionStart' src/main/runtime/rpc/methods/orchestration/worker src/main/runtime src/shared
fd -i 'worker-worktree-creation' src
rg -n -C 10 'zcode|composer|wait-for-setup' src/main/runtime/rpc/methods/orchestration/worker/*test.ts src/main/runtime/*test.tsRepository: stablyai/orca
Length of output: 45665
Record ZCode composer readiness as the setup wait outcome.
For a fresh non-structured ZCode worker, waitForFreshWorkerComposer returns void. The if (wait) guard therefore skips persistWorkerSetupWaitOutcome, leaving a running wait-for-setup receipt and stage unsettled. PTY startup runs behind the setup runner, so composer readiness is sufficient for this setup gate. Return a satisfied wait result with status: 'composer_ready'.
Suggested fix
await runtime.waitForFreshWorkerComposer(
terminalHandle,
agent,
params.timeoutMs ?? 60_000
- )
+ ).then(() => ({
+ satisfied: true,
+ status: 'composer_ready'
+ }))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ? await runtime.waitForFreshWorkerComposer( | |
| terminalHandle, | |
| agent, | |
| params.timeoutMs ?? 60_000 | |
| ) | |
| ? await runtime.waitForFreshWorkerComposer( | |
| terminalHandle, | |
| agent, | |
| params.timeoutMs ?? 60_000 | |
| ).then(() => ({ | |
| satisfied: true, | |
| status: 'composer_ready' | |
| })) |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The zcode first-dispatch branch skips setup-wait settlement and drops the PTY-exit signal from the wait it replaced. Details inline.
Reviewed changes
- zcode first-dispatch gate — a freshly launched zcode terminal worker (
agent === 'zcode' && !params.terminal) now waits onwaitForFreshWorkerComposer(composer-marker scan) instead ofwaitForTerminal('tui-idle'); reused terminals, other agents, and structured workers keep their existing paths. - Composer scanner options —
waitForWorktreeStartupDraftgainstimeoutMs/requireComposerMarker, disables the quiet-timer fallback under the strict marker, adds a settled guard inobserve, and registers the deadline before replaying buffered output. - Regressions added — captured-composer acceptance, negative startup-dialog evidence, replay-timer cleanup, dispatch ordering/timeout, terminal reuse, and renderer adoption.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| }) | ||
| : // ZCode emits SessionStart only after input; its first dispatch must wait for the composer. | ||
| agent === 'zcode' && !params.terminal | ||
| ? await runtime.waitForFreshWorkerComposer( |
There was a problem hiding this comment.
The zcode branch awaits waitForFreshWorkerComposer, whose return type is Promise<void>, so wait is undefined and the if (wait) block below (line 202) is skipped. On a --worktree new-child zcode start where the repo policy is wait-for-setup, persistWorkerSetupWaitOutcome / applyWaitForSetupOutcome never run: the setup receipt stays running while the dispatch is reported ready, no setup_settled stage is recorded, and a setup that ran and failed surfaces as an agent_readiness timeout rather than a setup failure.
Technical details
# Settle the setup wait outcome on the zcode branch
## Affected sites
- `src/main/runtime/rpc/methods/orchestration/worker/local-worker-start.ts:193` — branch result is `void`, so `if (wait)` at `:202` never runs
- `src/main/runtime/orca-runtime-activate-managed-worktree.ts:208` — `waitForFreshWorkerComposer(...): Promise<void>`
- `src/main/runtime/rpc/methods/orchestration/worker/worker-setup-gate.ts:53` — `persistWorkerSetupWaitOutcome` → `applyWaitForSetupOutcome`
- `src/main/runtime/rpc/methods/orchestration/worker/worker-topology.ts:130` — the only local path that flips a `wait-for-setup` receipt `running` → `succeeded`
## Required outcome
- A successful composer wait on a `wait-for-setup` worktree must settle the setup receipt (`state: 'succeeded'`) and record the `setup_settled` stage before task input is delivered, matching the invariant asserted for the non-zcode path at `workers-new-worktree.test.ts:369`.
## Suggested approach
- Return a satisfied wait result (e.g. `{ satisfied: true, status: 'running' }`) from `waitForFreshWorkerComposer` so the existing `if (wait)` block applies, or call `persistWorkerSetupWaitOutcome({ ...setupStage, wait: { satisfied: true, status: 'running' } })` on the zcode success path.
## Open question
- `monitorWorkerSetup` bails for `wait-for-setup`, so nothing reconciles the receipt later; confirm the receipt is intended to stay `running` if this branch is left as-is.| ): Promise<void> { | ||
| const initialPtyId = | ||
| this.getLivePtyForHandle(handle)?.pty.ptyId ?? this.getLiveLeafForHandle(handle).leaf.ptyId | ||
| const ptyId = await waitForWorktreeStartupDraft( |
There was a problem hiding this comment.
Unlike the replaced waitForTerminal('tui-idle'), waitForWorktreeStartupDraft has no exit/disconnect subscription — it settles only on the composer marker or the hard timer. A zcode worker that crashes or loses its PTY before painting ╭ therefore stalls for the full worker-start timeout (60s default) before failing with a generic timeout, where the old wait rejected immediately with terminal_exited.
Technical details
# Detect terminal exit while waiting for the composer
## Affected sites
- `src/main/runtime/orca-runtime-activate-managed-worktree.ts:215` — delegates to the composer wait with no exit signal
- `src/main/runtime/runtime-worktree-startup-readiness.ts:137` — the hard timer is the only failure backstop under `requireComposerMarker`
- `src/main/runtime/orca-runtime-resolve-exit-waiters.ts:29` — the `terminal_exited` rejection the old `tui-idle` wait relied on
## Required outcome
- A zcode launch whose PTY exits before the composer appears should fail promptly, not after `timeoutMs`.
## Suggested approach
- Race the composer wait against a pty-exit/disconnect signal (or re-check liveness on the existing data stream) and settle `null`/throw when the terminal dies.
## Open question
- If the agent process exits but the shell PTY survives, the old wait also ran to timeout — confirm the regression is limited to the PTY-exit/disconnect case and decide whether the latency is an accepted trade-off.
|
@Alex-wangyang this is a very good catch and a very good fix. Your diagnosis is exactly right, and it's a gap I left. I'm the author of the ZCode harness this builds on, and I want to be plain about the hole: I proved the negative half of this and then didn't follow it through. What I like about the fix:
Verified locally on your branch: One coordination note — please read before anyone merges: #23452 (@brennanb2025) is solving the general form of this. It lets an agent declare how its composer signals readiness, backed by a recorded screen, and it's driven by goose. You overlap on three files, including My read: yours should land first. It's non-draft, narrow, and fixes a bug in a released version today; #23452 is a 56-file draft. When that lands, the natural follow-up is for ZCode to become a declaration entry and this special case to disappear. Flagging it on that PR too so it isn't a surprise to either of you. Thanks for the packaged-app canaries and the before/after receipts — |
|
Closing and immediately reopening to trigger the full CI matrix — only |
|
Full CI matrix is green (31/31) now that the fork workflows ran. Security review: no network, exec, filesystem, env, or credential access — every added line is readiness logic, with Merging. Thanks again @Alex-wangyang — this closed a real gap in the harness I shipped, and the fact that you extended my transcript test rather than loosening it is exactly the right instinct. |
2 participants
ELI5
A fresh ZCode worker can show its input box while Orca still waits for it to become idle, so the first task never arrives. Use the ZCode composer signal Orca already recognizes to deliver that first task once the input box is ready.
What Changed
Why
The shipped ZCode configuration already has
zcode-composer-prompt, but terminal worker-start still waits for generictui-idle. ZCode continually redraws its banner, and the idle text tail loses the composer evidence. In the tested ZCode version, SessionStart hooks run on the first turn, after input, so those hooks cannot unblock this first dispatch.Reusing the existing scanner connects the native readiness mechanism to worker-start without another status producer, fake terminal titles, external launcher, permission override, or changes to ZCode itself. This is a narrow fix for the released interactive path; the broader client/transcript work in #16228 is separate.
Linked Issue
Fixes #23373
Visual Proof
No visual change: no renderer or TUI drawing code changes. The observable change is automated CLI task delivery. Before/after receipt excerpts from actual runs:
The after receipt alone is not used as completion evidence: two actual packaged-app canaries also wrote/read back the exact requested file, sent
worker_done, and released their terminal resources. No title manipulation or external starter was used.Testing
runtime-worktree-startup-readiness.test.ts,worker/zcode-worker-readiness.test.ts,zcode-readiness-transcript.test.ts,draft-paste-ready-scanner.test.ts,runtime-local-worktree-terminal-startup.test.ts, andworker/worker-terminal-custody-at-creation.test.ts.node node_modules/typescript/bin/tsc --noEmit -p config/tsconfig.node.json.git diff --checkpassed.AI Disclosure
Implemented and reviewed with OpenAI Codex (GPT-6).
Review
agent === 'zcode'; other agents, explicit terminal reuse, setup sequencing, and structured worker gates keep their existing paths.Agent skill upstream boundary
Author
GitHub: @Alex-wangyang. X/Twitter handle: not provided.
Checklist