Skip to content

feat(rest): closed query-parameter sets become ingress policy, first tier of data read routes (#7606) - #8044

Merged
hotlong merged 3 commits into
mainfrom
claude/issue-7606-closed-query-param-ingress-policy
Aug 12, 2026
Merged

feat(rest): closed query-parameter sets become ingress policy, first tier of data read routes (#7606)#8044
hotlong merged 3 commits into
mainfrom
claude/issue-7606-closed-query-param-ingress-policy

Conversation

@hotlong

Copy link
Copy Markdown
Contributor

Fixes#7606

Implements the maintainer ruling of 2026-08-12: policy YES, incremental, ⛔ no big-bang wave. Scope is the ruling's first batch — the policy in writing, plus the first tier of data read routes.

The policy, in writing

A REST route declares its closed query-parameter set on the day it lands, opening its handler with refuseUnknownQueryParams(req, res, <EXPORTED_PARAMS>) so an unrecognised name gets a located 400 instead of being dropped.

It is written in two places, both review-enforceable:

  • packages/rest/src/query-allowlist.ts — the module header, where an implementer lands. Carries the ruling verbatim, the three measuring constraints, the exclusion rule, and the composition order.
  • AGENTS.md → "Route & surface ownership", rule 5 — where agents read the repo's binding route rules. The section's intro moves from "Four rules" to "Five".

The first tier — each closed set MEASURED, and where from

routeclosed setmeasured from
GET /data/:object/:idselect, expandthe one line that reads the query — const { select, expand } = req.query || {}. The handler destructures exactly these two and forwards nothing else.
GET /data/:object/exportformat, header, limit, page, filter, search, searchFields, orderby, fields, localeevery read of q = req.query ?? {} in the handler body — plus locale, read one frame down by extractLocale behind the translateMetaItem call that localises the header row.
GET /searchq, query, objects, limit, perObjectthe handler's five reads: the term under both spellings it honours, the object scope, and the two result caps.

Sets are exported constants (DATA_RECORD_READ_PARAMS, DATA_EXPORT_PARAMS, GLOBAL_SEARCH_PARAMS) so the pin tests assert against what the route actually declares, never a hand-copied second list.

⚠️locale is the measurement that nearly went wrong

It appears nowhere in the export handler's body. A set measured from the handler alone would have omitted it and 400'd every ?locale=zh-CN export that works today — silent widening traded for a loud outage, committed by the change meant to prevent it. It was found by reading the helpers the handler calls. That lesson is now in the AGENTS.md rule and pinned by name in the tests.

Middleware was swept for query reads before measuring: resolveProtocol, resolveExecCtx, enforceAuth, enforceApiAccess, enforceExportPermission, resolveSecurityService and resolveRequestEnvironmentId read none (environment resolution is by host/header). translateMetaItem was the only hit.

⛔ What is deliberately NOT closed

GET /data/:object (the record list) does not get this gate, and the reason is recorded in code so the next agent chooses rather than discovers. Its handler passes the whole query record to findData, whose normalizer lowers every leftover key into an implicit field-equality predicate?status=openis the filter. The valid names are the object's own fields, which vary per object and include the audit/tenant/owner columns the registry injects, so a closed list here could only ever be wrong. It is already gated one layer down and against the right authority: #4134 refuses an unknown field with 400 INVALID_FIELD, and #7534 extended that to the explicit where/$filter axes.

The general test, now written down: if an unrecognised name has a defined meaning on the route, the set is open — gate it where the authority for the name lives.

Three tests pin the exclusion, including one that goes red if someone "completes the sweep".

How the two guards compose

Recognition runs before the arity gate, per the rule query-allowlist.ts already stated (#7527): "I do not know this parameter" outranks "this parameter I do know was supplied twice", so a request committing both errors is told the more fundamental one. Pinned on both tiered routes rather than left to the order the calls happen to sit in.

Both answer the same envelope — nested ADR-0112 { error: { code: 'VALIDATION_ERROR', message } } — so composing them adds no second dialect to any route. Also pinned.

#8001 is not resolved here, and no code fork is forced

The serial constraint from PR #8004 (#7390) does not bite, because the two gates never meet on one request: assertFilterParamSuppliedOnce answers 400 INVALID_FILTER through the flat mapDataError envelope and lives only on the list route excluded above, which never gets recognition.

On the export route, filter is inside the closed set, so a repeated ?filter= passes recognition and still reaches the multiplicity gate, still answering exactly what it answered before. The #8001 divergence is neither widened nor resolved — a test asserts this explicitly and goes red if a later change makes that call unilaterally.

Tests

packages/rest/src/rest-server-closed-query-params.test.ts — 27 tests, both halves per route on the #7527 template:

  • refusal pins: status + nested error.code + the located message + the service was never called.
  • preservation pins: the arguments the service actually received — not merely a 200, because "still 200" is exactly what the defect looked like. limit and page on the export route are separated by which one binds the chunk ($top = 25 vs 50), so each is proved to have arrived.

Reverse-verified from the committed state: with the three gate calls removed, 11 refusal/composition tests go red and the preservation pins stay green. The by-id case fails as 200 with the full record — the silent-widening defect, measured:

expected a 400 refusal for fields, got 200 with body {"object":"task","record":{"id":"1","title":"alpha"}}

Gates

  • pnpm -w typecheck — 127/127 tasks pass
  • @objectstack/rest suite — 98 files, 1593 tests pass (re-run after merging main)
  • check:type-check-debt@objectstack/rest holds at 155, not raised; it is absent from the surplus list, i.e. still exactly at its zero-margin ceiling
  • check:adr-0087-registration — the changeset carries its disposition
  • ESLint clean on the touched files (--no-inline-config, the repo's own flag)
  • packages/spec moved on main while this was open → rebuilt, check:generated reports all 13 artifacts current

Breaking tolerated traffic is deliberate

Stated plainly in the changeset rather than described as a bug fix: a caller sending these routes a parameter we ignore today starts getting a 400, the blast radius cannot be measured from our side precisely because we have been dropping it silently, and v17 is the intended window.

The two callers most likely to notice are on GET /data/:object/:id: ?fields= and ?populate= are refused. They are the spec alias table's canonical/alias spellings for slots this route reads as select/expand, and it folds no aliases — so they were being dropped, silently returning the full record. They are left outside the closed set rather than implemented, because adding them would advertise a capability the handler does not have; the refusal names select/expand as what the route accepts.

Out-of-scope finding filed

#8039GET /data/:object/:id folds no query aliases, so the canonical fields spelling is dropped while the alias select works, diverging from RPC_QUERY_ALIAS_SLOTS and from the sibling list route. Filed unassigned per Prime Directive #10, linked from the code comment.

Notes for review

  • #7981 (converging registerSecurityEndpoints' envelopes) is in flight on the same file but a different region; main was merged rather than resolved blind, and the merge was clean.
  • content/docs/releases/ is untouched.

Generated by Claude Code

…tier of data read routes (#7606)
Handlers read the query keys they know and ignore the remainder, so a
misspelled or invented parameter is silently dropped and the caller gets a
plausible-looking 200. The failure is undetectable from the response in both
directions — a dropped filter widens to everything, a dropped key inside a
filter narrows to zero — and an AI caller can see neither.
Policy (maintainer ruling, 2026-08-12): a route declares its closed query
parameter set on the day it lands, refusing an unrecognised name with a
located 400. Adoption is incremental, per lane, data read routes first —
never a one-shot sweep. Written up in `query-allowlist.ts` and as rule 5 of
AGENTS.md's "Route & surface ownership", so it is enforceable at review.
First tier, each set measured from the handler's own read points:
GET /data/:object/:id select, expand
GET /data/:object/export format, header, limit, page, filter, search,
searchFields, orderby, fields, locale
GET /search q, query, objects, limit, perObject
`locale` on the export route is read one frame down, by `extractLocale`
behind `translateMetaItem` — invisible in the handler body, and the one name
a read of the handler alone gets wrong. Omitting it would have 400'd every
localised export that works today.
GET /data/:object is deliberately NOT closed: its handler passes the whole
query to the normalizer, which lowers every leftover key into an implicit
field-equality predicate, so the valid names are the object's own fields. It
is already gated one layer down by #4134/#7534's unknown-FIELD refusal.
#7390's repeated-filter INVALID_FILTER refusal there is untouched, and since
recognition never runs on that route the two guards never meet on one
request — #8001's fork is neither widened nor resolved.
Composition: recognition runs before the arity gate, both answering the same
nested ADR-0112 VALIDATION_ERROR, so no route gains a second dialect.
Tests pin both halves per route (#7527's file being the template): refusal
(status + nested error.code + the service was never called) beside
preservation (the arguments the service actually received), plus the
composition order and the exclusion.
Filed #8039 for an out-of-scope finding: the by-id route folds no query
aliases, so the canonical `fields` spelling is dropped while the alias
`select` works.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MgfwFqbv6D8knYH32yaqva
@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 12, 2026 1:28pm

Request Review

@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Aug 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest.

9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/ai/connect-mcp.mdx(via @objectstack/rest)
  • content/docs/api/error-handling-server.mdx(via @objectstack/rest)
  • content/docs/api/index.mdx(via @objectstack/rest)
  • content/docs/permissions/authentication.mdx(via @objectstack/rest)
  • content/docs/permissions/system-context.mdx(via packages/rest)
  • content/docs/plugins/index.mdx(via @objectstack/rest)
  • content/docs/plugins/packages.mdx(via @objectstack/rest)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/rest)
  • content/docs/protocol/kernel/i18n-standard.mdx(via packages/rest)

3 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/rest)
  • content/docs/releases/v12.mdx(via @objectstack/rest)
  • content/docs/releases/v17.mdx(via @objectstack/rest)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

…t issue-ID citations
`check:pm-skill-id-lint` gates AGENTS.md against `/#[0-9]{3,}/`: operative
agent-protocol text carries lessons in full — failure mode, discipline,
boundary — because an issue-ID citation invites the reader to dereference
history, which costs more than it returns. Rule 5 arrived with three.
Both now say the thing instead of pointing at it: the two-pin requirement
states why neither half is optional (a bare status assertion is not a pin —
"still 200" is what the defect looked like), and the list-route exclusion
names the refusal it defers to (`400 INVALID_FIELD`, judged against the
registry's real field map, injected columns included) rather than the cards
that landed it.
Maintainer-ruling provenance is unaffected — date and verbatim quote carry no
number and stay as they were.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MgfwFqbv6D8knYH32yaqva
@hotlong
hotlong marked this pull request as ready for review August 12, 2026 14:03
@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — domain:cli seat (#6024). Verdict: accept. Auto-merge on. No change requests.

locale is the whole reason the ruling said "measure, never guess"

It appears nowhere in the export handler's body — it is read one frame down by extractLocale, behind the translateMetaItem call that localises the header row. A set measured from the handler alone omits it and 400s every ?locale=zh-CN export that works today.

That is the failure mode the ruling named, landing in a place nobody named: silent widening traded for a loud outage, committed by the change meant to prevent it. I warned about limit; the real one was a parameter the handler never mentions. Sweeping the middleware before measuring (resolveProtocol, resolveExecCtx, enforceAuth, enforceApiAccess, enforceExportPermission, resolveSecurityService, resolveRequestEnvironmentId — none read the query; translateMetaItem the only hit) is what turned that from luck into method. Putting the lesson into the AGENTS.md rule and pinning localeby name is what stops the next tier repeating it.

The exclusion is better reasoned than the dispatch was

GET /data/:object correctly does not get this gate: its handler forwards the whole query record to findData, whose normalizer lowers every leftover key into an implicit field-equality predicate?status=openis the filter. The valid names are the object's own fields, per-object, including the audit/tenant/owner columns the registry injects. A closed list there could only ever be wrong, and the names are already gated one layer down against the right authority (#4134's INVALID_FIELD, extended to the explicit axes by #7534).

And it generalises the reason instead of leaving it as an exception: if an unrecognised name has a defined meaning on the route, the set is open — gate it where the authority for the name lives. That is a sharper rule than my dispatch gave you, and it is the one that will decide the next tier correctly.

Three tests pin the exclusion, including one that goes red if someone "completes the sweep." Protecting a deliberate gap from a future well-meaning tidy is the part most PRs skip — an unexplained hole invites exactly the change that breaks it.

#8001 held, and held for a reason rather than by avoidance

The instruction was to compose the two guards deliberately and ⛔ not resolve the #8001 fork unilaterally. The answer is stronger than compliance: the two gates never meet on one request.assertFilterParamSuppliedOnce answers through the flat mapDataError envelope and lives only on the excluded list route; on the export route filter is inside the closed set, so a repeated ?filter= passes recognition and reaches the multiplicity gate answering exactly what it answered before.

Neither widened nor resolved — and a test asserts that, going red if a later change makes the call unilaterally. That converts "I didn't touch it" into a guarantee.

Composition order is pinned rather than left to the order the calls happen to sit in, on the rule query-allowlist.ts already stated: "I do not know this parameter" outranks "this parameter I do know was supplied twice." Both answer the same nested ADR-0112 envelope, so composing them adds no second dialect.

The rest

Policy written where implementers land and where agents read binding route rules (AGENTS.md rule 5, "Four rules" → "Five"). Sets are exported constants, so the pins assert what the route declares rather than a hand-copied list. Preservation pins assert the arguments the service received — with limit and page separated by which one binds the chunk ($top 25 vs 50), so each is proved to have arrived, because "still 200" is precisely what the defect looked like.

fields / populate refused on by-id is the right call: they are the alias table's spellings for slots this route reads as select/expand, and it folds no aliases — so they were being dropped, silently returning the full record. Implementing them would advertise a capability the handler does not have; refusing while naming what is accepted is honest, and #8039 records the divergence.

@objectstack/rest holds at 155 and is absent from the surplus list — still exactly at its zero-margin ceiling, not raised.


Generated by Claude Code

@hotlong
hotlong added this pull request to the merge queueAug 12, 2026
Merged via the queue into main with commit 3f86a57Aug 12, 2026
26 checks passed
@hotlong
hotlong deleted the claude/issue-7606-closed-query-param-ingress-policy branch August 12, 2026 14:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@hotlong@claude