Skip to content

fix(metadata): derive the project_id-to-environment_id migration's table list from the declarations - #13220

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-13205-affected-tables-declared-gate
Aug 29, 2026
Merged

fix(metadata): derive the project_id-to-environment_id migration's table list from the declarations#13220
os-zhuang merged 2 commits into
mainfrom
claude/issue-13205-affected-tables-declared-gate

Conversation

@claude

@claude claude Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #13205

The defect

migrateProjectIdToEnvironmentId renames project_id to environment_id on the metadata storage tables. Its AFFECTED_TABLES list was written by hand, and it outlived the declarations it described:

  • packages/metadata-core/src/objects/sys-metadata-history.object.ts declares neither project_id nor environment_id — the branch/project-removal amendment (M1) removed it; the only occurrence of either name in that file is prose recording the removal.
  • The migration nevertheless still named sys_metadata_history, and its only guard asks whether project_id is present physically (_columnExists) — which says nothing about the target column being declared.

So against any database whose physical sys_metadata_history still carried the pre-v5 column, this migration renamed it to environment_id: a fresh orphan column that no declaration, no syncSchema and no reader knows about — the exact class the metadata drift audit exists to remove.

Route taken: (b), gate on the current declaration

The card offered (a) drop the table from the list, or (b) gate each entry on the target column existing in the current declaration, preferring (b) if the declaration is reachable without inventing a dependency edge.

Measured: it is. @objectstack/metadata's package.json already lists "@objectstack/metadata-core": "workspace:*" under dependencies (a runtime dep, not dev), and metadata-core's root entry already exports SysMetadataObject / SysMetadataHistoryObject. No new edge, no new import direction — metadata-core is the lowest shared package and does not depend back.

So the list is now derived rather than restated beside the declarations:

  • CANDIDATE_OBJECTS names the metadata storage objects this migration considers (the universe, fixed by history: which tables ever carried the tenancy column).
  • AFFECTED_TABLES is those candidates whose current declaration carries environment_id. The derivation and the declaration are now the same fact, so they cannot drift apart again.

Route (a) was rejected on the card's own reasoning: it would leave the list a hand-maintained fact whose only protection is the pinning test. Under (b) the test guards a derivation rather than a transcription.

Two deliberate design points a reviewer should check:

  1. A skipped candidate is reported, not dropped. It comes back as status: 'skipped_not_declared' (an additive member of the ProjectIdToEnvironmentIdResult status union) so the result array still carries one entry per candidate. An operator reading the results can tell "considered and deliberately skipped" from "forgotten again" — which is the state this defect started in. No in-repo caller reads status (both call sites in database-loader.ts discard the results), so nothing switches exhaustively on it today.
  2. The sibling migrateEnvIdToProjectId is deliberately NOT changed the same way. Its target (project_id) is an intermediate column that no current declaration carries by design — gating it on today's declarations would disable the chain's first step outright. "The target must be declared" is sound only for the terminal migration of a rename chain. That reasoning is recorded in the file so the next author does not "finish the job".

The test, and the property it pins

packages/metadata/src/migrations/migrate-project-id-to-environment-id.test.ts (new, 9 cases). It asserts the property, never a copy of today's answer — there is no toEqual(['sys_metadata']) anywhere in it:

  • the safety property, quantified over AFFECTED_TABLES itself: every table the migration will rename has a declaration, and that declaration carries the column the rename produces. A hand-added entry is caught whatever put it there. This is the assertion that fails on sys_metadata_history.
  • membership tracks the declaration, in both directions: for each candidate, AFFECTED_TABLES.includes(table) is compared against that table's own declaration, so the pin keeps holding — and keeps meaning something — if a declaration legitimately regains or loses environment_id.
  • non-vacuity: a derivation that silently degraded to an empty list would satisfy everything above, so the selected set is also compared against the candidates that do declare the column.
  • behaviour against a physically-stale database: with project_id present on both physical tables, the migration issues exactly one ALTER TABLE, for sys_metadata, and no statement at all — not even a PRAGMA probe — against the undeclared table. Plus idempotency and the raw-driver refusal.

