Repository navigation
Conversation
PR #19542 changed only orchestration source. `pr-e2e-source-routing.mjs` had no route matching any of those paths, so the PR selected zero e2e specs, and `e2e` is deliberately absent from `verify.needs` anyway. The spec that would have caught the break -- "keeps unbound direct mail durable", in orchestration-idle-mail-delivery.spec.ts since #12584 -- never ran, and `orca orchestration send --to <handle>` between two plain terminals shipped failing with "Run is required". Route orchestration source at the 10 orchestration/worker specs, and give that subset its own `orchestration_e2e` job that IS in `verify.needs`. The whole suite stays advisory because it is red on main from unrelated specs; this subset is green on main's scheduled runs, so blocking on it does not inherit that redness. The blocking signal is the selected orchestration spec list being non-empty, not "orchestration source changed" the way hasSshSourceChange works: editing one of those specs alone selects it with no source match, and keying the split on source would drop it from the advisory lane while never adding it to the blocking one. The advisory `e2e` job gets the selection minus this lane's, so no PR pays for the same specs twice. Naming the code_paths output `orchestration_e2e` rather than `orchestration_e2e_should_run` is what lets verify's existing strict loop enforce the job with no new mechanism -- it derives every SHOULD_RUN from `code_paths.outputs.<job>`.
pr.yml now calls the reusable e2e.yml twice in one workflow run: the advisory `e2e` job and the blocking `orchestration_e2e` job. Both start e2e.yml's `build` job, and both would upload `e2e-build-out`. upload-artifact v4+ fails outright when two jobs in one run upload the same name, so a PR touching orchestration source AND any other routed source would kill whichever build finished second -- taking the new required check red for a reason that has nothing to do with the PR. `playwright-traces-changed` had the same collision on failure in both lanes. Add an optional `artifact_suffix` workflow_call input, default '', and append it to every artifact name e2e.yml uploads or downloads -- not just the two that can collide today, because a bare name left behind would fail only when a spec fails in both lanes, hiding the signal exactly when it matters. The advisory caller passes nothing; schedule and dispatch runs pass no inputs at all, so both keep the names they have always used. `orchestration_e2e` passes `-orchestration`. release-e2e-dispatch-contract's "hands the built relay artifact to every E2E run command" pinned the literal `e2e-build-out` on the upload and both downloads. It now compares the downloads against the upload's own name, which is what the test was really guarding: upload and download must agree. A mutant that renames one download still fails it. Verified: actionlint resolves the caller/callee input contract (a deliberately typo'd input name is reported), and both new assertions were mutation-checked.
📝 WalkthroughWalkthroughThe change adds orchestration-specific E2E source routing and separates orchestration specs from advisory specs. The PR workflow runs orchestration specs in a blocking Priority: ⬇️ Low Severity of issue fixed: Low Merge Risk: 🟡 Moderate · up to Deletion-only or rename-away orchestration changes can bypass the intended blocking E2E coverage. This routing gap should be fixed before merge so affected pull requests cannot pass without running the required specifications. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed context, motivation, implementation details, risks, and testing results. However, it does not follow the repository template and omits the required Linked Issue with a Fixes reference, Visual Proof section with N/A if applicable, AI Disclosure, Review, Agent skill upstream boundary, Notes, and checklist confirmations. Resolution Add the required template sections and headings. Include a Linked Issue section with a valid Fixes # reference, state N/A with a reason under Visual Proof if no visual proof applies, complete or mark the AI Disclosure and Agent skill upstream boundary sections as applicable, add Review and Notes content, and complete the checklist items. Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (2 skipped: 2 unsupported.)
✨ 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 |
![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: Advanced
Run ID: 7ccb56ea-75ab-43ec-a3c0-10c8c39adfcc
📒 Files selected for processing (6)
.github/workflows/e2e.yml.github/workflows/pr.ymlconfig/scripts/pr-e2e-orchestration-routing.test.mjsconfig/scripts/pr-e2e-source-routing.mjsconfig/scripts/pr-workflow-parallelism.test.mjsconfig/scripts/release-e2e-dispatch-contract.test.mjs
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| @@ -82,8 +86,16 @@ jobs: | |||
| CHANGED="$(git diff --name-only --diff-filter=AMCR --merge-base "$BASE" "$HEAD")" | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Include deleted orchestration source paths in routing.
Line 86 excludes D paths and keeps rename detection. A PR that only deletes, or renames away, a matched orchestration source file produces orchestration_e2e=false. The blocking job then skips, and verify accepts that skipped result.
Pass source-side deletions and renames to route matching. Keep deleted E2E spec paths out of the runnable spec list. Add a workflow-level regression case for a deletion-only orchestration change.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run reviewed the full six-file diff: the blocking orchestration e2e gate, the spec-route partition, the per-caller artifact namespace, and the contract tests that pin all three.
- Blocking orchestration e2e job — a new
orchestration_e2ereusable-workflow call inpr.yml, gated on orchestration source, wired intoverify's strictcheck_jobloop so it fails the required aggregate when any of the 10 specs fail. - Spec-route partition —
pr-e2e-source-routing.mjsgains anorchestration.runtime-and-cliroute plusselectOrchestrationPrE2eSpecs/selectAdvisoryPrE2eSpecs/hasOrchestrationE2eChange, making the advisory and blocking lanes an exact partition of the full selection so no spec runs twice. - Artifact namespace suffix —
e2e.ymlgains an optionalartifact_suffixinput applied to all nine upload/download steps, so two callers of the workflow in one run can't collide one2e-build-out. - Contract tests — one new test file asserts the routing, partition, verify wiring, and artifact-namespace invariants; two existing tests are updated for the partition and suffix.
I verified the routing script end-to-end (message-insert.ts and a bare orchestration spec edit both gate true; an unrelated file gates false), confirmed the orchestration route regex covers all on-disk orchestration/worker product source, and ran the three changed test files (35 passed).
ℹ️ One deliberate central coverage gap
The gate fires only when ORCHESTRATION_PRODUCT_SOURCE matches a changed file, and src/main/runtime/orca-runtime.ts and src/shared/agents-orchestration-steps.ts are named, tested exclusions. orca-runtime.ts is the runtime's central entry point, so an orchestration regression that lands there (or in any file the regex misses) merges with zero gate coverage — the exact #19542 shape this PR exists to stop. The cost rationale (routing it runs the gate on most PRs) is sound, but the coverage boundary is now a review-checklist item, and worth an explicit note that "orchestration source" downstream of orca-runtime.ts is not guarded.
Technical details
# Orchestration route coverage boundary
## Affected sites
- config/scripts/pr-e2e-source-routing.mjs:17-18 — ORCHESTRATION_PRODUCT_SOURCE regex
- config/scripts/pr-e2e-orchestration-routing.test.mjs:160-167 — asserts orca-runtime.ts and agents-orchestration-steps.ts select nothing
## Required outcome
- Human sign-off that the orca-runtime.ts gap is acceptable, or a narrower authority for it.
## Suggested approach (optional)
- None required if the gap is accepted as-is.ℹ️ Required gate carries first-run flake risk
The blocking lane serializes the 10 specs (--workers=1) on one ubuntu-latest runner, and your own local runs reproduced the shared waitForActivePanePtyId 15s first-pane-binding predicate (tests/e2e/helpers/terminal.ts:87) flaking under load. It is green on main's scheduled runs — the only red being the real #19542 regression — and declining to quarantine or widen the budget was the right call. But a required check has no continue-on-error, so if that predicate flakes on a busy CI runner, orchestration PRs block for a reason unrelated to their diff. The fix would live at the fixture-budget layer, not here; just confirming the reliability assumption is explicit before a gate lands on every orchestration PR.
Technical details
# Required-gate flake watch
## Affected sites
- .github/workflows/pr.yml:954-969 — orchestration_e2e required via verify.needs + strict check_job
- tests/e2e/helpers/terminal.ts:87 — waitForActivePanePtyId 15s budget, 355 call sites
## Required outcome
- Watch the first few orchestration-source PRs after merge; if waitForActivePanePtyId flakes on CI, raise the fixture budget there rather than quarantining specs or removing the gate.
## Open questions for the human (optional)
- Is a retry (e.g. a single rerun on flake) acceptable for a required gate, or is first-attempt determinism the bar?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
What the user notices
Before: A PR that changes only orchestration source merges with zero e2e specs run.
That is what happened to #19542 on 2026-09-08: its e2e job reported
skipping, it merged, andorca orchestration send --to <handle>between two plain terminals — the first command theorchestration skill guide teaches — started failing with
Run is required. Users reported"orchestration stopped working after updating". The spec that catches it,
tests/e2e/orchestration-idle-mail-delivery.spec.ts"keeps unbound direct mail durable", hadexisted since #12584 in August. It went red on main's scheduled e2e for two runs, which blocks
nothing, and #19684 then renamed it to "refuses unbound direct mail" and asserted the error —
merged as a test-only change hours before the first user report.
After: The same PR selects the 10 orchestration/worker specs, runs them in a new
orchestration e2e (blocking)check, andverify— the required aggregate — fails if any ofthem fails. The author sees a red required check on the PR instead of a user seeing the error
after updating.
The delete question
Net add. What was deliberately not added is where the work went.
hasOrchestrationE2eChangeis derived from the route's own spec list,not from a second copy of the prefixes. That is the mistake
hasSshSourceChange's commentalready documents ("two lists that must agree is how that drifted").
pr-code-change-scope.mjs. Naming thecode_pathsoutputorchestration_e2erather thanorchestration_e2e_should_runmeans verify's existing strictloop — which derives every
<JOB>_SHOULD_RUNfromcode_paths.outputs.<job>— enforces thejob with zero new mechanism. The only line added to
verify's script is onecheck_jobcall.e2ejob'stest_filesis now the selection minus thislane's specs, so no PR pays for the same 10 specs twice.
selectPrE2eSpecsis unchanged andstill the single authority; the two lanes are a partition of its output, and a contract test
asserts advisory ∪ blocking == the full selection for every case it exercises.
release-e2e-dispatch-contract.test.mjspinned theliteral
e2e-build-outon the upload and on both downloads. Three literals became onecomparison against the upload's own name, which is what the test was really guarding: upload
and download must agree. Strictly stronger — a mutant that renames one download still fails it.
config/reliability-gates.jsoncentry. The existing "keeps source-routed sentinelsregistered to their reliability gates" test names five specific gate ids and does not demand
one per route. Every gate in that manifest is a per-invariant record with an oracle,
assertion refs, and evidence runs; this change adds no invariant and no spec, so an entry
would be fabricated provenance.
pnpm run check:reliability-gatespasses unchanged.Proof
The gate cannot be executed locally — GitHub Actions is not runnable here — so the proof is in
two halves: the routing output that decides which specs the job runs, and the spec itself going
red on the reintroduced regression. Both were run on a local scratch state that re-applies the
#19542 deletion in
src/main/runtime/orchestration/db/messages/message-insert.ts(the unbound-RunINSERT OR IGNOREblock replaced byconst runId = msg.runId; if (!runId) throw new Error('Run is required')), never committed and reverted afterwards withgit checkout -- src/main/runtime/orchestration/db/messages/message-insert.ts.1. Routing: the file #19542 broke now selects the orchestration specs
--orchestration-source trueis what turnscode_paths.outputs.orchestration_e2einto'true',which is what makes the job run and what verify's strict loop requires to be
success.--advisory-specs []is the no-duplicate-run half: the advisorye2ejob does not start.On
origin/maintoday the same input prints[]and selects nothing.2. The routed spec is red on the regression
That is the user's error string, produced by the real runtime through the real RPC path — not a
mocked client. (
src/cli/handlers/orchestration.test.tsandsrc/cli/index-orchestration.test.tsvi.mockthe runtime client, which is why the unit layer stayed green through #19542.)3. Same command, same rebuild, after reverting the scratch edit
Both runs rebuilt
out/(noSKIP_BUILD), so neither read a stale bundle.Two callers, one run
pr.ymlnow invokes the reusablee2e.ymltwice. Both invocations start itsbuildjob, andactions/upload-artifactv4+ fails when two jobs in one workflow run upload the same name.Without a fix, a PR touching orchestration source and any other routed source would kill
whichever
builduploadede2e-build-outsecond — taking the new required check red for areason unrelated to the PR.
playwright-traces-changedhad the same collision when a spec failedin both lanes.
e2e.ymlgains an optionalartifact_suffixworkflow_callinput (default''), appended toevery artifact name it uploads or downloads — all nine steps, not only the two that can collide
today. A bare name left behind would fail only when a spec fails in both lanes, i.e. exactly when
the suite is already telling you something, and would hide it.
builde2e-build-out(upload)e2eshards,changed-e2e,ssh-docker-watcher-isolation,ssh-localhoste2e-build-out(download ×4)e2eshardsplaywright-traces-${{ matrix.shard_name }}changed-e2eplaywright-traces-changedssh-docker-watcher-isolationplaywright-traces-ssh-docker-watcher-isolationssh-localhostlocalhost-ssh-tracesThe advisory
e2ecaller passes nothing;orchestration_e2epasses-orchestration. Scheduleand dispatch runs pass no inputs at all, so they keep the artifact names they have always used.
e2e.ymlhas noconcurrency:at workflow or job level (confirmed — a group would serialize orcancel one call against the other), and a contract test now asserts it stays that way.
pr.ymlholds the only two callers of
e2e.ymlin the repo.Two new contract tests plus the reworked one, each mutation-checked:
actionlintresolves the caller/callee input contract, so a typo in the input name is a harderror rather than a silently ignored
with:key:Wire / SSH / folder workspace
host publishes. It is CI routing plus one workflow job.
SSH host; the Docker-SSH lane is selected by
hasSshSourceChange, which this change does nottouch (the new call to
selectAdvisoryPrE2eSpecsinsideshouldRunReusablePrE2esits behind anunchanged
hasSshSourceChange(...) ||short-circuit, so an SSH-source PR still starts thereusable workflow even when the advisory list is empty). An orchestration change that only
breaks on a remote execution host still merges green — see the report.
own fixtures build; no new assumption about git worktrees is introduced here.
ubuntu-latestthrough the existinge2e.ymlchanged-e2elane. Nothing added assumes macOS.orchestration_e2eis a plain reusable-workflowcall with no new shell.
Testing
pnpm tctc EXIT=0pnpm teston the 6 affected test filesTest Files 6 passed (6) / Tests 109 passed (109)Test Files 6 passed (6) / Tests 109 passed (109)pnpm test config/scripts(whole directory)Test Files 184 passed | 2 skipped (186) / Tests 1539 passed | 10 skipped (1549)Test Files 184 passed | 2 skipped (186) / Tests 1539 passed | 10 skipped (1549)pnpm run check:code-quality:changedcode quality: 0 new finding(s) across 4 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 issuesactionlint .github/workflows/pr.yml .github/workflows/e2e.ymlactionlint EXIT=0, no output (v1.7.12 from Homebrew — not present innode_modules/.bin)npx oxlint <changed files>oxlint-exit=0npx oxfmt --check <changed files>All matched files use the correct format.pnpm run check:max-lines-ratchetmax-lines ratchet OK — 7 grandfathered suppression(s), no new bypasses.pnpm run check:reliability-gatesReliability gate manifest check passed for 118 gate(s).git diff --check/git diff --cached --checkgit statusThe changed-code gate caught one real finding on the first pass of the follow-up commit —
typescript(restrict-template-expressions)on an array interpolated into an assertion message —fixed with an explicit
String(...)+.join(', '); the output above is the re-run.The blocking subset, run end to end twice
Both runs:
--project electron-headless --workers=1, all 10 specs. Run 1 builtout/; run 2used
SKIP_BUILD=1against that build.1 failed / 23 passed (4.8m)—orchestration-idle-mail-delivery.spec.ts:356SKIP_BUILD=1)1 failed / 23 passed (4.6m)—completed-worker-retirement-resume.spec.ts:39Both failures are the same shared fixture predicate, not an orchestration assertion:
That is
waitForActivePanePtyIdintests/e2e/helpers/terminal.ts:87, a 15s budget on the app'sfirst pane binding, used by 355 call sites across the suite. Follow-up isolation runs:
The run-1 failure was self-inflicted:
pnpm test config/scripts(1547 tests) was runningconcurrently. The run-2 and repeat failures were not — but this host was at load average 6.5–7.4
on 18 cores with 146 competing Electron/node processes from other agent worktrees, and 261% CPU
already consumed by other work. So:
orchestration-idle-mail-deliveryis 6/6 green here;completed-worker-retirement-resumeis 6/8 green here, failing only in the first-pane-bindingwait.
I did not quarantine either spec and did not widen the shared 15s budget. Quarantining a
spec that is green on CI would delete exactly the coverage this PR exists to add, and raising the
default in
tests/e2e/helpers/terminal.tstouches 355 call sites whose behaviour I cannot measurefrom this change (it would add 15s to every genuinely-failing test in the suite). The evidence
that matters for a gate that runs on
ubuntu-latestis main's scheduled e2e: across the last fourruns (34380808516, 34284171625, 34255148958, 34165484966) the only orchestration/worker spec that
failed was
orchestration-idle-mail-delivery.spec.ts, on the two runs before #19696 landed — areal regression, not flake — and the latest run has zero orchestration failures.
completed-worker-retirement-resume.spec.tshas not failed on CI in that window. This is the topopen risk in the report; confirming it against a real CI runner is the user's call. If it does
flake on CI, the fix is at the fixture-budget layer, not here.