Repository navigation
Conversation
#19542 tightened createTask so a Task without a Run throws instead of falling back to the legacy Run, and migrated every existing test to pass `runId: 'run_legacy_local'` (254 such lines) with writer-run-required.test.ts pinning the throw. #19551 added this file 16 seconds later, validated against a main that predated #19542, with two bare createTask calls. Each PR was green alone; together shard 3/8 fails all 7 cases here with "Run is required". The stale side is this test, not createTask: re-loosening the writer would undo a deliberate, pinned invariant to make a newer test pass. Pass the legacy Run at both sites, matching the shape #19542 established. Swept the rest: with this change, every test under src/main/runtime/ orchestration and src/main/runtime/rpc/methods/orchestration passes (194 files, 1611 tests); the remaining bare createTask calls are either the intentional pin in writer-run-required.test.ts or RPC-harness tests whose db is bound to a run by the harness.
|
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe supervised-worker and unsupervised-worker task fixtures now set Merge Risk: ⚪ Minimal · up to The stopping-worker task fixtures now attach created tasks to the legacy local run, preserving the existing test scenarios while satisfying the required task-run relationship. No production behavior changes, and current merge-readiness risk is minimal. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the root cause, rationale, scope, and verification in detail. However, it omits required template sections, including a valid Linked Issue, Visual Proof with an N/A explanation, Testing, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. Resolution Complete the required template sections. Add a valid
✨ 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 new issues found.
Reviewed changes
- Test guard given a Run — adds
runId: 'run_legacy_local'to the twodb.createTaskcalls indb-stopping-worker-task-guard.test.ts, restoring them after #19542 maderunIdmandatory.
Verified the fix is correct and consistent: run_legacy_local is LEGACY_RUN_ID and is seeded into runs on every fresh database (INSERT OR IGNORE INTO runs in db/schema/migrate-v2-v12.ts), so requireRun('run_legacy_local') passes in the :memory: db these tests use. The literal form also matches the dominant pattern across the orchestration test suite. Ran pnpm test src/main/runtime/orchestration/db-stopping-worker-task-guard.test.ts — 7 passed.
The PR body's root-cause and systemic write-up (cancelled checks read as green, no up-to-date-base requirement, no push-triggered CI on main) is well-documented and worth carrying into any follow-up on the merge gate itself, but it is outside this diff's scope.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…se static analysis is green)
|
|
||
| function localWorker() { | ||
| const task = db.createTask({ spec: 'local work' }) | ||
| const task = db.createTask({ runId: 'run_legacy_local', spec: 'local work' }) |
There was a problem hiding this comment.
Thanks — correct, and the PR body is fixed: create-core-tables-sql.ts only supplies the column DEFAULT; the run_legacy_local row itself is inserted by migrate-v2-v12.ts (INSERT OR IGNORE INTO runs) during schema setup, which is why requireRun passes on a fresh :memory: db.
|
Closing as a no-op: #19544 merged the identical two hunks (commit 12f53da), so the diff against main on this test file is now empty. Merging this would produce zero content change. The diagnosis stands and is recorded here for reference: #19542 tightened The gap is the merge gate, not review quality — |
ELI5
Two PRs merged to
main16 seconds apart. One made "every Task must belong to a Run" a hard rule. The other added a new test that creates a Task without a Run. Each passed CI on its own, because each was tested against amainthat did not yet contain the other. Together they fail: 7 tests in one file throwRun is required, and every PR in the repo has been red ontest / tests node 24 3/8(and thereforeverify) since. This PR passes the Run in the new test — 2 lines.Root cause
98fdbc4, merged 08:03:07Z) changedcreateTaskfromtask.runId ?? LEGACY_RUN_IDtoif (!runId) throw new Error('Run is required'). This was deliberate: it migrated every existing test to passrunId: 'run_legacy_local'(254 added lines, including inside the two test fixtures it touched) and addedwriter-run-required.test.tsto pin the throw.d346d6f, merged 08:03:23Z) addeddb-stopping-worker-task-guard.test.tswith two barecreateTaskcalls — inlocalWorker()(line 15) and a standalone control case (line 62).main@15adbd9(06:35Z);compare 15adbd9...98fdbc4= ahead_by 4. fix(orchestration): file federated worker mail under the coordinator Run #19542's tightening was never in the tree fix(orchestration): refuse Task re-open under a live worker; allow stop re-issue on a stranded row #19551 was tested on. Each PR's own shard 3/8 was green (07:23Z and 07:28Z respectively).Neither author could have seen it. This is not a review-quality failure; the gap is entirely in the merge gate (see below).
Why fix the test and not
createTaskRe-loosening
createTaskwould undo an intentional, pinned invariant to make a newer test pass — symptom-patching. The stale side is #19551's test. Both bare calls now passrunId: 'run_legacy_local', which is:migrate-v2-v12.tsinserts therun_legacy_localrow (INSERT OR IGNORE INTO runs) during schema setup, andcreate-core-tables-sql.tsmakes it the column default, sorequireRun('run_legacy_local')passes in the:memory:db these tests use.Sweep: is this the only stale caller?
Yes, measured two ways:
pnpm testoversrc/main/runtime/orchestration+src/main/runtime/rpc/methods/orchestration→ 194 files, 1611 tests, 0 failures. A brace-aware scan found 106 other barecreateTask({...})calls; all are benign — 1 is the intentional pin inwriter-run-required.test.ts, the other 105 are RPC-harness tests whererpc-test-harness.ts:47wrapsdb.createTaskto injectrunId ?? activeRunId.Evidence this is a main break (measured)
main@d346d6f, no other changes:pnpm test db-stopping-worker-task-guard.test.ts→ 7 failed,Run is required, exit 1.d346d6ffail shard 3/8 (Remove settled-worker automatic resume and hibernation fences #19544 head39bc081, feat(ai-vault): local full-text search over agent session history #18112 headfb56ee4); a control PR whose run was created 4 min earlier (fix(ephemeral-vm): re-attach runtime-owned SSH relays after an app restart #19394, pre-d346d6f) passed it.mainHEAD is stilld346d6f; nothing has landed since.Systemic: three gaps that compound, not one
Two ungated breaks reached
maininside 24 minutes:static analysisjobcancelled(0 required checks reachedsuccesson its head), leaving 7 orphan i18n entries that failverify:localization-runtime-catalogfor every PR (fixed separately in fix(i18n): drop orphan TerminalPane.minimumContrast entries that broke main static analysis #19566).Neither author could have seen it. This is not a review-quality failure. The full cause is three gaps in the merge gate that compound, in this order:
mainthat did not contain the other, and nothing forced re-validation against the base they actually landed on.main.pr.ymltriggers onpull_requestonly, andunit-tests.yml/e2e.ymlareworkflow_call.maingets no CI on push, so neither break was ever visible onmainitself — both were observable only as red on other people's PRs. That is why this went hours undetected and why several agents each rediscovered it while debugging their own PR.The first two are fixed by branch protection with strict status checks (required checks + up-to-date base) — which
maincurrently cannot have, because it has no branch protection and no rulesets at all (both APIs return empty/404). The third needs a push trigger onmain.Why a standalone PR rather than riding along in #19544 / #19337
Both of those PRs currently carry an identical copy of this 2-line change bundled into unrelated feature work (#19544 is +1014/−1592 across 74 files; #19337 is 23 files). Landing it on its own is deliberate:
mainsilently goes red again — the fix leaves with the feature. An independent commit survives that.static analysisiscancelledon its current head — the fourth cancelled gate today. Waiting on those PRs means waiting on the exact failure mode this write-up is about.If either of theirs merges first, this PR becomes a clean no-op on this file and closes.
This PR's own first CI run demonstrated gap 2. It was opened at 08:45:33Z; #19566 merged at 08:45:52Z — 19 seconds later. GitHub snapshots the PR base at trigger time, so the first run validated
e958ae5against the pre-#19566main(d346d6f) and failed the runtime-catalog step that #19566 had just fixed. A plain merge oforigin/mainre-ran it against the base it will actually land on. That is exactly the re-validation an up-to-date-base requirement forces.Verification
pnpm test src/main/runtime/orchestration/db-stopping-worker-task-guard.test.ts→ 7 passed.oxlintandoxfmt --checkclean on the file. Diff is exactly the tworunId:additions.