Repository navigation
Conversation
|
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:
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: 📥 CommitsReviewing files that changed from the base of the PR and between e317b8bfaf206c998c2315ef1a73dc65f1bff9c2 and 90098c0b4868d459f902f273576e17b3e794b70c. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 6 remain after this review. 📝 WalkthroughWalkthroughAdded Fleet Echo data contracts and bounded lane construction from active dispatches, worker stages, terminal signals, delivery state, and quiet time. Added runtime lookup and caller-run ownership checks. Added Fleet Echo metadata to orchestration RPC results, with 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3da21ff8-8a0f-437a-8839-fcf36d0ac62b
📥 Commits
Reviewing files that changed from the base of the PR and between e4e54a1 and 3c642f9065c4629b5fb233728599ffd120e52670.
📒 Files selected for processing (18)
skill-guides/orchestration.mdsrc/cli/bundled-skill-guides.tssrc/cli/handlers/orchestration-fleet-echo-format.test.tssrc/cli/handlers/orchestration-fleet-echo-format.tssrc/cli/handlers/orchestration.tssrc/cli/specs/orchestration.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration/db-dispatch-liveness.test.tssrc/main/runtime/orchestration/db.test.tssrc/main/runtime/orchestration/db.tssrc/main/runtime/rpc/methods/orchestration-check.test.tssrc/main/runtime/rpc/methods/orchestration-fleet-echo-sources.test.tssrc/main/runtime/rpc/methods/orchestration-fleet-echo-sources.tssrc/main/runtime/rpc/methods/orchestration-send.test.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration.tssrc/shared/orchestration-fleet-echo.test.tssrc/shared/orchestration-fleet-echo.ts
💤 Files with no reviewable changes (1)
- src/main/runtime/orchestration/db.test.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 7 remain after this review.
34e0dbd to
e2c4aa3
Compare
|
All eight addressed. Four were already fixed by rebasing onto the updated #14891 (the fence language, the Authorize the Run before attaching — you found a real hole, and a wider one than I had fixed. I had already gated Both now go through one helper: runIdIfCallerOwns(runtime, params.from, original.run_id)which returns the Run only when the caller's own bound Run matches, and I checked the other attach sites while I was in there. This is the same family as #14898, which I filed earlier for
The lane cap is now clamped inside Inferring acceptance from output — I have taken this partly, and pushed back on the rest. You are right that output is not an acceptance signal and that the consequence was mispriced: the guide told a coordinator to re-dispatch on What I have not done is report Instead the guide now separates the two sources and changes the prescribed action:
So the signal keeps its value, the inference is disclosed at the point of use, and the expensive action is no longer prescribed off a heuristic. If maintainers would rather the inferred verdict carry its own value — Verification after rebasing this branch onto the updated #14891: 179 tests across the five affected files, both typechecks and |
e2c4aa3 to
e317b8b
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 the rebased #14891Both branches were 91 commits behind
// Why: every send return site owes both passengers — the recipient warnings this call
// collected and the Run's fleet echo — so compose them once instead of at each return.
const sendReceipt = <T extends object>(
receipt: T
): Promise<T & { warnings?: SendRecipientWarning[]; fleet?: FleetEcho }> =>
attachFleetEcho(runtime, runId, fleetEnabled, withSendWarnings(receipt))Every site that was The group-send path takes return attachFleetEcho(runtime, runId, fleetEnabled, receipt)So on that path One new commit, and it is not a cleanup — it was forced.
Also new: an explicit Verification on the rebased head
CI still reports nothing on this PR, and not for a reason in the diff. Every |
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: 4235d7f3-d146-42a5-aa8b-44b9ffc4e1f8
📥 Commits
Reviewing files that changed from the base of the PR and between 32ee3b0 and e317b8bfaf206c998c2315ef1a73dc65f1bff9c2.
📒 Files selected for processing (18)
skill-guides/orchestration.mdsrc/cli/bundled-skill-guides.tssrc/cli/handlers/orchestration-fleet-echo-format.test.tssrc/cli/handlers/orchestration-fleet-echo-format.tssrc/cli/handlers/orchestration.tssrc/cli/specs/orchestration.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration/db-dispatch-liveness.test.tssrc/main/runtime/orchestration/db.test.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-lookup.tssrc/main/runtime/rpc/methods/orchestration-check.test.tssrc/main/runtime/rpc/methods/orchestration-fleet-echo-sources.test.tssrc/main/runtime/rpc/methods/orchestration-fleet-echo-sources.tssrc/main/runtime/rpc/methods/orchestration-send-fleet-echo.test.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration.tssrc/shared/orchestration-fleet-echo.test.tssrc/shared/orchestration-fleet-echo.ts
💤 Files with no reviewable changes (1)
- src/main/runtime/orchestration/db.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/cli/handlers/orchestration-fleet-echo-format.test.ts
- src/cli/handlers/orchestration-fleet-echo-format.ts
- src/main/runtime/orca-runtime.ts
- src/main/runtime/rpc/methods/orchestration-check.test.ts
- src/shared/orchestration-fleet-echo.ts
- src/cli/specs/orchestration.ts
- src/shared/orchestration-fleet-echo.test.ts
- src/main/runtime/rpc/methods/orchestration-fleet-echo-sources.ts
- src/cli/handlers/orchestration.ts
- src/main/runtime/rpc/methods/orchestration.ts
- src/main/runtime/orchestration/db-dispatch-liveness.test.ts
- src/main/runtime/rpc/methods/orchestration-fleet-echo-sources.test.ts
- skill-guides/orchestration.md
- src/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 8 remain after this review.
db.test.ts sits just under the 800-line max-lines budget, leaving no room for new coverage. Move the recordHeartbeat and getStaleDispatches tests into a new db-dispatch-liveness.test.ts — a coherent unit around detecting whether a dispatched task is still alive — and leave the dispatch-context CRUD coverage behind, which is a different concern. No assertions or fixture behaviour changed; only file location.
Add a `fleet` block that orchestration methods can attach to their response: a live snapshot of the caller's active dispatch lanes (handle, task, dispatch, lifecycle, quiet time, and delivery state), capped at 12 lanes with truncation reported. The `delivery` column is the point: `not_accepted` means a dispatched prompt never actually started a turn in the worker terminal, something a coordinator would otherwise only discover minutes later by noticing silence. This wires the fleet echo into `orchestration.check` only, including every wait/timeout return path — a complete, reviewable vertical slice covering the builder, runtime adapter, the check RPC method, CLI rendering, and the skill guide. The remaining eight orchestration methods (send, reply, inbox, task-create/list/update, dispatch, dispatch-show) get the same wiring in a follow-up PR.
- listActiveDispatchesForRun now requires dispatch_contexts.run_id to match the caller's Run and to agree with the task's, so a divergent denormalized row cannot leak another Run's lane. Regression test covers both. - A remote worker mailbox resolves its Run from the task instead of passing undefined, which had omitted the block entirely for remote worker checks. - The check fixture now creates its task in the bound Run; without it the assertion could pass against the legacy Run rather than the caller's. - quietMs null-checks the timestamp instead of testing truthiness, so an epoch-zero value means the same thing to it as it does to delivery. - The TZ-pinned test deletes TZ when it was originally unset, rather than restoring the literal string "undefined" into sibling tests. - check's usage string documents --no-fleet, and the guide's output example carries a fence language.
… a blind re-dispatch buildFleetEcho now clamps a caller-supplied limit to FLEET_ECHO_MAX_LANES. The cap is the response contract, not a default, and this block rides on every orchestration response. The guide told a coordinator to re-dispatch on `not_accepted`. That is right for a worker-start lane, where the verdict comes from the worker never reaching `input_accepted`. For a plain `dispatch --inject` there is no acceptance signal to read, so the verdict is inferred from the terminal having produced no output since the dispatch — and acting on a false positive duplicates work that is already running. The guide now distinguishes the two sources and says to confirm with `terminal read` first.
e317b8b to
90098c0
Compare
…from The block reported accepted/not_accepted from two observations of unequal strength under the same word. A worker-start lane reads stage from the worker-dispatch row; an unsupervised `dispatch --inject` lane has no such row and the verdict was inferred from the terminal having stayed quiet since dispatched_at. Lanes now carry deliveryEvidence, rendered as NOT_ACCEPTED:output or accepted:stage, so a coordinator can tell a read verdict from an inferred one without consulting the guide. The guide's claim that a worker-start verdict is authoritative was also stronger than the code supports, and is corrected here. markWorkerDispatchReady writes stage='input_accepted' as soon as sendTerminalAgentPrompt returns, so it records that the runtime wrote the prompt, not that the agent began a turn: a composer that swallowed the submit still reports accepted (stablyai#14525). Only not_accepted is authoritative on that path.
90098c0 to
c4f4fd8
Compare
…p repeating a healthy table Three changes to the same problem: the block rides on every run-scoped response, so what it costs and what it hides both matter. Ranking. The cap was applied by the query's oldest-first order, so a Run past 12 active lanes truncated its newest — which is where a freshly broken lane lives. Severity cannot be a SQL predicate because it needs runtime signals, so the query now reads a wider bounded window and the builder ranks it before cutting: a not_accepted read from the worker row, then one inferred from silence, then a dead process, then the longest silence. Truncation lands on the healthy tail. Past the window ranking degrades to age, which the guide states rather than hides. Repetition. A healthy fleet says the same thing on every response. Text mode now collapses it to one line and opens the full table as soon as any lane needs attention, so the table itself is the signal. JSON keeps every lane: a machine consumer can summarize, and a payload that changes shape with its contents is a worse contract than one that does not. Honesty. The guide now says the block answers which lane to look at, and does not cover a coordinator parked in check --wait, since it only arrives when asked. It also drops the lifecycle column from the table, which delivery already implies.
Extends the fleet echo from `check` to the eight remaining run-scoped methods: send, reply, inbox, taskCreate, taskList, taskUpdate, dispatch and dispatchShow. A coordinator now learns a lane's delivery state from whichever command it happens to run next, rather than only from check. `ask` and `reset` stay unwired on purpose: ask blocks until answered, so a snapshot taken at return time is stale by construction, and reset tears the Run down. Two methods needed a way to reach the caller's Run. inbox takes an optional callerTerminalHandle, matching the param already used by taskCreate, taskList, taskUpdate and dispatch; its existing output is unchanged and older clients that omit it simply get no block. dispatchShow resolves the caller's own Run and attaches only when it matches the task's Run, so it cannot hand one Run's lane roster to a caller working in another.
reply reaches a Run through the message id it was handed, so echoing that Run's lanes handed its roster to any caller holding an id from another Run. It now attaches only when the caller's own bound Run matches, with a regression test proving both directions. dispatchShow had the same shape and the same fix; both now share runIdIfCallerOwns rather than repeating the lookup, and inbox uses the matching resolveCallerRunId. The CLI also never sent callerTerminalHandle on inbox, so the block could not attach there at all — --terminal names whose mailbox to list, not who is asking.
orchestration-send.test.ts is at 790 counted lines on main; appending the five fleet-echo cases pushed it to 844 and over the 800-line budget. Same split as db-dispatch-liveness.test.ts rather than a suppression.
c4f4fd8 to
5e59261
Compare
…fleet block attaches dispatch-show reaches a Run through the task it was handed, so the runtime only attaches the block once it can prove the caller owns that Run. The CLI sent task, preamble, from and devMode and never the caller handle, so the gate resolved null and the block silently never appeared on one of the nine commands the guide lists. It reuses the handle --preamble already resolves, and skips the resolve entirely under --no-fleet. Also binds the tasks-dispatch fixture to the caller Run, which otherwise creates its task in the legacy Run and asserts against lanes the bound-Run query excludes. The new CLI cases live in their own file: orchestration.test.ts sits at its max-lines budget, and the remint case there now pins the reminted handle instead of the whole param object, which is what that case is actually about.
|
Pushed
const runId = runIdIfCallerOwns(runtime, params.callerTerminalHandle, taskRunId)and the CLI sent That is the second time this exact shape has bitten in this PR — The tasks-dispatch fixture now names the bound Run. Two notes on where the new tests live. The comment-length nitpick on Verification on |
|
Closing this along with #14891, which it was stacked on. The reasoning is recorded there: the fleet echo re-derives One observation from this branch is worth keeping, and I want to state it accurately rather than overclaim it. While wiring the echo into // src/cli/handlers/orchestration.ts:1219
const from = showPreamble ? await resolveCoordinatorTerminalHandle(flags, cwd, client) : undefined
await client.call('orchestration.dispatchShow', { task, preamble: showPreamble, from, devMode })That is not a defect on its own — nothing upstream is gated on it today; the gate this PR added was its own. It is a constraint on #14898 (READ-04): a Run-scoping fix for READ-01 and READ-02 on #15190 now read |
ELI5
#14891 makes
orca orchestration checktell you whether each of your Run's lanes actually received its prompt. This extends that to the other orchestration commands, so you find out from whichever one you happen to run next instead of having to remember to runcheck.What Changed
Extends the
fleetblock fromcheckto the eight remaining Run-scoped methods:send,reply,inbox,taskCreate,taskList,taskUpdate,dispatchanddispatchShow.askandresetstay unwired on purpose.askblocks until answered, so a snapshot taken at return time is stale by construction.resettears the Run down.Two methods needed a way to reach the caller's Run, and both are worth a look:
inboxtakes a new optionalcallerTerminalHandle. It already had aterminalparam, but that one means "whose mailbox to list", not "who is asking" — reusing it would attach the wrong pane's Run whenever a coordinator inspects another handle's inbox.callerTerminalHandleis the name and meaning already used bytaskCreate,taskList,taskUpdateanddispatch.inbox's existingmessages/countoutput is unchanged, and a client that omits the param simply gets no block.dispatchShowresolves the caller's own Run and attaches only when it matches the task's Run. Attaching the task's Run unconditionally would have handed any caller naming a--taskfrom another Run that Run's full lane roster. On a mismatch the block is omitted; nothing else about the method changes.Why
Same motivation as #14891 — relates to #14809 and #14523 — applied to the rest of the surface. The value of the fleet echo is that a coordinator never has to remember to check; leaving it on
checkalone preserves exactly the round-trip it was meant to remove.Kept separate from #14891 so the design can be argued on one method before it is spread across nine.
Linked Issue
Fixes #14907 — with this merged, the block rides on every Run-scoped command, which is what that issue asks for. #14891 delivers the same capability on
checkalone.Also relates to #14809 (the silence it removes, though the
--injectdefault is unchanged) and #14523 (theO(n)triage round-trips it removes, thoughterminal listpreview freshness is untouched).Visual Proof
N/Afor UI — CLI/RPC surface only, no renderer changes. The rendered block is unchanged from #14891; only the set of commands that carry it grows.Testing
orchestration-send.test.tsandorchestration-tasks-dispatch.test.ts, including a case per wired method and both the matching and mismatching Run paths fordispatchShow— 164 passing across the four affected test files.pnpm exec tsc --noEmit -p config/tsconfig.node.json— clean.pnpm exec oxlint src/ config/— clean.pnpm run lintandpnpm build— both pass locally.Tested on Windows 11.
AI Disclosure
Written with Claude Opus 5 via Claude Code. Design decisions were made in collaboration with the author; implementation and review passes were agent-assisted, then verified by the author.
Review
dispatchShowchange is a narrowing — it stops a caller from receiving another Run's lane roster. NotedispatchShowhas no caller authorization check today; that is pre-existing and out of scope here, but this PR makes sure the fleet block does not widen what it exposes.unknown. No new wire opcode;InboxParamsis a plainz.object, so an older host drops the unknowncallerTerminalHandleand an older client omitting it just gets no block.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Note on the last box:
pnpm lintand both typechecks pass locally. The fullpnpm testsuite cannot start on Windows from a clean install of this repo — see #14918. Briefly:windows-native-registry@3.2.2installs with an emptybuild/, so the Windows branch ofensure-native-runtimerefuses to run the suite. Not specific to this branch, and not a Node version problem (reproduced identically on Node 22 and 24).Running vitest directly, bypassing that wrapper, the suite has pre-existing failures on this Windows machine unrelated to this change (missing POSIX shell,
spawnSync … EINVAL, assertions expecting/and receiving\). Attribution was verified by running the entire suite on the merge base and on the feature branch and diffing the failing-file lists: the base fails 81 files, the branch fails the same 81 plus one timing-flaky test inremote-runtime-shared-control-connection.test.tsthat is untouched here and passes 28/28 in isolation.Author