Skip to content

fix(rest): refuse a ?limit= the door cannot read on import jobs, export, meta history and search (#20061, #20062) - #20137

Merged
objectstack-fleet[bot] merged 4 commits into
mainfrom
claude/issue-20061-rest-limit-parsing
Sep 25, 2026
Merged

objectstack-fleet[bot] merged 4 commits into
mainfrom
claude/issue-20061-rest-limit-parsing

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Fixes #20061
Fixes #20062
Clause-②: no (narrowing)

What

Four published REST doors read ?limit= with a bare Number(). They substituted, clamped or dropped the result and answered 200:

  • GET /data/import/jobs
  • GET /data/:object/export
  • GET /meta/:type/:name/history
  • GET /search

Each door now reads limit against its own declaration and refuses a value outside it. GET /data/import/jobs does the same for offset, because its declaration bounds offset too. The refusal is 400 with the data surface's existing VALIDATION_FAILED + fields[] envelope, and the service is never called. Nothing a conforming caller sends changes its answer.

The change is one private reader in packages/rest/src/rest-server.ts, readDeclaredQueryNumber, with no new package export:

  • import jobs read through ListImportJobsRequestSchema.shape.limit / .offset;
  • history reads through HistoryMetaItemRequestSchema.shape.limit;
  • export and search declare no request schema, so they read through a module-private z.number().int().optional(): a whole number, nothing about range.

A non-blank numeric string is coerced with Number() and handed to the schema. Anything else (abc, a blank string, a structured value) goes to the schema as it came, so the declaration refuses it by type. Number() never gets to invent a 0 or a NaN for it.

Before and after, measured on the real RestServer routes (spy protocol)

Every row answered 200 before this PR.

door request answered before answers now
import jobs ?limit=0 / ?limit=-3 limit 50 / 1 400, fields[0] = limit / min_value
import jobs ?limit=201 / ?limit=500 limit 200 400, max_value
import jobs ?limit=abc / 1.5 / Infinity / blank limit 50 / 1.5 / 200 / 50 400, invalid_type
import jobs ?offset=-1 / abc / 1.5 offset 0 / 0 / 1.5 400, min_value / invalid_type
export ?limit=abc / empty / blank X-Export-Limit: 1 (a one-row export) 400, invalid_type
export ?limit=1.5 / Infinity X-Export-Limit: 1.5 / 50000 400, invalid_type
meta history ?limit=abc / Infinity limit dropped: the whole change log 400, invalid_type
meta history ?limit= (empty) / blank limit 0: zero events 400, invalid_type
search ?limit=abc limit: NaN handed to searchAll: no overall cap 400, invalid_type
search ?limit=1.5 / Infinity / blank 1.5 / clamped to 100 / 0 400, invalid_type

Unchanged and pinned by the lit controls:

  • Import jobs: absent or empty limit → 50, absent or empty offset → 0. Conforming values reach findData unchanged.
  • Export: absent → 10000. ?limit=25 → 25. The door's own range handling is untouched: ?limit=0 → 1 and ?limit=60000 → 50000.
  • History: absent → no limit member. 5, 0 and 1.5 are forwarded unchanged, because the declaration is z.number() with no int() and no bounds.
  • Search: absent or empty → undefined. 20, 0 and 500 reach searchAll unchanged, and its [1, 100] clamp is its own business.

The refusal body carries:

  • code: 'VALIDATION_FAILED';
  • fields: [{ field: 'limit', code, message }];
  • an error sentence naming the parameter;
  • object, on the export door only. The handler throws validationFailure (@objectstack/types), and each door's existing catch (handleRouteError / mapDataError) writes the 400. So rest-server.ts gains no response write site, and check:route-envelope's siblingCode count on it is unchanged (green).

PM mechanism assumptions, measured

  1. The parseIntegerParam cycle: confirmed. packages/runtime/package.json depends on @objectstack/rest. rest's dependencies are core, metadata-core, observability, platform-objects, service-package, spec, types (plus exceljs, zod). Nothing was moved; packages/runtime/** and packages/spec/** are untouched.
  2. The declared-schema idiom holds, with one measured difference in fields[].code. zod 4.6.1 against the declared schemas, mapped through zodIssuesToFields:
    • NaN, a string, Infinity, and 1.5 against int() all give invalid_type;
    • below min gives min_value; above max gives max_value.
      These are all ADR-0114 catalog members, never zod's own codes. Runtime's parseIntegerParam answers invalid_number for the same abc / 1.5; see the open question in the report.
  3. The doors match the card, with two corrections.
    • Export serves no declared request schema. CreateExportJobRequestSchema is the contract of POST /api/v1/data/:object/export, which rest does not mount. ExportRequestSchema (contract.zod.ts) is bound to no route.
    • The search door holds: the only production caller of searchAll is the REST door (git grep, tests excluded). So packages/metadata-protocol/src/protocol.ts is not in this diff.
  4. The empty-string premise is partly falsified. Only import jobs and search answered ?limit= with their default. Export answered a one-row export (Number('') || 0 → Math.max(1, 0)), and history answered zero events (Number('') = 0, forwarded). The rule applied is the one parseEnumParam (runtime query-param.ts) states for the same spelling:
    • Where the old answer already was the absent answer, empty stays absent.
    • Where the old answer was an invented 0, empty is refused.
      Reading it as absent on export and history would have turned those two into a 10000-row export and the whole change log. That grows the window, the widening this claim stops on, so the reader carries an explicit per-door emptyIsAbsent.

No door accepts anything it refused before: none refused any single-valued limit before this PR. No conforming value's answer changes either, so the declaration stays no (narrowing).

Tests

The new file is packages/rest/src/rest-server-limit-param-parsing.test.ts (43 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 unfixed 6780e34a: 24 failed (every refusal row answered 200), 19 passed (every lit control).
  • Against the fix: 43 passed.

One existing fixture was corrected in rest-server-closed-query-params.test.ts. Its search closed-set loop sent limit: 'x' and expected 200; it now takes a valid value per name, as its export twin in the same file already does.

Runs at head 74898ed5, after merging origin/main:

  • Rest suite: pnpm --filter @objectstack/rest exec vitest run --project local --maxWorkers=2 → Test Files 196 passed (196), Tests 3327 passed | 1 skipped (3328).
  • Typecheck: pnpm --filter @objectstack/rest run typecheck → exit 0. tsc --noEmit passes, and check:test-typecheck: OK — ... 0 file(s) / 0 error(s).

Ablations went through scripts/ablation-replace.mjs in WRAP mode, run after the fix was committed. Vitest reads src/ through the relative import, so no dist was involved.

  • Import jobs: restoring Math.min(200, Math.max(1, Number(q.limit) || 50)) landed on disk (anchor 1 → 0, blob 02ceccd1 → cab07300). Result: Tests 8 failed | 35 passed, exactly the 8 import-jobs limit refusal rows.
  • Search: restoring req.query?.limit ? Number(req.query.limit) : undefined landed (blob 02ceccd1 → 9b589c00). Result: Tests 4 failed | 39 passed, exactly the 4 search refusal rows.
  • Both restores were proven: blob == HEAD 02ceccd1, git diff HEAD empty.

Public surface: rest's dist/index.d.ts and dist/index.d.cts, built from merge-base bc80e162's rest-server.ts and from HEAD's, are byte-identical (cmp). The restore was proven by blob hash. So no importing package's types can move, and only rest's own tests are owed.

Gates

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack on the real diff gives 61 commands, identical to the PM's list. All ran at 74898ed5 with exit codes captured before any pipe. --ran reconciliation: 61 derived, 59 run, 2 NOT-MEASURED, 0 UNRUN.

  • 59 exit 0. This includes check:adr-0087-registration --base origin/main (not-required (no-migration-prescription) accepted), check:changeset-no-major, check:empty-changeset, check:route-envelope, check:nul-bytes, check:doc-authoring, check:published-files, check:issue-citations and check:undeclared-dep-imports.
  • NOT MEASURED: check:dual-build-cjs-loads, exit 3 PREREQUISITE NOT MET: 43 workspace packages have no dist/. Declared narrowing: require('./dist/index.cjs') and import('@objectstack/rest') both load and expose RestServer.
  • NOT MEASURED: check:type-check-debt, exit 3 PREREQUISITE NOT MET: the full closure is not built. See the .d.ts identity above; CI builds the closure before this step.
  • pnpm lint (the lane addition) ran as the full union, eslint . --no-inline-config, at 74898ed5: exit 0.

First-party callers, measured

  • @objectstack/client sends limit as written: data.listImportJobs, data.export, meta.getHistory, search.
  • objectui at its pin f8a9d0fb: ImportWizard sends limit: 50, ResourceHistoryPage sends limit: 100, and the search and export emitters send limit only when it is a number greater than 0. None sends a value this PR refuses.

Acceptance notes


Generated by Claude Code

…ing it

GET /data/import/jobs, GET /data/:object/export, GET /meta/:type/:name/history
and GET /search read ?limit= with a bare Number() and answered 200 with a
substituted, clamped or dropped window. Each now reads the parameter through
its declared schema (ListImportJobsRequestSchema, HistoryMetaItemRequestSchema)
or, where the door declares none, as a whole number, and refuses anything else
with 400 VALIDATION_FAILED + fields[] via the shared error mapper.

Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn
Co-authored-by: Claude <noreply@anthropic.com>
…e the reader's bag parameter

The closed-set acceptance loop for GET /search sent limit=x, an unreadable value
the door now refuses; it takes a valid value per name, as the export twin already
does. The reader's parameter is renamed so the canonical query-slot census does
not read it as a query slot.

Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn
Co-authored-by: Claude <noreply@anthropic.com>
Brings in PR #20128 (app nav doc-audience prune), which landed on
rest-server.ts after this branch was cut. No conflict.

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

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest, touching 9 documentable anchor(s).

7 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/data-api.mdx (via /:object/export (route, bridged from symbol readDeclaredQueryNumber — its route source's handler names it), /data/:object/export (route, a path literal in a comment on a changed line))
  • content/docs/data-modeling/schema-design.mdx (via /:object/export (route, bridged from symbol readDeclaredQueryNumber — its route source's handler names it))
  • content/docs/kernel/contracts/metadata-service.mdx (via getHistory (sdk, the bare tail of client method meta.getHistory, bound to GET /api/v1/meta/:type/:name/history))
  • content/docs/permissions/permission-sets.mdx (via /:object/export (route, bridged from symbol readDeclaredQueryNumber — its route source's handler names it), /data/:object/export (route, a path literal in a comment on a changed line))
  • content/docs/plugins/adding-a-metadata-type.mdx (via /:type/:name/history (route, bridged from symbol readDeclaredQueryNumber — its route source's handler names it), /meta/:type/:name/history (route, a path literal in a comment on a changed line))
  • content/docs/protocol/objectui/actions.mdx (via /:object/export (route, bridged from symbol readDeclaredQueryNumber — its route source's handler names it))
  • content/docs/ui/actions.mdx (via /:object/export (route, bridged from symbol readDeclaredQueryNumber — its route source's handler names it))
What this run could not see
  • 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.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 15 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 536f2d52fc5d3aa6067782f884c1798614710f02 → packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 536f2d52fc5d3aa6067782f884c1798614710f02

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 536f2d52fc5d3aa6067782f884c1798614710f02 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review September 25, 2026 08:32
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Sep 25, 2026
Merged via the queue into main with commit 95ab93f Sep 25, 2026
36 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/issue-20061-rest-limit-parsing branch September 25, 2026 08:52
akarma-synetal pushed a commit to akarma-synetal/framework that referenced this pull request Sep 28, 2026
… read, held by a census (objectstack-ai#20139) (objectstack-ai#20345)

Fixes objectstack-ai#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 objectstack-ai#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 objectstack-ai#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 objectstack-ai#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 objectstack-ai#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 objectstack-ai#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 objectstack-ai#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 objectstack-ai#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>
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

Face review (post-hoc, released)

Served-tier: CONTRACT_REVIEW_TIER
Merge-sha: 95ab93f5c255a7a6151e848c5e01c455d60fa28f
Local-runs: none

Scope: the changeset (and named docs) of PR #20137, released; this review cannot block anything and exists to find false released prose.

Sentence verdicts

(.changeset/20061-rest-limit-parsing.md at the merge; no docs page in scope)

  1. Title and "BREAKING: shipped as minor under the launch-window convention (check-changeset-no-major refuses major until GA)." TRUE. Frontmatter '@objectstack/rest': minor; the gate is named in the dev's gate table on [finding] GET /data/import/jobs never refuses its declared limit bound (min(1).max(200)): ?limit=0 silently answers the default 50 rows and ?limit=500 answers 200 #20061.
  2. "GET /data/import/jobs, GET /data/:object/export, GET /meta/:type/:name/history and GET /search read ?limit= with a bare Number(). ... ?limit=0 on the import-job history answered the 50-row default against a declaration of min(1).max(200); ?limit=abc on the export downloaded one row; on the metadata history it returned the whole change log; on search it removed the overall cap." TRUE. Removed lines packages/rest/src/rest-server.ts (old Math.min(200, Math.max(1, Number(q.limit) || 50)), old Math.max(1, Number(q.limit) || 0), old Number.isFinite drop on history, old Number(req.query.limit) on search); search's cap is hits.length >= overallLimit in packages/metadata-protocol/src/protocol.ts:11814-11863, never true for NaN. "Import-job history" names the GET /data/import/jobs list; the door is named in the same sentence.
  3. "Since @objectstack/client sends limit exactly as the caller wrote it, the door is the only place such a value can be refused." TRUE. packages/client/src/limit-guard-family.test.ts:1-25 pins that the SDK does not judge limit (only undefined stays off the wire) for data.listImportJobs, data.export, meta.getHistory and search.
  4. "GET /data/import/jobs reads limit and offset through ListImportJobsRequestSchema (limit int().min(1).max(200), default 50; offset int().min(0), default 0)." TRUE. packages/spec/src/api/export.zod.ts:503-504; rest-server.ts:~10160-10169.
  5. "GET /meta/:type/:name/history reads limit through HistoryMetaItemRequestSchema.limit (z.number(), so any finite number, as before)." TRUE. packages/spec/src/api/protocol.zod.ts:1512+ (limit: z.number().optional()); rest-server.ts:~8146-8153; lit controls 0 and 1.5 forwarded (test :118-126).
  6. "GET /data/:object/export and GET /search declare no request schema. There limit must be a whole number. Their range handling is unchanged: the export floor of 1 and cap of 50000, and search's [1, 100] clamp." TRUE. UNDECLARED_ROW_COUNT_PARAM = z.number().int().optional() (:~763); export Math.max(1, ...) and Math.min(..., HARD_CAP) kept (:~10292-10296); protocol.ts:11814 clamps [1, 100].
  7. "A value outside the declaration answers 400 with ... { error, code: 'VALIDATION_FAILED', fields }. fields[0].field names the parameter. fields[0].code is the ADR-0114 member ...: invalid_type ..., min_value or max_value .... The service is never called." TRUE. readDeclaredQueryNumber throws validationFailure with zodIssuesToFields (:~817-841); mapping packages/spec/src/api/zod-issues-to-fields.ts:74-101; every refusal row asserts the protocol spy was not called (test :142-340).
  8. The per-door before/after table (ten rows). TRUE row by row against rest-server-limit-param-parsing.test.ts:149-171, 211-233, 258-280, 301-323 and the old expressions (e.g. import jobs Infinity clamped to 200 before, export Infinity capped to 50000 before, history empty string forwarded as 0 before, search 1.5 cap before).
  9. "A blank value such as ?limit=%20 is refused on every door." TRUE (' ' row on all four doors).
  10. "Unchanged: An absent limit keeps each door's default: 50 jobs, a 10000-row export, the full history, search's 20." TRUE. Schema default 50; requestedLimit ... : 10_000; history omits the member (test :129-132); protocol.ts:11814 default 20.
  11. "An empty ?limit= on the import-job history and on search still means absent, as it always did there." TRUE. emptyIsAbsent: true on those two doors, false on export and history (:~10166, :~10707, :~10294, :~8152); tests :45-51, :165-171.
  12. "Every conforming value reaches the service exactly as before ...: export ?limit=0 still exports one row, search ?limit=500 is still clamped to 100, and history still forwards 0 or 1.5." TRUE. Tests :78-86 (X-Export-Limit: 1), :155-162 (500 reaches searchAll, which clamps), :118-126.
  13. "Fix for a caller that now gets the 400: send limit as a whole number, within the declared range where the door declares one (1-200 for import jobs), or omit it to get the door's default." TRUE.
  14. ADR-0087 disposition comment ("not-required (no-migration-prescription) ... packages/spec is untouched"). TRUE. The merge touches no packages/spec path.

Still true on origin/main?

Yes. The reader is still at the four doors (rest-server.ts:7272 history, 9543-9545 import jobs, 9692 export, ~10224 search); the undeclared constant was renamed UNDECLARED_WHOLE_NUMBER_PARAM and the same reader was later extended to ?perObject, history ?sinceSeq, audit ?limit and the diff version params (#20139 class closure), which adds doors and contradicts nothing. packages/rest/CHANGELOG.md:153 carries the reviewed text verbatim under 95ab93f.

Finding

NONE.

Reviewed-by: session_01VvcEokUG1tvVxkceYfR5XB

VERDICT: CLEAN


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment