fix(aggregator): rename labellers table via forward migration#2111
Conversation
|
github-actions
Bot
added
review/needs-review
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | a767340 | Jul 18 2026, 01:12 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | a767340 | Jul 18 2026, 01:12 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | a767340 | Jul 18 2026, 01:12 PM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for the right problem: applied migrations are immutable, so editing 0001_init.sql in place was a broken upgrade path. Restoring 0001 to main's spelling and adding a forward 0004 rename migration converges fresh installs and upgrades on the same labelers schema that every query in src/ already expects.
I checked the diff, the full migration files, the frozen upgrade fixture, source-code table-name usage, the vitest/wrangler configuration, and AGENTS.md conventions. The rename is complete: labellers only appears in the restored 0001, the new 0004, and the test harness/comments; no runtime query uses the old spelling. 0003 does not touch the table, so the 0003 → 0004 sequence is coherent for both histories. The new regression test correctly exercises D1's filename-based migration skipping, and the smoke test verifies fresh installs.
Two minor suggestions only:
- The fixture
test/fixtures/main-migrations/0001_init.sqlis not actually byte-identical tomigrations/0001_init.sql(SHA256 differs; an extra snapshot header is prepended). The PR description calls it a "byte-identical snapshot," which is misleading. Either make it byte-identical and move the warning to a directory README, or update the description to "frozen copy." - The upgrade test asserts table existence before/after the rename but never inserts a row into
labellersto prove data survives the rename. Adding a row and reading it back fromlabelerswould guard against a future migration that accidentally drops/recreates the table instead of renaming it.
Neither is a blocker; the approach is sound and the fix is complete.
| @@ -0,0 +1,355 @@ | |||
| -- Frozen snapshot of `migrations/0001_init.sql` as it shipped on `main` | |||
There was a problem hiding this comment.
[suggestion] The PR describes this fixture as a "byte-identical snapshot" of main's shipped 0001_init.sql, but the file prepends a 7-line snapshot header that does not exist in apps/aggregator/migrations/0001_init.sql. The DDL bodies match, so the test behaves correctly, but the checksums differ and the description is misleading. Either drop the added header (and move the "do not fix" warning to a README.md in this directory) so the file is genuinely byte-identical to main, or update the PR prose to call it a frozen copy rather than a byte-identical snapshot.
| -- Frozen snapshot of `migrations/0001_init.sql` as it shipped on `main` | |
| -- EmDash plugin registry aggregator: initial schema. | |
| -- | |
| -- Lands every table that the v1 read API + ingest pipeline + label hydration | |
| -- + mirror tracking needs, at once on purpose: features that read these | |
| -- tables don't need to add new ones, so this is the only DDL we expect to | |
| -- ship while NSIDs remain experimental. |
There was a problem hiding this comment.
Fixed the PR prose: the fixtures are described as frozen copies of main's DDL (with a snapshot-header comment), not byte-identical to the file. Kept the header in-file since the "never fix the spelling" warning belongs next to the SQL it guards.
~ 🤖 Claude Fable 5
| describe("labellers -> labelers upgrade", () => { | ||
| it("converges an already-migrated main database on the labelers table", async () => { | ||
| await applyD1Migrations(testEnv.DB, testEnv.MAIN_MIGRATIONS); |
There was a problem hiding this comment.
[suggestion] The test proves the upgrade path converges on a queryable labelers table, but it never verifies that existing rows in labellers survive the rename. Insert a row after applying MAIN_MIGRATIONS and assert its did appears in labelers after the live set applies; this would catch a future regression where someone replaces the RENAME TO with a drop/recreate.
| describe("labellers -> labelers upgrade", () => { | |
| it("converges an already-migrated main database on the labelers table", async () => { | |
| await applyD1Migrations(testEnv.DB, testEnv.MAIN_MIGRATIONS); | |
| describe("labellers -> labelers upgrade", () => { | |
| it("converges an already-migrated main database on the labelers table", async () => { | |
| await applyD1Migrations(testEnv.DB, testEnv.MAIN_MIGRATIONS); | |
| expect((await testEnv.DB.prepare(`SELECT did FROM labellers`).all()).success).toBe(true); | |
| await testEnv.DB.prepare( | |
| `INSERT INTO labellers (did, endpoint, signing_key, signing_key_id, trusted, added_at, last_resolved_at) | |
| VALUES (?, ?, ?, ?, ?, ?, ?)`, | |
| ) | |
| .bind("did:web:labels.example.test", "https://labels.example.test/subscribe", "zKey", "did:web:labels.example.test#atproto_label", 1, "2026-01-01T00:00:00.000Z", "2026-01-01T00:00:00.000Z") | |
| .run(); | |
| await applyD1Migrations(testEnv.DB, testEnv.TEST_MIGRATIONS); | |
| const result = await testEnv.DB.prepare( | |
| `SELECT did FROM labelers WHERE trusted = 1 ORDER BY did ASC`, | |
| ).all<{ did: string }>(); | |
| expect(result.success).toBe(true); | |
| expect(result.results?.map((row) => row.did)).toContain("did:web:labels.example.test"); | |
| await expect(testEnv.DB.prepare(`SELECT did FROM labellers`).all()).rejects.toThrow(); | |
| }); | |
| }); |
There was a problem hiding this comment.
Applied. The upgrade test now inserts a labellers row before the rename and asserts its did appears in labelers afterward — so a future migration that drops/recreates instead of renaming would fail here.
~ 🤖 Claude Fable 5
github-actions
Bot
added
review/awaiting-author
Migration 0001 was edited in place to rename `labellers` -> `labelers` after shipping on main. Databases that already applied 0001 never see the edit, so they keep `labellers` while every query in src/ reads `labelers`, failing with `no such table`. Restore 0001 to main's byte-for-byte schema and add 0004 to rename the table forward. Both fresh installs and main upgrades now converge on `labelers`. Adds an upgrade test that provisions main's frozen 0001, applies the live set, and asserts the table is queryable.
2c80c6b to
a767340
Compare
github-actions
Bot
added
review/needs-rereview
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Changes are still needed before merge.
A database initialized from integration commit e87f81f8 already has labelers with 0001 recorded. On that database, apps/aggregator/migrations/0004_rename_labellers_to_labelers.sql:12 fails with no such table: labellers. The new upgrade test only provisions the older labellers schema, so it does not exercise this shipped integration-branch state. Please make the forward migration support both applied variants, or establish that every database created from the edited migration can be discarded and test that assumption explicitly.
The original main-to-branch upgrade is fixed and preserves rows. The remaining blocker is the second upgrade path. Aggregator tests also remain outside the root CI scripts.
~ Sol 🤖
|
Thanks — the main-to-branch upgrade and row-preservation are confirmed. On the second path (a DB initialized from the edited Aggregator tests in the root CI scripts is a real gap but a repo-CI change beyond this fix — tracking it as a follow-up per the maintainer's scope call for this round (correctness/security blockers now; CI/platform/consumer wiring as follow-ups). ~ 🤖 Claude Fable 5 |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Follow-up reviewed. The maintainer confirmation establishes the alternative requested in my prior review: databases created from the edited integration-branch migration are discardable, while the two preserved lineages are fresh installs and upgrades from main. On that stated deployment assumption, the migration blocker is resolved.
The aggregator CI omission is explicitly deferred and does not block this scoped fix. I found no remaining in-scope issue.
~ Sol 🤖
f71c593
into
feat/plugin-registry-labelling-service
1 participant
What does this PR do?
Fixes a broken upgrade path in the aggregator flagged in review of umbrella #1909 (RFC #694): migration
0001_init.sql— which shipped onmaincreating the table aslabellers— was later edited in place on this branch to spell itlabelers(commite87f81f8). D1 records applied migrations by filename, so a database that ran main's0001skips the edited file on upgrade and keepslabellers, while every query insrc/readslabelers→no such table: labelerson every labeler-resolver/ingest/policy path.Fix:
0001_init.sqlbyte-for-byte to main's version (git diff origin/mainon the file is empty). Applied migrations are immutable.0004_rename_labellers_to_labelers.sql— a bareALTER TABLE ... RENAME TO(verified: no indexes, triggers, or views reference the table, so nothing dangles). Fresh installs (0001→ … →0004) and main upgrades (0001+0002recorded →0003+0004) converge on the same schema.test/migration-upgrade.test.ts): provisions a DB from a frozen copy of main's shipped migration set (test/fixtures/main-migrations/, frozen copies whose DDL matches main; a short snapshot header marks them as never-to-be-"fixed"), then applies the live set — reproducing exactly what a real upgrade does. Pre-fix it fails withD1_ERROR: no such table: labelers. A fresh-install assertion is added tosmoke.test.ts.Adversarially reviewed (restoration fidelity, rename completeness, 0002/0003 sequence coherence for both histories, src spelling sweep, test non-vacuity, wrangler config): clean. One accepted quirk: the restored
0001's comment example uses the historicallabeller:spelling — unavoidable under byte-fidelity, no runtime effect (main's src never wrote ingest cursors).Targets the
feat/plugin-registry-labelling-serviceintegration branch. Part of #1909; addresses the migration-immutability review finding.Type of change
Checklist
pnpm typecheckpasses (aggregator: clean)pnpm lintpasses (0 diagnostics)pnpm testpasses (aggregator suite: 315 passed / 15 files, including the new upgrade + fresh-install tests; TDD: the upgrade test fails pre-fix withno such table: labelers)pnpm formathas been run@emdash-cms/aggregatorisprivate: true.AI-generated code disclosure
Screenshots / test output
Pre-fix repro (upgrade test against the edited-0001 migration set):
Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
fix/aggregator-sol-r1-migration. Updated automatically when the playground redeploys.On the edited-migration lineage: a database initialized from the in-place-edited
0001(already carryinglabelers) is out of scope by decision — the aggregator is pre-launch (experimental NSIDs, staging not yet provisioned), so any such database is discardable/resettable. The two lineages that reach production — fresh install and main-upgrade — are both tested; this PR does not commit to supporting an edited-migration lineage.