Skip to content

fix(spec)!: refuse a padded grouping.fields[].field name at the producer instead of handing three renderers a lookup that always misses - #17498

Merged
os-bill merged 3 commits into
mainfrom
claude/issue-17360-grouping-field-non-padded
Sep 10, 2026
Merged

fix(spec)!: refuse a padded grouping.fields[].field name at the producer instead of handing three renderers a lookup that always misses#17498
os-bill merged 3 commits into
mainfrom
claude/issue-17360-grouping-field-non-padded

Conversation

@os-bill

@os-bill os-bill commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Fixes #17360

Clause-②: yes — fixed by the ruling, not by the diff's size. This is a spec narrowing, and the objectui#8285 precedent says a spec narrowing declares yes. needs:contract-review rides on both carriers (already on the card; hung on this PR too), the changeset is minor with a **BREAKING** note, and the ADR-0087 disposition is registered. It needs an at-tier verdict on the head that lands — ⛔ this PR is a draft and the at-tier verdict is the domain:spec seat's to commission, so nothing here flips ready, enqueues, or arms auto-merge.

Implemented by an os-dev subagent in session session_01MkQhmuuJAVDjmeWNixwDDH, inheriting the claim and assignee from the domain:spec execution seat's Claim: comment on #17360 (no second claim posted, assignee untouched).

The ruling

Ruling C on objectui#7347, maintainer verbatim 「其他同意」, decision batch #110 item 5: refuse at the producer. GroupingFieldSchema.field gets a non-padded pattern with a refusal naming the field and the offending spelling.

Not .trim(). A trimming schema makes ' a ' and 'a' silently equivalent, which is the consumer-tolerance direction AGENTS.md #0.1 refuses. objectui's harvester trim stays as defence-in-depth; nothing is removed there. The sibling groupByField axis is explicitly not this card and is untouched.

⭐ The "reuse the vocabulary if one exists" conditional — I measured it and took the FIRST branch, which is not what I inherited

The ruling asks for a non-padded pattern "the same shape the field-name vocabulary already uses elsewhere in packages/spec if one exists — reuse it, do not invent a second".

The seat's hold notes measured that FieldNameSchema was retired under protocol 18, and concluded the conditional resolves to its second branch (define the pattern). I re-verified that against origin/main and half of it is right, but the conclusion is not — and the correction comes from the retired-def's own text:

packages/spec/src/migrations/entries/retired-defs/18.shared__FieldName.ts
  "No schema in either repo ever composed `FieldNameSchema`, so the promised brand
   safety was unobtainable. The real field-name contract is the inline
   `z.string().regex(/^[a-z_][a-z0-9_]*$/)` at `data/field.zod.ts` — untouched by
   this retirement."

So the branded schema is retired (confirmed: the retired-def entry and the semantic entry 18.branded-identifier-schemas-retired are both on origin/main), but a field-name vocabulary is alive and ubiquitous: /^[a-z_][a-z0-9_]*$/, spelled inline at 30+ sites including data/field.zod.ts:932 — the field name itself. The conditional therefore resolves to its first branch, and no protocol-18 retirement has to be re-opened to read it. ⛔ Nothing here revives the retired branded-identifier family — there is no 回翻 in this diff and the card did not have to be returned.

And then I did not adopt that vocabulary — measured, with the evidence:

grouping.fields[].field is authored as a field reference, not a machine name, and the repo proves it: packages/lint/src/validate-list-view-field-refs.test.ts:537 carries grouping: { fields: [{ field: 'owner.name' }] }, a dotted relationship path, in a test that asserts it produces no finding. /^[a-z_][a-z0-9_]*$/ refuses owner.name. Adopting it would have reddened an in-tree grouping fixture — which the acceptance criteria name as a finding to report, not a reason to widen — and would have been a different, larger narrowing than the one ruled.

⇒ The pattern is non-padded only: /^(?:\S|\S[\s\S]*\S)?$/. Exactly what the ruling asked for, nothing wider, nothing narrower. Stated in the source docblock so the next reader does not have to re-derive it.

What changed

packages/spec/src/ui/view.zod.ts — anchored by content on export const GroupingFieldSchema (it happened to still be at :853, but nothing here depends on that):

  • a module-level GROUPING_FIELD_NON_PADDED_PATTERN and a checkGroupingFieldName(raw) refusal builder, following this file's own house idiom for a dynamic per-value message (checkSubmitRedirectUrl + .superRefine), which is what lets the message name the offending spelling as well as the key;
  • field gains the .superRefine, and its .describe() states the rule.

