Repository navigation
fix(zcode): wait for composer before first worker dispatch #23374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -188,10 +188,17 @@ export async function startLocalWorker(args: { | |||||||||||||||||||||||||||
| effects, | ||||||||||||||||||||||||||||
| timeoutMs: params.timeoutMs ?? 60_000 | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| : await runtime.waitForTerminal(terminalHandle, { | ||||||||||||||||||||||||||||
| condition: 'tui-idle', | ||||||||||||||||||||||||||||
| timeoutMs: params.timeoutMs ?? 60_000 | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| : // ZCode emits SessionStart only after input; its first dispatch must wait for the composer. | ||||||||||||||||||||||||||||
| agent === 'zcode' && !params.terminal | ||||||||||||||||||||||||||||
| ? await runtime.waitForFreshWorkerComposer( | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The zcode branch awaits 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. |
||||||||||||||||||||||||||||
| terminalHandle, | ||||||||||||||||||||||||||||
| agent, | ||||||||||||||||||||||||||||
| params.timeoutMs ?? 60_000 | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
Comment on lines
+193
to
+197
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ 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, Suggested fix await runtime.waitForFreshWorkerComposer(
terminalHandle,
agent,
params.timeoutMs ?? 60_000
- )
+ ).then(() => ({
+ satisfied: true,
+ status: 'composer_ready'
+ }))📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
| : await runtime.waitForTerminal(terminalHandle, { | ||||||||||||||||||||||||||||
| condition: 'tui-idle', | ||||||||||||||||||||||||||||
| timeoutMs: params.timeoutMs ?? 60_000 | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| if (wait) { | ||||||||||||||||||||||||||||
| persistWorkerSetupWaitOutcome({ ...setupStage, wait }) | ||||||||||||||||||||||||||||
| if (!wait.satisfied) { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
| import { createOrchestrationWorkerReleaseHarness } from './worker-release.test-support' | ||
|
|
||
| describe('ZCode first dispatch readiness', () => { | ||
| const h = createOrchestrationWorkerReleaseHarness() | ||
| afterEach(() => h.cleanup()) | ||
|
|
||
| it('waits for the new composer before delivering exactly one dispatch', async () => { | ||
| h.setup() | ||
| const gate = h.deferred<void>() | ||
| vi.spyOn(h.runtime, 'waitForFreshWorkerComposer').mockReturnValue(gate.promise) | ||
| const pending = h.startWorker({ agent: 'zcode' }) | ||
| await vi.waitFor(() => | ||
| expect(h.runtime.waitForFreshWorkerComposer).toHaveBeenCalledWith( | ||
| 'term_worker', | ||
| 'zcode', | ||
| 60_000 | ||
| ) | ||
| ) | ||
| expect(h.runtime.waitForTerminal).not.toHaveBeenCalled() | ||
| expect(h.runtime.sendTerminalAgentPrompt).not.toHaveBeenCalled() | ||
| gate.resolve() | ||
| await pending | ||
| expect(h.runtime.sendTerminalAgentPrompt).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| it('keeps reused terminals on the normal idle wait', async () => { | ||
| h.setup() | ||
| vi.spyOn(h.runtime, 'waitForFreshWorkerComposer') | ||
| await h.startWorker({ terminal: 'term_worker' }) | ||
| expect(h.runtime.waitForFreshWorkerComposer).not.toHaveBeenCalled() | ||
| expect(h.runtime.waitForTerminal).toHaveBeenCalledWith( | ||
| 'term_worker', | ||
| expect.objectContaining({ condition: 'tui-idle' }) | ||
| ) | ||
| }) | ||
|
|
||
| it('never delivers a task after a startup timeout', async () => { | ||
| h.setup() | ||
| vi.spyOn(h.runtime, 'waitForFreshWorkerComposer').mockRejectedValue(new Error('timeout')) | ||
| await expect(h.startWorker({ agent: 'zcode' })).rejects.toThrow('Expected worker-start') | ||
| expect(h.runtime.sendTerminalAgentPrompt).not.toHaveBeenCalled() | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| import { readFileSync } from 'node:fs' | ||
| import { join } from 'node:path' | ||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
| import { | ||
| waitForWorktreeStartupDraft, | ||
| type WorktreeStartupReadinessHost | ||
| } from './runtime-worktree-startup-readiness' | ||
|
|
||
| describe('fresh worker composer readiness', () => { | ||
| afterEach(() => vi.useRealTimers()) | ||
|
|
||
| function fixture(replay?: string) { | ||
| let listener = (_data: string): void => {} | ||
| const unsubscribe = vi.fn() | ||
| const host: WorktreeStartupReadinessHost = { | ||
| getPtyId: () => 'pty-1', | ||
| getForegroundProcess: async () => 'zcode', | ||
| subscribeToData: (_ptyId, onData) => { | ||
| listener = onData | ||
| return unsubscribe | ||
| }, | ||
| readRecentOutput: () => replay, | ||
| write: vi.fn() | ||
| } | ||
| return { host, emit: (data: string) => listener(data), unsubscribe } | ||
| } | ||
|
|
||
| it('accepts the captured composer while the banner continues repainting', async () => { | ||
| vi.useFakeTimers() | ||
| const h = fixture() | ||
| const pending = waitForWorktreeStartupDraft(h.host, 'term-1', 'zcode', { | ||
| timeoutMs: 45_000, | ||
| requireComposerMarker: true | ||
| }) | ||
| const data = readFileSync(join(__dirname, '__fixtures__', 'zcode-composer-ready.txt'), 'utf8') | ||
| for (let offset = 0; offset < data.length; offset += 4096) { | ||
| h.emit(data.slice(offset, offset + 4096)) | ||
| } | ||
| await expect(pending).resolves.toBe('pty-1') | ||
| expect(h.unsubscribe).toHaveBeenCalledOnce() | ||
| expect(h.host.write).not.toHaveBeenCalled() | ||
| expect(vi.getTimerCount()).toBe(0) | ||
| }) | ||
|
|
||
| it('cleans up the deadline when the composer was already captured', async () => { | ||
| vi.useFakeTimers() | ||
| const h = fixture('\x1b[?1049h╭') | ||
| await expect( | ||
| waitForWorktreeStartupDraft(h.host, 'term-1', 'zcode', { | ||
| timeoutMs: 45_000, | ||
| requireComposerMarker: true | ||
| }) | ||
| ).resolves.toBe('pty-1') | ||
| expect(h.unsubscribe).toHaveBeenCalledOnce() | ||
| expect(vi.getTimerCount()).toBe(0) | ||
| }) | ||
|
|
||
| it('does not accept shell decoration or a square startup dialog', async () => { | ||
| vi.useFakeTimers() | ||
| const h = fixture('╭ shell\n\x1b[?1049h\x1b[?2004h┌ Sign in ┐') | ||
| const pending = waitForWorktreeStartupDraft(h.host, 'term-1', 'zcode', { | ||
| timeoutMs: 45_000, | ||
| requireComposerMarker: true | ||
| }) | ||
| await vi.advanceTimersByTimeAsync(44_999) | ||
| expect(h.unsubscribe).not.toHaveBeenCalled() | ||
| await vi.advanceTimersByTimeAsync(1) | ||
| await expect(pending).resolves.toBeNull() | ||
| expect(h.unsubscribe).toHaveBeenCalledOnce() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike the replaced
waitForTerminal('tui-idle'),waitForWorktreeStartupDrafthas 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 generictimeout, where the old wait rejected immediately withterminal_exited.Technical details