Repository navigation
Conversation
📝 WalkthroughWalkthroughThe update adds bounded orchestration-agent readiness checks, abort propagation, strict TUI-idle gating, guarded prompt delivery, deterministic terminal identities, and unknown-outcome tracking. Worker and federated startup flows now record committed terminals and worktrees through staged effects and reconciliation helpers. Runtime queues, remote calls, PTY probes, and environment transport support cancellation. Database transitions distinguish retryable pre-input failures from blocked unknown outcomes. Tests and reliability gates cover readiness, cancellation, fencing, SSH reconciliation, residual resources, and finalization failures. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
905f540 to
355f99b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
src/main/runtime/rpc/methods/orchestration-workers-new-worktree.test.ts (1)
637-653: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate the abort on the provisioning call, not on task creation.
startWorkercreates the task synchronously (Line 77) before invoking the handler, sowaitFor(() => db.listTasks()).toHaveLength(1)is satisfied almost immediately andcontroller.abort()can land before the handler reachescreateManagedWorktree. Waiting on the mocked provisioning call makes theworktree_createstage assertion deterministic.♻️ Proposed tightening of the wait gate
- const pending = startWorker({ name: 'cancelled-create' }, controller.signal) - await vi.waitFor(() => expect(db.listTasks()).toHaveLength(1)) + const createManagedWorktree = vi.mocked(runtime.createManagedWorktree) + const pending = startWorker({ name: 'cancelled-create' }, controller.signal) + await vi.waitFor(() => expect(createManagedWorktree).toHaveBeenCalledOnce()) controller.abort() - await expect(pending).resolves.toMatchObject({ + const { task } = await pending + await expect(pending).resolves.toMatchObject({ result: { state: 'outcome_unknown', failedStage: 'worktree_create' } }) - expect(db.getTask(db.listTasks()[0]!.id)?.status).toBe('blocked') + expect(db.getTask(task.id)?.status).toBe('blocked')src/main/runtime/rpc/methods/orchestration-worker-topology.ts (1)
192-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
workerSetupSourcehere.Line 195 re-implements the source derivation this file just exported;
workerSetupSource(true, Boolean(params.setup))yields the same value.src/main/runtime/rpc/methods/orchestration-worktree-commit.ts (1)
11-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo recorders differ only by persist target — consider one generic factory.
The bodies are identical apart from the db method, effect type, and residual filter; a shared factory parameterized on
(effect, persistStage)would keep the repeated-commit semantics in one place.src/main/ipc/worktrees.test.ts (1)
3862-4086: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFour new SSH cases repeat the same repo/provider/mux fixture.
A small local factory (
createSshFixture({ addWorktree, listWorktrees })) would cut ~80 lines and keep the behavioral difference of each case visible.src/main/runtime/rpc/methods/orchestration-federated-worker-start.ts (1)
137-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the contract preflight flag when calling
callOrchestrationWorkerServer.The trailing
truemaps tocontractPreflightComplete, but that is opaque at the call site. Since adding a new option argument would be a breaking signature change, consider extracting a named local option/record for this path until the signature can move to an options object.src/main/runtime/rpc/methods/orchestration-worker-terminal-provisioning.ts (1)
44-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth provisioning functions are the same routine with two swapped hooks.
provisionWorkerTerminalandprovisionFederatedWorkerTerminaldiffer only in the stage recorder (recordWorkerStagevsrecordRemoteAttachmentStage), the residual filter, and the tracker label. Consider extracting a shared implementation taking{ recordStage, residualResources, operationLabel }so the commit/abort semantics can't drift between local and federated paths.src/main/runtime/rpc/methods/orchestration-federation-effects.ts (1)
67-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared classifier instead of an inline shape check.
isAgentSessionOperationOutcomeUnknowninsrc/shared/orchestration-agent-prompt-outcome.tsalready implements exactly this predicate and is used by the worktree provisioning paths; calling it here keeps the unknown-outcome contract single-sourced.♻️ Proposed refactor
- if ( - error && - typeof error === 'object' && - (error as { agentSessionOperationOutcome?: unknown }).agentSessionOperationOutcome === 'unknown' - ) { + if (isAgentSessionOperationOutcomeUnknown(error)) { return true }src/main/runtime/orca-runtime.ts (1)
18895-18951: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing
!args.signal?.abortedguard on the local startup-terminal blocks, inconsistent with the remote path.
createManagedRemoteWorktree's sequenced-startup block guards withif (sequencedStartup && this.ptyController?.spawn && !args.signal?.aborted) {(Line 20037), but the two local equivalents — the folder-repo branch (if (effectiveStartup && this.ptyController?.spawn) {, Line 18896) and the main git-repo branch (if (sequencedStartup && this.ptyController?.spawn) {, Line 19665) — omit the abort check even though both now accept and propagate the identicalargs.signal.Effect: if the caller's signal is already aborted by the time either local block is reached (worktree already created),
createTerminal(...)will still be invoked; it aborts internally before spawning a PTY (if (launchOpts.signal?.aborted) throw new Error('client_disconnected')), so no duplicate terminal is created, but the failure falls through to the generic warning path and surfaces a misleadingwarning: "Failed to create the startup terminal for <path>: client_disconnected"on an otherwise fully-successfulCreateWorktreeResult, even though the cancellation was expected/benign.Add the same
&& !args.signal?.abortedguard to both local blocks for consistency with the remote path and to avoid the spurious "failed" warning under legitimate cancellation.🐛 Proposed fix (git-repo branch)
- if (sequencedStartup && this.ptyController?.spawn) { + if (sequencedStartup && this.ptyController?.spawn && !args.signal?.aborted) { const startupCommit = createOrchestrationOperationCommitTracker(Also applies to: 19665-19739, 20037-20064
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b5e8099-170e-4e23-9e8a-8b87f8baaf95
📥 Commits
Reviewing files that changed from the base of the PR and between 905f540ae33ac8584cafa91669247e119429fcd5 and 355f99b.
📒 Files selected for processing (53)
ORCHESTRATION_IMPLEMENTATION_CHECKLIST.mdconfig/reliability-gates.jsoncsrc/cli/runtime/client-timeout-policy.test.tssrc/cli/runtime/client-timeout-policy.tssrc/cli/runtime/client.tssrc/main/index.tssrc/main/ipc/created-worktree-reconciliation.test.tssrc/main/ipc/created-worktree-reconciliation.tssrc/main/ipc/pty.tssrc/main/ipc/runtime-environment-call-queue.tssrc/main/ipc/runtime-environment-transport-routing.tssrc/main/ipc/worktree-remote.tssrc/main/ipc/worktrees.test.tssrc/main/providers/local-pty-provider.test.tssrc/main/providers/local-pty-provider.tssrc/main/providers/ssh-pty-foreground-process.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/providers/types.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration/coordinator.tssrc/main/runtime/orchestration/db.tssrc/main/runtime/orchestration/environment-transport.tssrc/main/runtime/rpc/methods/orchestration-agent-prompt-readiness.tssrc/main/runtime/rpc/methods/orchestration-federated-home-receipt.tssrc/main/runtime/rpc/methods/orchestration-federated-worker-start.tssrc/main/runtime/rpc/methods/orchestration-federation-effects.tssrc/main/runtime/rpc/methods/orchestration-federation-readiness.test.tssrc/main/runtime/rpc/methods/orchestration-federation-worktree-provisioning.tssrc/main/runtime/rpc/methods/orchestration-federation.test.tssrc/main/runtime/rpc/methods/orchestration-federation.tssrc/main/runtime/rpc/methods/orchestration-migration-behavior.test.tssrc/main/runtime/rpc/methods/orchestration-worker-finalization.tssrc/main/runtime/rpc/methods/orchestration-worker-prompt-delivery.tssrc/main/runtime/rpc/methods/orchestration-worker-start-receipt.tssrc/main/runtime/rpc/methods/orchestration-worker-terminal-identity.tssrc/main/runtime/rpc/methods/orchestration-worker-terminal-provisioning.tssrc/main/runtime/rpc/methods/orchestration-worker-topology.tssrc/main/runtime/rpc/methods/orchestration-workers-new-worktree.test.tssrc/main/runtime/rpc/methods/orchestration-workers.tssrc/main/runtime/rpc/methods/orchestration-worktree-commit.tssrc/main/runtime/rpc/methods/orchestration.test.tssrc/main/runtime/rpc/methods/orchestration.tssrc/main/runtime/runtime-rpc.test.tssrc/main/runtime/runtime-rpc.tssrc/shared/orchestration-agent-prompt-outcome.tssrc/shared/orchestration-dispatch-readiness.tssrc/shared/remote-runtime-client.test.tssrc/shared/remote-runtime-client.tssrc/shared/runtime-rpc-call-queue.test.tssrc/shared/runtime-rpc-call-queue.tstools/repro-orchestration-long-prompt.mjs
| let result: CreateWorktreeResult | ||
| try { | ||
| result = await createRemoteWorktree( | ||
| { | ||
| repoId: repo.id, | ||
| name: args.name, | ||
| ...(args.displayName ? { displayName: args.displayName } : {}), | ||
| ...(args.baseBranch ? { baseBranch: args.baseBranch } : {}), | ||
| ...(args.compareBaseRef ? { compareBaseRef: args.compareBaseRef } : {}), | ||
| ...(args.branchNameOverride ? { branchNameOverride: args.branchNameOverride } : {}), | ||
| ...(args.runHooks ? { setupDecision: 'run' as const } : {}), | ||
| ...(!args.runHooks && args.setupDecision ? { setupDecision: args.setupDecision } : {}), | ||
| ...(args.sparseCheckout ? { sparseCheckout: args.sparseCheckout } : {}), | ||
| ...(args.linkedIssue != null ? { linkedIssue: args.linkedIssue } : {}), | ||
| ...(args.linkedPR != null ? { linkedPR: args.linkedPR } : {}), | ||
| ...(args.linkedLinearIssue ? { linkedLinearIssue: args.linkedLinearIssue } : {}), | ||
| ...(args.linkedLinearIssueWorkspaceId !== undefined | ||
| ? { linkedLinearIssueWorkspaceId: args.linkedLinearIssueWorkspaceId } | ||
| : {}), | ||
| ...(args.linkedLinearIssueOrganizationUrlKey !== undefined | ||
| ? { linkedLinearIssueOrganizationUrlKey: args.linkedLinearIssueOrganizationUrlKey } | ||
| : {}), | ||
| ...(args.linkedGitLabMR != null ? { linkedGitLabMR: args.linkedGitLabMR } : {}), | ||
| ...(args.linkedGitLabIssue != null ? { linkedGitLabIssue: args.linkedGitLabIssue } : {}), | ||
| ...(args.linkedBitbucketPR != null ? { linkedBitbucketPR: args.linkedBitbucketPR } : {}), | ||
| ...(args.linkedAzureDevOpsPR != null | ||
| ? { linkedAzureDevOpsPR: args.linkedAzureDevOpsPR } | ||
| : {}), | ||
| ...(args.linkedGiteaPR != null ? { linkedGiteaPR: args.linkedGiteaPR } : {}), | ||
| ...(args.pushTarget ? { pushTarget: args.pushTarget } : {}), | ||
| ...(args.workspaceStatus ? { workspaceStatus: args.workspaceStatus as never } : {}), | ||
| ...(args.manualOrder !== undefined ? { manualOrder: args.manualOrder } : {}), | ||
| ...(args.createdWithAgent ? { createdWithAgent: args.createdWithAgent } : {}), | ||
| ...(args.pendingFirstAgentMessageRename === true | ||
| ? { pendingFirstAgentMessageRename: true } | ||
| : {}), | ||
| ...(args.automationProvenance ? { automationProvenance: args.automationProvenance } : {}), | ||
| ...(args.cliProvenance ? { cliProvenance: args.cliProvenance } : {}) | ||
| }, | ||
| repo, | ||
| this.store as unknown as Store, | ||
| headlessWindow, | ||
| args.onWorktreeCreateCommitted | ||
| ) | ||
| } catch (error) { | ||
| if (args.signal?.aborted) { | ||
| throw new OrchestrationOperationOutcomeUnknownError('Remote worktree creation', error) | ||
| } | ||
| throw error | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect createRemoteWorktree's signature to see whether it accepts/needs an AbortSignal.
ast-grep run --pattern 'export async function createRemoteWorktree($$$) { $$$ }' --lang typescript src/main/ipc/worktree-remote.ts
rg -n -A5 -B2 'export (async )?function createRemoteWorktree' src/main/ipc/worktree-remote.tsRepository: stablyai/orca
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== createRemoteWorktree signature and call sites =="
sed -n '1530,1555p' src/main/ipc/worktree-remote.ts
echo
rg -n 'createRemoteWorktree\(' src/main src/renderer 2>/dev/null | head -100
echo
echo "== Orca-runtime relevant section =="
sed -n '19920,20010p' src/main/runtime/orca-runtime.ts
echo
echo "== Local creation signal usage relevant symbols =="
rg -n 'createWorktree|createLocalWorktree|reconcileWorktree|signal|AbortSignal|signal:' src/main/ipc src/main/runtime orca-runtime.ts worktree-remote.ts 2>/dev/null | head -200Repository: stablyai/orca
Length of output: 28184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== imports and signal occurrences inside src/main/ipc/worktree-remote.ts =="
sed -n '1,140p' src/main/ipc/worktree-remote.ts
echo
rg -n 'AbortSignal|signal|addEventListener|aborted|fetch|execFile|spawn|createProcess|ssh|rsync' src/main/ipc/worktree-remote.ts
echo
echo "== createRemoteWorktree implementation body =="
sed -n '1540,1955p' src/main/ipc/worktree-remote.ts
echo
echo "== createLocalWorktree around signal usage =="
sed -n '1956,2070p' src/main/ipc/worktree-remote.ts
echo
echo "== surrounding createWorktree dispatcher =="
sed -n '1220,1275p' src/main/ipc/worktrees.tsRepository: stablyai/orca
Length of output: 35068
Forward signal into createRemoteWorktree cancellation points.
createRemoteWorktree(...) currently never reads args.signal, so aborting during this wrapper only reclassifies the later failure as OrchestrationOperationOutcomeUnknownError instead of interrupting cached SSH refresh/fetch/local-fallback operations. Add a signal?: AbortSignal option and thread it through those git/provider calls that local creation/cancellation support.
| failDispatchBeforeInput(ctxId: string, error: string): DispatchContextRow | undefined { | ||
| const ctx = this.db.prepare('SELECT * FROM dispatch_contexts WHERE id = ?').get(ctxId) as | ||
| | DispatchContextRow | ||
| | undefined | ||
| if (!ctx) { | ||
| return undefined | ||
| } | ||
| this.db | ||
| .prepare( | ||
| `UPDATE dispatch_contexts | ||
| SET status = 'failed', last_failure = ?, | ||
| capability_revoked_at = COALESCE(capability_revoked_at, datetime('now')) | ||
| WHERE id = ?` | ||
| ) | ||
| .run(error, ctxId) | ||
| this.db.prepare("UPDATE tasks SET status = 'ready' WHERE id = ?").run(ctx.task_id) | ||
| return this.db.prepare('SELECT * FROM dispatch_contexts WHERE id = ?').get(ctxId) as | ||
| | DispatchContextRow | ||
| | undefined | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
New dispatch-input transitions skip the BEGIN IMMEDIATE/COMMIT wrapper used by every sibling transition. Both methods mutate dispatch_contexts, tasks (and question threads) as independent statements, so a mid-sequence failure leaves the dispatch settled while the task is still dispatched — an unrecoverable, self-contradictory record for exactly the retryable/unknown classification this PR introduces.
src/main/runtime/orchestration/db.ts#L3578-L3597: wrap the dispatch update and thetasks→readyupdate inBEGIN IMMEDIATE…COMMITwithROLLBACKon error, mirroringfailWorkerStart.src/main/runtime/orchestration/db.ts#L3599-L3619: wrap the dispatch update,tasks→blockedupdate, andcloseQuestionsForDispatch(ctxId)in the same transaction shape, mirroringmarkWorkerStartUnknown.
📍 Affects 1 file
src/main/runtime/orchestration/db.ts#L3578-L3597(this comment)src/main/runtime/orchestration/db.ts#L3599-L3619
| function assertReadinessWindow(options: ReadinessOptions): void { | ||
| if (options.signal?.aborted) { | ||
| throw new Error('request_aborted') | ||
| } | ||
| if (Date.now() >= options.deadlineMs) { | ||
| throw new Error('Agent did not become ready (timeout).') | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve timeout classification when the deadline signal aborts.
AbortSignal.timeout() aborts the combined signal at the deadline, so these paths surface request_aborted or a raw abort error instead of the readiness timeout. Check Date.now() >= deadlineMs before signal.aborted, and re-run that normalization in both wait catch blocks.
src/main/runtime/rpc/methods/orchestration-agent-prompt-readiness.ts#L17-L23: prioritize deadline expiry over generic signal abort.src/main/runtime/rpc/methods/orchestration-agent-prompt-readiness.ts#L114-L119: normalize an abort raised bywaitForTerminalafter the deadline.src/main/runtime/rpc/methods/orchestration-agent-prompt-readiness.ts#L152-L157: apply the same normalization to the pre-write readiness guard.
📍 Affects 1 file
src/main/runtime/rpc/methods/orchestration-agent-prompt-readiness.ts#L17-L23(this comment)src/main/runtime/rpc/methods/orchestration-agent-prompt-readiness.ts#L114-L119src/main/runtime/rpc/methods/orchestration-agent-prompt-readiness.ts#L152-L157
| try { | ||
| const remoteTimeoutMs = remainingOrchestrationAgentReadinessTime(deadlineMs, signal) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Deadline exhaustion before the attach RPC is recorded as an unknown outcome.
remainingOrchestrationAgentReadinessTime throws inside the try, so the catch at Line 226 marks the dispatch start_unknown even though no attach request was ever sent and the remote state is definitively unchanged. That pushes a reconcilable-but-clean case into the manual unknown-outcome bucket. Computing it before Line 113 (or classifying it as a deterministic failure) keeps unknown reserved for genuinely ambiguous outcomes.
| db.recordWorkerStage({ | ||
| dispatchId, | ||
| stage: 'worktree_created', | ||
| worktreeId: created.worktree.id, | ||
| effects, | ||
| residualResources: effects | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'residualResources|residual_resources' --type=ts -g '!**/*.test.ts' | head -120Repository: stablyai/orca
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -e
echo "== Locate target file =="
git ls-files | rg 'orchestration-worker-topology\.ts$|orchestration-worker-terminal-provisioning\.ts$|orchestration-worktree-commit\.ts$|\.test\.ts$' | head -80
echo
echo "== File sizes =="
wc -l src/main/runtime/rpc/methods/orchestration-worker-topology.ts src/main/runtime/rpc/methods/orchestration-worker-terminal-provisioning.ts src/main/runtime/rpc/methods/orchestration-worktree-commit.ts 2>/dev/null || true
echo
echo "== Target occurrences (case-insensitive residual in ts) =="
rg -n -C3 -i 'residual|appendReusedWorkerWorktreeEffects|recordWorkerStage|reused' src/main/runtime/rpc/methods --type=ts | head -240
echo
echo "== Target file outline =="
ast-grep outline src/main/runtime/rpc/methods/orchestration-worker-topology.ts --view compact 2>/dev/null || trueRepository: stablyai/orca
Length of output: 27331
🏁 Script executed:
#!/bin/bash
set -e
echo "== Target file lines 120-210 =="
sed -n '120,210p' src/main/runtime/rpc/methods/orchestration-worker-topology.ts | nl -ba -v120
echo
echo "== Related recordWorkerStage definitions/usages =="
rg -n -C3 'function recordWorkerStage|recordWorkerStage\(|residualResources|residual_resources' src/main src -g '*.ts' | head -260
echo
echo "== Federation residual helpers and reused effects =="
sed -n '1,120p' src/main/runtime/rpc/methods/orchestration-federation-effects.ts | nl -ba -v1
sed -n '130,185p' src/main/runtime/rpc/methods/orchestration-federation.ts | nl -ba -v130
echo
echo "== Deterministic effect shape probe from source text =="
python3 - <<'PY'
from pathlib import Path
p = Path('src/main/runtime/rpc/methods/orchestration-worker-topology.ts')
text = p.read_text()
for needle in ['appendReusedWorkerWorktreeEffects', 'recordWorkerStage', 'effects.filter']:
print(f'-- {needle} --')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: stablyai/orca
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -e
echo "== Target file lines 120-210 =="
sed -n '120,210p' src/main/runtime/rpc/methods/orchestration-worker-topology.ts
echo
echo "== Related recordWorkerStage definitions/usages =="
rg -n -C3 'function recordWorkerStage|recordWorkerStage\(|residualResources|residual_resources' src/main src -g '*.ts' | head -260
echo
echo "== Federation residual helpers and reused effects =="
sed -n '1,120p' src/main/runtime/rpc/methods/orchestration-federation-effects.ts
sed -n '130,185p' src/main/runtime/rpc/methods/orchestration-federation.ts
echo
echo "== Deterministic effect shape probe from source text =="
python3 - <<'PY'
from pathlib import Path
p = Path('src/main/runtime/rpc/methods/orchestration-worker-topology.ts')
text = p.read_text()
for needle in ['appendReusedWorkerWorktreeEffects', 'recordWorkerStage', 'effects.filter']:
print(f'-- {needle} --')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PYRepository: stablyai/orca
Length of output: 30416
🏁 Script executed:
#!/bin/bash
set -e
echo "== orchestration-worker-topology relevant function =="
sed -n '1,115p' src/main/runtime/rpc/methods/orchestration-worker-topology.ts
echo
echo "== consumer around residual_resources =="
sed -n '200,235p' src/main/runtime/rpc/methods/orchestration-worker-control.ts
echo
echo "== all leftover/residual/resource terms outside tests =="
rg -n -i 'residual|orphan|retain|deprovision|cleanup|setup_decision|reused|worktree_not_found_on_server' src/main src -g '*.ts' -g '!*.test.ts' | head -260Repository: stablyai/orca
Length of output: 30047
🏁 Script executed:
#!/bin/bash
set -e
echo "== orchestrator methods that call createWorkerWorktree / reuse worktree =="
rg -n -C4 -i 'createWorkerWorktree|appendReusedWorkerWorktreeEffects|existing_worktree|worktree_not_found|worktree_create' src/main/runtime/rpc/methods src/main -g '*.ts' -g '!*.test.ts' | head -300
echo
echo "== static deterministic behavior probe from source =="
python3 - <<'PY'
from pathlib import Path
files = {
'topology': Path('src/main/runtime/rpc/methods/orchestration-worker-topology.ts'),
'terminal_provisioning': Path('src/main/runtime/rpc/methods/orchestration-worker-terminal-provisioning.ts'),
'worker_setup_gate': Path('src/main/runtime/rpc/methods/orchestration-worker-setup-gate.ts'),
'worktree_commit': Path('src/main/runtime/rpc/methods/orchestration-worktree-commit.ts'),
'federation_effects': Path('src/main/runtime/rpc/methods/orchestration-federation-effects.ts'),
'federation': Path('src/main/runtime/rpc/methods/orchestration-federation.ts'),
}
for name, path in files.items():
text = path.read_text()
print(f'\n-- {name} --')
for term in ['recordWorkerStage', 'residualResources', 'reused', 'not_applicable', 'createWorkerStage', 'appendFederation']:
if term in text:
for i, line in enumerate(text.splitlines(), 1):
if term in line:
print(f'{i}: {line}')
PYRepository: stablyai/orca
Length of output: 28709
Limit worktree_created residuals to created resources.
This path appends reused worktree/setup effects, so storing the full effects array reports caller-owned worktrees/non-applicable setup as residuals. Peer worktree provisioning filters residuals with isFederationResidualEffect; filter with the same predicate here.
🔧 Proposed fix
db.recordWorkerStage({
dispatchId,
stage: 'worktree_created',
worktreeId: created.worktree.id,
effects,
- residualResources: effects
+ residualResources: effects.filter((effect) => effect.action?.startsWith('created'))
})📝 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.
| db.recordWorkerStage({ | |
| dispatchId, | |
| stage: 'worktree_created', | |
| worktreeId: created.worktree.id, | |
| effects, | |
| residualResources: effects | |
| }) | |
| db.recordWorkerStage({ | |
| dispatchId, | |
| stage: 'worktree_created', | |
| worktreeId: created.worktree.id, | |
| effects, | |
| residualResources: effects.filter((effect) => effect.action?.startsWith('created')) | |
| }) |
| const terminalIdentity = createOrchestrationWorkerTerminalIdentity(started.dispatch.id) | ||
| appendReusedWorkerWorktreeEffects(effects, resolvedWorktree?.id) | ||
| let terminalHandle = params.terminal | ||
| let failedStage = 'terminal_create' | ||
| let setupReceipt: WorkerSetupReceipt = { | ||
| requested: 'not_applicable', | ||
| effective: 'not_applicable', | ||
| source: 'existing_worktree', | ||
| hookFound: false, | ||
| startupPolicy: 'start-immediately', | ||
| state: 'not_applicable' | ||
| } | ||
| let setupReceipt: WorkerSetupReceipt = initialWorkerSetupReceipt(false) | ||
| try { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Failure receipts for new-worktree dispatches start as not_applicable/existing_worktree.
initialWorkerSetupReceipt(false) is used even when createsWorktree is true, so any failure before createWorkerWorktree returns (Line 174) reports requested: 'not_applicable', source: 'existing_worktree' in failWorkerStartWithReceipt. The federated path already seeds this correctly (orchestration-federation.ts Line 112 passes createsWorktree and params.setup).
🐛 Proposed fix
- let setupReceipt: WorkerSetupReceipt = initialWorkerSetupReceipt(false)
+ let setupReceipt: WorkerSetupReceipt = initialWorkerSetupReceipt(
+ createsWorktree,
+ params.setup,
+ startOptions.setupSource
+ )📝 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.
| const terminalIdentity = createOrchestrationWorkerTerminalIdentity(started.dispatch.id) | |
| appendReusedWorkerWorktreeEffects(effects, resolvedWorktree?.id) | |
| let terminalHandle = params.terminal | |
| let failedStage = 'terminal_create' | |
| let setupReceipt: WorkerSetupReceipt = { | |
| requested: 'not_applicable', | |
| effective: 'not_applicable', | |
| source: 'existing_worktree', | |
| hookFound: false, | |
| startupPolicy: 'start-immediately', | |
| state: 'not_applicable' | |
| } | |
| let setupReceipt: WorkerSetupReceipt = initialWorkerSetupReceipt(false) | |
| try { | |
| const terminalIdentity = createOrchestrationWorkerTerminalIdentity(started.dispatch.id) | |
| appendReusedWorkerWorktreeEffects(effects, resolvedWorktree?.id) | |
| let terminalHandle = params.terminal | |
| let failedStage = 'terminal_create' | |
| let setupReceipt: WorkerSetupReceipt = initialWorkerSetupReceipt( | |
| createsWorktree, | |
| params.setup, | |
| startOptions.setupSource | |
| ) | |
| try { |
| onCommitted: () => { | ||
| if (committed) { | ||
| return | ||
| } | ||
| committed = true | ||
| try { | ||
| onCommitted?.() | ||
| } catch (error) { | ||
| commitError = error | ||
| } | ||
| }, | ||
| rethrowIfCommitted: (error) => { | ||
| if (committed) { | ||
| throw new OrchestrationOperationOutcomeUnknownError(operation, commitError ?? error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Surface commit-observer failures as unknown outcomes.
Line 34 suppresses callback failures, and rethrowIfCommitted only runs on a later error. A successful operation can therefore return after its durable commit record failed, losing recovery metadata for an already-created resource.
Proposed fix
let committed = false
- let commitError: unknown
+ let commitError: OrchestrationOperationOutcomeUnknownError | undefined
return {
onCommitted: () => {
if (committed) {
return
}
committed = true
try {
onCommitted?.()
} catch (error) {
- commitError = error
+ commitError = new OrchestrationOperationOutcomeUnknownError(operation, error)
+ throw commitError
}
},
rethrowIfCommitted: (error) => {
if (committed) {
- throw new OrchestrationOperationOutcomeUnknownError(operation, commitError ?? error)
+ throw commitError ?? new OrchestrationOperationOutcomeUnknownError(operation, error)
}
}
}📝 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.
| onCommitted: () => { | |
| if (committed) { | |
| return | |
| } | |
| committed = true | |
| try { | |
| onCommitted?.() | |
| } catch (error) { | |
| commitError = error | |
| } | |
| }, | |
| rethrowIfCommitted: (error) => { | |
| if (committed) { | |
| throw new OrchestrationOperationOutcomeUnknownError(operation, commitError ?? error) | |
| } | |
| } | |
| let committed = false | |
| let commitError: OrchestrationOperationOutcomeUnknownError | undefined | |
| return { | |
| onCommitted: () => { | |
| if (committed) { | |
| return | |
| } | |
| committed = true | |
| try { | |
| onCommitted?.() | |
| } catch (error) { | |
| commitError = new OrchestrationOperationOutcomeUnknownError(operation, error) | |
| throw commitError | |
| } | |
| }, | |
| rethrowIfCommitted: (error) => { | |
| if (committed) { | |
| throw commitError ?? new OrchestrationOperationOutcomeUnknownError(operation, error) | |
| } | |
| } |
| export function resolveOrchestrationAgentReadinessTimeoutMs( | ||
| method: string, | ||
| params: unknown | ||
| ): number | null { | ||
| if (isInjectedOrchestrationDispatch(method, params)) { | ||
| return ORCHESTRATION_DISPATCH_READY_TIMEOUT_MS | ||
| } | ||
| if (method !== 'orchestration.workerStart' && method !== 'orchestration.federationAttachStart') { | ||
| return null | ||
| } | ||
| const requested = | ||
| typeof params === 'object' && params !== null | ||
| ? Number((params as { timeoutMs?: unknown }).timeoutMs) | ||
| : Number.NaN | ||
| return Number.isFinite(requested) && requested > 0 | ||
| ? requested | ||
| : ORCHESTRATION_DISPATCH_READY_TIMEOUT_MS | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js setTimeout silently fire immediately for delays greater than 2147483647ms, and does AbortSignal.timeout share this limitation?
💡 Result:
No. setTimeout() in Node.js clamps delays above 2147483647 ms to 1 ms, so it’s not synchronous/immediate, just effectively “almost right away” on the next timer turn. (nodejs.org)
AbortSignal.timeout(delay) is documented to abort after delay ms, but the docs I found don’t spell out a separate max-delay rule. My best read is that it likely shares the same practical timer limit because it’s a timer-driven API, but that part is an inference rather than an explicit docs guarantee. (nodejs.org)
Citations:
- 1: https://nodejs.org/download/release/v20.20.1/docs/api/timers.html?utm_source=openai
- 2: https://nodejs.org/download/release/latest-v24.x/docs/api/globals.html?utm_source=openai
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
fd -a 'orchestration-dispatch-readiness|remote-runtime-client' . | sed 's#^\./##'
echo "== target file outline/size =="
wc -l src/shared/orchestration-dispatch-readiness.ts
cat -n src/shared/orchestration-dispatch-readiness.ts
echo "== remote-runtime-client relevant symbols =="
rg -n "isSafeTimerDelayMs|MAX_TIMER_DELAY_MS|sendRemoteRuntimeRequest|setTimeout\\(onTimeout|timeoutMs|AbortSignal\\.timeout" src/shared/remote-runtime-client.ts
sed -n '1,260p' src/shared/remote-runtime-client.ts
echo "== references to resolver =="
rg -n "resolveOrchestrationAgentReadinessTimeoutMs|sendRemoteRuntimeRequest|resolve.*Readiness.*Timeout" src test .github . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 120Repository: stablyai/orca
Length of output: 25146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== timer-delay module =="
wc -l src/shared/timer-delay.ts
cat -n src/shared/timer-delay.ts
echo "== client-timeout-policy =="
cat -n src/cli/runtime/client-timeout-policy.ts
echo "== orchestration-agent-prompt-readiness =="
sed -n '1,80p' src/main/runtime/rpc/methods/orchestration-agent-prompt-readiness.ts
echo "== focused sendRemoteRuntimeRequest timeout usage =="
sed -n '130,180p' src/shared/remote-runtime-client.ts
sed -n '400,490p' src/shared/remote-runtime-client.tsRepository: stablyai/orca
Length of output: 10525
Clamp caller-supplied timeoutMs at MAX_TIMER_DELAY_MS.
resolveOrchestrationAgentReadinessTimeoutMs currently returns any finite positive requested timeout, but AbortSignal.timeout and Node timers are bounded, so an oversized value for workerStart/federationAttachStart can make the readiness wait expire almost immediately instead of waiting the requested duration. Apply the existing safe-timer cap here.
The gate this PR relies on can itself report ready too earlyThis PR replaces the blind 500 ms delay with a wait for Measured on Orca 1.4.176, Windows 11, Claude Code panes, brand-new folders:
The detector is correct. Effect on the composed path: Full evidence and the field incident are recorded on #9976. @innocarpe has since added deterministic coverage for this exact screen in #10020, which is the narrower fix; this PR remains the broader readiness and reconciliation change. Correction to my earlier version of this commentI suggested giving Claude a |
|
Thanks for the deterministic evidence on the Claude trust gate. I updated #10020 with a regression covering the narrower launchAgent quiescence race from #9976: after the quiet period, the unchanged "Yes, I trust this folder" screen must produce a blocked tui-idle result rather than readiness. The upstream and fork mirror now both point to 0a0df3a643, with 1,079 runtime tests passing and 1 skipped. #11048 remains the broader readiness, cancellation, and reconciliation change; I have not modified its external branch or resolved its existing review threads. |
Summary
tui-idlebefore dispatch authority and prompt injectioncheck --typeshelp contractReproduction
dispatch --injectpreviously used a blind 500 ms delay. A slow-starting agent could still be showing its directory-trust prompt, so the injected submit answered that prompt and swallowed the task. The new regression holds startup beforetui-idleand verifies no writes occur until readiness.Validation
pnpm typecheck:nodepnpm typecheck:clipnpm check:max-lines-ratchetgit diff origin/main...HEAD --checkCloses #10728
Closes #10663