Repository navigation
Conversation
…e restamped orchestration-all-start-versions-migration.test.ts creates the CURRENT schema and rewrites user_version backwards, so a migration that forgets its ALTER TABLE passes: the column was already there. The hand-written old-schema tests only fabricate the columns their own case cares about, so they miss it too. #19542 was a different mechanism, but the same shape -- every layer asserted against state it had built itself. These four fixtures were written by v1.4.180, v1.4.190, v1.4.198 and v1.4.199 running their own OrchestrationDb in a throwaway worktree, so a forgotten column is genuinely absent. The test asserts a migrated file reaches the same schema a fresh install creates, that every Run, Task, Dispatch, message and federated attachment still resolves, that the legacy graph is adopted exactly once, and that a second open changes nothing. Keeps the all-start-versions test: it covers every integer stamp, which binary fixtures cannot afford. The two are complements, and a comment there now says so.
…ored-version start path The first eight cases all entered migration at the v6 floor. Unbound direct mail sits under run_legacy_local at every tag up to v1.4.198, so hasConsistentLegacyAdoption distrusts the stamp and the whole chain replays -- which means the incremental path, where the stored 25/29/30 is trusted and only the tail migrations run, had no coverage at all. That is the path most upgrading users are on, and the one a wrong migration guard breaks. Each tag now also has a fixture without that message, and a case asserting the start version resolved from the raw file BEFORE OrchestrationDb opens it: the v6 floor when a legacy graph is present, the stored stamp when it is not. Changing migrateV39's guard to `current >= 25` -- a bug only the incremental path can see -- turns exactly the three old no-unbound-mail fixtures red and leaves 929 tests green, including all four unbound-mail ones. Also fixes a hole that edit exposed in the schema fingerprint: table_info does not report CHECK constraints, so a skipped CHECK-widening rebuild was invisible. The fingerprint now scans each table's DDL for CHECK clauses with balanced parentheses.
📝 WalkthroughWalkthroughAdds eight release-generated orchestration SQLite fixtures from four tags, with two historical mail variants per tag. Adds a generator and population module that record fixture metadata and expected rows. Adds migration tests for schema fingerprints, data access, legacy graph adoption, version resolution, reopening, and fixture immutability. Marks fixture files as binary and documents their generation and migration behavior. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The change is test-only and current fixtures remain valid, but aligning the duplicate adoption-count assertions would prevent confusing failures when fixtures evolve. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed change context, rationale, testing evidence, and compatibility analysis. However, it omits several required template sections, including ELI5, Why, Linked Issue with a completed issue number, Visual Proof or explicit N/A, Review, Agent skill upstream boundary, Notes, and the Checklist. Resolution Add the missing template sections. Complete the required Linked Issue entry, state N/A with a reason for Visual Proof if applicable, document AI disclosure when required, complete the Review, Agent skill upstream boundary, Notes, and Checklist sections, and include a concise ELI5 and Why section. Full details: Docstring CoverageExplanation Docstring coverage is 21.05% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 4 files. (3 skipped: 3 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.
🧹 Nitpick comments (1)
src/main/runtime/orchestration/db/shipped-schema-fixtures.test.ts (1)
368-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the two
legacy_adoptionsexpectations.Although all current fixtures set
legacyAdoptionsCountto0, line 317 includes this value while line 371 ignores it. If a fixture records a nonzero value, the assertions can disagree. Use the same expression in both places.♻️ Proposed alignment
- expect(adoptions, `${tag} legacy_adoptions rows`).toBe(carriesLegacyGraph ? 1 : 0) + expect(adoptions, `${tag} legacy_adoptions rows`).toBe( + expected.legacyAdoptionsCount + (carriesLegacyGraph ? 1 : 0) + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: de2a66cf-43d0-4e89-979a-79e6a64e873b
📒 Files selected for processing (15)
.gitattributessrc/main/runtime/orchestration/db/shipped-schema-fixtures.test.tssrc/main/runtime/orchestration/orchestration-all-start-versions-migration.test.tstests/fixtures/orchestration-db/README.mdtests/fixtures/orchestration-db/generate-fixtures.mjstests/fixtures/orchestration-db/manifest.jsontests/fixtures/orchestration-db/populate-fixture.mjstests/fixtures/orchestration-db/v1.4.180-no-unbound-mail.sqlitetests/fixtures/orchestration-db/v1.4.180.sqlitetests/fixtures/orchestration-db/v1.4.190-no-unbound-mail.sqlitetests/fixtures/orchestration-db/v1.4.190.sqlitetests/fixtures/orchestration-db/v1.4.198-no-unbound-mail.sqlitetests/fixtures/orchestration-db/v1.4.198.sqlitetests/fixtures/orchestration-db/v1.4.199-no-unbound-mail.sqlitetests/fixtures/orchestration-db/v1.4.199.sqlite
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ One minor observation — nothing blocking. This is a genuinely valuable complement to
orchestration-all-start-versions-migration.test.ts, and the core mechanism is sound.
Reviewed changes
- Fixture test (
shipped-schema-fixtures.test.ts) — opens 8 real SQLite files written by v1.4.180/190/198/199 and asserts the migrated schema fingerprint equals a fresh install's (sorted columns, CHECK clauses, indexes/triggers/views), alongside row addressability and legacy-adoption idempotency on reopen. - Fixture corpus + manifest (
tests/fixtures/orchestration-db/) — 8.sqlitefiles, amanifest.jsonpinning each file'ssha256and expected ids/counts, and two Node generators that reproduce them from each tag's ownOrchestrationDb. .gitattributes— marks the fixture*.sqlitefiles asbinary.
The key assertion expect(schemaFingerprint(db.db)).toEqual(freshSchemaFingerprint()) is an exact, sorted comparison that genuinely fails on a forgotten column or a CHECK narrow — I ran the file locally (33 passed) and the mechanism is confirmed by the PR's own red/red proof. This covers the class of migration regressions the current restamp test cannot.
ℹ️ Row-dropping migrations are still unobserved
The PR description lists "drops a row" among what this test catches, but no assertion actually reads the row counts. manifest.json records expected.tableRowCounts for every fixture and the Fixture type declares it, yet the test never asserts against it — and databaseShape only compares first-open vs reopen (idempotency), so a migration that deletes rows in a table no addressability probe touches (e.g. mutation_receipts, deliveries) passes silently. Either assert the stable subset of post-migration counts against the manifest, or drop the field and soften the claim.
Technical details
# Unused row-count data leaves "drops a row" uncovered
## Affected sites
- `src/main/runtime/orchestration/db/shipped-schema-fixtures.test.ts:42` — `tableRowCounts: Record<string, number>` declared, never read by any assertion.
- `tests/fixtures/orchestration-db/manifest.json` — `expected.tableRowCounts` populated for all 8 fixtures.
- `tests/fixtures/orchestration-db/populate-fixture.mjs` — collects the counts via `tableRowCounts(db)`.
## Required outcome
- A migration that deletes rows in a table not covered by the addressability probes should fail the test, OR the "drops a row" claim and the unused `tableRowCounts` field should be removed.
## Suggested approach (optional)
- Assert post-migration counts only for tables whose count is unaffected by the replayed migrations (e.g. `messages`, `tasks`, `dispatch_contexts`, `remote_dispatch_attachments`, `mutation_receipts`); exclude known-shifting tables (`runs`, `legacy_adoptions`, `run_coordinator_handles` seeding).
## Open questions for the human (optional)
- Is per-table row-count fidelity worth the maintenance cost of regenerating counts whenever a future migration legitimately mutates rows?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
What the user notices
Before: a user upgrading from v1.4.180/190/198 opens Orca and the orchestration DB migrates.
If a migration forgot an
ALTER TABLE, or its guard was wrong for the version the user is actuallyon, nothing in CI noticed:
user_versionis still stamped 40, so the app believes it is current.The user finds out when a query hits the missing column —
orca orchestration checkor a federatedworker's control mail fails at runtime, on their machine, with a
no such columnerror and no pathback. That is the mechanism behind the #19542 class: every layer asserted against state it had
built itself, so it could not see a state a real release had written.
After: eight SQLite files that v1.4.180, v1.4.190, v1.4.198 and v1.4.199 actually wrote are
checked in, and a test opens each with current code on every run of the unit suite. A migration
that forgets a column, narrows a CHECK, drops a row, loses a Run/Task/Dispatch id, flips a
readflag, guards itself wrongly for the incremental upgrade path, or is not idempotent on the second
open fails at PR time instead of on a user's machine. Nothing about the running product changes;
this PR is test surface only.
Two variants per tag, because a release wrote two states
The same release produced two on-disk states that migrate down different paths:
unbound-mail<tag>.sqliterun_legacy_local, so the skew probe distrusts the stamp and replays the whole chainno-unbound-mail<tag>-no-unbound-mail.sqliteThe second is the path most upgrading users are on, and the only one that can catch a migration
whose guard is right when replayed from the bottom and wrong when entered at the stored version.
Proof 3 below is exactly that bug.
The delete question
Nothing was deleted, and I looked hard at the obvious candidate.
orchestration-all-start-versions-migration.test.tsis the weak test this replaces in spirit, andthe brief allowed replacing it. I kept it: it walks every integer stamp from 0 to
SCHEMA_VERSIONin about half a second, which eight binary fixtures can never afford. It is weak inexactly one dimension — it builds the current schema and only rewrites
user_version, so aforgotten column is already present. The new test is strong in exactly that dimension and narrow in
versions. They are complements, and a comment in the old file now says so and names its counterpart
rather than leaving a future reader to rediscover the overlap.
I did not add a second map/ledger beside anything: the whole change is a fixture directory, one
generator, and one test. No product code is touched.
Proof
1. The gate is green on the branch, twice
2. A migration that forgets a column: caught ONLY here
Scratch state (local, reverted with
git checkout -- <file>): add a column tocreate-core-tables-sql.tsso fresh installs get it, and write no migration — the exact bug therestamp test cannot see.
924 other orchestration tests stay green, including
orchestration-all-start-versions-migration.test.ts. That class of regression currently has zerocoverage anywhere else in the repo.
3. A migration guard that is wrong only on the incremental path: caught ONLY by the new variants
Scratch state: change
migrateV39's guard fromcurrent >= 39tocurrent >= 25. Replaying fromthe v6 floor still runs it; entering at a stored 25/29/30 skips it, and the
worker_terminal_archivesCHECK is never widened.Exactly the three old
no-unbound-mailfixtures. All fourunbound-mailfixtures stay green,because they replay from 6 and never take that branch — which is why the amendment was needed.
Before this run the fingerprint compared
table_infoonly, and that edit was invisible to theentire suite (
932 passed); the fingerprint now scans each table's DDL for CHECK clauses,because
table_infodoes not report them.4. The brief's
migrate-v40edit: red here, and honestly, red elsewhere tooScratch state: delete the
ALTER TABLE … ADD COLUMN home_run_idfrommigrate-v40.ts.The all-start-versions test stays green, as the brief predicted. But two existing tests
(
federated-home-run-migration,nested-worker-depth-migration) also go red, because theyhand-fabricate a pre-v40
remote_dispatch_attachmentswithALTER TABLE … DROP COLUMN. So the v40edit is not unique evidence for this PR; proofs 2 and 3 are. Reporting both rather than only the
flattering one.
Wire / SSH / folder workspace
published field is touched. The fixtures do contain a
remote_dispatch_attachmentsrow written bya pre-v40 host, which is the storage side of a federated attachment, and the test asserts v40
still backfills its
home_run_id— the on-disk half of the fix(orchestration): accept a missing Run id from older federation coordinators #19689 failure. The wire half is PR 3.opened locally by whichever runtime owns it; an SSH host runs the same
OrchestrationDbon its ownfilesystem, so this gate covers an SSH host's DB exactly as it covers a local one. Nothing here
reports on, stops, or lists remote work, so no liveness vocabulary is involved.
worktree or repo, so nothing here assumes a git workspace. The generator uses
git worktreeonlyto check out release tags of the Orca repo itself when regenerating fixtures — that is a
maintainer action, never something a user's workspace shape reaches.
os.tmpdir()withpath.joinand readsuser_versionfrom the file header rather than opening the committed file (a WAL open leaves-wal/-shmbeside it even read-only, on every platform)..gitattributesmarkstests/fixtures/orchestration-db/*.sqlitebinaryso a Windows checkout cannot eol-mangle them.The generator is Node-only (
import.meta.dirname,esbuild's JS API,spawnSync('git', …)withwindowsHide) with no shell and no macOS assumption.Testing
node config/scripts/run-typecheck-projects-in-parallel.mjs(pnpm tc)./node_modules/.bin/tsc --noEmit -p config/tsconfig.node.json --composite falsesrc/main/runtime/orchestration(whole layer)pnpm run check:code-quality:changed0 new finding(s) across 4 changed file(s)…passed since aac38d698ff7pnpm run check:react-doctor:changed0 issuesoxlinton the changed filesoxfmt --checkon the changed filesAll matched files use the correct formatpnpm run check:max-lines-ratchetOK — 7 grandfathered suppression(s), no new bypassespnpm run check:ts-nocheck-ratchetOK — 171 grandfathered file(s), no new suppressionsgit diff --checkgit status-wal/-shmbeside the fixtures after a test rungit diff --cached --statBin 0 -> N bytesVitest was run directly as
./node_modules/.bin/vitest run --config config/vitest.config.ts …rather than through
pnpm test, which re-verifies the lockfile and triggers a node-pty rebuild inthe
node_modulesshared by every PR worktree.No lint disables, no
@ts-ignore/@ts-expect-error. Largest new file is the test at 377 lines(cap 800); the two
.mjsare 154 and 144 lines.No Playwright spec is added, so the e2e build steps do not apply.