Repository navigation
fix(orchestration): stop minting and printing the dispatch capability - #23994
Conversation
…ability Worker lifecycle reports and questions no longer depend on the per-dispatch capability token that lives only in the agent's conversation. The host now: - ignores capability_hash/capability_revoked_at for authorization on every row and checks the exact worker process instead (ask gains that check); - refuses a report whose calling terminal is provably another orchestration party (a Run coordinator or another Dispatch's worker), treating env that names no live pane here as absent; - applies one worker-state rule locally and remotely: a stop in flight refuses, while stop_unknown and start_unknown accept and settle. Minting and printing the flag are unchanged, so an older host and older preambles keep working.
Dispatches no longer mint a per-Dispatch token, and preambles, the bundled skill guide and the ask resume hint stop printing --dispatch-capability. The consumer-generation bump and delivery fence that minting carried stay, now as setDispatchConsumer. Readers that inferred meaning from capability_hash read what they meant instead: worker-show's injected stage comes from the attached consumer, and a failed start copies custody identity only when no authority was ever attached. The CLI keeps accepting and forwarding the flag for older hosts. Cancelling a Task is recorded as failed with a reason; task-list now shows that reason and the guide and task-update notes document the recipe.
…lity-stop-minting
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| state: 'unsupervised' as const, | ||
| stage: dispatch.capability_hash ? 'injected' : 'context_only', | ||
| // Why: only `dispatch --inject` attaches a consumer to a Dispatch that has no worker row. | ||
| stage: dispatch.consumer_generation > 0 ? 'injected' : 'context_only', |
There was a problem hiding this comment.
worker-show stage from injected to context_only, making the displayed stage unreliable for those existing Dispatches.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one incomplete skill cleanup, plus a couple of stale test remnants.
Reviewed changes
- Capability minting removed —
mintDispatchCapabilitybecomessetDispatchConsumer(still bumpsconsumer_generation, fences unacked deliveries, returnsvoid);dispatch-capability.ts/dispatch-capability-hash.tsdeleted;prepareStartingWorkerAuthorityandprepareRemoteAttachmentAuthoritystop writingcapability_hash. - Preamble no longer prints the flag —
buildDispatchPreambledropsdispatchCapability; the worker contract,askresume hint, and recovery text are updated. worker-showstage re-sourced — an unsupervised Dispatch'sinjected/context_onlynow readsconsumer_generation > 0instead ofcapability_hash.- Failed-start identity guard —
recordFailedStartDispatchIdentitynow keys onprocess_incarnation IS NULL, with a new test that fails on the old predicate. - Cancellation convention —
task-listappends a failed Task'sresult, andtask-updatedocs plus the skill describefailed+result: cancelled. - Test/e2e updates — capability plumbing removed across federation, current-authority, commit-notify, SSH-bridge, and completed-worker fixtures.
ℹ️ Two bundled-skill spots still tell workers to preserve a capability argument
worker-contract.md now says "keep any other flag it carries (an older Orca host adds --dispatch-capability)", but two other bundled guides still name a capability argument the preamble no longer carries: orchestration.md ("Workers use the exact executable, handle, capability, Task ID, and Dispatch ID in the live preamble") and messaging-and-gates.md ("a dispatched worker instead copies the exact --from and capability arguments in its preamble"). Both are served to workers, so a worker on a new host reads guidance for an argument that isn't there. A one-line reword in each matches the worker-contract.md treatment.
ℹ️ Nitpicks
worker-start-readiness-settlement.test.tsstill passesdispatchCapability: 'capability'inside itsas neverargs object; the field is now ignored.orchestration-structured-worker-start-failure.test.tsstubsprepareStartingWorkerAuthority: () => 'capability'even though the method now returnsvoid.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…nt party - One module owns the unproven/settleable worker states and the refusal rule; local send records it, ask and remote throw it. A stale process is worker_identity_changed on every path. - The caller fence passes the worker's own terminal when its --from handle went stale. - Document the shared-tmux-server limit; drop the dead dispatch_capability_invalid rejection member; tests assert dispatch state, not the capability column.
…y' into dispatch-capability-stop-minting # Conflicts: # src/cli/bundled-skill-guides.ts
…lity retention - dispatch --inject no longer re-points the row createDispatchContext just wrote; worker-show reports every worker-less Dispatch as context_only, since Orca keeps no record of the paste. Tests re-point through a fixture. - failWorkerStart always records when the lifecycle closed; nothing authorizes on it. - Restore the ask resume hint's echo of a passed --dispatch-capability: an old host checks it before --resume. - Move the cancellation convention to #23983.
There was a problem hiding this comment.
ℹ️ No issues in the new code — the increment is a clean refactor. One description mismatch to fix.
Reviewed changes
Since the prior review at 3a54435, commit 3f8c829 refactors and reverts several parts of the PR:
- Production
setDispatchConsumerremoved —dispatch --injectno longer re-points the rowcreateDispatchContextjust wrote; it already carriesassigneePaneKey/processIncarnation, so report authority and mailbox generation reads stay consistent. The re-point survives only as areattachDispatchConsumertest fixture. worker-showstage simplified — every worker-less Dispatch now reportscontext_only; theconsumer_generation > 0 ? 'injected' : …sourcing is gone.failWorkerStartdropsretainCapability—capability_revoked_atis now always recorded, and nothing reads it.askresume hint restored — it echoes a passed--dispatch-capabilityagain, for an older host that verifies it before--resume.- Cancellation convention removed from this PR — the
task-listfailure-reason output, thetask-updatenotes/spec, and the skill docs moved to #23983.
ℹ️ The PR description still describes the pre-3f8c829 changes
The body and the "What Changed" table were written before commit 3f8c829, so they now promise three things the diff no longer does: the task-list cancellation reason and task-update guidance (moved to #23983), the ask timeout hint not echoing the token (restored for old hosts), and worker-show sourcing injected/context_only from consumer_generation > 0 (now always context_only). Reviewers reading the description will look for behavior the branch doesn't have.
Technical details
# Update the PR description to match the head commit
## Affected sites
- PR description, `ELI5` and "What Changed" table — claims cancellation is a readable `task-list` outcome and that `task-update` documents it.
- PR description, "`ask` times out" row — says the resume hint has no token; `question-handler.ts:98-113` now echoes a passed `--dispatch-capability`.
- PR description, mechanism bullet on readers — says `worker-show` reads `stage` from `consumer_generation > 0`; `worker-observation.ts:181` now hardcodes `context_only`.
- PR description, "Testing" section — references `orchestration-task-list-failure-reason.test.ts` and `manual-dispatch-observation` keeping `stage: injected`, both removed/changed.
## Required outcome
- The description matches the diff at `3f8c829`: cancellation explicitly deferred to #23983, the `ask` hint described per its restored behavior, and `worker-show` described as always `context_only` for worker-less Dispatches.
## Suggested approach (optional)
- Reuse the commit body of `3f8c829`, which already enumerates exactly these four deltas.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…y' into dispatch-capability-stop-minting # Conflicts: # src/main/runtime/rpc/methods/orchestration/messaging/send.test.ts
…p test fixtures - RpcRequest and the SSH bridge stop carrying orchestrationCapability; the CLI's wire field stays for older hosts. - Fixtures pass identity to createRootDispatch instead of re-pointing to the same values; drop absence checks for a flag that can no longer be produced.
There was a problem hiding this comment.
ℹ️ No new code issues — the increment is a clean refactor. A few capability-cleanup remnants remain.
Reviewed changes
Since the prior review at 3f8c829, two commits landed: c43e6a5 merges the advanced base, and 60b9005 is the refactor.
- Removed the internal
RpcRequest.orchestrationCapability— the main-runtime request type no longer carries the field. The wire envelope (RuntimeOrchestrationEnvelope) and thesrc/clisend/ask handlers still forward it, so an older host keeps receiving the flag. - SSH relay bridge stops forwarding the capability —
ssh-remote-orca-cli.tsnow passescompatibilityEnvelopethrough and drops the per-callorchestrationCapability, consistent with the host ignoring it. - Test identity set at creation instead of re-pointing — current-authority, mutation-ledger, runtime-update-settlement, and send-dispatch-authority tests pass
paneKey/processIncarnationtocreateRootDispatchrather thanreattachDispatchConsumer; the latter stays a fixture for suites that genuinely fence a consumer. - Tautological absence checks dropped — preamble/federation/new-worktree prompts no longer assert the absence of a flag that can no longer be produced, and
capability_hash: nullexpectations are gone. - Base merge —
c43e6a5brings the base branch's removal of theexpectDispatchActivehelper and the pre-upgrade-capability test; identity coverage survives insend.test.tsandworker-report-authority.test.ts.
Verification: ran the 17 changed unit-test files (preamble, send, composed-workers, manual-dispatch-observation, worker-interactive-wait, worker-start-prompt-contract, workers-new-worktree, orchestration-current-authority-precedence, orchestration-mutation-ledger, orchestration-runtime-update-settlement, orchestration-worker-dispatch-db, ssh-remote-orca-cli, db-task-dispatch-lifecycle-guards, send-dispatch-authority, and the three federation suites) — 276 passed.
ℹ️ The capability cleanup is still incomplete
60b9005's stated goal is dropping no-op capability fixtures, but it leaves two in place, and two bundled guides still tell workers to copy a "capability" argument the preamble never carries. The description also still claims the internal field survives.
Technical details
# Finish the capability cleanup
## Affected sites
- `src/main/runtime/rpc/methods/orchestration/worker/worker-start-readiness-settlement.test.ts:45` — still passes `dispatchCapability: 'capability'` in the no-longer-needed args object.
- `src/main/runtime/orchestration-structured-worker-start-failure.test.ts:113` — still stubs `prepareStartingWorkerAuthority: () => 'capability'`, a method that now returns `void`.
- `skill-guides/orchestration.md:52` — "Workers use the exact executable, handle, capability, Task ID, and Dispatch ID in the live preamble".
- `skill-guides/orchestration/references/messaging-and-gates.md:39` — "a dispatched worker instead copies the exact `--from` and capability arguments in its preamble".
- PR description, "Mechanism" bullet and "No new wire fields. `RpcRequest.orchestrationCapability` stays" line — the field was removed by `60b9005`.
## Required outcome
- Drop the two no-op test fixture fields.
- Reword the two bundled guides to drop "capability", mirroring `worker-contract.md` (which now says only that an older host adds the flag).
- Update the description so it reflects the removed internal field and the SSH-bridge change; `RuntimeOrchestrationEnvelope.orchestrationCapability` is the field that remains for older hosts.
## Note
- The two guide lines were raised in the initial review and are unchanged by these commits; they are flagged again only because this commit's stated scope is a capability cleanup.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…y' into dispatch-capability-stop-minting
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review at 60b9005, commit 07b297e finishes the capability cleanup that review asked for — no production behavior change.
- Bundled skill guides reworded —
orchestration.mdandmessaging-and-gates.mdno longer tell workers to preserve a "capability" argument the live preamble cannot carry; both now say the worker keeps whatever flags the preamble does carry. - Embedded guides regenerated —
bundled-skill-guides.tscarries the reworded strings;verify:bundled-skill-guidespasses clean. - No-op test fixtures removed —
worker-start-readiness-settlement.test.tsdrops the ignoreddispatchCapability: 'capability'arg, andorchestration-structured-worker-start-failure.test.tsstubsprepareStartingWorkerAuthorityas() => {}to match itsvoidreturn. - PR description updated — the body now describes the removed
RpcContext/RpcRequestfield and the SSH-bridge change, withRuntimeOrchestrationEnvelope.orchestrationCapabilitynamed as the field kept for older hosts.
Verified locally: verify:bundled-skill-guides (exit 0), orchestration-skill-guidance.test.mjs (29 passed), and the two touched test files (3 passed). Remaining dcap_ / dispatchCapability references in src/cli handlers and transcript-redaction tests are the intentional older-host compatibility path documented in the body.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…y' into dispatch-capability-stop-minting
…y' into dispatch-capability-stop-minting
…top-minting # Conflicts: # src/cli/bundled-skill-guides.ts # src/main/runtime/orchestration/coordinator-dispatch-unobserved-prompt.test.ts # src/main/runtime/orchestration/db-task-dispatch-invariant.test.ts # src/main/runtime/orchestration/db-task-dispatch-lifecycle-guards.test.ts # src/main/runtime/orchestration/db/dispatch-context/dispatch-capability.ts # src/main/runtime/orchestration/db/federation/remote-dispatch-attachment-authority.ts # src/main/runtime/orchestration/worker-start-unobserved-prompt-settlement.test.ts
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (65)
💤 Files with no reviewable changes (13)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change removes Dispatch capability creation and transport from worker setup, generated commands, RPC requests, and SSH compatibility paths. Dispatch authority preparation now returns no capability, and worker preambles omit the capability flag. Tests and guidance are updated to reflect these changes. The change also updates keyed retry guidance to specify using the same terminal. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to This change stops issuing per-dispatch capability tokens and relies on worker identity and lifecycle checks instead. No concrete defect was found. Live remote or SSH worker behavior was not exercised end to end, so owners should watch that path after merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Removing the unused secret reduces its exposure in worker instructions without further weakening the inspected authorization checks. No introduced authorization bypass was established, but remote caller identity and some recovery and compatibility paths remain incompletely verified. 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 | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation
Resolution Implement an out-of-band capability path required by Full details: Out of Scope Changes checkExplanation The PR changes behavior that Resolution Limit the PR to out-of-band capability delivery and the required readers, preamble, and tests. Keep existing minting and revocation behavior unless a separate linked issue requires changing it. If the no-capability design is intentional, split the lifecycle-authority and revocation changes into a separately scoped issue and PR. Full details: Docstring CoverageExplanation Docstring coverage is 17.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 48 files. (3 skipped: 3 unsupported.)
✨ 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 |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review at 07b297e, the only new commit is 275b5ed, a merge of origin/main. No PR-owned file changed, so there is no new behavior to review.
- Base retargeted to
main—#23982, which this PR was stacked on, is now merged intomainase0387040, and275b5edmerges it in. Restricted to the PR's files,git diff b2320c0..275b5edis empty (the merge touched only 259 base-owned files), and the net diff againstmainis unchanged (65 files, +316/−529).
ℹ️ The description still frames this PR as stacked on #23982
The body opens with "Stacked on #23982 (review that first; this diff is the second commit onward)". #23982 is now merged into main and this PR targets main, so the diff is no longer "the second commit onward" and there is no separate parent to review first — the sentence now misdescribes what a reviewer is looking at.
Technical details
# Update the PR body's stack note
## Affected sites
- PR description, second line — "Stacked on #23982 (review that first; this diff is the second commit onward)".
## Required outcome
- The body reflects that the base is `main` and #23982 is merged, so the diff is this PR's changes alone.
## Suggested approach (optional)
- State that #23982 has merged to `main`, or drop the line.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
bundled-skill-guides.ts conflicted (generated); regenerated from the merged skill-guides sources. Main dropped the dispatch capability from the preamble params (#23994) and added a guide line, so the parity test stops passing a capability and bounds the kernel at main's new length plus the address line.
…rker no longer has #23994 stopped minting the per-dispatch capability, so capability_hash is null. The --help spawn failure used to stop this test before it got here.
…spawn (#24440) * test(e2e): fake Codex answers the --no-daemon --help probe without a spawn Since #23933 Orca runs `codex --help` before a path-named Codex launch. The fakes logged it as an agent spawn and held the probe for its 5 s timeout. Move the app-server refusal and --help answer into one shared FAKE_CODEX_LAUNCH_PROBES_SOURCE used by every fake Codex. * test(e2e): stop asserting the dispatch capability column a current worker no longer has #23994 stopped minting the per-dispatch capability, so capability_hash is null. The --help spawn failure used to stop this test before it got here.
ELI5
#23982 made the Orca host stop checking the secret token that supervised workers had to repeat on every report. This PR stops creating the token at all, and stops printing it: preambles and the bundled orchestration skill no longer mention
--dispatch-capability. Workers now report with the plain command.Stacked on #23982 (review that first; this diff is the second commit onward).
What Changed
Before → after:
worker-start,dispatch --inject, remote workersdcap_…token, store its hash, paste it into the preamblesend/asklines have no--dispatch-capabilityworker-contract.md)--dispatch-capability <capability>asktimes out--resume)worker-showfor an unsupervised dispatchstage: injectedafterdispatch --inject(read from the token hash), elsecontext_onlystage: context_only; Orca keeps no record of the paste, and nothing reads the distinctionMechanism:
mintDispatchCapabilityis deleted: ondispatch --injectit re-pointed the rowcreateDispatchContexthad just written with the same pane and process, so its only lasting effect was the hash.prepareStartingWorkerAuthorityandprepareRemoteAttachmentAuthoritykeep their re-point and consumer fencing and return nothing.dispatch-capability-hash.tsis deleted, and the host dropsorchestrationCapabilityfromRpcContext,RpcRequestand the in-process SSH CLI bridge, because nothing on the host reads it.capability_hashnow read what they meant:worker-show's stage for a worker-less Dispatch iscontext_only; theinjectedvalue was only ever derived from the hash.process_incarnation IS NULL), instead ofcapability_hash IS NULL, which would now always be true. Without this, a stalled start that kept its authority would have its pane overwritten from the custody row (the [Bug]: supervised worker-start: lost prompt injection reported as input_accepted, dispatch capability bound to wrong pane #17741 mismatch) and its lateworker_donerejected.failWorkerStart'sretainCapabilityoption is gone: nothing authorizes oncapability_revoked_atsince fix(orchestration): accept worker reports without the dispatch capability #23982, and its type comment now says it means "lifecycle closed at", so a failed start records it like every other close.--dispatch-capability, so a new CLI still works against an older host that mints and checks it.Why
Once the host ignores the token (#23982), printing it only costs: secret scanners block commands that contain it, it sits in argv and transcripts, and agents lose it on compaction. Stopping minting removes the last reason for workers to see it.
Alternatives considered:
--help/ reject it: would break every older host's preamble. The CLI keeps forwarding it;sendandaskusage already omit it.inject_modecolumn forworker-show: a schema bump for a field with no in-repo consumer.setDispatchConsumerto bumpconsumer_generationas an "injected" flag: a DB method and transaction whose only effect is a counter used as a boolean.Mixed versions
askresume hint echoes itworker-showstagereadscontext_onlyafterdispatch --injecttoo; no client branches on itNo new wire fields. The CLI still sends
orchestrationCapability(the sharedRuntimeOrchestrationEnvelope) for older hosts; this host parses requests as plain JSON and ignores the extra field.Linked Issue
Part of STA-8833 (item 1). Items 2 and 3 are out of scope.
Fixes #17632
Refs #23656 (item 1 only), #16903, #12769, #15124. The cancellation convention (decision 7) is in #23983.
Visual Proof
N/A — CLI text and orchestration state change with no UI. The end-to-end transcript below shows the new preamble lines.
Testing
End to end (macOS, headless isolated runtime built from this branch; fake Codex worker started in a tab; worker commands run inside the worker's own terminal): the injected
worker_doneline anddispatch-show --preamblenow readorca-dev orchestration send --from <worker> --type worker_done … --task-id … --dispatch-id … --outcome succeeded. The coordinator reporting as the worker isconsumer_fenced;askcopied fromdispatch-show --preambleis accepted; the worker'sworker_donecompletes the Task;worker-stopon a user-opened tab endsstop_unknownand the lateworker_donecompletes the Task and moves the worker tosucceeded.Playwright (electron-headless):
orchestration-worker-settlement-release-cli,orchestration-worker-transcript-providersandsettled-worker-tab-survives-restart(daemon live) pass with the fake agents no longer depending on the token.completed-worker-retirement-resumeandsettled-worker-tab-survives-restart(daemon exited) fail identically onmainata0abcb6818b, before the lines this PR touches: the fake Codex also sees acodex --helpspawn.Unit: preamble tests assert no
--dispatch-capabilityordcap_; worker-start, composed-worker, new-worktree and federation tests assert the prompt carries no token andcapability_hashstays null;worker-start-unobserved-prompt-settlementgains a case that a stalled start keeps the identity its authority attached (fails with the oldcapability_hash IS NULLpredicate);manual-dispatch-observationexpectsstage: context_only; consumer-fencing suites re-point through a test fixture;orchestration-skill-guidance.test.mjsasserts the recipes carry no flag.Commands run:
./node_modules/.bin/vitest run --config config/vitest.config.ts src/main/runtime src/main/ssh src/main/ipc src/cli src/shared config/scripts/orchestration-skill-guidance.test.mjs— all pass except fish-shell tests that depend on the local fish installpnpm run typecheck:tsc:node,pnpm run typecheck:tsc:cli— cleanoxlinton changed files,pnpm run audit:anti-slop,pnpm run check:code-quality:changed,pnpm run verify:bundled-skill-guides,pnpm run verify:skill-bundle-manifest— cleanWindows ("windows 2", Git Bash, worktree at
3a5443552e, which includes this change):vitest runoversrc/main/runtime/orchestration,.../rpc/methods/orchestration/messaging, andorchestration-mutation-recovery— 144 files pass.orchestration-mutation-recovery.test.tsfails 3 cases with Windows argument quoting ("orca-dev" "orchestration" …);main's copy of that file fails the same 3 cases on the same machine, because the test doesn't pinprocess.platform.Not tested: a live SSH/connected-server worker end to end.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Follow-ups left out on purpose: deleting the
dcap_transcript redaction and thecapability_hash = NULLwrites on remote attachments waits for a downgrade window (they keep rows minted by older hosts safe); the preamble can later drop--fromand let the CLI use its own identity (#14570).Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)