The refusal lands at grouping.fields[N].field — the offending element's own key — and reads, for ' business_unit ':

grouping.fields[].field names the field exactly as it is stored, with no leading or trailing whitespace — received " business_unit ". The group header query is compiled from this string verbatim … Write "business_unit". (ruled 2026-09-10.)

Deliberately left alone: the empty string still parses. A blank name is already refused loudly one layer down by compileListViewGroupQuery's grouping_field_blank (400, path ['grouping','fields',N,'field']); this narrowing exists for the silent case, and refusing the blank here too would be scope the ruling did not ask for.

Also: the ADR-0087 semantic entry 18.ui-list-view-grouping-field-padded-refused, registry.ts regenerated with gen:migration-registry (⛔ no hand edit — the insertion is at :10355, inside the os-generated semantic:18 markers at :5452/:10565), the changeset, and the regenerated content/docs/references/ui/view.mdx.

⭐ Acceptance — the fixture enumeration, which is the item that bites

Harvested with the TypeScript parser, not a grep — the tool's own predicate rather than a re-implementation:

pass population result
1 — literal grouping: { fields: [ … ] } 50 occurrences across 19 files 1 refused
2 — over-approximating: every shape-exact { field, order?, collapsed? } literal in packages/** 906 literals 1 refused

Lit controls: business_unit present in the harvest; priority present (it is the GroupingFieldSchema.parse fixture that pass 1 structurally could not see, which is why pass 2 exists). Dark control: zz_no_such_grouping_field absent.

The one refused spelling is ' ' at packages/spec/src/ui/view-grouping-query.test.ts:507. It is a negative fixture handed straight to compileListViewGroupQuery with no Zod parse anywhere on its path, pinning that same grouping_field_blank refusal — so it is not a fixture that has to parse, and the producer now refuses it one layer earlier for the same reason. Zero in-tree fixtures redden, and the pattern was not widened to fit anything.

Pins added in view.test.ts: the by-name refusal at grouping.fields.1.field (asserted through ListViewSchema so the path is the real one), the six whitespace shapes, a not-a-trim arm, the 20 harvested spellings as an it.each table, and a lit/dark control test that also carries the discriminator (' owner.name' is refused) — because the accepting arms alone would pass just as well against the old bare z.string().

Verification

Reverse verification (ablation). Predicted direction: red. Removed the .superRefine and rebuilt nothing (the test resolves ./view.zod relatively — no dist on the path). On-disk proof before and after: injected-text count 2 to 0, git diff --stat HEAD non-empty. Result: 9 pins red, exactly the ruling's ones (× refuses a padded grouping field name BY NAME at grouping.fields[N].field, the six whitespace shapes, the not-a-trim arm, the lit/dark discriminator); the 20 accepting arms stayed green, which is the expected asymmetry. Restore leg via git checkout HEAD -- PATH under a trap … EXIT INT TERM with an absolute path, proven byte-identical: worktree blob 848f63fa33f3e63da434e59bcc51abcdbdfc8718 equals the HEAD blob.

run result
@objectstack/spec full suite 498 files / 13724 passed
@objectstack/spec typecheck exit 0 (source + scripts + test layer)
@objectstack/lint · @objectstack/rest · @objectstack/platform-objects 3743 · 3146 (+1 skipped) · 561, all passed
repo-wide eslint . --no-inline-config exit 0 over 6569 files, 0 errors 0 warnings
derived gate families (dispatch-gates.mjs) 108 derived, 106 run green, 2 NOT MEASURED, 0 unrun

The downstream three are run because an accept-set narrowing changes the runtime face even though the exported byte shape does not; their first run was the MODULE_NOT_FOUND class (unbuilt @objectstack/formula) and was re-run after building the closures rather than recorded as red.

NOT MEASURED, called out separately from red — neither is a finding:

  • pnpm check:dual-build-cjs-loads — exit 3, PREREQUISITE NOT MET: 33 packages have no dist/. Needs a whole-monorepo build; declared to CI.
  • pnpm check:type-check-debt — exit 3, PREREQUISITE NOT MET: @objectstack/metadata-core unbuilt, and the gate refuses to measure a different world. Its coverage half (check:type-check-coverage) ran green.

check:react-declaration-parity, run exactly as dispatched: exit 0, manifest sdui.manifest.json at the repository root (66910 bytes) — not the packages/spec path a literal $PWD gives you, which the gate itself rejects with a loud "this gate did NOT run". ⚠️ The snapshot's freshness relative to .objectui-sha remains unmeasured (#17405), so this green is about parity against the committed manifest, not about the manifest being current.

Two residuals I am not able to close from here and am therefore declaring rather than papering over: the gate derivation ran on a tree ~9 commits behind origin/main, and one file it derives from (scripts/measure-reserved-identity-name-census.mjs) changed in that range, so CI's re-derivation on the true merge base is the authority on the family set; and the 45 artifact-roster families, the 11 wide-population families and the 6 path-scheduled CI jobs are outside the derived total by construction.

⚠️ One thing the seat should know: the declared file face has a co-tenant

The claim's batch-independence line reads "measured disjoint from … every open PR at claim time". Re-checked at write time: PR #17298 (feat(spec)!: retire the type: 'page' list-view mount, open, mergeable_state: dirty) touches all three of view.zod.ts, view.test.ts and registry.ts.

I did not widen my face and I did not reorder anything. The hunks are disjoint by a wide margin — #17298's earliest view.zod.ts hunk is at :1629 against my :853, its earliest view.test.ts hunk at :3754 against my ~:2050, and registry.ts is generated and regenerates — so this merges cleanly rather than needing the serial hold that #17447 got. Recording it because the claim's reading of it was different from mine, and that is the seat's to arbitrate, not mine.


Generated by Claude Code

…ducer

WIP checkpoint before the heavy verification run.

Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
Co-authored-by: Claude <noreply@anthropic.com>
…ducer

`GroupingFieldSchema.field` was a bare `z.string()`, so `'  business_unit  '`
was valid authored metadata. objectui's projection harvester trims the name for
`$select` while three renderers bucket rows by the raw name, so the server
answers under `business_unit`, every per-row lookup reads `undefined`, and the
view collapses into one `(empty)` group / `Uncategorized` lane holding every
record — a silent wrong answer that reads as a true statement about the data.

`field` now carries a non-padded pattern: the refusal lands at
`grouping.fields[N].field` and names the offending spelling verbatim.

Not a `.trim()` — a trimming schema makes `'  a  '` and `'a'` silently
equivalent, the consumer-tolerance direction AGENTS.md #0.1 refuses.

Deliberately not the snake_case machine-name grammar `/^[a-z_][a-z0-9_]*$/`:
a grouping level is authored as a field reference and a dotted relationship
path (`owner.name`) is an in-tree spelling of one.

Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
Co-authored-by: Claude <noreply@anthropic.com>
…name

Generated projection of the `grouping.fields[].field` describe() change.
`gen:schema` + `gen:docs`; no hand edit.

Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added documentation Improvements or additions to documentation protocol:ui tests tooling labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

5 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
  • 4 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 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 — 135 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 0ee32edef5909be073ed6476b7ae864640afc19fpackageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 0ee32edef5909be073ed6476b7ae864640afc19f

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

os-bill commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Seat verdict — clause-② contract review at tier. PASS with advisories, zero must-fix.

Head: db549a42c1cebf2a00eb3410e4d97427f4496d50, confirmed unchanged since review. Readings 2026-09-10T17:53–18:03Z.

Landing pre-checks

# check reading
at-tier verdict, clause ② fires PASS at contract-review tier, on this exact head
check-clause2-carriers --pair 17498 exit 0 — declaration readable, both carriers agree
check-governed-merges --test, final 6-path list exit 0 (not governed); lit control AGENTS.mdexit 3
checks on this head NOT YET — 33 of 34 green, Lint & Repo Gates still in_progress; combined status success

Not enqueued yet, and ④ is why. Lint & Repo Gates is precisely where check-changeset-no-major.mjs and check-adr-0087-registration.mjs actually run — the two gates the review could only answer by READING their sources. A gate job's conclusion is the thing that lets a PR through; an aggregate reading is not. This lands when that job reports completed: success.

What the review settled

  • The narrowing is exactly the one ruled. The shipped pattern is trim-equivalence: 24,000 generated cases, 0 divergences from s === s.trim(); lit control /^\S*$/ on the same harness shows 1152 divergences, so the harness can see a difference when one exists.
  • It is not a .trim(). An accepted value returns byte-identical; a padded one returns success:false rather than a rewritten value. The only .trim() in the diff is inside the message builder computing the remedy.
  • It did not adopt the machine-name grammar. owner.name parses under the shipped pattern and is refused by /^[a-z_][a-z0-9_]*$/ — that string is the one input on which the two candidate grammars disagree, and the shipped side accepts it.
  • Zero in-tree fixtures redden. One padded grouping literal exists (view-grouping-query.test.ts, { field: ' ' }) and it never reaches a Zod parse. Lit control: 4566 non-padded field: literals across 780 files, so the near-zero is a genuine absence.
  • minor is correct and major would be REFUSED. This is the one that resolves opposite to general semver instinct, and it is why the repo's own rule was the thing to read: during the launch window check-changeset-no-major.mjs refuses major outright, and the mandatory breaking-ness carriers are the BREAKING banner plus the ADR-0087 ledger disposition — both present. The registered (not already-registered) disposition is also the correct one for an id this diff adds.
  • registry.ts is a regeneration, not a hand edit. All five fields byte-identical to the entry file (reason 1750 chars), pure +49/-0 insertion, landing at the bisect-exact index in the sorted generated region.

⚠️ Advisory 1 — the PR body under-declares the #17298 co-tenancy by one file, and it is the one that matters

The body says #17298 touches "all three of view.zod.ts, view.test.ts and registry.ts". It touches four of this PR's paths. The fourth is content/docs/references/ui/view.mdx — the only overlapping path routed merge=os-regen (.gitattributes:153).

The body's disjointness argument is a line-distance argument. Its figures are correct (:1629 and :3754 both re-measured) and it is sound for the three code files. It buys nothing on the mdx path, because that driver does not text-merge — and this seat has measured it exiting 0 while silently dropping one side.

Registry adjacency, the one conflict condition .gitattributes documents, does not apply: #17298 adds zero semantic:18 entries.

This is a reason to run the survivor probe after landing, ⛔ not a reason to hold.

Advisory 2 — the accept set is trim-equivalence, including Unicode

NBSP (U+00A0), ideographic space (U+3000) and a BOM (U+FEFF) are all refused as padding. That is correct and arguably the point, but neither the changeset nor the .describe() names the class — both say only "leading or trailing whitespace". No code change requested.

Advisory 3 — one sentence in the body is imprecise in mechanism

The body says the machine-name grammar was rejected because it "would have reddened an in-tree grouping fixture". That fixture would not have reddened: validate-list-view-field-refs.ts never parses through Zod (zero code hits for safeParse / .parse( / GroupingFieldSchema; lit control validateListViewFieldRefs = 1), so it never reaches the schema.

The conclusion is still right and is independently justified — owner.name is a legitimate authored field reference and the ruling said "non-padded". The shipped ledger reason text makes the correct argument and does not repeat the error, so nothing shipped is wrong. ⚠️ Noting it because I made the same claim in my own comment on #17360 and it was imprecise there too.

Post-landing probe — validated as discriminating

Dry-run on today's origin/main (change absent) reads A=0, B=0, C=0 with all three lit controls lit (1, 2, 1). A post-landing reading of 2 / 2 / 1 proves the landing; any zero with its control lit proves a drop.

(A) git grep -c 'GROUPING_FIELD_NON_PADDED_PATTERN' origin/main -- packages/spec/src/ui/view.zod.ts        expect 2
    lit: git grep -c 'export const GroupingFieldSchema' origin/main -- packages/spec/src/ui/view.zod.ts    expect 1

(B) ⭐ THE OS-REGEN SURVIVOR CHECK
    git grep -c -F 'NO leading or trailing whitespace: the name is compiled into the aggregate query verbatim'         origin/main -- content/docs/references/ui/view.mdx                                                 expect 2
    0 = the driver dropped this side; 1 = it half-merged. Either is a silent regression no gate reports.
    lit: git grep -c -F 'Group sort order' origin/main -- content/docs/references/ui/view.mdx              expect 2

(C) git grep -c "id: 'ui-list-view-grouping-field-padded-refused'" origin/main -- packages/spec/src/migrations/registry.ts   expect 1
    lit: git grep -c "id: 'ui-mcp-connect-agent-unknown-keys-refused'" origin/main -- .../registry.ts      expect 1

Once ④ reports green: clear needs:contract-review on both carriers (this PR and card #17360) with the four-step write, flip ready, enqueue.


Generated by Claude Code

@os-bill
os-bill marked this pull request as ready for review September 10, 2026 18:27
@os-bill
os-bill enabled auto-merge September 10, 2026 18:28
@os-bill
os-bill disabled auto-merge September 10, 2026 18:28
@os-bill
os-bill added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit f8e5790 Sep 10, 2026
43 checks passed
@os-bill
os-bill deleted the claude/issue-17360-grouping-field-non-padded branch September 10, 2026 18:55
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 protocol:ui size/m tests tooling

Projects

None yet

2 participants