fix(orchestration): quarantine completed workers without an explicit role lease - #9946
fix(orchestration): quarantine completed workers without an explicit role lease#9946connfy wants to merge 2 commits into
Conversation
…role lease Prevent post-worker_done panes from executing coordinator control ops or self-promoting from ordinary user chat; require a durable role-lease ceremony or fresh redispatch (ORCH-R15). Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds durable coordinator role leases and authority resolution based on dispatch, handle, and pane identity. Orchestration RPC methods now validate coordinator control and accept caller identity metadata. CLI commands propagate terminal and pane identity, with a new 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/runtime/rpc/methods/orchestration-role-lease.test.ts (1)
168-183: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest claims heartbeat coverage but doesn't exercise it.
The test title says "worker_done / heartbeat sends" but only sends a
worker_donemessage; noheartbeat-type send is issued. Since the PR objectives explicitly call out heartbeat as a path that "remain[s] available," add an assertion (or a second test) that actually sendstype: 'heartbeat'and confirms it isn't blocked by the role-lease guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc73e470-8021-49ed-8b79-f4dfb6723dbb
⛔ Files ignored due to path filters (1)
src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
src/cli/handlers/orchestration.tssrc/cli/specs/orchestration.tssrc/main/runtime/orchestration/db.tssrc/main/runtime/orchestration/preamble.test.tssrc/main/runtime/orchestration/preamble.tssrc/main/runtime/orchestration/role-lease.test.tssrc/main/runtime/orchestration/role-lease.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/rpc/methods/orchestration-gates.tssrc/main/runtime/rpc/methods/orchestration-role-lease.test.tssrc/main/runtime/rpc/methods/orchestration-role-lease.tssrc/main/runtime/rpc/methods/orchestration.test.tssrc/main/runtime/rpc/methods/orchestration.ts
| // Why: ORCH-R15 role checks must see the live pane, not a spoofable --from alone. | ||
| async function resolveCallerRoleIdentity( | ||
| client: Parameters<CommandHandler>[0]['client'] | ||
| ): Promise<{ callerTerminalHandle?: string; callerPaneKey?: string }> { | ||
| const callerTerminalHandle = await resolveTaskCreatorTerminalHandle(client) | ||
| const envPane = process.env.ORCA_PANE_KEY | ||
| return { | ||
| callerTerminalHandle, | ||
| callerPaneKey: envPane && envPane.length > 0 ? envPane : undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when live caller identity is unavailable.
resolveCallerRoleIdentity() can return no callerTerminalHandle, while the RPC contract falls back to callerTerminalHandle ?? from. Consequently, Line 786 and other --from-accepting commands can let a user-controlled handle become the authorization identity, defeating the “not spoofable by --from alone” guarantee. Reject identity-less protected calls, or prevent from from being used as the caller identity.
Also applies to: 786-786
| function isEquivalentPaneKey(a: string, b: string): boolean { | ||
| if (a === b) { | ||
| return true | ||
| } | ||
| const aLeaf = parsePaneKey(a)?.leafId | ||
| const bLeaf = parsePaneKey(b)?.leafId | ||
| return Boolean(aLeaf && bLeaf && aLeaf === bLeaf) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate isEquivalentPaneKey — extract to shared module.
Identical logic already exists as a private helper in db.ts (lines 22-29). Both files already import parsePaneKey from ../../../shared/stable-pane-id, so this is a good candidate to move there once and import in both places — avoids two independently-maintained copies of identity-matching logic that authority resolution and dispatch lookups both depend on.
♻️ Proposed fix
-function isEquivalentPaneKey(a: string, b: string): boolean {
- if (a === b) {
- return true
- }
- const aLeaf = parsePaneKey(a)?.leafId
- const bLeaf = parsePaneKey(b)?.leafId
- return Boolean(aLeaf && bLeaf && aLeaf === bLeaf)
-}
+import { isEquivalentPaneKey } from '../../../shared/stable-pane-id'And remove the duplicate private copy from db.ts, importing it there too.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7350682a-4ae1-4684-90c3-44fee6d43d6e
📒 Files selected for processing (10)
src/cli/handlers/orchestration.test.tssrc/cli/handlers/orchestration.tssrc/main/runtime/orchestration/db-empty-dispatch-shortcircuit.benchmark.test.tssrc/main/runtime/orchestration/db.test.tssrc/main/runtime/orchestration/db.tssrc/main/runtime/orchestration/role-lease.test.tssrc/main/runtime/orchestration/role-lease.tssrc/main/runtime/rpc/methods/orchestration-role-lease.test.tssrc/main/runtime/rpc/methods/orchestration-role-lease.tssrc/main/runtime/rpc/methods/orchestration.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/runtime/rpc/methods/orchestration-role-lease.ts
- src/main/runtime/orchestration/role-lease.ts
- src/main/runtime/rpc/methods/orchestration-role-lease.test.ts
- src/main/runtime/orchestration/db.ts
| const second = dispatchWorker(d, { | ||
| handle: 'term_worker', | ||
| paneKey: 'tab_w:leaf_w', | ||
| spec: 'redispatched work' | ||
| }) | ||
| d.updateTaskStatus(second.task.id, 'completed') | ||
| d.completeDispatch(second.ctx.id) | ||
|
|
||
| expect( | ||
| d.findActiveCoordinatorLease({ | ||
| handle: 'term_worker', | ||
| paneKey: 'tab_w:leaf_w' | ||
| }) | ||
| ).toBeUndefined() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert lease consumption before completing the redispatch.
The current assertion runs after completeDispatch(second.ctx.id), so it cannot prove that redispatch revoked the lease. Assert immediately after dispatchWorker, then retain the quarantine assertion after completion.
Proposed test adjustment
const second = dispatchWorker(d, {
handle: 'term_worker',
paneKey: 'tab_w:leaf_w',
spec: 'redispatched work'
})
- d.updateTaskStatus(second.task.id, 'completed')
- d.completeDispatch(second.ctx.id)
-
expect(
d.findActiveCoordinatorLease({
handle: 'term_worker',
paneKey: 'tab_w:leaf_w'
})
).toBeUndefined()
+ expect(
+ resolveOrchestrationAuthority(d, {
+ handle: 'term_worker',
+ paneKey: 'tab_w:leaf_w'
+ }).role
+ ).toBe('worker')
+
+ d.updateTaskStatus(second.task.id, 'completed')
+ d.completeDispatch(second.ctx.id)
|
Withdrawn at owner direction. The motivating event was an operator mistaking a completed worker terminal for the coordinator, not a product defect that should constrain Orca role changes. Intentional worker-to-coordinator promotion is a legitimate workflow, so this role-lease/quarantine design and all follow-up hardening are being fully withdrawn without merge. |
1 participant
Summary
worker_donethe pane is post-completion quarantined and cannot run coordinator lifecycle/task-control ops (task-create/update, dispatch, reset, run/run-stop, gate-*, coordinator-shaped sends, self role-lease grant).orchestration role-lease --role coordinatorceremony (or never having been a worker / fresh redispatch). Ordinary owner chat is not promotion; denial is fail-closed with no mutation.Integration with open PRs
Test plan
pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orchestration/role-lease.test.ts src/main/runtime/rpc/methods/orchestration-role-lease.test.ts src/main/runtime/orchestration/preamble.test.ts src/main/runtime/rpc/methods/orchestration.test.ts src/main/runtime/orchestration/db.test.ts src/main/runtime/orchestration/lifecycle-reconciliation.test.ts src/cli/handlers/orchestration.test.ts src/main/runtime/orchestration/db-empty-dispatch-shortcircuit.benchmark.test.ts(276 passed)pnpm run typecheck:nodepnpm run typecheck:clipnpm exec oxlint --quiettask-create/gate-create/ coordinator-shaped send are denied and row counts remain unchanged.role-lease --role coordinatorpersists across DB reopen and allows the promoted pane to create work.Full
pnpm run lintreaches a pre-existing unrelatedswitch-exhaustiveness-checkfailure insrc/renderer/src/components/skills/skill-freshness-group.tsx; the branch's own switch error was fixed, andoxlint --quietplus the max-lines ratchet pass.Made with Cursor