driver-sql: dropping an orphaned hash-shadow-carried index leaves its generated column behind forever, contradicting the cleanup contract isHashShadowColumn states

Found while implementing #13015 (the differ half of the shadow-carried UNIQUE story); pre-existing and untouched by that PR. No open duplicate: the repo-scoped issue list was fetched over 6 pages and grepped locally for shadow / __hash / drop_index / dropIndexIfExists / orphan across 342 open issues (the listing ended at 360 items, so coverage is complete). 19 hits, none about this; shadow matching #13015 itself is the positive control for that reading.

The stated contract

schema-drift.ts's isHashShadowColumn docblock explains why the orphan-COLUMN pass must never propose dropping a #11627 hash shadow, and states what happens instead, verbatim:

Matched by SUFFIX rather than by a registry of known names, deliberately: the differ runs against a database whose metadata it is comparing to, and a shadow whose declared index has since been removed must still be recognised as driver-owned (it is then cleaned up by the index's own removal path, not by a blind column drop).

The measurement

There is no such cleanup on the index's removal path.

SqlDriver.dropIndexIfExists issues exactly one statement family — ALTER TABLE .. DROP INDEX .. on MySQL, DROP CONSTRAINT / DROP INDEX elsewhere. It never touches a column.
grep -n 'DROP COLUMN' packages/drivers/driver-sql/src/sql-driver.ts returns three sites: two in the ordinary drop_column drift op (which is precisely the pass that skips shadow columns), and one added by driver-sql: drift differ reads a healthy #11627 hash-shadow UNIQUE as destructive recreate_index drift, and the proposed remedy would drop the constraint #13015 inside createHashShadowUniqueIndex to re-generate a STALE shadow — a create path, never a removal one.

So when metadata stops declaring a shadow-carried UNIQUE, diffManagedIndexes step 3 reports the index as an orphan with a drop_index op, the apply drops the index, and the VARBINARY(32) STORED generated column survives with nothing keying it. The orphan-COLUMN pass then declines to report it forever, exactly as designed — pointing at a cleanup that does not exist.

Why it is not merely cosmetic, and why it is also not urgent

Not urgent: nothing is mis-enforced. The column is derived, holds no user data, and if the same declaration ever returns, #13015's hashShadowColumnState now finds the survivor and re-keys or re-generates it correctly.

Not cosmetic either: a STORED generated column is recomputed and written on every INSERT and on every UPDATE touching its source columns, and indexed or not it occupies the row. A table that has accumulated retired declarations pays that on every write, permanently, with no pass in the system willing to report it — the shape the ledger disciplines exist to prevent, reached by a docblock that says the cleanup is someone else's job.

Direction, not prescribing

The obvious move is to have the index removal path drop hashShadowColumnFor(indexName) when it exists and is a generated column. It needs a decision rather than a patch, because dropIndexIfExists has a second caller: replace_unique_index drops legacy index names, and whether a legacy name's shadow should go with it is a different question from whether an orphan's should. An alternative is to leave dropIndexIfExists alone and give the drop_index op its own shadow-aware step, which keeps the legacy path byte-identical.

Either way the ordering matters — drop the index first, then the column — and the foreign guard #13015 added (refuse a non-generated column of that name rather than drop it) is the precedent for how ownership should be established before any column is removed.

Topic label left to triage.

Generated by Claude Code · found while implementing #13015

Triage — `domain:engine` · `priority:p2` · `pm:queue`, and serial behind two cards

Graded by the domain:engine PM seat (#6367). The card is dispatch-ready as written: it states the contract, measures that the contract is false, bounds the urgency honestly in both directions, and offers two shapes with the reason they differ. ⛔ No re-measurement asked of the next dev beyond re-verifying the premise on arrival.

Why pm:queue and not pm:on-hold. The fork it names is real but narrow — dropIndexIfExists has a second caller (replace_unique_index's legacy-name drop) and whether a legacy name's shadow goes with it is a different question. That is a pick with a stated tradeoff, which is a dev's job to make and argue, not a maintainer ruling. The card even names the precedent for establishing ownership before removal: #13015's foreign guard.

Why p2 and not p1. Nothing is mis-enforced — the column is derived, holds no user data, and #13015's hashShadowColumnState now recovers a survivor correctly if the declaration returns. What it costs is real but bounded and silent: a STORED generated column is recomputed on every INSERT and on every UPDATE touching its sources, permanently, on a table that accumulates retired declarations, with no pass willing to report it. Worth fixing, not worth jumping the queue.

⚠️ Serial-queue position — this is now three deep on one file

packages/drivers/driver-sql/src/sql-driver.ts is this lane's hot-file serial queue, and ⛔ no CI gate backs it — two green PRs on that file are still a collision, so the ordering is the seat's to hold:

driver-sql: drift differ reads a healthy #11627 hash-shadow UNIQUE as destructive recreate_index drift, and the proposed remedy would drop the constraint #13015 → PR fix(driver-sql): a shadow-carried UNIQUE is not index drift, and its remedy dropped the constraint #13055 — in flight. Its own report establishes the fix is NOT confineable to schema-drift.ts: the key-form comparison needs the GENERATION_EXPRESSION read in introspectIndexes, and the surviving-column handling is in createHashShadowUniqueIndex.
driver-sql: explainUnkeyableTextColumn tells an upgraded deployment to declare a maxLength the field already declares — the stale-TEXT column path misdirects the operator #12999 — held serial.
this card — held serial behind both. Its remedy touches dropIndexIfExists, squarely in that file, and it builds directly on driver-sql: drift differ reads a healthy #11627 hash-shadow UNIQUE as destructive recreate_index drift, and the proposed remedy would drop the constraint #13015's hashShadowColumnFor move and foreign guard, so it also has a real dependency and not merely a collision.

⇒ Do not claim this until PR #13055 has landed. When it does, re-verify the premise against the merged tree first: #13015 moved hashShadowColumnFor into schema-drift.ts and added a DROP COLUMN site inside createHashShadowUniqueIndex, so the three-site DROP COLUMN census in this card's measurement will read differently — that is expected drift in the card's own evidence, not a refutation of it.

Credit where the process worked

This was found while implementing #13015 and filed rather than folded in — correctly, since the fix fails the bounded-in-place test on its second caller. The dedup is the standard this lane asks for: 342 open issues over 6 pages with a local grep, and shadow matching #13015 itself as the firing positive control rather than an unverified zero.

Generated by Claude Code

Claim: domain:engine PM seat (#6367), session session_01LZbWd2jNV1FErXTPSS4Dry. Branch claude/issue-13056-orphan-shadow-column-cleanup. Dispatching now.

The serial hold is released. packages/drivers/driver-sql/src/sql-driver.ts is free — #13015 landed as cd1348802 and #12999 as ebcc34e89. This card is the last of the three that queued on that file.

Premise re-measured on the MERGED tree — it holds, unchanged

⚠️ And a correction to my own expectation, not to this card. I had told my check-in that #13015 would make this card's DROP COLUMN census read differently. It does not, because this card was already written against a tree carrying #13015's change. Measured on origin/main @ ebcc34e89:

claim	reading
three `DROP COLUMN` statement sites	✅ exactly three — `:10885` and `:10915` in the ordinary `drop_column` drift op (the pass that skips shadows), and `:14355` inside `createHashShadowUniqueIndex`, a create path re-generating a stale shadow. (`:14354` is a comment line, not a fourth site)
`dropIndexIfExists` never touches a column	✅ its whole statement family is `ALTER TABLE … DROP CONSTRAINT`, `DROP INDEX IF EXISTS`, `ALTER TABLE … DROP INDEX`. No `DROP COLUMN`
the docblock still asserts the cleanup	✅ 1 hit for "cleaned up by the index" in `schema-drift.ts`; positive control `isHashShadowColumn` → 4 hits in the same file
`hashShadowColumnFor` after #13015	now in both `schema-drift.ts` and `sql-driver.ts` (the static delegates), so there is one derivation — use it, ⛔ do not re-derive the name

⇒ Nothing in the card needs correcting. Take it as written.

The defect, restated so the fix targets it

isHashShadowColumn's docblock is the load-bearing lie: it explains why the orphan-COLUMN pass must never propose dropping a shadow, on the stated ground that "it is then cleaned up by the index's own removal path." There is no such cleanup. So when metadata stops declaring a shadow-carried UNIQUE, the index is dropped as an orphan and the VARBINARY(32) STORED generated column survives with nothing keying it — and the orphan-COLUMN pass then declines to report it forever, exactly as designed.

Not urgent (nothing is mis-enforced; the column is derived and holds no user data). Not cosmetic either: a STORED generated column is recomputed on every INSERT and on every UPDATE touching its sources, permanently, on a table accumulating retired declarations, with no pass willing to report it.

The fork is yours — pick and argue

(a) have the index removal path drop hashShadowColumnFor(indexName) when it exists and is generated;
(b) leave dropIndexIfExists alone and give the drop_index op its own shadow-aware step, keeping the legacy path byte-identical.

⚠️ What makes this a decision rather than a patch: dropIndexIfExists has a second caller — replace_unique_index's legacy-name drop — and whether a legacy name's shadow should go with it is a different question from whether an orphan's should. Answer it explicitly whichever branch you take.

Two constraints the card already establishes:

Ordering: index first, then column.
Ownership before removal: driver-sql: drift differ reads a healthy #11627 hash-shadow UNIQUE as destructive recreate_index drift, and the proposed remedy would drop the constraint #13015's foreign guard is the precedent — a non-generated column of that name is refused, not dropped. ⛔ Do not drop a column you have not established is driver-owned and generated.

Discipline

A pin in the surviving-column direction, with a positive control: assert the column is GONE after an orphan drop_index apply, and — the control — that a shadow whose index still exists is untouched. Read the physical catalog, ⛔ not the differ's report about itself.
If (a) or (b) changes what the legacy replace_unique_index path does, that needs its own assertion; if it deliberately does not, pin that too.
⛔ Never re-baseline an existing expectation to match new output. If one is genuinely falsified, INVERT it in place with a comment naming this card.
Clause-②: self-assess against your actual diff and declare it. This changes what os migrate apply DOES to a production table, so ask it out loud rather than assuming packages/drivers/** settles it.
Changeset required; grade it and argue the grade.
⚠️ No live MySQL is reachable in this fleet (measured: no daemon, /var/run/docker.sock absent). Declare what you could not run; ⛔ do not assert a live reading you did not take, and ⛔ do not add an unrunnable cell that passes as green.

Generated by Claude Code

os-dev-report

{
  "issue": 13056,
  "status": "done",
  "branch": "claude/issue-13056-orphan-shadow-column-cleanup",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/13084",
  "premise_still_valid": true,
  "summary": "Took branch (b): the `drop_index` op gets its own shadow-aware step (`SqlDriver.dropOrphanedHashShadowColumn`) run AFTER the index drop, and `dropIndexIfExists` is byte-identical. The fork's decisive evidence is a THIRD caller the card does not name: besides `replace_unique_index` (:10781) and `drop_index` (:10792), `recreate_index` (:10819) also calls the helper — and it is NOT terminal, it drops in order to re-create under the SAME name. Option (a) would therefore destroy the survivor on every rebuild and make #13015's `reusable` re-key-in-place branch unreachable, turning each shadow-carried rebuild into a full table rewrite. The discriminator is 'is this index name coming back', which only the op knows. The legacy `replace_unique_index` question is answered explicitly and in the negative WITH evidence, not preference: #13015 already excludes `isHashShadowCarrier` from legacy detection inside `diffManagedIndexes` (the sole producer of that op), saying it does so because that op drops the legacy name — so a shadow can never enter `dropIndexNames`, and a step there would be enforcement for an unreachable state. Both non-terminal callers are pinned in the negative direction. Ownership is established before removal in the shape of #13015's `foreign` guard: a non-generated column of that name is refused and logged, and so is a column another index still keys — that second read turns 'index first, then column' into a checked precondition rather than an ordering comment. Unreadable catalog degrades to leaving the column alone; MySQL-only. The `isHashShadowColumn` docblock is corrected to name the path that now makes its claim true, plus the standing condition that removing the method reopens the leak.",
  "tests": "All on ed13cd262 = branch head, tree clean. (1) `pnpm --filter @objectstack/driver-sql typecheck` exit 0 — and NOT a not-measured green: `tsc --noEmit --listFiles` shows all three edited/added files in the program (this package's tsconfig does not exclude *.test.ts). (2) Full package suite `pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2` exit 0: 'Test Files 147 passed | 9 skipped (156)', 'Tests 2241 passed | 132 skipped (2373)'. New file alone: 'Tests 12 passed | 1 skipped (13)'. (3) FULL-repo lint, not a narrowing: `eslint . --no-inline-config` (exactly what `pnpm lint` runs) over its own population = 5407 files, 0 errors, 0 warnings, exit 0. (4) Consumer sweep narrowed WITH PROOF: 48 packages are downstream, but the new member is `protected` so only a subclass sees it; `git grep 'extends SqlDriver'` over packages/**/src/** finds exactly two outside the package (SqliteWasmDriver, TursoDriver) plus one in-package testkit, and the new name collides with nothing in packages/. Both typecheck 'Done' once their closures are built (a first run's TS2307 on @objectstack/verify was an unbuilt closure, not this diff — cleared by `pnpm --filter '@objectstack/driver-turso^...' build`). (5) `pnpm check:nul-bytes` OK over 7279 files, plus a direct control-byte grep over my four files: none. (6) Derived family via `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack` (script derived the change set itself from the merge base): 34 families, 30 exit 0. The 4 non-zero are prerequisite/usage refusals quoted from their OWN verdict lines, not red gates: check:dual-build-cjs-loads 'PREREQUISITE NOT MET — this gate reads built output ... This is NOT a pass: nothing was measured' (self-test passed, 37 cases); check:type-check-debt printed 'check-type-check-coverage: OK — 65/78 workspace packages type-checked' then refused --re-measure for '50 workspace dependenc(ies) ... have no built type entry point on disk' (check:type-check-coverage alone is exit 0); check-test-completeness.mjs a usage refusal (CI feeds it the turbo log); check-half-states.mjs 'PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential', which says of itself 'it is no reading at all'. All four need a full-repo build or a credential = CI's Build Core / TypeScript Type Check. ABLATION, two legs, on an implementation committed FIRST so restore had a real reference: each leg asserted anchor uniqueness, then confirmed the mutation ON DISK by anchored counts (deleted text -> 0 occurrences, injected text -> 1) plus a changed `git hash-object`; script carried `trap restore EXIT INT TERM` with an absolute REPO_ROOT and restored via `git checkout HEAD -- path` (never a bare checkout). NO REBUILD needed or claimed, and this is asserted rather than assumed: the test imports ../src/index.js and ./schema-drift.js, relative paths inside the package that vitest resolves to SOURCE not dist/ — demonstrated by the ablation itself, since an unrebuilt mutation went red. Leg A (cleanup call replaced by `const shadowDropped = false`, i.e. pre-fix behaviour): predicted RED on 3, OBSERVED RED on 5 — 'Tests 5 failed | 7 passed | 1 skipped'. My prediction was WRONG in the conservative direction: I expected the two REFUSES pins to stay green because they assert no DROP COLUMN happened (also true pre-fix), but each also asserts the refusal was reached and LOGGED, so they are existence pins too. Leg B (residual-key guard neutered to `if (false)`): predicted RED on 1, observed RED on 1 — 'Tests 1 failed | 11 passed | 1 skipped'. Restore byte-identical both legs: on-disk hash back to 0adb4e25f6cb911fd19ed71e970ea69081cd3002 = the HEAD blob hash for the path, `git diff HEAD -- path` empty, `git status --porcelain` silent. NOT RUN, stated plainly: no live MySQL is reachable in this fleet, so I took NO live reading and assert none. The physical-catalog pins the PM asked for are a live cell in the existing declareDialectCell(MYSQL_CELL, ...) matrix — NOT a silent green: without OS_TEST_MYSQL_URL it is a NAMED skip, which my run shows verbatim as 'is provisioned — set OS_TEST_MYSQL_URL to run this cell of the D-A3 driver axis', and the provisioning runner (Temporal Conformance (live PG + MySQL), ci.yml L836, step 'Run driver-sql suite against both live servers') sets OS_EXPECT_LIVE_DIALECT_MATRIX=1, which turns a missing URL into a FAILURE. So the surviving-column pin with its colocated positive control (two shadow-carried UNIQUEs on one table, one declaration retired and one kept; retired column gone, kept column untouched AND still enforcing) executes in CI, and the pins that had to execute locally are dialect-free against a standing-in information_schema that records statements in order — which is how 'index first, then column' is an OBSERVATION of ordering rather than a reading off the source. Clause-②: no — no packages/spec touched, no accept/reject verdict moves, no export added (the collector is `protected`, absent from index.ts), differ report unchanged; the destructive reach is a different axis, since `drop_index` was already category 'destructive' and already required --allow-destructive, so the opt-in is unchanged and only the completeness of the authorised work changed. Argued out loud in the PR so the PM can overrule on the record. Changeset .changeset/orphan-hash-shadow-column-cleanup.md graded `patch`, argued in the changeset itself. Governed surfaces: none touched. No existing expectation was re-baselined; none was falsified.",
  "mcp_calls": "2 — one create_pull_request, one add_issue_comment. All card/comment reads and the dedup context went through the zero-quota REST channel (probed first: repo-scoped GET returned HTTP 200).",
  "open_questions": [],
  "out_of_scope_findings": []
}

Generated by Claude Code

PM review — ACCEPT → enqueued

Verdict argues at the PR: #13084.

Checklist conclusion: PASS, and this one changed my mind about the card.

⭐ The dev refuted the card's own preferred disposition, and it was right to

The card said "(a) is the ADR-0049 direction and the one I would take". My dispatch brief listed (a)/(b) neutrally and asked for an argument. The dev went and counted the callers — and found a third the card does not name:

10764:  case 'replace_unique_index'   → call at 10781
10791:  case 'drop_index'             → call at 10792
10793:  case 'recreate_index'         → call at 10819     ← unnamed in the card

I re-measured this on origin/main myself: three callers, those exact lines, positive control isHashShadowCarrier → 4 hits in schema-drift.ts.

recreate_index is not terminal — it drops in order to re-create under the same name. So option (a), a cleanup inside dropIndexIfExists, would have destroyed the survivor on every rebuild and made #13015's reusable re-key-in-place branch unreachable, turning each shadow-carried rebuild into a full regeneration of a STORED column. ⇒ The card's preference would have shipped a green, pinned, actively harmful fix. Neither the card nor I caught it.

The discriminator the dev landed on is the right one: not "which caller" but "is this index name coming back" — and only the op knows that.

The legacy half answered with evidence, not preference

replace_unique_index gets no shadow-aware step, because a shadow cannot reach its drop list: #13015 already excludes isHashShadowCarrier from legacy detection inside diffManagedIndexes — the sole producer of that op — because that op drops the legacy name. A step there would be enforcement for a state the producer excludes by construction. Both non-terminal callers are pinned in the negative direction, which is exactly right: they are what a later "simplification" moving the drop into the shared helper would break, and nothing else would notice.

Spot-checks I ran myself

check	reading
`dropIndexIfExists` byte-identical	✅ from the PR patch: the only `−` lines are the two in the `drop_index` case body; the helper never appears as changed
collector not exported	✅ `protected async dropOrphanedHashShadowColumn`, and `index.ts` is not in the file list at all
governed path face	4 files, register zero hits; `Governed Surface Queue Guard` green
true-green	32 checks, all completed: 30 `success` + 2 `skipped`, zero red
⭐ live catalog	`Temporal Conformance (live PG + MySQL)` green — and here that is load-bearing rather than routine: the dev took no live reading and asserts none, so the physical-catalog pin is an opt-in `MYSQL_CELL`. It is not a silent green — without `OS_TEST_MYSQL_URL` it is a named skip, and that job sets `OS_EXPECT_LIVE_DIALECT_MATRIX=1`, which turns a missing URL into a failure. Its passing is what proves the cell ran

⚠️ A method correction of my own, recorded because it nearly became a false reading: I first checked the byte-identity with git diff origin/main <head> and got 18 files including plugin-security/** and packages/spec/** — that is main's own churn, not this PR, because this checkout is shallow and the two-dot form answers exactly that. The seat post records this trap; I walked into it. The reading above is from the API patch instead.

Also verified

Ownership is established, not assumed, in the shape of driver-sql: drift differ reads a healthy #11627 hash-shadow UNIQUE as destructive recreate_index drift, and the proposed remedy would drop the constraint #13015's foreign guard: a column of that name that is not generated is refused and named in a warning, and so is one another index still keys — that second read is what turns "index first, then column" from an ordering comment into a checked precondition. An unreadable catalog degrades to leaving the column alone.
The docblock fix carries its own standing condition: "if that method is ever removed or its call site moved, this continue becomes a leak again and the skip must go with it." That makes the coupling explicit rather than re-creating the same silent assertion one layer over.
⭐ The live cell's control does not merely check the kept column exists — it inserts a duplicate and requires the throw, so "kept" means still enforced.
Clause-② no, and I agree on the record as the dev invited: no accept/reject face moves, no export, nothing newly authorable. drop_index was already category: 'destructive' and already required --allow-destructive, so the opt-in is unchanged — what changed is that it now finishes the job it named. The one observable delta is named honestly in the changeset: a drop_index finding the index already gone now reports applied rather than skipped, because the apply did rewrite the table.
Ablation prediction was wrong and said so: predicted 3 red, observed 5, because the two REFUSES pins also assert the refusal was reached and logged — existence pins as well as absence pins. Disclosed rather than quietly reconciled.

⇒ Enqueued. This closes the sql-driver.ts serial queue that has run three cards deep since #13015.

Generated by Claude Code

test(driver-sql): budget the #13056 live-MySQL orphan-shadow tests (#…

test(driver-sql): budget the objectstack-ai#13056 live-MySQL orphan-s…

Navigation Menu

Repository navigation

driver-sql: dropping an orphaned hash-shadow-carried index leaves its generated column behind forever, contradicting the cleanup contract isHashShadowColumn states #13056

The stated contract

The measurement

Why it is not merely cosmetic, and why it is also not urgent

Direction, not prescribing

Triage — `domain:engine` · `priority:p2` · `pm:queue`, and serial behind two cards

⚠️ Serial-queue position — this is now three deep on one file

Credit where the process worked

Premise re-measured on the MERGED tree — it holds, unchanged

The defect, restated so the fix targets it

The fork is yours — pick and argue

Discipline

PM review — ACCEPT → enqueued

⭐ The dev refuted the card's own preferred disposition, and it was right to

The legacy half answered with evidence, not preference

Spot-checks I ran myself

Also verified

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

driver-sql: dropping an orphaned hash-shadow-carried index leaves its generated column behind forever, contradicting the cleanup contract isHashShadowColumn states #13056

Description

The stated contract

The measurement

Why it is not merely cosmetic, and why it is also not urgent

Direction, not prescribing

Activity

os-zhuang commented on Aug 29, 2026

Triage — domain:engine · priority:p2 · pm:queue, and serial behind two cards

⚠️ Serial-queue position — this is now three deep on one file

Credit where the process worked

os-zhuang commented on Aug 29, 2026

Premise re-measured on the MERGED tree — it holds, unchanged

The defect, restated so the fix targets it

The fork is yours — pick and argue

Discipline

os-zhuang commented on Aug 29, 2026

os-zhuang commented on Aug 29, 2026

PM review — ACCEPT → enqueued

⭐ The dev refuted the card's own preferred disposition, and it was right to

The legacy half answered with evidence, not preference

Spot-checks I ran myself

Also verified

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Triage — `domain:engine` · `priority:p2` · `pm:queue`, and serial behind two cards