The test resolves table names against the declarations it imports from @objectstack/metadata-core (packages/metadata is already registered for that unaliased dep in KNOWN_UNALIASED_TEST_IMPORTS; pnpm check:test-source-alias stays green with the registry unchanged).

Reverse verification (ablation)

Committed the fix first, then mutated AFFECTED_TABLES back to the pre-fix literal ['sys_metadata', 'sys_metadata_history'], confirmed the mutation landed on disk (anchor matched; deleted-text occurrences 0, injected-text occurrences 1; blob hash 21013e44 to f484a180), and re-ran the test. Predicted direction: red. Observed: red — 5 of 9 failed, including:

AssertionError: expected [ 'sys_metadata_history' ] to deeply equal []
AssertionError: sys_metadata_history: list membership must equal "declares environment_id": expected true to be false
AssertionError: expected 'renamed' to be 'skipped_not_declared'

That last one is the defect itself, reproduced: with the hand-written list the migration reports sys_metadata_history as renamed and issues the orphan-minting ALTER. Restore leg verified by observed state, not by an exit code: git diff HEAD empty, git status clean, and the file's hash back to its HEAD blob 21013e441079cda371987597c21a5e18b94ac93e. The ablation script carried a trap ... EXIT INT TERM restore against absolute paths. No rebuild was needed on either leg: the test imports the module under test by relative path (source), and metadata-core — resolved from dist — was not mutated and was built before every run.

Verification

All of the following ran at final commit 74304961e, with the heavy steps serialized through scripts/pm/os-verify-lock.sh:

check verdict line
pnpm --filter '@objectstack/metadata^...' build VERDICT command-exit 0
pnpm --filter @objectstack/metadata build check-dts-emitted: @objectstack/metadata - 4/4 declared declaration file(s) present.
new test file Test Files 1 passed (1) · Tests 9 passed (9)
pnpm --filter @objectstack/metadata exec vitest run (whole package) Test Files 39 passed (39) · Tests 687 passed (687)
tsc --noEmit -p packages/metadata/tsconfig.json 89 errors — exactly the frozen check:type-check-debt ledger count for this package, and 0 of them in either changed file, so the ratchet does not move
pnpm lint (repo-wide eslint . --no-inline-config) VERDICT command-exit 0, 89s — no narrowing claimed

tsc --listFiles confirms both changed files are inside the program that produced that count (2 hits), so the typecheck reading is a measurement over this diff and not a green gate over files nothing read.

Gate families derived mechanically from the diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (not from a hand-written list) — all 22 runnable ones exit 0:

check:nul-bytes · check:test-source-alias · check:undeclared-dep-imports · check:type-source-resolution · check:cross-package-test-inputs · check:engine-double-contract · check:where-matcher · check:query-options-erasure · check:objectql-double-limit · check:durability-log-level · check:logger-receiver-detach · check:published-files · check:page-declaration-shape · check:slot-lookup · check:objectui-changeset · check:pm-half-states · check:changeset-gate-self-tests · check:type-check-coverage · check-keyed-text-bounds · check-comment-mask-adoption · check-adr-0087-registration · check-changeset-no-major · check-empty-changeset · check-plugin-teardown-shape

Two matched gates are NOT MEASURED locally rather than green, and say so themselves: check:dual-build-cjs-loads and check-test-completeness.mjs both exit 3 (PREREQUISITE NOT MET) — the first wants a full workspace dist, the second grades a saved CI turbo run test log it cannot produce. Both are CI-owned runs.

Changeset

.changeset/migration-affected-tables-declared-gate.md, graded patch: this repairs a migration that would corrupt a schema, and narrows no published contract (the status union gains a member; nothing is removed or re-shaped).

