Skip to content

fix(driver-sql): render the hash-shadow NULL-safe duplicate report through formatDuplicateGroups - #16579

Merged
os-musk merged 4 commits into
mainfrom
claude/issue-16289-hash-shadow-null-safe-duplicate-groups
Sep 7, 2026
Merged

fix(driver-sql): render the hash-shadow NULL-safe duplicate report through formatDuplicateGroups#16579
os-musk merged 4 commits into
mainfrom
claude/issue-16289-hash-shadow-null-safe-duplicate-groups

Conversation

@os-musk

@os-musk os-musk commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16289

What

packages/drivers/driver-sql/src/sql-driver.ts — the hash-shadow arm's NULL-safe branch was the fourth site that renders duplicate groups and the only one still hand-rolling the shape formatDuplicateGroups (#14902) exists to own. It now calls the helper. One hunk, seven lines out, one line in.

The surrounding framing is byte-identical to the other two durability logs, mechanically:

$ grep -o -F 'report = ` Conflicting group(s): ${formatDuplicateGroups(duplicates)}.`;' \
    packages/drivers/driver-sql/src/sql-driver.ts | sort | uniq -c
      3 report = ` Conflicting group(s): ${formatDuplicateGroups(duplicates)}.`;

Three occurrences of one distinct string (:11941 this arm, :11994 and :12059 the plain-unique logs), plus the drift entry at :10844 which embeds the same helper output in a drift message. No wording was improved at any site, here or elsewhere.

The drift, re-derived on this branch's own head

probe reading
helper formatDuplicateGroups sql-driver.ts:4213, overflow tail at :4218…and ${n} more group(s)
its three existing callers :10844 (drift entry), :12000, :12065 (plain-unique logs)
the hand-rolled fourth site .slice(0, 5) at :11942, divergent tail at :11946…and ${n} more, no group(s)

All five anchors matched the line numbers recorded on the card, because this branch is cut from the same commit the triage measured (ce8caba9). They were re-derived by symbol, not trusted.

So two durability logs about the same failure class, emitted from the same catch, disagreed on how they say "there are more" — the exact drift the helper's own docblock names as the reason it exists.

Adopting the helper moves the tail and nothing else — the route note asked for this to be checked rather than assumed. The five-group half of the helper is character-for-character the expression that was inlined here (.slice(0, 5), then `(${g.key}) × ${g.rows} rows`, then .join('; ')), so ordering, truncation and per-group spelling are untouched; the probe that feeds both is the same probeNullSafeUniqueDuplicates call with the same arguments. The ablation below shows it from the other side: both legs render five (key) × 2 rows groups over the same fixture and differ only in what follows the last ;.

⛔ The existing pin's green is NOT coverage for this change

sql-driver-12998-shadow-null-safe-key.test.ts asserted the message at :203 only, and only as a prefix:

expect(diagnosis).toMatch(/Conflicting group\(s\):/);

Measured on the pre-change file, with a firing control so the zero is a real zero:

probe reading
expect( occurrences in that file 23 (the file does assert things — the control)
Conflicting group in that file 1 hit, :203 — the control fires
the word more anywhere in that file 0 hits, grep exit 1
more group(s) in any *.test.ts in the repo 0 hits before this PR

⇒ nothing in the tree could see the overflow tail, so adopting the helper would have silently repaired an unmeasured divergence and a green suite afterwards would have proved nothing about it. Extending coverage was therefore part of the work.

(One correction to the dispatch's reading, which said 24 expect( calls: measured 23, by both grep -c and grep -o | wc -l. The material half — only :203 touches the message, nothing touches the tail — is unchanged.)

The coverage, and why it lives in that file

New live cell in the same pin file, counts the sixth conflicting group in the shared "more group(s)" tail:

It belongs in this file and not in a cheaper unit test because the arm is only reachable on live MySQL: the hash-shadow route exists because MySQL refuses to key a TEXT column directly, and SQLite and Postgres never refuse. There is no non-live path to this catch.

Firing control: the test FAILS on the unmodified fourth site

Not "the suite is green afterwards". The fix was committed first, then the fourth site alone was reverted to its pre-change bytes, both legs run against the same live MySQL 8.0.46, and the tree restored from HEAD (never a bare git checkout --), with a trap on absolute paths.

On-disk proof, not the editor's exit code — the mutation reproduced main's blob exactly:

HEAD_BLOB=91337b2b75ebf3481f6553f1dda318db2b820575     # the fix
PRE   helper-call-lines=3  drift-tail=0
POST  helper-call-lines=2  drift-tail=1                 # anchored counts, both directions
MUT_BLOB=393a83f1a55b1e3c495fe50464314bd51f481f55       # == the pre-image blob in this PR's own diff header
LEG_A_EXIT=1                                            # RED on the pre-fix site
RESTORE_DIFF_EMPTY_EXIT=0                               # git diff HEAD -- THE_FILE is empty
REST_BLOB=91337b2b75ebf3481f6553f1dda318db2b820575      # == HEAD_BLOB
RESTORED helper-call-lines=3  drift-tail=0
LEG_B_EXIT=0                                            # GREEN with the fix

Leg A, the whole point — the drift printed by a real MySQL, and the four pre-existing tests passing straight through it:

 ✓ collides two NULL-organization rows under an org-scoped shadow unique
 ✓ leaves plain composite key parts un-coalesced
 ✓ diagnoses existing NULL-org duplicates instead of failing the boot unexplained
 ✓ names a NULL-organization duplicate in declared terms, never as a collision
 × counts the sixth conflicting group in the shared "more group(s)" tail
   → expected '[sql-driver] cannot create hash-shado…' to match /; …and 1 more group\(s\)\./

and the message it was matched against ended:

… (organization_id="__global__", v="eee…") × 2 rows; …and 1 more. The constraint 'COALESCE(organization_id, '__global__'), v' is NOT enforced until …

…and 1 more. — the divergence, live, in the shipped text. Leg B renders …and 1 more group(s). and the file is Tests 5 passed (5).

⭐ The four pre-existing tests passing on both legs is the measurement the card asked for, demonstrated rather than argued: this change is invisible to every test that existed, so the pin's green is not coverage for it.

Which tree the legs ran on, stated rather than implied: the ablation ran at fdf3defef, before the origin/main merge and before a later commit bound the fixture to a variable so tsc accepts it. The subject under test did not move across that range — git rev-parse fdf3defef:…/sql-driver.ts and a2cd9f58e:…/sql-driver.ts are the same blob 91337b2b7, and git diff --quiet between the two commits on that path exits 0. The test file's only delta is that binding (5 insertions, 1 deletion); every assertion line is unchanged.

A second, confirming run of the same two legs on the final head was attempted and not measured: os-verify-lock returned VERDICT queue-timeout (exit 99) · never acquired · waited 540s, the shared heavy-verify lock being held for 824s by a sibling's pnpm --filter @objectstack/spec test && pnpm --filter @objectstack/spec typecheck. Exit 99 is "never got a turn", not a result, and the tree was left unmutated — the legs above stand on the blob identity just stated, and are not re-reported as if they had run twice.

Verification

Live MySQL 8.0.46 was provisioned in the dispatch container from the distro archive — the same route sql-driver-15479-…'s header records — and pointed at CI's own URL (mysql://root:root@127.0.0.1:3306/conformance), so the live cells here ran against a real server rather than skipping.

run reading
pnpm --filter '@objectstack/driver-sql^...' build VERDICT command-exit 0 (dependency closure, first command in the worktree)
pnpm --filter @objectstack/driver-sql exec tsc --noEmit --listFiles exit 0, grep -c 'error TS' = 0, 577 files in the program
↳ does that program contain the new test? --listFiles hit for sql-driver-12998-shadow-null-safe-key.test.ts = 1 — measured, not assumed
pnpm --filter @objectstack/driver-sql test (no live URLs) exit 0Test Files 155 passed | 10 skipped (165), Tests 2389 passed | 141 skipped (2530)
live MySQL: 12998 (this pin) exit 0Tests 5 passed (5)
live MySQL: 14902 + 15479 + 11627 (the other three renderers and the #11627 control) exit 0Tests 23 passed | 1 skipped (24), including #15479's leaves the NULL-safe arm saying the NULL-safe thing

Every heavy run went through scripts/pm/os-verify-lock.sh and every verdict above is that script's VERDICT command-exit line or the runner's own exit written to disk before any pipe — never a bare $? after a | tail.

Repo-wide lint stays CI's; what is here is a proven narrowing. pnpm exec eslint --no-inline-config --format json over the two files this diff touches: exit 0, 2 files linted (count read from the JSON, not asserted), 0 errors, 0 warnings. The narrowing is a measurement rather than a skip because of the third piece: this repo runs one eslint.config.mjs which, quoting its own comment at :326, "never enables type-aware linting (no parserOptions.project, no typed @typescript-eslint rules) for ANY file, test or not" — measured there with a positive control. With no cross-file type information in play, this diff cannot move the verdict on a file it does not touch, so the linted population (the config's own **/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs} minus NEVER_LINTED) is unaffected outside those two files.

Gates

Derived from the tree, not from memory: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands on the final head a2cd9f58e, giving 57 runnable families over the change set it derives itself (3 paths, three-dot against merge base 90e7e6de1).

The branch carries a merge of origin/main 90e7e6de1, which cleared the tool's STALE TREE warning at the time. main moved again while the sweep ran (d1c86a745, a docs/ADR commit), so the final derivation prints the warning once more, naming one stale file: scripts/publish-smoke.sh. Stated rather than hidden — no path of this diff is in it, and CI derives on the merge result anyway.

Each printed command was run byte for byte, its exit code captured to disk before any pipe, and the record reconciled against the tool's own output:

node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --ran ran.list
✓ dispatch-gates --ran: 57 derived famil(ies) accounted for — 57 run, 0 NOT-MEASURED.

55 exit 0. Two exit 3, and 3 is this repo's PREREQUISITE NOT MET — read as NOT MEASURED, never as red and never as pass, in their own words: "⛔ This is NOT a pass and NOT a finding: nothing was measured."

  • pnpm check:dual-build-cjs-loads — "this gate reads built output, and some package has no dist/" (74 packages).
  • pnpm check:type-check-debt — "25 workspace dependenc(ies) of the ledgered packages have no built type entry point on disk".

Both want the whole workspace built (turbo run build --filter='./packages/*'), which is what lint.yml does before its own step; this container built only driver-sql's dependency closure, per the local-validation scope. Left to CI, recorded as not measured rather than folded into a green.

Outside that 57 the tool lists 39 artifact-roster families, whose silent verdict is a fact about a list rather than about these paths — explicitly not part of the runnable total. Two things were done with that block rather than reading it as clearance:

  • 4 of them keep their roster in a directory one of these paths is in, where the tool says the silence is evidence in neither direction. Those four were run: check-changeset-fixed (0), check:authz-resolver (0), check:error-code-casing (0), check:filter-alias-parity (0).
  • 9 of them run only the checker's own --self-test, each marked ⚠ checker-health only — NOT a PR verdict. Their green is not read as one here. check:partof-closing-keyword is one of those nine, so the Fixes #16289 line above is verified by CI's PR-context step and not by anything run locally.

Outside the local total by the tool's own declaration, and left to CI: 10 families whose declared population is too wide to place, 5 whose argv takes a value only a CI run supplies, and the always-runs tail.

Scope

Only the hash-shadow NULL-safe arm. The other three call sites were read and left untouched; no other duplicate-group or diagnostics code was widened, and no wording was improved anywhere. .changeset/hash-shadow-null-safe-duplicate-group-tail.md is a patch on @objectstack/driver-sql: the diff changes the text of a message that ships from a released package, so skip-changeset would be wrong (Post-Task Checklist §3).

Clause-② — re-derived from the actual diff: no. A durability log message is not a published contract surface. No exported symbol moves — formatDuplicateGroups stays module-local and unexported, and its docblock says why (SqlDriver's .d.ts carries protected members, so a method there would move a published entry point for a string helper). No accept set widens, no schema or spec key is touched. The diff is one call site plus its coverage.

验收备注


Generated by Claude Code

Claude and others added 4 commits September 7, 2026 09:42
…rough formatDuplicateGroups

The fourth duplicate-group site — the hash-shadow arm's NULL-safe branch — still
hand-rolled the `.slice(0, 5).join('; ')` shape the module-local helper was added
to own, and its overflow tail read `…and N more` where the helper's reads
`…and N more group(s)`. Two durability logs about the same failure class, emitted
from the same `catch`, disagreed on how they say "there are more".

The surrounding ` Conflicting group(s): ….` framing is byte-identical to the other
three call sites; only the tail moves.

The pin file asserted the `Conflicting group(s):` PREFIX only, so nothing in the
tree could see the tail. A sixth conflicting group is the smallest fixture that
renders it, and the new live cell asserts five groups shown plus the counted tail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg
…ate-group tail

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg
…cepts it

`initObjects`' parameter type does not declare `indexes`, so an inline object
literal carrying it is rejected by the excess-property check even though the
driver reads the key (filed separately). Every other fixture in this file binds
a variable first; the new one now does too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg
@github-actions github-actions Bot added size/s documentation Improvements or additions to documentation tests tooling labels Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 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 — 10 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 409da7807226f89f25c4b9e52477fb709925d26fpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 1a14f408363e420b5deac3d14c9b031e0c298599 — the merge of head a2cd9f58ee68ce7f73e31a25e2f3b253bf3634b9 into base 409da7807226f89f25c4b9e52477fb709925d26f, 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 1a14f408363e420b5deac3d14c9b031e0c298599 && git checkout 1a14f408363e420b5deac3d14c9b031e0c298599
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 409da7807226f89f25c4b9e52477fb709925d26f a2cd9f58ee68ce7f73e31a25e2f3b253bf3634b9 && git checkout -B drift-repro 409da7807226f89f25c4b9e52477fb709925d26f && git merge --no-ff a2cd9f58ee68ce7f73e31a25e2f3b253bf3634b9

node scripts/docs-audit/affected-docs.mjs --json 409da7807226f89f25c4b9e52477fb709925d26f

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

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/s tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

driver-sql: the hash-shadow NULL-safe arm still hand-rolls duplicate-group formatting, the drift formatDuplicateGroups exists to prevent

2 participants