Repository navigation
Commit 329ea2e
Fixes #20139
Clause-②: no (narrowing)
## What
Five published REST doors in `packages/rest/src/rest-server.ts` still
read a numeric query parameter with a bare `Number()`, and each answered
`200` on a value it could not read. Every one now reads through
`readDeclaredQueryNumber`, the private reader PR #20137 added. The
reading is against the parameter's own declaration where one exists, and
as a whole number otherwise. A census test holds the family closed.
| door | parameter | now read through |
|:--|:--|:--|
| `GET /meta/:type/:name/history` | `sinceSeq` |
`HistoryMetaItemRequestSchema.shape.sinceSeq` (`z.number().optional()`)
|
| `GET /meta/:type/:name/audit` | `limit` |
`AuditMetaItemRequestSchema.shape.limit` (`z.number().optional()`) |
| `GET /meta/:type/:name/diff` | `from` / `to` (and `fromVersion` /
`toVersion`) | a whole number: no declared request schema |
| `GET /search` | `perObject` | a whole number |
| `GET /approvals/requests` | `limit` / `offset` | a whole number |
The refusal is `400` with the data surface's existing
`VALIDATION_FAILED` + `fields[]` envelope. `fields[0].field` names the
parameter as the caller spelled it, `fields[0].code` is `invalid_type`,
and the service is never called. Nothing a conforming caller sends
changes its answer. There is no new error code and no new export, and
`packages/spec` is untouched.
Mechanics:
- **Existing catches.** History, audit and diff reach `handleRouteError`
through their existing catch, and search reaches `mapDataError`.
- **Approvals.** Its catch answers every throw with `500
APPROVAL_REQUEST_LIST_FAILED`, so the two reads sit in a scoped `try`
whose catch hands the refusal to `handleRouteError`. The refusal is the
caller's `400`, not the route's `500`.
- **Dead drops removed.** The `Number.isFinite` drops that the reads
made dead are gone: history `sinceSeq`, audit `limit`, and the approvals
`limit` / `offset`.
- **Renamed constant.** The module-private `UNDECLARED_ROW_COUNT_PARAM`
is now `UNDECLARED_WHOLE_NUMBER_PARAM`. The schema is the same
(`z.number().int().optional()`), but it now also reads a row offset and
a history version, which "row count" misnamed. No `export` line is
added, removed or changed anywhere in the diff.
- **Reader docblock.** The refusal paragraph now names the approvals
door's scoped catch, and a new section points at the census. The
function body is unchanged.
- **Rider.** The publish route comment `404 [no_draft]` now reads ``404
`NO_DRAFT` when nothing to publish``, in the form commit `1c8b320a`
used.
## Before and after, measured on the real `RestServer` routes
"Before" is what the spy service received on `a78f731ad`. Every row
answered `200` there.
| door | request | before | now |
|:--|:--|:--|:--|
| history | `?sinceSeq=abc`, `?sinceSeq=Infinity` | `sinceSeq` dropped:
the log read from the start | `400`, `sinceSeq` |
| history | `?sinceSeq=` (empty or blank) | `sinceSeq: 0` forwarded, a
cursor nobody sent | `400` |
| audit | `?limit=abc`, `?limit=Infinity` | `limit` dropped: the
producer default of 100 | `400`, `limit` |
| audit | `?limit=` (empty or blank) | `limit: 0`, which the
implementation clamps to one event | `400` |
| diff | `?from=abc&to=3` | only `toVersion: 3` sent: "the version
before 3" was diffed | `400`, `from` |
| diff | `?to=abc` | no bound sent: the current body was diffed | `400`,
`to` |
| diff | `?from=1.5`, `?from=%20` | `fromVersion: 1.5` / `0` forwarded |
`400` |
| diff | `?fromVersion=abc`, `?toVersion=abc` | dropped | `400`, naming
the spelling used |
| search | `?perObject=abc` | `perObject: NaN` handed to `searchAll`: no
per-object cap | `400`, `perObject` |
| search | `?perObject=1.5`, `Infinity`, blank | 1.5 / clamped to 25 /
`0` | `400` |
| approvals | `?limit=abc`, `?limit=Infinity` | `limit` dropped: the
unpaged 500-row list, no `total` | `400`, `limit` |
| approvals | `?limit=` (empty or blank) | `limit: 0` + `total`: a
one-row page | `400` |
| approvals | `?limit=10&offset=abc` | `offset` dropped: page 1 | `400`,
`offset` |
| approvals | `?offset=` (empty) | `offset: 0`: the service's 50-row
paged mode | `400` |
| approvals | `?limit=1.5`, `?offset=1.5` | 1.5 handed to the engine |
`400` |
**Unchanged, pinned by lit controls:**
- **Absent** means what it meant: no `sinceSeq` member, no audit `limit`
member, no diff version members, `perObject` undefined, and both
approvals members undefined with no `total`.
- **Conforming values** reach the service as before, including the
ranges no card here takes a position on:
- history `sinceSeq` `5` / `0` / `1.5`;
- audit `limit` `25` / `0` / `1.5` / `900`;
- diff `from=2&to=3`, `fromVersion=1&toVersion=4`, and `from=0`;
- search `perObject` `5` / `0` / `50`;
- approvals `limit=50&offset=0`, `limit=25&offset=50`, `limit=0` and
`offset=-1`.
- **Precedence.** `from` still wins over `fromVersion`, as the old `??`
had it.
**The empty string, per door.** It is decided by the reader's own rule:
from what the door answered for `?x=` before.
- **Stays absent** where that already was the absent answer: `/diff`'s
`parseV('')` and search's falsy guard.
- **Is refused** where `Number('')` invented a `0` that changed the
answer:
- history's `sinceSeq: 0`, which the SQL repository applies by skipping
rows at or below `event_seq` 0;
- audit's one-event clamp;
- approvals' one-row page, or the 50-row paged mode that absent does not
trigger.
## The census
`packages/rest/src/rest-server-query-number-census.test.ts` parses
`rest-server.ts` with the TypeScript AST and finds every numeric
coercion:
- `Number` / `parseInt` / `parseFloat` called or `new`-ed, bare or as
`Number.parseInt` / `Number.parseFloat`;
- one of them passed or assigned as a value (`.map(Number)`);
- unary `+`.
Every site must be in the test's ledger under one of three dispositions:
- **`reader`:** the one `Number(raw)` inside `readDeclaredQueryNumber`;
- **`not-a-query-value`:** the reason names the value's source;
- **`exempt`:** the reason says why a refusal would be wrong, and a test
pins it.
An unclassified site fails with its line, its key and the two ways to
fix it. A ledger row whose site has gone fails too.
It counts every coercion, not "the ones that look like query reads", and
the reason is measured: the census found a member the card does not
list. `GET /meta/:type/:name/diff` read `from` / `to` with `Number(raw)`
inside a local `parseV` helper, one frame away from `req.query`, and a
pattern such as `Number(req.query.` or `Number(q.` does not see through
that frame. The cost is one ledger row per non-query coercion added to
this file. There are 10 coercion sites in about 13,700 lines today.
A key is `anchor » coercion text`. The anchor is the route (from the
registration's `method` + `path`), or else the enclosing named function;
line numbers are not used. An edit elsewhere in the file never moves a
key. Editing a ledgered coercion does move its key, on purpose.
Census result on this branch: 10 sites, all classified.
| site | disposition |
|:--|:--|
| `function readDeclaredQueryNumber » Number(raw)` | reader |
| `function importJobToProgress » Number(row?.… ?? 0)` (six counters) |
not-a-query-value: persisted `sys_import_job` counters |
| `GET ${basePath}/forms/:slug/lookup/:field »
Number(picker.maxResults)` | not-a-query-value: stored form metadata
`publicPicker.maxResults`, declared `z.number().int().min(1).max(50)` |
| `POST ${metaPath}/:type/:name/rollback » Number(toVersionRaw)` |
exempt |
| `GET ${dataPath}/:object/export » Number(q.page)` | exempt |
Scope notes:
- The file has no `parseInt`, `parseFloat` or unary `+` today.
- **For #20320's claimant:** no numeric query read sits in the `GET
/meta/:type` list handler (about :5880 to :6300 on `a78f731ad`). That
move needs no census row unless it adds a coercion.
- The census is file-scoped, as the card says, so a handler moved out of
`rest-server.ts` leaves its view.
- **Out of scope by construction:** arithmetic that coerces as a side
effect (`x * 1`, `Math.trunc(x)`).
### The two exemptions, justified and pinned
**Export `?page=`** stays `Number(q.page) || 500`.
- **What it does.** `page` sets only the chunk size of the export's own
`findData` loop, clamped to [50, 5000].
- **Rows are unchanged.** Measured on the real route with 1200 rows, the
streamed body is byte-identical for `page` absent, `abc`, empty, blank,
`1.5`, `Infinity`, `-5`, `50` and `5000`.
- **Unreadable reads as absent.** `abc`, empty and blank produce the
same three `findData` calls as an absent `page` (500, 500, 200).
- **The pin is live.** `page=50` really does change the chunking to 24
calls of 50.
- **The call.** No value of `page` widens or substitutes the answer,
which is the family's definition. A refusal would turn a correct export
into a `400`.
- **Caveat.** One place lets chunk size reach the output: the #3391
masked-row header fallback. When no security service is reachable, it
infers readable columns from whatever the first chunk holds. That is a
property of the fallback for every `page`, absent included, and refusing
`?page=abc` would not change it. See Acceptance notes.
**Rollback `toVersion`** is read body-first (`body.toVersion ??
body.version ?? req.query.toVersion`) and is then checked, not served.
- **Already refused.** A non-finite result, or one below 1, is refused
`400 INVALID_REQUEST` before `rollbackMetaItem` runs. It is pinned for
`abc`, empty and `Infinity`, with a lit control where `3` reaches the
verb as `3`.
- **Outside the family.** Nothing is dropped or substituted.
- **Not moved to the reader.** Its refusal is the door's own
`INVALID_REQUEST`, and moving it onto `VALIDATION_FAILED` would be a
wire change to a door that already refuses. No card here decides that.
## PM mechanism assumptions, measured
1. **The sites.** The five listed doors were confirmed red on
`a78f731ad`, through the real routes, before any source edit: all 30
refusal rows answered `200`, and all 25 lit controls passed. The census
then found the list incomplete by one door, `/diff`, which is fixed
here, the same class on the class-closure card. It also found two
coercions over a query value that are not family members: rollback and
export `page`, ledgered above.
2. **The fences.** No hunk touches them. Measured as the diff's hunks
against `a78f731ad`: :103 to :111, :670 to :737, :7485 to :7493, :7592
to :7600, :7677 to :7685, :7736 to :7757, :8084 to :8101, :9810 to
:9818, :10220 to :10231 and :12777 to :12793. None falls in:
- the `STORED_VERSION_DOOR_POLICY` docblock (about :3463 to :3473);
- the plain read's `?state=draft` branch (about :6921 to :6946);
- the `GET /meta/:type` list handler.
3. **The rider.** Done, in commit `1c8b320a`'s form.
## Tests
All at head `d07bbaa79`, after merging `origin/main` `d3958bac6` (no
rebase, no conflict; it touched `driver-sql` and one rest test file, not
`rest-server.ts`).
- **New per-door pins:**
`packages/rest/src/rest-server-query-number-reads.test.ts`, 55 cases.
Each refusal pins `status` 400, `code` `VALIDATION_FAILED`,
`fields[0].field` and `fields[0].code`, and that the service spy was
never called. Each lit control pins the argument the service received.
- Against the unfixed source (commit `54658aa45` carries the pins
alone): `Tests 30 failed | 25 passed (55)`. Every failure is "expected a
400 refusal … got 200".
- **New census:**
`packages/rest/src/rest-server-query-number-census.test.ts`, 14 cases.
They cover the walker's recognition of every spelling (and of nothing
else), classification, staleness, the single reader, reasons, and both
exemption pins.
- **Rest suite:** `pnpm --filter @objectstack/rest exec vitest run
--project local --maxWorkers=2` gives `Test Files 206 passed (206)`,
`Tests 3747 passed | 2 skipped (3749)`. `pnpm --filter @objectstack/rest
run test:repo` gives `Tests 8 passed (8)`.
- **Typecheck:** `pnpm --filter @objectstack/rest run typecheck` exits
0, and `check:test-typecheck: OK — … 0 file(s) / 0 error(s)`, so both
new test files compile in the test-layer program.
## Ablations
The fix was committed first (`5ef104dd1`). Each leg ran through
`scripts/ablation-replace.mjs` in WRAP mode, which carries an
EXIT/INT/TERM restore. Before the tests ran, the wrapped command proved
the mutation was on disk with a marker count. Vitest reads
`rest-server.ts` through the relative import and the census reads it
from disk, so no `dist/` is involved.
| leg | mutation | on disk | result |
|:--|:--|:--|:--|
| A | search `perObject` back to `req.query?.perObject ?
Number(req.query.perObject) : undefined` | anchor 1 → 0; marker count 1;
blob `9505bebc` → `0b580156` | `Tests 5 failed | 64 passed`: exactly the
4 `perObject` refusal rows + the census, which names
`rest-server.ts:10283 — GET ${basePath}/search »
Number(req.query.perObject)` |
| A′ | `/diff` back to the old `parseV` helper | anchor 1 → 0; marker
count 1; blob → `85ca1f0e` | `Tests 10 failed | 59 passed`: the 9 diff
refusal rows + the census, which names `GET ${metaPath}/:type/:name/diff
» Number(raw)`, the helper frame a textual heuristic misses |
| B | a new `const extra = Number(req.query.extra);` in the search
handler | anchor 1 → 0; marker count 1; blob → `1d6d2ea5` | `Tests 1
failed | 68 passed`: the census alone; every per-door pin stayed green,
since behaviour was unchanged |
- **Restores proven.** Every restore was proven by the tool and again by
hand: blob `9505bebc` equals HEAD's, `git diff HEAD` is empty, and the
marker count is back to 0.
- **The first B attempt was a no-op.** Its replacement contained its own
anchor, so the tool refused (anchor count 1 → 1), restored, and no test
ran. It was re-anchored on a comment line and re-run; that run is the
one reported.
## Gates
`node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack
--commands` derives 62 commands from merge base `d3958bac6`. All were
run at `d07bbaa79` with exit codes captured before any pipe. The `--ran`
reconciliation reads `62 derived, 60 run, 2 NOT-MEASURED, 0 UNRUN`.
- **60 exit 0.** These include:
- `check:adr-0087-registration --base origin/main`,
`check:changeset-no-major --base origin/main`, `check:empty-changeset
--base origin/main`, `check:changeset-gate-self-tests`;
- `check:route-envelope`, which stays green: the approvals refusal goes
through `handleRouteError` and adds no dialect body;
- `check:cross-package-test-inputs`, which stays green: the census reads
a file in its own directory;
- `check:nul-bytes`, `check:doc-authoring`, `check:published-files`,
`check:undeclared-dep-imports` (`typescript` is a declared devDependency
of rest), `check:test-source-alias`, `check:type-check-coverage` and
`check:issue-citations`.
- **NOT MEASURED: `check:dual-build-cjs-loads`.** Exit 3 `PREREQUISITE
NOT MET`, because 43 workspace packages have no `dist/`. Declared
narrowing: after `pnpm --filter @objectstack/rest build`
(`check-dts-emitted: 2/2`), `require('./dist/index.cjs').RestServer` and
`import('@objectstack/rest')` both answer `function`.
- **NOT MEASURED: `check:type-check-debt`.** Exit 3 `PREREQUISITE NOT
MET`, because the full closure is not built. Rest's own typecheck is
green above, and the diff moves no export. CI builds the closure before
this step.
- **`pnpm lint`** (this lane's addition) ran as the full union, `eslint
. --no-inline-config`, at `d07bbaa79`: exit 0.
- **Board check:** `node scripts/check-issue-citations.mjs --base
origin/main` after the merge exits 0, with `citations judged: 13 … 13
resolves`.
## First-party callers, measured
- **`@objectstack/client`** sends each of these parameters as
`String(number)`: `getHistory` `sinceSeq`, `getAudit` `limit`,
`diffItem` `from` / `to`, `search` `perObject`. `approvals.listRequests`
sends no `limit` / `offset`.
- **objectui at its pin `f8a9d0fb`:**
- The Console approvals inbox sends `limit: 50` with `offset: 0`, or
`rows.length`.
- `metadata-client` `diff` / `history` send numbers.
- The badge and record-panel approvals reads send no paging.
None sends a value this PR refuses.
## Acceptance notes
- **File surface beyond the claim's list.** All of it is in
`rest-server.ts` and outside both fences; each item is named above:
- the `/diff` door's two reads, the class member the census found;
- `UNDECLARED_WHOLE_NUMBER_PARAM`'s rename and docblock, which touch the
export and search `limit` call lines by name only;
- `readDeclaredQueryNumber`'s docblock (no body change);
- the import line for `AuditMetaItemRequestSchema`.
- **Rollback `toVersion`** refuses with the text "'toVersion' (positive
integer) is required", but `?toVersion=1.5` passes its `isFinite` /
below-1 check and reaches `rollbackMetaItem` as 1.5. Read only, not
measured. It is outside this family, since nothing is dropped or
substituted before a check. Carrier: none.
- **Export `?page=` reads like a page number but is a chunk size.**
`?page=2&limit=100` exports rows 1 to 100 in chunks of 50, not "page 2".
That is naming, readable values are outside this family, and it is
noted, not filed. Carrier: none.
- **The #3391 masked-row header fallback** infers readable columns from
the first chunk, so which rows that chunk holds (and therefore `page`)
can decide the header when no security service is reachable. This is
pre-existing and documented at #3547, and it is independent of whether
`page` is readable. Read only. Carrier: none.
- **`/diff`: `from` silently wins over `fromVersion`** when both are
sent, as before. The precedence is preserved, not judged.
- **Bounds.** No position on them, as with PR #20137: history `sinceSeq`
and audit `limit` admit fractions and negatives by declaration, and the
diff versions admit `0` and negatives. That is the spec seat's question.
- **The census's `not-a-query-value` rows** are judged once, by a
person. A future edit that routes a query value through one of those
expressions changes its key and reddens the census, which forces a
re-judgement.
- **The `Clause-②` line.** The claim's line reads bare `Clause-②: no`,
while its prose names `no (narrowing)` for the changeset. This body and
`.changeset/20139-rest-query-number-census.md` carry `no (narrowing)`,
per the dispatch order and PR #20137's form.
- **CI-owned, not run locally:** the full `Test Core` shards, `Dogfood`,
`Build Core`, `Temporal Conformance`, and the workspace type-check
lanes.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01UYBdGBzWSrAMzpW8ah3GbP)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5cf58eb commit 329ea2e
4 files changed
Lines changed: 1066 additions & 46 deletions
File tree
- .changeset
- packages/rest/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
0 commit comments