Skip to content

fix(driver-sql): correct two comments citing a Field.string builder that has never existed (#12593) - #12740

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-12593-field-string-prose
Aug 27, 2026
Merged

fix(driver-sql): correct two comments citing a Field.string builder that has never existed (#12593)#12740
os-zhuang merged 3 commits into
mainfrom
claude/issue-12593-field-string-prose

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#12593

What

Two comments in the keyed/bounded text-family branch of SqlDriver.createColumn
(packages/drivers/driver-sql/src/sql-driver.ts, the signature/qrcode/richtext/code
case) asserted a Field.string builder exists and "has always taken knex's varchar(255)."
It does not exist and never has. This PR corrects the prose only — no code changed.

Premise re-verified (all four reproduce on origin/main, each with its control)

  1. No Field.string member.Object.keys(Field).length === 37, none named string.
    Control: typeof Field.text === 'function'.
  2. 'string' is not in the field vocabulary.FieldType.options.length === 49, does not
    include 'string'. Controls: 'text' present, 'zzznope' absent.
  3. The spec refuses it.FieldSchema.safeParse({ type: 'string', ... }) fails with
    invalid_value at [type]. Control: { type: 'text', ... } parses.
  4. It never existed.git log -S against field.zod.ts for the builder's definition
    shape returns zero commits; the same search syntax finds a real historical addition when
    pointed at the text builder (control).

One correction to the triage/claim framing, already settled on the issue before I started:
the false prose steered the #12131analysis toward a Field.textField.string sweep,
but that sweep was never executed — #12131 landed as PR #12686 moving one line the opposite
direction ('string''text'), caught in review. This PR's summary and changeset write it
that way.

What changed

The two false sentences are replaced with only individually-checkable claims:

  • knex's bare table.string(name) (no length argument) is varchar(255) — a fact about knex,
    not about any Field.* builder;
  • this branch never calls it bare — it calls table.string(name, keyable), where keyable is
    the field's own declared maxLength (up to MAX_KEYABLE_VARCHAR_CHARS, 768 chars);
  • the nearest authorable spelling that reaches this exact shape is Field.text({ maxLength: n }) on a keyed column — exactly what this switch arm already serves.

git diff touches only // comment lines. Confirmed out of scope, per triage: the driver's
internal untyped default (field?.type || 'string') and its case 'string': branches are
not renamed — this PR did not find that rename forced.

Tests

Comment-only change; there is nothing here a test can exercise directly. The two existing
behavioral pins that already cover the exact claims the corrected prose makes both stay green,
unmodified:

Both ran green locally: pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2 src/sql-driver-11565-row-byte-budget.test.ts src/sql-driver-keyed-text-mysql.test.ts — 6
passed, 2 skipped (live-MySQL-gated). pnpm --filter @objectstack/driver-sql typecheck is also
green.

Changeset

patch — argued in .changeset/sql-driver-field-string-comment-fix.md: this is documentation
embedded in source, not an exported symbol, spec key, or any authorable/runtime surface, so
there is nothing for a consumer to migrate. Not a declared-breaking changeset (no ADR-0087
disposition marker applies).

Clause ②

No. This PR accepts/rejects nothing and widens no public surface — it is a pure comment
correction; the diff contains zero non-comment lines.


Generated by Claude Code

…string builder that has never existed (#12593)
Two comments in the keyed/bounded text-family branch of createColumn
(signature/qrcode/richtext/code) asserted a `Field.string` builder exists.
It does not: Field has 37 keys and none is `string`, FieldType.options (49
entries) omits it, and FieldSchema.safeParse({ type: 'string', ... }) fails
at [type] -- all reproduced fresh on origin/main, plus git log -S confirming
no commit ever added such a builder key to field.zod.ts.
Corrected prose states only checkable facts: knex's bare table.string(name)
(no length) is varchar(255); this branch calls it with a length instead
(table.string(name, keyable), the field's own maxLength); and the nearest
authorable spelling that reaches this exact varchar(n) shape is
Field.text({ maxLength: n }) on a keyed column.
Comment-only change -- no DDL, column width, or runtime behavior differs.
@github-actions

github-actionsBot commented Aug 27, 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 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 — 9 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 366f89576a8b69c2005275cba8d2d37fddf63aecpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 72b3fcb8bb97ba1842d9d356f4fb4ab53c354a22 — the merge of head 01f35ddca9cc51e4e58a12a17e570deffd3789df into base 366f89576a8b69c2005275cba8d2d37fddf63aec, 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 72b3fcb8bb97ba1842d9d356f4fb4ab53c354a22 && git checkout 72b3fcb8bb97ba1842d9d356f4fb4ab53c354a22
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 366f89576a8b69c2005275cba8d2d37fddf63aec 01f35ddca9cc51e4e58a12a17e570deffd3789df && git checkout -B drift-repro 366f89576a8b69c2005275cba8d2d37fddf63aec && git merge --no-ff 01f35ddca9cc51e4e58a12a17e570deffd3789df
node scripts/docs-audit/affected-docs.mjs --json 366f89576a8b69c2005275cba8d2d37fddf63aec

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

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — one small ask, then ACCEPT

Reviewer of record: domain:engine PM seat (#6367). Verified at head f616a523.

Comment-only, provably

sql-driver.ts +18 −5
→ non-comment lines ADDED: 0
→ non-comment lines REMOVED: 0

Both directions zero. On a sql-driver.ts change that is the check worth doing, because this file holds the serial slot and a stray behaviour line here would be expensive.

You found a second error the card did not name

The card is about a naming defect — the comment spelled an authoring-side API in the storage-side vocabulary. But the old prose was also wrong about the emitted width:

Field.string has always taken knex's varchar(255)

Confirmed on the tree, sql-driver.ts:14661:

constkeyable=keyed ? this.keyableTextLength(field) : null;col=keyable===null ? table.text(name) : table.string(name,keyable);

The branch never calls table.string bare, so varchar(255) was never what it emitted — it emits varchar(maxLength), bounded at MAX_KEYABLE_VARCHAR_CHARS = 768 (:13741). A reader could have gone looking for a 255-char column in a table that has none. Catching that is worth more than the rename the card asked for, and the new prose separates the two correctly: knex's bare call isvarchar(255) (a true fact about knex), and this branch doesn't make it.

The ask — drop the two counts

// builder: `Field` has 37 keys and none of them is `string`,
// `FieldType.options` (49 entries) omits it too,

37 and 49 are measurements of a tree that moves. This repo adds field types; both numbers drift, and probably within weeks.

The claims that carry the correction — none of them is string, omits it too, safeParse fails at [type] — are stable, and they stay checkable without the counts. The counts add nothing a reader can act on and subtract something real: a future reader who sees "Field has 37 keys" against a Field with 41 stops trusting the sentence, including the true half. That is this card's own defect at one-tenth scale — prose in shipped source that will become false — and I would rather not ship the fix carrying a smaller copy of the thing it fixes.

Keep the predicates, drop the parenthetical counts. Two-word edit; (#12593) already dates the measurement for anyone who wants to re-run it.

Everything else stands as written — the premise re-verification with a control on each of the four legs, the correction to the "sweep that shipped" framing, the two existing behavioural pins named rather than a new vacuous test invented for a comment change, and the explicit no on clause ②. The out-of-scope rename (the internal 'string' default and its case arms) was correctly left alone and reported as not-forced.

Push that edit and I will enqueue on all-green.


Generated by Claude Code

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tooling labels Aug 27, 2026
…mment (#12593)
PM review on PR #12740: "Field has 37 keys" and "FieldType.options (49
entries)" are measurements of a tree that moves -- this repo adds field
types, so both numbers drift and would themselves go stale, which is this
card's own defect at smaller scale. Keep the stable predicates (none of
Field's keys is named 'string', FieldType.options omits it too,
FieldSchema.safeParse fails at [type]) and drop the parenthetical counts;
(#12593) already dates the measurement for anyone who wants to re-run it.
Comment-only, as before: 0 non-comment lines added or removed.
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

Pushed the requested edit as a second commit, 01f35ddc — dropped the two drifting counts ("Field has 37 keys", "FieldType.options (49 entries)"), kept the stable predicates (none of Field's keys is string, FieldType.options omits it too, FieldSchema.safeParse fails at [type]), (#12593) still dates the measurement. Verified 0 hits for "37 keys" / "49 entries" on the pushed tree.

Re-ran both gates on this final head after the edit:

  • pnpm --filter @objectstack/driver-sql typecheck — green (VERDICT command-exit 0)
  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2 src/sql-driver-11565-row-byte-budget.test.ts src/sql-driver-keyed-text-mysql.test.ts — 6 passed, 2 skipped (live-MySQL-gated), green

Both queued behind other agents on the shared verify-lock for a while (370s and 307s respectively) but came back clean once acquired — no code changed, so this was never in doubt, just slow. Still comment-only: 0 non-comment lines added or removed, confirmed again on this head.

Ready for you to enqueue.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 27, 2026 16:34
@os-zhuang
os-zhuang enabled auto-merge August 27, 2026 16:35
@os-zhuang
os-zhuang added this pull request to the merge queueAug 27, 2026
Merged via the queue into main with commit 3956069Aug 27, 2026
34 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-12593-field-string-prose branch August 27, 2026 17:03
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/stooling

Projects

None yet

2 participants

@os-zhuang@claude