Skip to content

feat(lint): refuse a min/max roll-up whose answer cannot fit the summary column - #17012

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-16237-summary-rollup-refusal-lint
Sep 9, 2026
Merged

feat(lint): refuse a min/max roll-up whose answer cannot fit the summary column#17012
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-16237-summary-rollup-refusal-lint

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Fixes #16237

A min/max roll-up over a non-numeric child field is refused at authoring time, at error.

Clause-② (carried from the dispatch, verbatim)

Clause-②: no
  A new error-severity authoring-time lint rule that refuses a declaration combination
  accepted today. The accept set narrows; nothing widens. Written in the inline rule-id
  shape it adds no export, so no published surface moves. 拉回已声明契约 ⇒ 常规档.

The defect

Three shipped statements, each re-read on this branch's base:

  1. packages/spec/src/data/field-value.zod.ts puts summary in NUMERIC_VALUE_TYPES, and valueSchemaFor answers z.number().finite() for every member of that class. The value contract for a summary field is a finite number.
  2. packages/drivers/driver-sql/src/sql-driver.tscase 'summary': col = table.float(name); break; inside createColumn. (Located by the assignment. There is a second switch casing to summary inside varcharColumnChars, which returns null; that one is a decoy.)
  3. FieldSchema.summaryOperations admits min/max over ANY child field, and objectql's aggregateSummaryValue returns the driver's answer verbatim — only an empty-set fallback stands between the backend and the stored value.

