Repository navigation
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78c4bf49-1f9c-4a17-9a80-38a39f7eb4ac
📥 Commits
Reviewing files that changed from the base of the PR and between e4e54a1 and c8b46454f4fe006d2394963fad3ca93e609775e0.
📒 Files selected for processing (8)
src/cli/handlers/orchestration-dispatch-show-scope.test.tssrc/cli/handlers/orchestration-lifecycle-rejection.test.tssrc/cli/handlers/orchestration-worker-settlement.tssrc/cli/handlers/orchestration.test.tssrc/cli/handlers/orchestration.tssrc/cli/specs/orchestration.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration.ts
💤 Files with no reviewable changes (1)
- src/cli/handlers/orchestration.test.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 7 remain after this review.
|
@coderabbitai review |
|
Jinwoo-H
changed the title
|
Final owner validation on Three independent Luna review scopes finished clean after validated fixes for trusted-presentation compound lookup, identity-free lifecycle callers, zero-lookup proof, and renderer IPC coverage. The completed-worker retirement spec's later PaneManager mount failure reproduces unchanged on latest main; both candidate close modes first passed the new trusted dispatch read, so it is documented as a pre-existing neighboring failure rather than masked here. The PR remains non-draft and unmerged; CI on this pushed head is the remaining gate. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/runtime/orca-runtime.ts (1)
13311-13338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a short comment explaining the remint gate.
The reminting block (deriving a fresh handle from
claimedPaneKey, then adopting it only when the reminted authority'spaneKeymatches) has no inline explanation. Every other non-obvious block in this file carries a one-line// Why:comment. Static analysis also flags this segment's complexity as high.Add a brief
// Why:comment stating that the remint is safe becauselaunchTokenHashis still checked against whichever terminal ends up resolved, so a caller cannot widen its authority by supplying an arbitraryclaimedPaneKey. Consider extracting the remint lookup into a small private helper (e.g.,resolveRemintedOrchestrationTerminal(claimedPaneKey)) to keepverifyOrchestrationCompatibilityCalleritself shorter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a831c3c1-bb4f-480e-919e-c84f61f5a680
📥 Commits
Reviewing files that changed from the base of the PR and between 0cc1f02babb1a19388be6df1a8b65b5f3eaa8605 and 478880cd946af7f799506ec07a0d9806ba5b6adc.
📒 Files selected for processing (20)
config/reliability-gates.jsoncsrc/main/ipc/runtime.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration-compatibility-authority.test.tssrc/main/runtime/orchestration/db.tssrc/main/runtime/rpc/core.tssrc/main/runtime/rpc/dispatcher.tssrc/main/runtime/rpc/methods/orchestration-check.test.tssrc/main/runtime/rpc/methods/orchestration-dispatch-show-run-scope.test.tssrc/main/runtime/rpc/methods/orchestration-manual-dispatch-release.test.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration.tstests/e2e/completed-worker-retirement-resume.spec.tstests/e2e/orchestration-dispatch-presentation.tstests/e2e/orchestration-dispatch-show-run-isolation.spec.tstests/e2e/orchestration-legacy-worker-missing-terminal-recovery.spec.tstests/e2e/orchestration-legacy-worker-restart-recovery.spec.tstests/e2e/orchestration-low-level-dispatch-release.spec.tstests/e2e/orchestration-worker-settlement-release-cli.spec.tstests/e2e/orchestration-worker-terminal-visibility.spec.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 6 remain after this review.
|
@coderabbitai review |
|
2cb55fd to
5527d66
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. |
|
Owner revalidation on rebased head
PR remains non-draft, open, unmerged, and current with main. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f02edb61-3b69-4070-a53d-fd371f95a920
📥 Commits
Reviewing files that changed from the base of the PR and between 6ee265e and 5527d66d4324751f77156e566deab9c7338caf84.
📒 Files selected for processing (30)
config/reliability-gates.jsoncsrc/cli/handlers/orchestration-dispatch-show-scope.test.tssrc/cli/handlers/orchestration-worker-settlement.tssrc/cli/handlers/orchestration.test.tssrc/cli/handlers/orchestration.tssrc/cli/runtime-client.test.tssrc/cli/runtime/client.tssrc/cli/specs/orchestration.tssrc/main/ipc/runtime.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration-compatibility-authority.test.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-context-store.tssrc/main/runtime/orchestration/db/tasks/task-store.tssrc/main/runtime/rpc/core.tssrc/main/runtime/rpc/dispatcher.tssrc/main/runtime/rpc/methods/orchestration-check.test.tssrc/main/runtime/rpc/methods/orchestration-dispatch-show-run-scope.test.tssrc/main/runtime/rpc/methods/orchestration-manual-dispatch-observation.test.tssrc/main/runtime/rpc/methods/orchestration-manual-dispatch-release.test.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration.tssrc/shared/protocol-version.tstests/e2e/completed-worker-retirement-resume.spec.tstests/e2e/orchestration-dispatch-presentation.tstests/e2e/orchestration-dispatch-show-run-isolation.spec.tstests/e2e/orchestration-legacy-worker-missing-terminal-recovery.spec.tstests/e2e/orchestration-legacy-worker-restart-recovery.spec.tstests/e2e/orchestration-low-level-dispatch-release.spec.tstests/e2e/orchestration-worker-settlement-release-cli.spec.tstests/e2e/orchestration-worker-terminal-visibility.spec.ts
💤 Files with no reviewable changes (1)
- src/cli/handlers/orchestration.test.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- src/main/runtime/rpc/methods/orchestration-check.test.ts
- src/main/runtime/rpc/core.ts
- src/cli/specs/orchestration.ts
- src/cli/handlers/orchestration.ts
- tests/e2e/completed-worker-retirement-resume.spec.ts
- src/main/runtime/rpc/methods/orchestration-manual-dispatch-release.test.ts
- src/main/runtime/rpc/dispatcher.ts
- src/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.ts
- tests/e2e/orchestration-legacy-worker-missing-terminal-recovery.spec.ts
- tests/e2e/orchestration-legacy-worker-restart-recovery.spec.ts
- src/cli/handlers/orchestration-dispatch-show-scope.test.ts
- tests/e2e/orchestration-worker-terminal-visibility.spec.ts
- src/main/runtime/orca-runtime.ts
- config/reliability-gates.jsonc
- src/main/runtime/orchestration-compatibility-authority.test.ts
- tests/e2e/orchestration-dispatch-presentation.ts
- src/main/runtime/rpc/methods/orchestration.ts
- src/main/ipc/runtime.ts
- src/main/runtime/rpc/methods/orchestration-dispatch-show-run-scope.test.ts
- tests/e2e/orchestration-low-level-dispatch-release.spec.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 4 remain after this review.
| @@ -41,7 +42,7 @@ export class RuntimeClient { | |||
| private readonly remotePairing: PairingOffer | null | |||
| private readonly environmentSelector: string | null | |||
| private readonly remoteCompat: RemoteRuntimeCompatGate | |||
| private orchestrationContractCheck: Promise<void> | null = null | |||
| private orchestrationContractCheck: Promise<RuntimeStatus> | null = null | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not cache the dispatch-show capability preflight.
orchestrationContractCheck can describe a previous runtime instance. If the host changes after this cache is populated, orchestration.dispatchShow can reach an older host without checking ORCHESTRATION_DISPATCH_SHOW_SCOPE_RUNTIME_CAPABILITY. That host can drop callerTerminalHandle and restore the cross-Run disclosure path.
Run a fresh status.get capability check for each orchestration.dispatchShow call. Keep the cache for mutations if needed. Add a regression test that replaces a compatible host with an older host between two calls from the same RuntimeClient.
Proposed fix
private async ensureOrchestrationContractCompatible(
timeoutMs: number,
requireDispatchShowScope = false
): Promise<void> {
- if (!this.orchestrationContractCheck) {
- this.orchestrationContractCheck = this.checkOrchestrationContractCompatibility(timeoutMs)
- }
- const status = await this.orchestrationContractCheck
+ const status = requireDispatchShowScope
+ ? await this.checkOrchestrationContractCompatibility(timeoutMs)
+ : await (this.orchestrationContractCheck ??=
+ this.checkOrchestrationContractCompatibility(timeoutMs))
if (requireDispatchShowScope) {Also applies to: 77-81, 198-207
|
Thanks for the revalidation. Per your ownership of #14900, I’m leaving the branch untouched and will not rebase or push; the remaining gate is CI on |
|
Acknowledged the unresolved capability-preflight security finding on the current head |
|
Acknowledged on the current head |
5527d66 to
02c35e2
Compare
|
@Jinwoo-H a correction I owe you. On 09-20 a branch refresh of mine ported your validated series (13 commits ending at |
ELI5
A terminal in Run A could name a task from Run B and read Run B's dispatch metadata. Latest
mainstill reproduces this: the byte-identical 12-case oracle passes only 2 cases. This PR makes the host verify the real caller and scope the lookup to the caller's Run (or exact settled Dispatch), so foreign and nonexistent tasks look the same.Root cause
orchestration.dispatchShowlooked up a task/dispatch by task ID before establishing caller authority. A caller-supplied terminal handle was also insufficient on its own because it could be spoofed, stale, or reminted across a runtime restart.Fix
dispatch: nullfor both foreign and absent reads; return the sametask_not_foundshape for foreign and absent preambles.(task_id, task.run_id)lookup for trusted presentation so a mismatched persisted Task/Dispatch row cannot cross Runs.This is deliberately bounded Run isolation on one host. It is not remote privilege escalation, file access, or credential exposure.
Why this boundary
Authorization lives at the authoritative Run-scoped database/RPC lookup rather than in CLI presentation. Caller-supplied handles grant no authority, and invalid evidence fails before lookup. Empty/not-found semantics match neighboring read APIs while avoiding an existence oracle.
No RPC method, required field, result shape, or stream opcode changes. The existing caller field remains additive and optional, consistent with
docs/reference/remote-wire-compatibility.md.Deterministic reproduction
Oracle blob:
098dc85e769309181b9e9d8e3479210bdefba2a0pnpm exec vitest run --config config/vitest.config.ts \ src/main/runtime/rpc/methods/orchestration-dispatch-show-run-scope.test.tsThe same 12-case oracle covers same-Run success, cross-Run/absent equivalence, foreign/absent preambles, unknown caller/no Run, malformed and stale evidence with zero lookups, persisted Task/Dispatch Run mismatch, settled Dispatches, caller rebind, mixed-version omission, independent concurrent callers, and unchanged persisted records.
origin/main6ee265e579+ test-only oracle478880cd94A real paired/headless Electron journey also creates two independent Runs with real terminal identities: same-Run
dispatch-showsucceeds, Run A cannot observe Run B, and host persistence remains unchanged.Validation
pnpm lint: passed, including 86 reliability gates and the max-lines ratchet.pnpm typecheck:cliandpnpm typecheck:node: passed. The full renderer typecheck remains blocked by unrelated latest-main fixture errors insrc/renderer/src/components/tab-bar/open-tab-search-retention.test.ts.git diff --check: clean.One neighboring retirement/resume E2E consistently reaches and passes the trusted dispatch read, then times out later mounting a fallback terminal. The unmodified spec fails identically at the same PaneManager line on latest main; both candidate close modes were run independently and reproduce that unrelated existing failure.
Compatibility and risk
orchestration.dispatch-show-run-scope.v1and fail closed before sending the request to an older host that would drop the caller field. New hosts advertise this capability; old clients/hosts remain outside the fixed authority contract until updated.Review
Three independent GPT-5.6 Luna reviews completed clean final rounds:
Validated findings fixed: compound trusted-presentation lookup, raw identity-free lifecycle callers, reliability wording, zero-lookup proof including the legacy lookup, and explicit trusted renderer IPC in worker visibility E2E. The old-host preflight finding was validated and fixed with an explicit runtime capability fence; it now fails closed before the caller-scoped read is sent. CodeRabbit's Windows-invalid
/tmp/repofixture finding is resolved.Fixes #14898. Related: #14891, #14892, and STA-4459.
Checklist
mainAI disclosure
OpenAI Codex assisted with reproduction, implementation, review, and validation. The final diff and evidence were inspected before handoff.