Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe Merge Risk: 🔵 Low · up to Dispatch-show now sends optional caller identity metadata while retaining existing authorization behavior. A test-coverage gap remains around verifying that the metadata reaches the runtime handler, but no concrete runtime regression is established. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR sends callerTerminalHandle and keeps resolution best-effort, but the implementation resolves ORCA_TERMINAL_HANDLE and pane identity before using --from. Linked issue Resolution Change resolveOptionalCallerTerminalHandle so explicit --from is checked first, followed by live ORCA_TERMINAL_HANDLE validation, pane reminting, and omission when unresolved. Update tests and the PR description to match the implemented order.
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 |
26739e5 to
3e8021e
Compare
|
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. |
Greptile SummaryThe PR adds best-effort caller identity forwarding to
Confidence Score: 5/5The PR appears safe to merge with no actionable current failures identified. The optional identity is forwarded without becoming required, unknown RPC fields remain wire-compatible, and the runtime continues to return the same dispatch-show result because it does not yet consume the new field.
|
| Filename | Overview |
|---|---|
| src/cli/handlers/orchestration/dispatch-handlers.ts | Forwards a best-effort caller handle while retaining strict coordinator resolution for preamble previews. |
| src/cli/handlers/orchestration/terminal-identity.ts | Adds optional identity resolution that avoids focus-based attribution and preserves existing headless-read behavior. |
| src/main/runtime/rpc/methods/orchestration.ts | Extends the dispatch-show parameter schema so the optional caller identity survives request parsing. |
| src/cli/handlers/orchestration-dispatch-show-caller.test.ts | Exercises identity forwarding, fallback suppression, stale pane reminting, and preamble compatibility. |
Reviews (1): Last reviewed commit: "fix(cli): send the caller handle from di..." | Re-trigger Greptile
3e8021e to
ab691f1
Compare
|
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. |
|
Rebased onto Two conflicts, both from the file moves:
Two follow-ups the rebase surfaced:
Verified on Windows 11 / Node 24.19.0 / pnpm 12: The full No live CLI session was re-run against the rebased branch; the checks above are automated gates only. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/runtime/rpc/methods/orchestration/runs/tasks-dispatch.test.ts (1)
558-563: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake this RPC test assert
callerTerminalHandleat the method boundary.
rpc-test-harness.tsparses requests withDispatchShowParams, while the handler ignores this field. Sincez.objectstrips undeclared fields, removingcallerTerminalHandlefromDispatchShowParamswould not change the currentdispatch.task_idassertion. Add a boundary spy or parsed-request assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 0f1239d8-b327-4546-a7d5-3452ba8edcda
📒 Files selected for processing (7)
src/cli/handlers/orchestration-caller-identity-cli.test.tssrc/cli/handlers/orchestration-dispatch-show-caller.test.tssrc/cli/handlers/orchestration-module-boundaries.test.tssrc/cli/handlers/orchestration/dispatch-handlers.tssrc/cli/handlers/orchestration/terminal-identity.tssrc/main/runtime/rpc/methods/orchestration/runs/tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cli/handlers/orchestration/dispatch-handlers.ts
- src/cli/handlers/orchestration/terminal-identity.ts
- src/cli/handlers/orchestration-dispatch-show-caller.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
--from names the terminal to speak as, not the process speaking. Coalescing the two meant a preamble read reported the coordinator as its own caller, and when --from was absent the coordinator's focus-based fallback could name a pane the caller does not own. Resolve the caller from its own ambient identity (env handle, then pane remint) and fall back to --from only for a shell that carries no pane identity, so the Run scoping in stablyai#14898 keys on the real caller.
|
Closing. This PR only sends the caller's handle on |
ELI5
dispatch-shownever tells the runtime who is asking. That means any future rule of the form "you may only inspect dispatches belonging to your own Run" cannot work when the command is run from the CLI, however carefully the runtime enforces it. This sends the caller's identity along — and sends nothing when there is no identity to send, so no command that works today starts failing.What Changed
orca orchestration dispatch-shownow resolves a caller handle on every read and passes it ascallerTerminalHandle, following the patterntaskCreate/taskList/taskUpdatealready use andresolveRunScopealready consumes.Resolution is best-effort, via a new
resolveOptionalCallerTerminalHandleinsrc/cli/handlers/orchestration/terminal-identity.ts:--fromORCA_TERMINAL_HANDLEORCA_PANE_KEYundefinedAny failure at any step yields
undefinedrather than an error.callerTerminalHandlewas added toDispatchShowParams(src/main/runtime/rpc/methods/orchestration/schemas.ts) so zod does not strip it; the handler still ignores it, so authorization behaviour is unchanged.--preamblekeepsmain's strict resolution byte-for-byte and produces identical preamble text.Why
On
main,src/cli/handlers/orchestration/dispatch-handlers.ts:fromis resolved only for--preamble, and only so the previewed preamble text embeds a realistic coordinator handle.callerTerminalHandleis never sent at all. #14898 reports thatdispatchShowresolves another Run's context without a Run check; a fix for that needs to know who is calling, and today the runtime cannot know because the CLI does not say.Why best-effort, and why that matters. An earlier revision of this change made the resolution mandatory. A review caught that as a regression with no benefit: the handler ignores the field at this step, so requiring it could only break callers — a headless invocation, or one with no active terminal — for nothing.
--fromis a mechanical escape that requires knowing the handle and does not remove the problem.Why there is no focus-based fallback. An "active terminal" fallback selected by focus can, on a remote or multi-window host, pick a terminal belonging to someone else. Sending a wrong identity is worse than sending none. The test suite arms the focus selector to reject in
beforeEach, so any future code path that reaches for it fails the suite rather than silently working.Linked Issue
Fixes #15952
Unblocks #14898, which is the enforcement half and is not attempted here.
Visual Proof
N/A— CLI plumbing with no user-visible output change.--preambleoutput is byte-identical tomain.Testing
Rebase onto
upstream/main(2026-09-07, head9d29e6878e), after #16904 moved the RPC methods intosrc/main/runtime/rpc/methods/orchestration/*and split the CLI handlers. Run on Windows 11, Node 24.19.0, pnpm 12:The full
src/clirun has 13 pre-existing failures on this machine (executable-path resolution inorchestration-mutation-recovery,orchestration-gate-cli,orchestration-timeout-cli,orchestration-worker-cli,runtime/client-recovery). The identical 13 fail on a cleanupstream/maincheckout at9d29e6878ewith the same command, so they are environmental and not caused by this diff.Original implementation pass, before the rebase:
pnpm typecheck,pnpm build(includingbuild:native) andoxlintall exit 0; new suite 9/9; nine orchestration suites 148/148.New coverage for the four cases an earlier review named as missing: a caller with no resolvable identity, SSH/remote pane remint plus remote-pane-gone, the focus selector never being called (armed to reject, so any use fails the suite), and a folder workspace.
Platforms: Windows 11. The remote and folder-workspace tests exercise the resolver contracts rather than a live relay or a real folder workspace — stated plainly because a reviewer should not read them as end-to-end. The rebase pass above is automated gates only; no live CLI session was re-run against it.
AI Disclosure
Written with Claude Opus 5 at high effort under my direction and reviewed by an independent Codex (
gpt-5.6-sol) pass, which raised the mandatory-resolution regression as a P1 and confirmed it closed on a second verification pass. The rebase ontoupstream/mainwas likewise done with Claude Opus 5.Review
Cross-platform: no path handling.
SSH / remote: the pane-remint step is the remote-aware branch; failures there are absorbed, and the focus fallback that could select a foreign terminal was deliberately excluded.
Wire compatibility: a new optional param on an existing RPC call, ignored by the handler — safe in both directions today. Making it required later would break an old CLI against a new runtime with no fallback, so the enforcement step needs a capability or a fallback decided first, per
docs/reference/remote-wire-compatibility.md.Agent compatibility: unaffected.
Performance: at most one extra handle validation per
dispatch-show.UI: none.
Security: sends an identity the runtime already receives from sibling commands; it grants nothing, since the handler ignores it.
Notes
Rebase notes, for reviewers comparing against the pre-#16904 version of this branch:
DispatchShowParamsis now insrc/main/runtime/rpc/methods/orchestration/schemas.ts, not the old monolithicorchestration.ts.isLiveTerminalHandleonmainnow probesterminal.resolveIdentitybefore falling back toterminal.show, so the new suite's mocks assertterminal.resolveIdentity.orchestration.test.tshad moved toorchestration-caller-identity-cli.test.ts; it is deleted there instead, sinceorchestration-dispatch-show-caller.test.tscovers the same scenario plus the new field.orchestration-module-boundaries.test.tsnow clearsORCA_TERMINAL_HANDLE/ORCA_PANE_KEYin itsbeforeEach. Without that, its boundary assertion depended on whether the suite happened to run inside a live Orca pane.Known prerequisites for the enforcement step, recorded so they are not rediscovered (all re-verified against
9d29e6878e):requireWorkerDoneSettlementcallsdispatchShowwith only{ task }from the worker CLI and would break if the handle became required.dispatchShowis absent fromCOORDINATOR_PREFLIGHT_METHODS/CURRENT_AUTHORITY_PREFLIGHT_METHODSand fromcurrentCallerHandleinsrc/main/runtime/rpc/orchestration-legacy-compatibility.ts, so the handle would not be recognised on the legacy-compatibility path.Agent skill upstream boundary
Checklist
N/Awith reason for visual proofpnpm lint,pnpm typecheckandpnpm buildpass locallyAuthor