So { type: 'summary', summaryOperations: { object: 'invoice_line', field: 'shipped_at', function: 'max' } } — an ordinary "latest shipment" roll-up — computes an instant into a column the value contract says holds a finite number. Nothing between author and driver correlated the two. Refused at authoring rather than tolerated in a consumer (Prime Directive #12).

The rule as landed

rollup/non-numeric-aggregand, severity error, in lintDataModel beside rollup/missing-summary. The rule id is an inline string literal, not an exported constant — the rollup/* shape, not the unique/* shape — so the diff adds zero exports.

  • Accepted: the numeric class and the boolean class.
  • Refused: the temporal class and everything else, for min/max only.
  • Silent where it cannot resolve: unknown child object, a field the child does not declare, or a field with no declared type.

Which spec set the predicate reads, and why it is the right one

The accept set is composed as NUMERIC_VALUE_TYPES union BOOLEAN_VALUE_TYPES, imported from @objectstack/spec/data.

Deliberately not read: NON_TEXT_STORED_VALUE_TYPES, whose membership is these same two classes today. It is defined by a third question — "is the stored value never text" (#14079) — and excludes the temporal class for a dialect reason, not for this one. Composing the union states why each half is in, so a future member of that set cannot widen this door as a side effect.

Why it is NOT isAggregateCompatibleWithFieldType

That table deliberately accepts min/max over the temporal class, because there the answer is returned to a caller and "return[s] a value of the field's OWN type" (#15768). Reusing it here would have accepted the card's own example and the rule would have been green because it never fires.

Two questions that look alike and are not: "can every backend give one answer" versus "does that answer fit the column this roll-up is stored into". This predicate is that table's min/max row narrowed by exactly the temporal class.

A discrimination test pins the disagreement in both directions: the analytics table accepts every temporal min/max pair this rule refuses, and the two agree on every other FieldType. It is the assertion that stops someone simplifying the predicate back into a single call later.

Population re-measurement, with a firing control

Measured on this branch (base fe2b7554a, merged to 2fd714f4f), over every tracked .ts / .json / .yml / .mdx file excluding dist, node_modules, CHANGELOGs, generated bundles and content/docs/references/:

TOTAL roll-up declarations: 72
by function: {"count":27,"sum":37,"avg":2,"max":2,"min":1,"(none)":3}

--- min/max sites ---
ok               max(task.estimate) types=[number]  packages/objectql/src/summary-backfill.test.ts:138
ok               min(task.estimate) types=[number]  packages/objectql/src/summary-backfill.test.ts:139
ok               max(task.estimate) types=[number]  packages/objectql/src/summary-rollup.test.ts:313

min/max total=3 ok=3 NON-NUMERIC=0 unresolved=0
CONTROL FIRED false

The zero carries a firing positive control — the card's own example, max(invoice_line.shipped_at) over a datetime, appended to the same scanner input:

NON-NUMERIC ***  max(invoice_line.shipped_at) types=[datetime]  control.ts:8
min/max total=4 ok=3 NON-NUMERIC=1 unresolved=0
CONTROL FIRED true

All three in-repo min/max roll-ups aggregate task.estimate, a number. On a population of 0 this adds a door, not a migration — no app in this tree reddens.

Verification

Every reading below was taken at 986236bff, the final commit.

Check Result
pnpm --filter @objectstack/lint test 0 — 103 files, 3598 tests passed
pnpm --filter @objectstack/lint typecheck 0tsc --noEmit + check:test-typecheck: OK
pnpm --filter @objectstack/lint build 0check-dts-emitted: 4/4
pnpm --filter @objectstack/cli exec vitest run --project unit test/data-model-rules.test.ts test/score.test.ts 0 — 62 tests passed (the consumer that owns lintDataModel's existing tests and the metadata-eval rubric)
pnpm --filter @objectstack/spec check:api-surface 0 — "public API surface + factory signatures unchanged"
pnpm --filter @objectstack/spec check:export-origins 0 — "5299 exports across 17 entry points resolve exactly as recorded"
pnpm lint (eslint . --no-inline-config, whole repo) 0, no output
pnpm check:nul-bytes 0 — 8413 text files, no raw control bytes
pnpm check:type-check-debt 0 — 5 ledger entries re-measured, 55 raw errors, none above its recorded number
pnpm check:type-check-coverage 0
node scripts/pm/dispatch-gates.mjs --commands derived set 56 families derived, 56 run; --ran reconciles 0 UNRUN
pnpm check:dual-build-cjs-loads NOT MEASURED (exit 3, PREREQUISITE NOT MET) — see below

api-surface / export-origins: zero movement, and no generated artifact under packages/spec/ is dirty. Fence 2 does not fire.

check:dual-build-cjs-loads is the one derived family this branch could not measure: it reads built output and nine packages unrelated to this diff (client-react, four connectors, two knowledge plugins, organizations, studio) have no dist in this worktree. Its own text says "This is NOT a pass: nothing was measured." It wants a whole-tree pnpm build, which is CI's run — declared here as a narrowing, not read as a clearance. This diff adds no package entry point and moves no exports, so it cannot move that gate.

The gate is not vacuous — ablation

The predicate was mutated to the analytics table's own min/max row (numeric union temporal union boolean), which is what "simplifying it back into isAggregateCompatibleWithFieldType" produces:

  • Mutation proved on disk before the run: anchor occurrences 1 to 0, marker 0 to 1, blob hash b419e1c1 to abaf144f.
  • Mutated run: 4 of 15 failed — the card's own example, the whole temporal class, the totality pin, and the discrimination case.
  • Restore leg: git checkout HEAD -- PATH, hash back to b419e1c1 (byte-identical to the HEAD blob), git diff HEAD empty, marker count 0.
  • Restored run: 15/15 passed.

The mutation script carried trap restore EXIT INT TERM throughout; no ablation artifact remains in the diff.

Changeset — both halves, measured on the deciding tree (986236bff)

Half 1 — is the changed PATH in packages/lint's own files[]? No.

files[] = ["dist","README.md","CHANGELOG.md"]; 17 published entries. src/data-model-rules.ts published: false. Discriminating negative controls, siblings the same mechanism does not ship: tsup.config.ts false, tsconfig.json false, src/index.ts false. Positive controls: README.md true, CHANGELOG.md true.

Half 2 — is the changed TEXT in what the package actually publishes? Yes.

Fourteen published dist/ entries scanned:

Probe Published entries containing it
SUBJECT — the rule id rollup/non-numeric-aggregand 2 (dist/index.cjs, dist/index.js)
SUBJECT — the refusal message text 2 (dist/index.cjs, dist/index.js)
POSITIVE CONTROL — the already-shipped sibling id rollup/missing-summary 2
NEGATIVE CONTROL — text of the new test file 0
NEGATIVE CONTROL — a TSDoc sentence from the edited source (comments stripped) 0

The rule ships in the package's dist, so it is a behaviour change for consumers of @objectstack/lint and of os lint: .changeset/rollup-non-numeric-aggregand.md, @objectstack/lint: minor. Not skip-changeset.

Governance

node scripts/pm/check-governed-merges.mjs --test .changeset/rollup-non-numeric-aggregand.md \
  packages/lint/src/data-model-rules.ts packages/lint/src/data-model-rules.summary-rollup.test.ts

governed-surface predicate: 0 of 3 path(s) hit the register (5 surfaces, repo-agnostic).
  ✅  NOT governed — ordinary queue landing applies to a PR with exactly this file list.
exit=0

Re-run on the final file list, not recalled.

验收备注

Findings for the dispatching seat to file. None is folded into this PR.

  1. AGGREGATE_FIELD_TYPE_COMPATIBILITY has no consumer at all. Its module TSDoc says the table "is the contract both consumer legs execute — the compile-time refusal in the dataset compiler (No layer refuses an incoherent aggregate / field-type pair — a dataset measure avg over a datetime works on SQLite and errors on Postgres #16099) and the authoring-time lint rule — so the two cannot drift into two accounts of one pair." Measured repo-wide at 986236bff, excluding dist: isAggregateCompatibleWithFieldType and AGGREGATE_FIELD_TYPE_COMPATIBILITY appear only in their own module, their own test, the api-surface snapshot and two changesets. Neither leg exists. Control: isIncoherentAggregate, cited in the same module's TSDoc, does have a production consumer (packages/lint/src/validate-functional-completeness.ts), so the grep is not blind. This is declared-not-enforced (Prime Directive chore: version packages #10's corollary) and a contract-text defect, corroborating this card rather than fixed by it. After this PR the table's only caller in the tree is this rule's discrimination test.

  2. summaryOperations.field's describe states the contract in one of its two spellings and enforces neither. In packages/spec/src/data/field.zod.ts it reads "Field on child object to aggregate (ignored for count)", while the AI blueprint's StrictSummaryOperations says "Numeric field on the CHILD to aggregate". packages/spec/src/data/field.zod.ts is inside [finding] packages/spec/liveness/field.json: two live displayField notes credit objectui with reading the snake_case spellings that objectui#7155 removed #15540's live claim, so this is reported, not written.

  3. sum / avg over a non-numeric child roll-up is unrefused too. This rule is scoped to min/max as dispatched. The aggregate table's own rows exclude those pairs, but per finding 1 nothing consults them, so the exclusion is not enforced anywhere. A different shape from this card, and it needs its own ruling rather than a widening here.

  4. Not measured, asserted nowhere: MySQL / Postgres insert behaviour when a non-numeric value reaches the float column. Neither engine was provisioned in this container. Nothing in the code, the tests or this body claims anything in either direction.

Not filed, noted only: the population scanner and the ablation script are one-shot instruments and were deliberately not left in the tree.

Generated by Claude Code


Generated by Claude Code

…ary column

`FieldSchema.summaryOperations` admits `min`/`max` over ANY child field, and
`aggregateSummaryValue` returns the driver's answer verbatim (only an empty-set
fallback). A `summary` field is a member of the spec's `NUMERIC_VALUE_TYPES`, so
`valueSchemaFor` answers `z.number().finite()` for it. An ordinary "latest
shipment" roll-up — `max` over a `datetime` child field — therefore computes an
instant into a column the value contract says holds a finite number, and nothing
between author and driver correlated the two.

Add `rollup/non-numeric-aggregand` (error) to `lintDataModel`, beside
`rollup/missing-summary`. Its predicate is the roll-up door's OWN: the numeric
class union the boolean class, read from `NUMERIC_VALUE_TYPES` and
`BOOLEAN_VALUE_TYPES` — the analytics table's min/max row narrowed by exactly
the temporal class, because that table answers "can every backend give one
answer" while this door answers "does that answer fit the column this roll-up is
stored into".

Silent on anything the pass cannot resolve (unknown child object, undeclared
field, missing type), per the aggregate table's own consumer tier.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x
The scope note claimed the analytics table 'already refuses' sum/avg over a
non-numeric child. Measured on this tree, AGGREGATE_FIELD_TYPE_COMPATIBILITY and
isAggregateCompatibleWithFieldType have no consumer at all outside their own
module and test, so the table's rows EXCLUDE those pairs but nothing enforces
that exclusion. Say what the tree does, and record the gap as a gap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x
@github-actions github-actions Bot added the size/m label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 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 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 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; 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 — 5 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 92949593df61413dcd79f339e609e8e8df6bc1a7packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 92949593df61413dcd79f339e609e8e8df6bc1a7

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

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 9, 2026
@zhuangjianguo
zhuangjianguo marked this pull request as ready for review September 9, 2026 02:51
@zhuangjianguo
zhuangjianguo added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit ba17017 Sep 9, 2026
35 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-16237-summary-rollup-refusal-lint branch September 9, 2026 03:17
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