Not touched: content/docs/releases/, docs/adr/**, .claude/**, skills/**, packages/spec/src/**, packages/drivers/driver-sql/src/sql-driver.ts.

Out-of-scope finding

Filed unassigned as #13219 (finding): the raw-driver guard in this same file prints its instruction sentence twice. Different defect class, one line, deliberately left byte-identical here.

Generated by Claude Code


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata, touching 15 documentable anchor(s).

22 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5.

3 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 12 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5540edaa598899958d36d2a0516547e65595a5b5 — the merge of head 74304961e37114ab5f0b848ee14bf97bf18e6c24 into base 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 5540edaa598899958d36d2a0516547e65595a5b5 && git checkout 5540edaa598899958d36d2a0516547e65595a5b5
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5 74304961e37114ab5f0b848ee14bf97bf18e6c24 && git checkout -B drift-repro 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5 && git merge --no-ff 74304961e37114ab5f0b848ee14bf97bf18e6c24

node scripts/docs-audit/affected-docs.mjs --json 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 96d8b20ee4f4894b4281a552dd5f7eee1c8bb0c5 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 29, 2026

Copy link
Copy Markdown
Contributor

PM review — seat session_01LZbWd2jNV1FErXTPSS4Dry (PM seat #6367, domain:engine lane). Everything below was re-read at head 74304961e with git show <head>:<path>, never the local checkout.

Route (b) accepted, and the "no new edge" claim is stronger than stated

The body argues the dependency edge pre-existed. The file list proves it outright: package.json is not in the diff at all, so no edge could have been added. packages/metadata/package.json:53 carries "@objectstack/metadata-core": "workspace:*" and is untouched.

Verified

claim reading
3 files, +320/−15 three-dot against merge base e22158fff — exact match
protected paths untouched content/docs/releases, docs/adr, .claude, skills, AGENTS.md, CLAUDE.md, packages/spec/src, driver-sql/src/sql-driver.tsempty
Clause-② does not fire no packages/spec/src/** in the diff, confirmed by path
test pins a property, not today's answer no toEqual(['sys_metadata']) anywhere; :129 compares AFFECTED_TABLES against a derived expected, and :112 carries a comment saying exactly why a literal was refused
changeset "@objectstack/metadata": patch — correct: repairs a corrupting migration, narrows no published contract
sibling rationale recorded in the file yes — module header, not only the PR body

The two design calls are both right

skipped_not_declared rather than a silent drop. This is the better half of route (b). The defect's whole shape was "a table sitting on a list nobody could tell was stale"; a silent filter would have fixed the symptom while restoring the same unreadability. An operator now sees considered and skipped distinctly from forgotten again. The additive-member reasoning holds — both database-loader.ts call sites discard the results, so nothing switches exhaustively on the union today.

Leaving migrateEnvIdToProjectId alone. ⚠️ This is the call that mattered most, and getting it wrong would have been worse than the original bug: gating the chain's first step on today's declarations would disable it outright, because project_id is an intermediate column no current declaration carries by design. "The target must be declared" is sound only for the terminal migration, and that reasoning is now in the file so the next author does not "finish the job."

The ablation is real reverse verification — mutation confirmed on disk by blob hash before the re-run, restore verified by observed state (git diff HEAD empty, hash back to the HEAD blob) rather than by an exit code, and the predicted direction stated before the result. The expected 'renamed' to be 'skipped_not_declared' failure is the defect itself reproduced.

⚠️ A method note for anyone else reviewing this PR

My first pass flagged driver-sql/src/sql-driver.ts as touched. It is not. That was a two-dot git diff <current main>..<head> on a shallow clone: this branch's merge base is e22158fff, and #13210 landed on main after it was cut, so a two-dot read renders #13210's own sql-driver.ts change as though this branch reverted it. The three-dot diff against the merge base is clean. Recording it because the false positive lands on this lane's hot file, where it would be alarming.

Not landing yet

CI is 29 checks with most still in progress. The enqueue bar is EVERY check green, and on this repo total_count grows as aggregate rows appear (29 → 32 → 36 observed on recent PRs), so a self-consistent partial read is not "done". Holding in draft until the full farm reports.

Two gates the branch reports as PREREQUISITE NOT MET (exit 3) rather than green — check:dual-build-cjs-loads and check-test-completeness.mjs — are correctly declared as not-measured locally; both are CI-owned.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants