[STA-5729] Prevent live coordinator takeover of orchestration Runs - #16859
brennanb2025 wants to merge 20 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (79)
🚧 Files skipped from review as they are similar to previous changes (76)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe change adds persisted Run coordinator process and host identity, authority revisions, migration support, and continuity-aware rebinding. RPC dispatch now derives caller evidence and validates coordinator ownership across Run, task, gate, worker, send, reply, and legacy operations. Same-process handle remints preserve continuity, while live or unverifiable takeovers return Merge Risk: 🟡 Moderate · up to This PR tightens Run coordinator ownership, but unresolved authorization and takeover-race paths could allow an unattested caller or stale coordinator to perform protected worker actions; additional edge cases may cause false takeover rejection or misleading RPC failures. Merge should wait for these bounded correctness and security risks to be addressed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed, on-topic, and covers the change, rationale, linked issue identifier, testing, validation, and residual risks. It does not include a dedicated Visual Proof section or the exact template checklist headings, but the core required information is present. Full details: Docstring CoverageExplanation Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 49 files. (29 skipped: 2 unsupported, 1 too large, 26 over the file limit.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
…binding-loss # Conflicts: # src/main/runtime/rpc/methods/orchestration-workers.ts
|
⛔ Please do not merge yet. This remains a separate genuine root-cause fix, but the branch currently has merge conflicts. Resolve the conflicts, then rerun the review and validation checks before considering it merge-ready. |
…binding-loss # Conflicts: # config/reliability-gates.jsonc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/cli/handlers/orchestration/gate-handlers.ts (1)
58-61: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLocalize the new read-only notices.
src/cli/handlers/orchestration/gate-handlers.ts#L58-L61: use an intent-named localization ID.src/cli/handlers/orchestration/run-handlers.ts#L89-L92: use an intent-named localization ID.Source: Learnings
src/main/runtime/rpc/methods/orchestration.ts (1)
1590-1608: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared
binding.currentConsumercomputation. Both handlers repeat the same four-part check: a caller handle is present, the Run is non-legacy,resolveAttestedRunCoordinatorPanereturned a pane, anddb.getCurrentRunForPanemaps that pane back to the Run. A future change to the ownership rule must be applied in both places.
src/main/runtime/rpc/methods/orchestration.ts#L1590-L1608: replace the inlinecallerPaneKeyresolution andbinding.currentConsumerexpression with a call to one shared function, for exampleresolveRunBindingStatus(runtime, run, params.callerTerminalHandle, orchestrationCompatibilityEvidence).src/main/runtime/rpc/methods/orchestration-gates.ts#L206-L226: call the same shared function withparams.fromas the caller handle.Place the function next to
resolveAttestedRunCoordinatorPaneinsrc/main/runtime/rpc/methods/orchestration-coordinator-caller.ts.src/main/runtime/rpc/methods/orchestration-federation-output.test.ts (1)
106-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the local
startRequestwith the shared fixture builder.
createFederationWorkerStartRequestinsrc/main/runtime/rpc/methods/orchestration-federation-test-request.tsbuilds the same request and accepts overrides. The local copy now duplicates theorchestrationCompatibilityEvidencefield and must be kept in sync manually.Use
createFederationWorkerStartRequest(taskId, { name: 'windows-output' })instead.Based on coding guidelines: "check whether an existing implementation already does the job (or nearly does). Extend or generalize it instead of building a parallel version".
Source: Coding guidelines
src/main/runtime/orca-runtime.ts (1)
18149-18154: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGive the provider deadline a margin below the local timeout.
deadlineMsand the outerwithTimeoutResulttimeout both usePTY_CONTROLLER_LIST_TIMEOUT_MSand start at the same instant, so they expire at the same wall-clock time. The provider RPC has no time left to return its result before the local race gives up, so under normal network latency this call trends toward'unverifiable'even when the provider could have answered in time.Elsewhere in this file, the same pattern reserves a margin for exactly this reason:
const providerListOpts = { deadlineMs: Date.now() + Math.max(1, listBudgetMs - PTY_CONTROLLER_LIST_PROVIDER_MARGIN_MS) }Apply the same margin here so the provider deadline expires before the local timeout.
♻️ Proposed fix
- const deadlineMs = Date.now() + PTY_CONTROLLER_LIST_TIMEOUT_MS + const deadlineMs = + Date.now() + Math.max(1, PTY_CONTROLLER_LIST_TIMEOUT_MS - PTY_CONTROLLER_LIST_PROVIDER_MARGIN_MS) const listed = await withTimeoutResult( this.ptyController.listProcesses(hostScope.kind === 'ssh' ? hostScope.targetId : null, { deadlineMs }), PTY_CONTROLLER_LIST_TIMEOUT_MS )src/main/runtime/rpc/methods/orchestration-runs.test.ts (1)
91-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the pane-key mock independent of call count.
The mock returns the transient pane for exactly the first two calls, then
null. This couples the test to the current number ofgetTerminalPaneKeycalls inside therunCreatepath. If a future change adds or removes one pane lookup, the assertion can still pass while exercising a different branch, so thestable_pane_requiredinvariant stops being covered.♻️ Proposed refactor
- const transientPaneKey = 'tab_stale:33333333-3333-4333-8333-333333333333' - vi.spyOn(runtime, 'getTerminalPaneKey') - .mockReturnValueOnce(transientPaneKey) - .mockReturnValueOnce(transientPaneKey) - .mockReturnValue(null) + const transientPaneKey = 'tab_stale:33333333-3333-4333-8333-333333333333' + // Why: the pane disappears after the initial resolution, regardless of how many lookups run. + let paneResolutionsRemaining = 2 + vi.spyOn(runtime, 'getTerminalPaneKey').mockImplementation(() => + paneResolutionsRemaining-- > 0 ? transientPaneKey : null + )src/main/runtime/rpc/methods/orchestration-federation-control-mail.test.ts (1)
81-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one parameterized coordinator-attestation mock across the federation control-mail and worker-start prompt tests. The current local copies duplicate the same caller, pane, and launch-token evidence and can drift as the attestation contract evolves.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08a81dfc-7898-4a78-8e7d-885d8cb64a5d
📒 Files selected for processing (79)
config/reliability-gates.jsoncconfig/scripts/orchestration-skill-guidance.test.mjsskill-guides/orchestration.mdsrc/cli/bundled-skill-guides.tssrc/cli/handlers/orchestration-gate-cli.test.tssrc/cli/handlers/orchestration-run-cli.test.tssrc/cli/handlers/orchestration-terminal-identity.test.tssrc/cli/handlers/orchestration/gate-handlers.tssrc/cli/handlers/orchestration/run-handlers.tssrc/cli/handlers/orchestration/task-handlers.tssrc/cli/handlers/orchestration/terminal-identity.tssrc/cli/index.test.tssrc/cli/root-help-text-primary.tssrc/cli/runtime-client.test.tssrc/cli/runtime/client.tssrc/cli/runtime/orchestration-caller-handle-remint.tssrc/cli/specs/orchestration.test.tssrc/cli/specs/orchestration.tssrc/main/ipc/pty-controller-process-inventory.test.tssrc/main/ipc/pty/runtime/operations.tssrc/main/runtime/orca-runtime-process-incarnation-liveness.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration-compatibility-authority.test.tssrc/main/runtime/orchestration-mailbox-detached-routing.test.tssrc/main/runtime/orchestration-mailbox-notification-consistency.test.tssrc/main/runtime/orchestration-mailbox-notification-test-harness.tssrc/main/runtime/orchestration-mailbox-routing-races.test.tssrc/main/runtime/orchestration-message-delivery-identity.test.tssrc/main/runtime/orchestration/db/contract-constants.tssrc/main/runtime/orchestration/db/runs/run-binding.tssrc/main/runtime/orchestration/db/runs/run-create.tssrc/main/runtime/orchestration/db/runs/run-lookup.tssrc/main/runtime/orchestration/db/schema/create-core-tables-sql.tssrc/main/runtime/orchestration/db/schema/migrate-v31.tssrc/main/runtime/orchestration/db/schema/migrate.tssrc/main/runtime/orchestration/orchestration-adopted-run-binding.test.tssrc/main/runtime/orchestration/orchestration-run-delivery-db.test.tssrc/main/runtime/orchestration/orchestration-schema-version-skew.tssrc/main/runtime/orchestration/run-coordinator-authority-migration.test.tssrc/main/runtime/orchestration/run-coordinator-authority.tssrc/main/runtime/orchestration/run-coordinator-handle-migration.test.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/orchestration/worker-terminal-process-liveness.tssrc/main/runtime/rpc/dispatcher-orchestration-caller-evidence.tssrc/main/runtime/rpc/dispatcher.tssrc/main/runtime/rpc/methods/orchestration-ask.test.tssrc/main/runtime/rpc/methods/orchestration-caller-attestation.test.tssrc/main/runtime/rpc/methods/orchestration-check.test.tssrc/main/runtime/rpc/methods/orchestration-composed-workers.test.tssrc/main/runtime/rpc/methods/orchestration-coordinator-caller.tssrc/main/runtime/rpc/methods/orchestration-federation-control-mail.test.tssrc/main/runtime/rpc/methods/orchestration-federation-lifecycle-settlement.test.tssrc/main/runtime/rpc/methods/orchestration-federation-output.test.tssrc/main/runtime/rpc/methods/orchestration-federation-test-request.tssrc/main/runtime/rpc/methods/orchestration-federation.test.tssrc/main/runtime/rpc/methods/orchestration-gate-run-authorization.test.tssrc/main/runtime/rpc/methods/orchestration-gates.tssrc/main/runtime/rpc/methods/orchestration-recipient-routing.test.tssrc/main/runtime/rpc/methods/orchestration-rpc-test-harness.tssrc/main/runtime/rpc/methods/orchestration-run-coordinator-observation.tssrc/main/runtime/rpc/methods/orchestration-run-remint-authority.test.tssrc/main/runtime/rpc/methods/orchestration-run-scope.tssrc/main/runtime/rpc/methods/orchestration-runs.test.tssrc/main/runtime/rpc/methods/orchestration-runs.tssrc/main/runtime/rpc/methods/orchestration-send.test.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration-worker-start-prompt-contract.test.tssrc/main/runtime/rpc/methods/orchestration-workers-new-worktree.test.tssrc/main/runtime/rpc/methods/orchestration-workers.tssrc/main/runtime/rpc/methods/orchestration.tssrc/main/runtime/rpc/orchestration-11745-regression-verification.test.tssrc/main/runtime/rpc/orchestration-legacy-compatibility-dispatcher.test.tssrc/main/runtime/rpc/orchestration-legacy-fence-jurisdiction.test.tssrc/main/runtime/rpc/orchestration-legacy-mail.tssrc/main/runtime/rpc/orchestration-legacy-question-takeover.test.tssrc/main/runtime/rpc/orchestration-legacy-run-routing.test.tssrc/main/runtime/rpc/orchestration-legacy-takeover-delivery.test.tssrc/main/runtime/rpc/orchestration-runtime-update-settlement.test.tssrc/main/runtime/rpc/orchestration-task-dispatch-invariant.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| const values = params as Record<string, unknown> | ||
| const terminalHandle = [values.callerTerminalHandle, values.from, values.terminal].find( | ||
| (value): value is string => typeof value === 'string' | ||
| ) | ||
| return { terminalHandle: terminalHandle ?? '', ...UNATTESTED_CALLER } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Check whether any orchestration.* RPC method is registered without a params schema.
rg -nP -C6 "name:\s*'orchestration\.[A-Za-z]+'" src/main/runtime/rpc/methods | rg -n -B2 -A6 "name:\s*'orchestration\." | head -200
rg -nP -C4 'parseRpcRequestParams' src/main/runtime/rpcRepository: stablyai/orca
Length of output: 22778
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- applicable convention and learning files ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- target file ---'
cat -n src/main/runtime/rpc/dispatcher-orchestration-caller-evidence.ts
printf '%s\n' '--- parser ---'
cat -n src/main/runtime/rpc/dispatcher-request-parsing.ts
printf '%s\n' '--- dispatcher relevant slices ---'
sed -n '45,105p' src/main/runtime/rpc/dispatcher.ts
sed -n '155,215p' src/main/runtime/rpc/dispatcher.ts
printf '%s\n' '--- caller-evidence references ---'
rg -n -C4 'resolveRpcCallerEvidence|effectiveParams|orchestrationCompatibilityEvidence' src/main/runtime/rpcRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- applicable conventions ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/repo-wide.md
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src.md
printf '%s\n' '--- applicable learning ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-runtime-rpc.md
printf '%s\n' '--- schema-less method declarations ---'
rg -n -C3 'params\s*:\s*null' src/main/runtime/rpc
printf '%s\n' '--- orchestration method declarations ---'
rg -n -C2 "name:\s*['\"]orchestration\." src/main/runtime/rpc/methods
printf '%s\n' '--- compatibility params contract ---'
rg -n -C5 'params\??:|params:' src/main/runtime/rpc/orchestration-legacy-compatibility.ts src/main/runtime/rpc/core.tsRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- dispatcher constructor and registry contract ---'
sed -n '1,55p' src/main/runtime/rpc/dispatcher.ts
rg -n -C5 'export type RpcAnyMethod|export type RpcMethod|defineMethod|class RpcMethodRegistry|methods\?:' src/main/runtime/rpc/core.ts src/main/runtime/rpc
printf '%s\n' '--- legacy compatibility route ---'
sed -n '55,112p' src/main/runtime/rpc/orchestration-legacy-compatibility.ts
printf '%s\n' '--- default registry assembly ---'
rg -n -C5 'new RpcDispatcher|methods:|RPC_METHODS|ORCHESTRATION_.*METHODS' src/main/runtime/rpc src/main | head -240Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- dispatcher top ---'
sed -n '1,52p' src/main/runtime/rpc/dispatcher.ts
printf '%s\n' '--- core declarations ---'
rg -n -A12 -B3 '^(export )?(type|interface) Rpc(AnyMethod|Method)|^(export )?function define(Method|StreamingMethod)|class RpcMethod' src/main/runtime/rpc/core.ts
printf '%s\n' '--- compatibility implementation ---'
sed -n '55,110p' src/main/runtime/rpc/orchestration-legacy-compatibility.ts
printf '%s\n' '--- explicit custom orchestration schema-less tests ---'
rg -n -C4 "orchestration\.[A-Za-z].*|params:\s*null" src/main/runtime/rpc --glob '*test.ts' | rg -n -B2 -A4 'orchestration\.|params:\s*null' | head -160Repository: stablyai/orca
Length of output: 21781
Guard params before reading caller-handle fields.
If a custom orchestration.* method has params: null, the dispatcher passes undefined to resolveRpcCallerEvidence. Reading values.callerTerminalHandle then throws a TypeError instead of returning the unattested-caller envelope. Use (params ?? {}) as Record<string, unknown>.
| if ( | ||
| !run || | ||
| !isCallerCurrentRunCoordinator(runtime, run, params.from, coordinatorPane) || | ||
| (params.run && params.run !== run.id) | ||
| ) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable conventions ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -type f -name '*.md' -print | sort
printf '%s\n' '--- worker handler ---'
sed -n '1,240p' src/main/runtime/rpc/methods/orchestration-workers.ts
printf '%s\n' '--- coordinator-related symbols ---'
rg -n -g '*.ts' 'workerStart|takeover|coordinator|authority|dispatchReady|waitForTerminal|mark.*Ready|ready' src/main/runtime/rpc/methods src/main/runtime/orchestration | head -240Repository: stablyai/orca
Length of output: 44494
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable conventions ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src-main-daemon.md
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src.md
printf '%s\n' '--- worker handler continuation ---'
sed -n '220,430p' src/main/runtime/rpc/methods/orchestration-workers.ts
printf '%s\n' '--- worker topology authority and effects ---'
rg -n -A35 -B10 'requireWorkerAuthority|persistWorker|markWorkerDispatchReady|send.*Prompt|prompt|authority' src/main/runtime/rpc/methods/orchestration-worker-topology.ts src/main/runtime/rpc/methods/orchestration-worker-setup-gate.ts src/main/runtime/rpc/methods/orchestration-worker-start-receipt.ts src/main/runtime/rpc/methods/orchestration-federated-worker-start.ts
printf '%s\n' '--- targeted authority test ---'
sed -n '700,805p' src/main/runtime/rpc/methods/orchestration-workers-new-worktree.test.ts
printf '%s\n' '--- authority transfer callers ---'
rg -n -g '*.ts' -A25 -B12 'commit.*Coordinator|takeover|coordinator_authority_revision|coordinatorHandle|coordinatorPaneKey' src/main/runtime/rpc/methods src/main/runtime/orchestration | head -260Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- authority-related files ---'
fd -t f -e ts src/main | rg 'coordinator|authority|orchestration-worker-control|orchestration-workers'
printf '%s\n' '--- runtime-rpc learnings ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-runtime-rpc.md
printf '%s\n' '--- coordinator caller and authority predicate ---'
cat -n src/main/runtime/rpc/methods/orchestration-coordinator-caller.ts
rg -n -g '*.ts' -A35 -B12 'function isCurrentRunCoordinator|export function isCurrentRunCoordinator|coordinator_authority_revision|commit.*Coordinator|takeover' src/main/runtime/orchestration src/main/runtime/rpc/methodsRepository: stablyai/orca
Length of output: 521
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- authority-related files ---'
fd . src/main -t f -e ts | rg 'coordinator|authority|orchestration-worker-control|orchestration-workers'
printf '%s\n' '--- runtime-rpc learnings ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-runtime-rpc.md
printf '%s\n' '--- coordinator caller and authority predicate ---'
cat -n src/main/runtime/rpc/methods/orchestration-coordinator-caller.ts
rg -n -g '*.ts' -A35 -B12 'function isCurrentRunCoordinator|export function isCurrentRunCoordinator|coordinator_authority_revision|commit.*Coordinator|takeover' src/main/runtime/orchestration src/main/runtime/rpc/methodsRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- RPC mutation context ---'
rg -n -g '*.ts' -A30 -B15 'orchestrationMutation|type.*Mutation|mutationReceipt|defineMethod|handler\(' src/main/runtime/rpc
printf '%s\n' '--- run-use binding and completion ---'
sed -n '60,245p' src/main/runtime/rpc/methods/orchestration-runs.ts
printf '%s\n' '--- bindRun implementation ---'
rg -n -g '*.ts' -A70 -B15 'function bindRun|bindRun\(' src/main/runtime/orchestrationRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- files that define or pass orchestrationMutation ---'
rg -l 'orchestrationMutation' src/main/runtime/rpc | sort
printf '%s\n' '--- dispatcher mutation handling ---'
rg -n -A45 -B20 'orchestrationMutation|mutationReceipt|orchestrationRequestId|method.handler' src/main/runtime/rpc/dispatcher.ts src/main/runtime/rpc/core.ts
printf '%s\n' '--- run-use bind section ---'
sed -n '205,275p' src/main/runtime/rpc/methods/orchestration-runs.ts
printf '%s\n' '--- bindRun definitions ---'
rg -l 'bindRun' src/main/runtime/orchestration | sort
rg -n -A75 -B15 'bindRun' src/main/runtime/orchestration/db.ts src/main/runtime/orchestration/dbRepository: stablyai/orca
Length of output: 43794
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- orchestration mutation executor ---'
cat -n src/main/runtime/rpc/orchestration-mutation-executor.ts
printf '%s\n' '--- executor references and concurrency tests ---'
rg -n -A35 -B15 'OrchestrationMutationExecutor|getOrchestrationMutationExecutor|concurrent|Promise\.all|pending|recordReceipt' src/main/runtime/rpc/orchestration-mutation-executor.ts src/main/runtime/rpc/*test.ts src/main/runtime/rpc/methods/*test.ts | head -320Repository: stablyai/orca
Length of output: 47016
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- run binding takeover conditions ---'
sed -n '80,245p' src/main/runtime/orchestration/db/runs/run-binding.ts
printf '%s\n' '--- coordinator observation ---'
cat -n src/main/runtime/rpc/methods/orchestration-run-coordinator-observation.ts
printf '%s\n' '--- run-use authority tests ---'
rg -n -A25 -B15 'takeover|run-use|incumbent|authority' src/main/runtime/rpc/methods/orchestration-runs*.test.ts src/main/runtime/orchestration/*authority*.test.ts | head -300Repository: stablyai/orca
Length of output: 42726
Authorization Bypass (CWE-862): Missing Authorization
Reachability: External · Exploitability: Difficult
Revalidate Run authority after awaited stages.
The entry check does not protect effects after await. run-use can update Run authority while this handler is suspended, after which the handler can still record stages, send the agent prompt, and mark the dispatch ready. requireWorkerAuthority checks the worker terminal, not the Run coordinator. Revalidate Run authority before each effect or serialize worker start with run-use.
| const lifecyclePayload = isDispatchMutationMessageType(params.type) | ||
| ? parseRemoteWorkerPayload(params.payload) | ||
| : undefined | ||
| const lifecycleDispatch = | ||
| typeof lifecyclePayload?.dispatchId === 'string' | ||
| ? db.getDispatchContextById(lifecyclePayload.dispatchId) | ||
| : undefined | ||
| const dispatchCapabilityAuthenticatesLifecycle = Boolean( | ||
| orchestrationCapability && lifecycleDispatch?.capability_hash | ||
| ) | ||
| if (!dispatchCapabilityAuthenticatesLifecycle) { | ||
| assertCallerHandleMatchesEvidence(runtime, from, orchestrationCompatibilityEvidence, { | ||
| callerAuthority: attestedCaller, | ||
| allowLegacyAuthority: Boolean(legacyCoordinatorAuthority) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace every effect path reachable after the lifecycle capability bypass in orchestration.send.
set -euo pipefail
rg -n -C6 'dispatchCapabilityAuthenticatesLifecycle|verifyDispatchCapability|convertLifecycleMessageToRejection' src/main/runtime/rpc src/main/runtime/orchestration
# Confirm test coverage for a bogus capability against a capability-backed Dispatch.
rg -n -C10 'mintDispatchCapability' src/main/runtime/rpc --glob '*attestation*.test.ts'Repository: stablyai/orca
Length of output: 29945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository guidance ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -type f -name '*.md' -maxdepth 3 -print
printf '%s\n' '--- orchestration.send path ---'
sed -n '520,815p' src/main/runtime/rpc/methods/orchestration.ts
printf '%s\n' '--- capability verification ---'
sed -n '1,105p' src/main/runtime/orchestration/db/dispatch-context/dispatch-capability.ts
printf '%s\n' '--- rejection persistence and notification ---'
sed -n '55,100p' src/main/runtime/orchestration/db/messages/message-inbox.ts
rg -n -C5 'notifyMessageArrived|send\s*\(' src/main/runtime/rpc/methods/orchestration.ts src/main/runtime/orca-runtime.tsRepository: stablyai/orca
Length of output: 36412
Broken Authentication (CWE-287): Improper Authentication
Reachability: External · Exploitability: Moderate
Validate the Dispatch capability before skipping caller attestation.
The current check tests only capability presence. An invalid capability bypasses caller attestation, then reaches message insertion and notifyMessageArrived before db.verifyDispatchCapability rejects it. Call db.verifyDispatchCapability at the bypass point and skip attestation only when it returns valid: true.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Superseded without a direct successor PR. This branch is about 2,200 commits behind main. Its authority columns never landed, and main has since solved live coordinator takeover a different way, with caller-evidence attestation. What takeover means for the new structured-session actors is carried by #22555:
Closing. |
1 participant
ELI5
Orca orchestration is operated by agents. Each Run has one coordinator agent so exactly one actor changes the task graph and consumes its FIFO mailbox. Before this fix, another live macOS pane could run
run-useand silently replace that coordinator even though the app, runtime, and original agent process were still available. The original agent then lost its Run binding (run_required) and worker completion mail could appear to stall or move to the wrong consumer.This PR binds Run authority to the attested coordinator process incarnation and execution host, not just mutable terminal routing metadata. Same-process handle/pane remints preserve authority and pending Delivery; a distinct agent can replace an ordinary coordinator only after the owning host proves the incumbent process exited. Copied handles, invented Dispatch capabilities, stale observations, and a second live coordinator cannot acquire authority.
Coordinator authority FAQ for agents
If I want another coordinator agent to take over, what do I do?
run-useis an authority claim, not a read-only selection. From the intended replacement agent's stable terminal, make one claim:Orca decides which safe case applies:
consumer_generationand the outstanding Delivery.liveowner: failconsumer_fencedwitheffectsApplied: false; continue from the owner. If transfer is intentional, first bring the old coordinator to a stable handoff point and stop/exit its process, then retry from the replacement.unverifiableowner: fail with no effects; restore connectivity. Loss of contact on SSH, WSL, Windows, relay, or a federated runtime is never proof of exit.exited: claim succeeds, advances the consumer generation, fences the old Delivery, and binds the replacement.There is no ordinary live-to-live transfer or force-steal command. A seamless transfer while both agents remain live would require a separate owner-authorized protocol; this PR does not add one.
The retained adoption contract has one explicit exception. If Orca automatically adopted a legacy Run and its original coordinator is unavailable or cannot prove retained authority, the replacement agent may run:
--takeover-legacyexplicitly fences the retained legacy coordinator even though it is not the ordinary proven-exit path. It is valid only for the automatically adopted Run and must not be used while that legacy coordinator is actively coordinating. It is not a force override for an ordinary Run.What does it mean that a coordinator was taken over?
Authority for that one Run moves to a different attested agent process. Orca changes the coordinator handle, pane, process incarnation, host scope, and authority revision; for a real replacement it advances
consumer_generationand fences the old outstanding Delivery. The old coordinator can no longer mutate the Run or acknowledge that Delivery.The Run, Tasks, Dispatches, worker processes, PTYs, filesystems, and pending worker mail are preserved. Takeover fences the old coordinator, not the workers. A same-process remint is continuity rather than takeover and therefore does not advance the generation or discard/recreate Delivery.
What can only the current coordinator do?
Coordinator authority is per Run, not an OS-wide, repository-wide, or Orca-wide privilege. The current coordinator is the sole agent allowed to:
Workers retain only their exact Dispatch-scoped capability for heartbeat,
ask, escalation, andworker_done. That capability is also bound to the dispatched pane and process incarnation. A copied handle, bogus/replayed capability, or contradictory--fromgrants nothing.Explicit inspection (
run-show,task-list --run,gate-list --run, and inbox) remains read-only for non-owners and headless callers.check --peekis non-consuming but still coordinator-authorized for an ordinary current Run.binding.currentConsumer: falsemeans only “this caller is not attested as the current consumer”; it never proves the Run is unowned.Why does Orca have coordinators?
The coordinator is the single-writer/single-consumer boundary for a durable Run. Without it, two agents can race Task and Dispatch state, launch duplicate workers, resolve gates differently, acknowledge different views of the same Delivery, or both believe they own recovery. Workers remain independently authenticated so their lifecycle messages survive a legitimate coordinator replacement.
Are humans expected to operate these commands?
No. The normal principals are autonomous agent terminals: one coordinator agent and zero or more worker agents. There is no hidden “human override” in this authority model. The guide and CLI help therefore give agents machine-readable recovery:
effectsApplied,coordinatorStatus,claimantStatus,inspectCommandArgs,retryCommandArgs, andnextSteps.On rejection, an agent must follow the exact returned arguments with the same Orca executable.
coordinatorStatus: livemeans do not retry;unverifiablemeans restore owning-host connectivity;claimantStatus: changedmeans the claimant changed during proof and the returned retry may be run once from one stable replacement process.Classification and root cause
Distinct from STA-4390. Evidence for
run_7403b96004cashows a live macOS coordinator lost its binding once and had torun-useagain, advancingconsumer_generationto 8 while the app/runtime remained available. Database and source tracing plus deterministic regression tests prove that an ordinaryrun-usecould overwrite a live binding because persisted authority lacked process-incarnation/host identity and takeover was not atomically fenced against owning-host liveness. STA-4390 concerns Windows daemon/session identity replacement; this is a local coordinator-authority defect tracked as STA-5729.What changed
exitedproof for distinct ordinary replacement, revalidate the claimant, and atomically fence concurrent stale observations and prior Delivery.--fromvalues.run-use --help.Autonomous-agent validation
Real Codex (
gpt-5.5, xhigh) and Claude (opus, high) agents independently read the branch guide and branch-built CLI help and were quizzed on live, unverifiable, exited, same-process remint, read-only inspection, ordinary takeover, adopted legacy takeover, prohibited live-to-live transfer, copied handles/capabilities, and the agent-operated CLI model.Both chose the safe action in every scenario and correctly identified the coordinator-only powers and preserved state. Their ambiguity review led to an additional guide/help tightening: a one-shot
run-useis now the explicit decision point; no-effect failures exposecoordinatorStatus/claimantStatus;currentConsumer: falsenever means unowned; and nesting-depth handle classification never grants coordinator or Dispatch authority. The currently installed packaged runtime still serves its pre-PR guide, so the validation intentionally used the checked-in updated guide and branch-built CLI help that this PR will ship.Verification
origin/maininto the branch before final review.pnpm tc:node;pnpm tc:cli.git diff --check; max-lines ratchet; reliability gates./usr/local/bin/orca-devwas denied by local permissions.pnpm run check:code-quality:changedremains blocked in this shell because it is running Node 26 while the repository requires Node 24; the engine warning pollutes the command's oxlint JSON. Direct oxlint, oxfmt, typechecks, max-lines, reliability, bundled-guide/manifest, and diff checks are green.Electron validation used Playwright CDP only; no computer-use, accessibility, or OS-level input automation was used.
Residual risk
Physical Windows, WSL, SSH, Linux, and paired-runtime acceptance journeys were not run. Deterministic coverage exercises host-scoped liveness, daemon/session replacement, federation/version skew, folder workspaces, provider deadlines, pending Delivery, and additive wire behavior; no stream opcode changed. Host and client versions can roll independently, so an old authority-owning host retains pre-fix behavior until that host is updated.
Linear: STA-5729
Author: @BrennanKB5