fix(analytics): a min/max over a string-valued field is described as string, not number - #16238
Conversation
…ring` Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
📓 Docs Drift Check3 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not 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
Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 16d13e7adfdef16fa4603e58b5c19874d2152c8c && git checkout 16d13e7adfdef16fa4603e58b5c19874d2152c8c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 1ebde49536b431dc891b4bbe1a144b638c6205ae d09f74de149854419d0094fec28cdc9fea91cc54 && git checkout -B drift-repro 1ebde49536b431dc891b4bbe1a144b638c6205ae && git merge --no-ff d09f74de149854419d0094fec28cdc9fea91cc54
node scripts/docs-audit/affected-docs.mjs --json 1ebde49536b431dc891b4bbe1a144b638c6205ae |
PM verification — PASSMeasured at head Fence — heldLocal merge-base diff (⛔ not GitHub's list): 3 files — the rule, its test, one changeset. The executable change is four lines, and that mattersStripping the docblock out of the diff leaves almost nothing: One The membership is COMPOSED, not re-listed — checked at the sourceThis is the part that decides whether the fix rots. The set spreads three spec classes rather than copying their members, so a type moved between classes upstream moves here with it. Read at
All three match the docblock's table exactly. Each is declared A type ADDED to the enum fails loudly — verified, not taken on trustThe durability claim rests on the pin walking the vocabulary, so I read it rather than the prose.
|
Contract review (clause ②) — PASS · landing held on the pair declaration — PR #16238 at head
|
Fixes #16098
The sibling population of the temporal half.
minandmaxreturn a value of the aggregated field's own type, so aminover atext/select/lookup/autonumbercolumn carries a string — and every one of those columns was still described astype: "number", the same self-contradiction the temporal fix removed fordate/datetime/time.measureResultTypealready answeredundefinedfor "no correction", so this is rows added to one table, not a second mechanism: the rule keeps its single home,queryDataset's ADR-0021 result-column enrichment keeps applying it once downstream of all four producers, and no call site changes.The framing: the card's "uncertain" members were measured, not guessed
The card split the population into certain and uncertain and warned that a correction table covering the uncertain members "would be inventing answers". That is true only of a guess. Every member below was put in a bucket by reading the two shipped statements of what the type STORES — the spec's runtime value contract (
packages/spec/src/data/field-value.zod.ts, ADR-0104 D1) anddriver-sql's DDL column switch — plus, where those were open, the producer itself.Two of the three members the card flagged as uncertain turned out to have a measured answer:
autonumberis a string.renderAutonumberreturnsvalue: prefix + String(seq).padStart(width,'0') + suffix— a string by construction, zero-padded under the contract default format{0000}; the DDL answerscol = table.string(name); andRUNTIME_OWNED_FIELD_TYPESmakes the runtime the producer of that string. Measured directly on SQLite in this container,min/maxover a column of padded record numbers answers the padded TEXT ('0003'/'0012'),typeofstring. The spec's value contract answersz.unknown()here, but that is a statement about what a CALLER may write, not about what the producer emits — reading the openness as "unknowable" is what would have made this member a guess.summaryis numeric. Both shipped statements agree: it is a member of the spec'sNUMERIC_VALUE_TYPES(sovalueSchemaForanswersz.number().finite()) and the DDL answerscol = table.float(name). So thenumberit already carried is CORRECT, not merely unexamined — there is nothing to correct.One did not, and is left alone with the measurement stated:
boolean.Per-
FieldTypeverdict — all 49 membersstringandtimeareDimensionTypewords; everyundefinedrow keeps thenumberits producer minted. The threeundefinedbuckets are deliberately kept apart: "already correct", "no backend-independent answer exists" and "the answer is not on this input" are different findings, and collapsing them into one default branch is exactly how the uncertain members would get silently swallowed.textstringSTRING_VALUE_TYPES⇒z.string(); TEXT columntextareastringSTRING_VALUE_TYPES⇒z.string(); TEXT columnemailstringSTRING_VALUE_TYPES⇒z.string(); TEXT columnurlstringSTRING_VALUE_TYPES⇒z.string(); TEXT columnphonestringSTRING_VALUE_TYPES⇒z.string(); TEXT columnpasswordstringsecretstringsys_secretref, masked on read — a string either waymarkdownstringhtmlstringrichtextstringcodestringcolorstringsignaturestringqrcodestringselectstringSINGLE_OPTION_TYPES— one option code;optionCodesstringifies a numerically-spelled code before the value schema is builtradiostringselectlookupstringREFERENCE_VALUE_TYPES⇒ReferenceIdValueSchema(z.string()) — the id, never the expanded recordmaster_detailstringReferenceIdValueSchematreestringReferenceIdValueSchemauserstringsys_user; identical storageautonumberstringtable.string; measured on SQLite (see above)datetimeYYYY-MM-DD(landed earlier)datetimetimetimetimenumberNUMERIC_VALUE_TYPES; a numeric columncurrencypercentNUMERIC_VALUE_TYPES; a numeric columnratingNUMERIC_VALUE_TYPES; a numeric columnsliderNUMERIC_VALUE_TYPES; a numeric columnprogressNUMERIC_VALUE_TYPES; a numeric columnsummaryNUMERIC_VALUE_TYPESand DDLtable.float— both statements say numericbooleantogglemultiselectminoverjsonbon Postgres, serialized TEXT on SQLitecheckboxesmultiselecttagsmultiselectimagefileimageavatarimagevideoimageaudioimagecompositerepeaterrecordlocation{lat,lng}object in a JSON columnaddressvectorjsonz.unknown())formulaTwenty-one corrected, three already corrected by the temporal half, twenty-five left as they were.
What is left uncorrected, and why
boolean/toggle— measured, and the readings do not convergemin/maxoverbooleanat all:function min(boolean) does not exist, SQLSTATE 42883. There is no value.0/1as JS numbers — abooleancolumn has NUMERIC affinity, andformatOutput'sbooleanFieldsread-coercion pass is keyed to declaredField.booleanCOLUMNS, which an aggregate alias is not.SqlDriver.aggregate()seam in the earlier driver-level measurement (driver-sql (PG): sum/avg/min/max over a boolean column throw the raw PostgreSQL 42883 with no ADR-0112 envelope (status undefined) #11455) readsfalse/true, i.e. JS booleans.So the three readings disagree about whether a value exists at all, and about what kind the one answering backend reports.
DimensionTypedoes carry abooleanword, so the correction is spellable here — which is precisely why it is not made: spelling it would ship one of three disagreeing readings as a published declaration. The column keeps thenumberit has, which is also the accurate word for the raw SQLite value. The missing refusal is owned by #16099 (needs-user-decision: no layer refuses an incoherent aggregate / field-type pair), and the boundary noted honestly: the driver-seam reading is cited from #11455, not re-measured here.The JSON-column classes, and the file types
MULTI_OPTION_TYPESandSTRUCTURED_JSON_TYPESareJSON_COLUMN_TYPESindriver-sql, so the aggregate runs over a JSON column: nominoverjsonbon Postgres, serialized TEXT on SQLite. Same shape as the boolean case, same owner in #16099.FILE_REFERENCE_TYPESis left alone for a second, additive reason: its stored form is mid-migration under ADR-0104 D3. The value contract's stored schema is already the opaquesys_fileid (FileReferenceIdValueSchema, a string) while the DDL still gives these types a JSON column for the pre-D3 inline metadata object. Two shipped statements, two different stored forms; correcting to either would describe half the deployments.formula— the answer exists, but not on this rule's inputFieldSchema.returnType(number/text/boolean/date) declares it, and its own JSDoc names "dataset measures" as the intended consumer. This rule cannot read it: its input is the declaredFieldTypealone, because that is allAnalyticsServiceConfig.sourceFieldMetareturns ({ type?, defaultCurrency?, max? }).returnTypeis also optional — "absent when the type can't be proven" — so even with the plumbing the rule would answer for some formula fields and not others. Carrying it changes the host callback contract and its call site; filed as #16236 rather than guessed at.What the rule deliberately cannot see:
multiplesourceFieldMetareturns nomultipleflag, so aselect/radio/lookup/userdeclaredmultiple: true— stored as a JSON array — is indistinguishable here from its single-valued form and is corrected tostringwith the rest of its class. That is the safe direction rather than an oversight: where the backend answers at all it is SQLite comparing the serialized TEXT, which is a string; where it does not answer (Postgres overjsonb) there is no response for any word to mis-describe. Stated in the module header rather than left implicit.Why
string, and not a sixth wordfields[].typeis not aFieldTypeposition — it speaksDimensionType(string/number/boolean/time/geo). Both words this rule mints are already carried by DIMENSION columns in the very same response:dataset-compiler.dimensionTypemaps adatedimension to'time'and alookupdimension to'string', and bothbuildFieldMetas copy that through. Spelling a textual measure'text'would have added a sixth word to a five-word vocabulary and left every existing consumer branch unreached — the same argument that chosetimeoverdatetime. Section D pins both halves of it.Membership is composed, not re-listed
STRING_SOURCE_FIELD_TYPESis built from@objectstack/spec's own value classes —STRING_VALUE_TYPES,SINGLE_OPTION_TYPES,REFERENCE_VALUE_TYPES— plusautonumberas the one local extra, the same constructiondriver-sqluses for itsJSON_COLUMN_TYPES. What the platform says a field type STORES and what this rule says aminover it RETURNS therefore cannot drift.The enum-walking pin, and its population
src/__tests__/measure-result-type.test.tssection A walks both closed vocabularies:AggregationFunction— 6 members, each with a row saying whether this rule speaks about it. The pre-existing guard, kept.FieldType— 49 members, each with a row carrying its bucket and the measurement behind it, asserted equal toFieldType.optionsin both directions. A field type added to the spec fails that assertion instead of silently inheriting the flatnumber; a field type REMOVED fails it too.Per member the pin asserts
minandmaxanswer the bucket's verdict, and that the other four aggregates never correct anything — so the table is walked 49 × 6 rather than sampled. A further assertion requires all five buckets to be non-empty, so the honest split cannot be quietly collapsed into one default branch. Section E drives the string family end to end throughqueryDataset(the primary producer, the supplementary-sub-query producer and the__compareproducer), and asserts the four uncorrected representatives —boolean,json,formula,summary— still saynumber, each for its own recorded reason.Total: 121 tests in this file, up from 27.
Verification
All measurements taken at
d09f74de1, the head of this branch. Every exit code captured immediately after a single redirected command, never through a pipe.pnpm --filter @objectstack/service-analytics exec vitest run --maxWorkers=2 src/__tests__/measure-result-type.test.tsTest Files 1 passed (1),Tests 121 passed (121)pnpm --filter @objectstack/service-analytics testTest Files 94 passed (94),Tests 2134 passed (2134)pnpm --filter @objectstack/service-analytics typechecktsc --noEmit)--commandsBoth heavy runs went through
scripts/pm/os-verify-lock.sh; the results quoted are itsVERDICT command-exitlines, not a bare$?.The typecheck really covers the edited files.
tsc --noEmit --listFilesnames both of them —measure-result-type.tsand__tests__/measure-result-type.test.ts— so "typecheck clean" is a statement about this diff and not a true sentence that says nothing about it.Gate family. Derived mechanically with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackon this final head; its Reconciliation line reads55 famil(ies)(44 by path + 6 by kind + 7 declared whole-tree, 2 reached both ways), and the list run is--commands, which prints exactly those 55. The Artifact-rosters block (39 families) is excluded from that total by the tool and was not counted as part of it. Included and green:check:nul-bytes,check:cross-package-test-inputs,check:engine-double-contract,check:type-check-coverage,check:type-check-debt,check:published-files,check:changeset-gate-self-tests,check:empty-changeset,check:changeset-no-major,check:closing-keyword-parity.One family needed a second run to be measured at all:
pnpm check:type-check-debtfirst exited 3 — its--re-measureOOM'd. That isPREREQUISITE NOT MET, which the gate's own output says is "NOT a pass and NOT a finding: nothing was measured". Cause is mine, not the gate's: the outerNODE_OPTIONS=--max-old-space-size=4096was BELOW the ceiling the gate pins for its inner tsc, which its own output states astsc runs under --max-old-space-size=6144 MB. Re-run at8192it exits 0:12 ledger entr(ies) re-measured in 127.3s, 140 raw tsc error(s) total, none above its recorded number,surplus: none.Lint is a declared narrowing, not a skipped step.
eslintwas run over the files this diff touches rather than over the repo, and the narrowing is measured on all three counts: (1) the receiving population comes from eslint's own configuration, not from my judgement — asked about the third file in the diff, the changeset.md, it answersFile ignored because no matching configuration was supplied, so its population over this diff is exactly the two.tsfiles; (2) the count is read from--format json— 2 entries, 0 errors, 0 warnings, exit 0; (3) the invariance for untouched files is a property of the config, whicheslint.config.mjsstates of itself: this repo runs one config that "never enables type-aware linting (noparserOptions.project, no typed@typescript-eslintrules) for ANY file, test or not" — so no edit in this diff can move a verdict on a file it does not touch. The repo-wide run remains CI's.Reverse verification — direction predicted before running
Predicted, and written down first: deleting only the string branch from
measureResultTypemust turn RED every assertion expecting'string'— the 21string-bucket rows in section A, the section C producer control, the section D lookup-dimension control, and all four section E end-to-end cases, 27 in total — while leaving the temporal half (section B) and everynumbercontrol (section D) GREEN. Ordinary direction: the change corrects a value on existing entries, mints no column and removes no limb, so nothing downstream can gain or lose a finding.Measured: 27 failed | 94 passed (121), and the failing set is exactly the predicted one, one row per string-bucket member by name plus the four end-to-end cases and the two controls. The temporal assertions stayed green throughout, which is what shows the new rows are load-bearing on their own rather than riding on the landed half.
Ablation discipline, in full:
202f797d8before the mutation, soHEADgenuinely held it.1 → 0, injected marker count1, and the blob hash moved806b5a36… → 6be5d73b…. The run would have aborted on any of the three.../measure-result-type.js), not across a packageexportsboundary, so nodistsits between the mutation and the test. The ablation itself is the proof — mutatingsrc/alone moved the result.trap … EXIT INT TERMwith an absolute path, spelledgit checkout HEAD -- "$REPO_ROOT/RELPATH"so it cannot read the mutation back out of the index.HEADblob (806b5a36…both sides),git diff HEADis empty andgit status --porcelainis clean.The call site in
analytics-service.tswas deliberately NOT the ablation subject: it is held by another open PR, so this branch does not touch it even transiently. The claim that ablation would otherwise have tested — that the correction is applied downstream of all four producers — is pinned instead by section C, which drives one selection down bothbuildFieldMetaproducers and requires their column metadata to be equal.Scope
measure-result-type.tsand its test only; the fenced files (analytics-service.ts,preview-evaluator.ts,text-match-sql.ts/like-pattern.ts) are untouched — widening the table needed no call-site change, which is what "rows in one table, not a second mechanism" buys.Changeset:
minoron@objectstack/service-analytics, matching the temporal half's grade for the same reason — a published wire surface reports a different value for existing queries, which is behaviour a consumer branches on. No spec change here, so no@objectstack/specbump.Filed while working this card, not addressed here: #16236 (
sourceFieldMetacannot carryformula.returnType) and #16237 (asummaryroll-up ofmin/maxover a non-numeric child field writes into a column both statements declare numeric). Both unassigned, both out of scope: #16236 needs the fenced call site, #16237 lives inpackages/spec/packages/objectql.🤖 Generated with Claude Code
Generated by Claude Code