Repository navigation
Conversation
… end #19542 broke `orchestration send --to <handle>` between two terminals in no Run — the FIRST command the orchestration guide teaches — and every gate was green. The CLI tests mock the runtime client, so the runtime's throw was never produced; the RPC harness uses `:memory:` with a bound Run, so the unbound path was never taken; the guide-contract check only matched strings against ORCHESTRATION_COMMAND_SPECS. No gate ran the taught argv through a real binary against a real runtime. This spec does. It boots Electron, spawns the compiled out/cli/index.js, and runs the guide's sequence in the printed order — plain-terminal send, check, reply, restart, run-create, task-create, worker-start, the worker's own check/heartbeat/ask/escalation/worker_done, the coordinator's wait/reply/ack, retain/release, the run:/dispatch:/@group address forms, gates, low-level dispatch --inject, and worker-stop — asserting each receipt's shape rather than a zero exit. Commands run with ORCA_TERMINAL_HANDLE set instead of an added --from, because that is how the guide's argv actually runs. The second half is a drift gate: the spec records every argv it ran and compares it with the commands parsed out of skill-guides/orchestration*.md at test time. A documented command nobody executes fails, and so does a flag the spec executes that the guide never teaches. Excuses are per (verb, flag) with a stated reason, and an excuse that stops matching the guide fails too. The static contract test stays: it runs on every PR in seconds with no build and is the only gate covering the documented commands one local profile cannot reach. Both now read the guide through one parser so they cannot disagree about what is documented.
|
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 (10)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughAdds a shared Markdown parser for documented orchestration commands. Adds a command ledger that detects guide coverage gaps and stale excuses. Adds helpers for compiled CLI execution, fake agents, terminal topology, and mail inspection. Adds a Playwright E2E contract covering orchestration flows across an application restart. Updates the static contract test to use the shared parser and corrects an idle-mail test comment. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The new orchestration guide contract coverage is ready to merge without identified current risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the change, motivation, implementation, regression proof, scope, and testing. However, it does not follow the required template: it omits the required Linked Issue entry, Visual Proof or an explicit N/A statement, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections. Resolution Add the missing template sections. Provide a linked issue after
✨ 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No blocking issues — two loose assertions inline worth tightening; the rest is a careful, well-grounded E2E contract.
Reviewed changes
- Shared guide parser (
config/scripts/orchestration-guide-invocations.ts) — extracts the guide's documentedverb --flagpairs; both the existing static contract test and the new spec now read the guide through it, so they cannot disagree about what is documented.projectDiris a parameter rather thanimport.meta.dirname, correctly, since Playwright transpiles specs to CJS. - Drift-gate ledger (
tests/e2e/helpers/orchestration-guide-command-ledger.ts+ its unit test) — records every orchestration argv the spec runs and, at the end, fails if the guide documents an unexecuted (and unexcused) pair or the spec executes an undocumented (and unexcused) pair;findStaleExcuseskeeps the excuse list honest on every PR via vitest. - End-to-end spec (
tests/e2e/orchestration-guide-contract.spec.ts) — boots Electron twice across a restart, spawns the compiledout/cli/index.js, runs the guide's sequence in order against a live runtime, asserts receipt shapes, and reads SQLite directly for durable/run_unbound/legacy-adoption state. - Supporting helpers — compiled-CLI runner, fake codex agent with per-handle stdin ledger, terminal topology/resolution, and thin additions to the existing mailbox reader (
readMailRowsById,countLegacyAdoptions). - Comment-only fix in
orchestration-idle-mail-delivery.spec.ts— corrects a stale comment that described the pre-#19696 refusal behavior.
ℹ️ Nitpicks
- The (a)–(g) sections are one ~700-line
test(); wrapping each intest.step(...)would make a failure in this 10-minute sequence report which stage collapsed instead of one flat stack trace.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| 'implementing', | ||
| '--json' | ||
| ]) | ||
| expect(receipt<{ message?: { id: string }; relay?: unknown }>(heartbeat)).toBeTruthy() |
There was a problem hiding this comment.
The toBeTruthy() wrapper is dead: receipt already throws on a non-zero exit or ok !== true, and the envelope object it returns is always truthy. Call receipt<{ message?: { id: string }; relay?: unknown }>(heartbeat) directly — the send is still asserted by its exit code and ok flag.
| const workerCheck = receipt<{ count: number }>( | ||
| orca(workerHandle, ['orchestration', 'check', '--terminal', workerHandle, '--json']) | ||
| ) | ||
| expect(workerCheck.count).toBeGreaterThanOrEqual(0) |
There was a problem hiding this comment.
count >= 0 cannot fail for the { count: number } shape, and receipt already proves the command exited 0, so this assertion pins nothing. The worker inbox is empty at this point — toBe(0) (or asserting on messages) would actually pin the behavior this line claims to check.
This branch has not been deployed
1 participant
What the user notices
Before: an orchestration-source PR could delete the unbound-Run write path and
merge green. It did: #19542 broke
orca orchestration send --to <handle>between twoplain terminals — the first command
skills get orchestrationteaches — and usersreported "orchestration stopped working after updating". Every gate passed because no
gate ran the guide's argv through a real CLI binary against a real runtime: the CLI
tests
vi.mockthe runtime client, the RPC harness uses:memory:with a Run alreadybound, and the guide-contract check only matched strings against
ORCHESTRATION_COMMAND_SPECS.After: a PR that breaks any command the guide teaches fails
tests/e2e/orchestration-guide-contract.spec.ts. The spec boots Electron, spawns thecompiled
out/cli/index.js, and runs the guide's sequence in the order the guideprints it, asserting each receipt's shape. The same spec fails if the guide starts
teaching a command nobody executes, or if the spec starts executing a flag the guide
never teaches. Nothing about the shipped product changes; the change is entirely in
what can merge.
The delete question
Nothing was deleted outright, and one candidate was deliberately kept.
config/scripts/orchestration-guide-command-contract.test.mjswas the obvious delete:the new spec executes the taught commands, which strictly dominates matching their
strings. It stays because it covers what the spec cannot and the spec cannot cover it:
routed to it and needs a full
electron-vite+build:cli;cannot reach (remote placement, legacy takeover, abandon). Those are excused from the
E2E's coverage gate by name; the static test still proves the CLI accepts them.
What was removed is the duplication between them: both now read the guide through one
parser,
config/scripts/orchestration-guide-invocations.ts, so they cannot disagreeabout what "documented" means. The spec's own duplication was also removed — its DB
reads went into the existing
tests/e2e/helpers/orchestration-mail-store.tsratherthan a second SQLite reader beside it.
The stale comment at
orchestration-idle-mail-delivery.spec.ts:353("the send isrefused outright") was deleted; the test under it was already correct.
Proof
1. The regression this exists to catch (mandatory proof)
Local scratch edit re-applying the #19542 deletion in
src/main/runtime/orchestration/db/messages/message-insert.ts— the unbound block andits
INSERT OR IGNORE INTO runsreplaced with:Rebuilt (no
SKIP_BUILD, so global setup ranelectron-vite build --mode e2eandpnpm run build:cli) and ran the spec:Step (a) — the first command in the guide — fails, on the exact string users saw.
Reverted with
git checkout -- src/main/runtime/orchestration/db/messages/message-insert.ts,rebuilt, re-ran:
2. The drift gate bites
Scratch edit appending one fenced line to
skill-guides/orchestration/references/coordinator-loop.md:The static contract test stays green (the CLI accepts that verb and those flags). The
spec goes red:
Reverted with
git checkout -- skill-guides/orchestration/references/coordinator-loop.md.3. What the sequence actually executes
Through the compiled CLI, against a live runtime, across a restart, with
ORCA_TERMINAL_HANDLEset instead of an added--from(which is how the guide's argvreally runs — inside an Orca terminal that resolves the caller):
send --to <handleB> --subject --body --jsonfrom a terminal in no Run →exit 0,
message.id, and the row asserted directly in SQLite asrun_id = 'run_unbound',read = 0.check --terminal <B> --jsonreturns it;reply --id --body --json;check --terminal <A> --jsonsees the reply andthread_idequals the original id.byte-identical (
toEqualon the pre-restart rows),SELECT COUNT(*) FROM legacy_adoptionsis unchanged,
check --terminal <B> --peek --format --jsonreturns the unread row withits subject rendered, and the row is still
read = 0afterwards (peek is non-consuming).inbox --full --json;run-create --objective --json;run-list;run-show --id;run-use --id;task-create --specandtask-create --spec --deps;task-list --ready --brief(asserts the dependent Task is absent) andtask-list --run;worker-start --spec --worktree current --agent codex --jsonwith a fake codex on PATH.received, then the worker runs
check --terminal <worker>,send --type heartbeat --phase,ask --question --options --timeout-ms(exits 1 ontimeout and returns the pending question id),
ask --resume(returns the coordinator'sanswer),
send --type escalation, a secondcheckbefore settling, andsend --type worker_done --outcome succeeded.check --wait --types "worker_done,escalation,question" --timeout-ms 60000 --json→ the question with a delivery id;reply --id;check --ack <delivery_id> --wait --types --timeout-ms --json→ the batch containingworker_done;worker-list --run --include-remote;worker-show --dispatch;worker-read --dispatch --limit 50;worker-retain(assertsstate: 'retained') thenworker-release.--to <handle>,--to run:<id>(asserted to land in that Run'srows),
--to dispatch:<id>before release (and the worker sees it on its nextcheck),and the group forms the guide documents in
messaging-and-gates.md:--to @alland--to @worktree:<id>, each asserted onrecipientsand the returnedmessagesarray.dispatch --task --to --inject --jsoninto an operator-created agent terminal(
low-level-topology.md),gate-create/gate-list/gate-resolve,request-show --requeston the request id theworker_donereceipt returned (assertedcompleted),check --run <id>,worker-start --task, andworker-stop --dispatch(asserted to close that exact terminal and leave the inject target alive).
The coverage gate is what keeps that list honest: it fails if the guide documents
something the spec does not run. Eleven pairs are excused by name with a reason, in
tests/e2e/helpers/orchestration-guide-command-ledger.ts:worker-start --on/--repo/--setup(needs a second execution host),worker-start --name/--retry-of/--terminal(needs a new worktree / a proven failedDispatch / an idle pre-existing agent terminal),
worker-start --model/--effort(onlyforwarded when the worker server advertises support, and the fake agent takes no model
argv),
run-use --takeover-legacy(needs a pre-cutover database), andworker-abandon --dispatch/--json(already driven byorchestration-low-level-dispatch-release.spec.ts). One flag is excused in the otherdirection:
ask --json, because the guide printsaskwithout it.An excuse that stops matching the guide is itself a failure (
findStaleExcuses), andthat check runs in vitest on every PR, not only in E2E.
Wire / SSH / folder workspace
published field; it only calls existing ones. It does not cover the mixed-version case
— a client and host at different versions is PR 3's scope.
worker-start --on <environment>is on the coverage gate's excuse list for exactlythat reason. Nothing here assumes local-only behaviour in product code.
setup produces, because the restart fixture (
attachRepoAndOpenTerminal) needs one.A non-git folder workspace would exercise the same orchestration paths; that gap is
named in the report.
Testing
pnpm tcpnpm test tests/e2e/helpers/orchestration-guide-command-ledger.unit.test.ts config/scripts/orchestration-guide-command-contract.test.mjsTest Files 2 passed (2) / Tests 5 passed (5), 220 msTest Files 2 passed (2) / Tests 5 passed (5), 204 mspnpm run check:code-quality:changedcode quality: 0 new finding(s) across 10 changed file(s). / type-aware code quality: 0 new finding(s) / React Doctor: 0 new finding(s) / Changed-code quality gate passed since aac38d698ff7.pnpm run check:react-doctor:changedorca no score 0 issuesgit diff --checkgit statusSKIP_BUILD)✓ 1 ... (21.9s) / 1 passed (32.1s)SKIP_BUILD=1run 21 passed (22.8s)SKIP_BUILD=1run 31 passed (20.4s)tests/e2e/orchestration-idle-mail-delivery.spec.ts(comment-only edit)10 passed, 2 failed; both failures are the pre-existing 15 swaitForActivePanePtyIddefault timing out under machine load, and both pass on re-run:2 passed (15.7s)Two additional runs failed for an environment reason worth stating plainly: this
worktree symlinks a
node_modulesshared with four other concurrent agents, and anypnpm <script>in this repo re-verifies dependencies and re-runspostinstall. Twice asibling agent's
node-ptyrebuild landed mid-run, once removingspawn-helper(
"Daemon's node-pty install is gone (worktree deleted?)"fromworker-start) and oncefailing
pnpm run build:cliwith anode-gyperror. Both name the primary worktree'snode_modulespath in the error. Neither is reachable in CI, where each job installs itsown tree, and both passed on immediate re-run with no code change.
Spec file is 729 lines (cap 800); no lint disables were added.