Skip to content

fix(rest): import-runner builds the canonical QueryAST through a typed findData envelope - #16950

Draft
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-16638-import-runner-canonical-query
Draft

fix(rest): import-runner builds the canonical QueryAST through a typed findData envelope#16950
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-16638-import-runner-canonical-query

Conversation

@claude

@claude claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Part of #16638 — the card's packages/rest half. It deliberately does not carry a closing keyword: the class this card names is not fully closed by the two files this seat was fenced to, and the remaining half is described under "Not landable alone" below. #16638 remains open after this merges.

Clause-②: yes
Re-declared from the DELIVERED diff, against the dispatch's no. The path limb is untouched (packages/spec is not in this diff) and no widening tell fires — no schema key, no closed-set member, no published export, no registry entry is added, and the helper's parameter is NARROWED from any. What moved is the thing the no was justified by: "nothing published moves" is false as delivered. ImportProtocolLike is an exported type of the published @objectstack/rest (packages/rest/src/index.ts), its findData(args: any) never declared which dialect the runner sends, and this diff changes what it sends. That is measured below, not inferred — a real implementor in a sibling published package breaks.


⛔ Not landable alone

The card, triage and the dispatch all rest on one premise:

No behaviour is at stake. The aliases DO fold: $filter resolves to where and $top to limit by the spec's own RPC_QUERY_ALIAS_SLOTS, with the value moved verbatim, so these three calls reach engine.find with the same option bag a canonical literal would produce.

That premise holds for two of the three production callers of runImport and fails for the third.

runImport's p is an injected ImportProtocolLike, not ObjectStackProtocolImplementation. The alias table lives inside the normalizer, so it folds only for callers that route through it:

runImport caller what it passes as p folds?
rest-server.ts:8944POST /data/:object/import await this.resolveProtocol(...) yes
rest-server.ts:9108 — async import-job worker await this.resolveProtocol(...) yes
plugin-auth/src/admin-import-users.ts:433POST /api/v1/auth/admin/import-users a hand-written adapter no

That adapter reads the wire dialect straight off the request and calls the engine itself:

// packages/plugins/plugin-auth/src/admin-import-users.ts:353-357  (unchanged by this PR)
async findData(args: any) {
  const where = args?.query?.$filter ?? {};
  const limit = args?.query?.$top ?? 2;
  return engine.find(args.object, { where, limit, context: SYSTEM_CTX } as any);
},

With the canonical spelling it reads undefined and falls back to where: {} — an unfiltered probe. Measured, cross-package, against the rebuilt @objectstack/rest dist:

packages/plugins/plugin-auth  src/admin-import-users.test.ts  (26 tests | 2 failed)
  x matches by email: updates profile fields only, never credentials or email
      expected 2 to be 1                      (data.summary.updated)
  x matches by phone_number when enabled
      1st vi.fn() call:
      [ "sys_user",
      -   ObjectContaining { "where": { "phone_number": "+8613800000009" } },
      +   { "context": {...}, "limit": 2, "where": {} },
      ]

The engine now receives where: {} where it received where: { phone_number: ... }. In findExisting that means the duplicate probe stops matching on the key: with two or more users it returns ambiguous for every row, and with exactly one it updates the wrong user. content/docs/permissions/authentication.mdx:979 states the contract this falsifies, in published docs:

mode: 'insert' | 'upsert' with matchBy: 'email' | 'phone'.

So the complete change is larger than the fenced file surface, and this branch must not land without it:

  1. packages/plugins/plugin-auth/src/admin-import-users.ts:353-356 — read where / limit. Different package, published, needs its own changeset. Out of this PR's declared file surface.
  2. packages/rest/src/import-runner-selfref.test.ts:45-46 and packages/rest/src/import-runner-bulk.test.ts:151-152 — doubles that read args.query.$filter; both are RED on this branch. Out of surface.
  3. packages/rest/src/import-runner-idempotency.test.ts:52-53 — the same double. It is GREEN, and that is worse: with $filter undefined its filter degrades to {}, so it matches every row and the assertions pass for the wrong reason. Out of surface.
  4. The durable fix for the class: ImportProtocolLike.findData(args: any) is itself the erasure one level up — it is why implementors froze on an undeclared dialect. Typing it is a contract decision on a published extension point, not a mechanical edit.

Everything below describes what this PR does contain, fully measured.

What changed

The three literals

// before                                              // after
findArgsBase({ $filter: { [f]: display }, $top: 2 })   query: { object: referenceObject, where: { [f]: display }, limit: 2 }
findArgsBase({ $filter: filter, $top: 2 })             query: { object: objectName, where: filter, limit: 2 }
findArgsBase({ $filter: { id: { $in: ids } }, $top: ids.length })
                                                       query: { object: objectName, where: { id: { $in: ids } }, limit: ids.length }

$filter to where, $top to limit, plus the object the declared query requires — the same mechanical rewrite #16337 left signposted at rest-server.ts:8972-8975.

The helper — the actual deliverable

// before
const findArgsBase = (query: any) => ({
  object: '',
  query,
  ...(environmentId ? { environmentId } : {}),
  ...(context ? { context } : {}),
});

// after
const findArgsBase = (request: FindDataRequest) => ({
  ...request,
  ...(environmentId ? { environmentId } : {}),
  ...(context ? { context } : {}),
});

The dispatch offered FindDataRequest['query'] or dropping the helper entirely. This takes the whole FindDataRequest, which is a strict superset of the first option: the request-level object is compiled too, so the object: '' placeholder that all three call sites had to override is gone, and each site now spells a real query: { ... } slot — which is what lets the existing pin machinery census this file with a plumbing change rather than a second strategy.

Ablation — three legs

Every mutation proven on disk by blob hash before its reading was taken; every restore by git checkout HEAD -- ABSOLUTE_PATH under an EXIT INT TERM trap, verified by hash equality and an empty git status --porcelain (which, unlike git diff HEAD, also catches a staged index).

leg tree tsc --noEmit reading
A delivered exit 0 green
A' one literal reverted to $filter / $top, helper still typed exit 1 src/import-runner.ts(410,47): error TS2353: Object literal may only specify known properties, and '$filter' does not exist in type 'QueryInput'.
C the ACTUAL pre-card file at $BASE 9a89a00 — three wire literals, query: any and all exit 0, 0 errors the pre-card world: the identical spelling cost no diagnostic
restore back to HEAD exit 0 green again

On-disk proof, leg A':

HEAD blob:                     32a731bb57d82c73544f832a409de27ce55a75ab
disk blob after mutation:      b3ec5040c28d13ac39d2761b2e1f09edd5ca2f05
canonical anchor count  1 -> 0
injected `$filter` count 0 -> 1
disk blob after restore:       32a731bb57d82c73544f832a409de27ce55a75ab
git diff HEAD lines: 0     git status --porcelain lines: 0

Leg C is the discriminating control the card demands: without it, leg A' only shows an error, not that this annotation is what produces it. $BASE is the commit pinned at worktree creation, never the shared moving origin/main ref. (A first pass at leg C mutated the signature to any in place and exited 1 on TS6196: 'FindDataRequest' is declared but never used — an artifact of the mutation itself, with zero diagnostics on the literal. Measuring the real $BASE file removes that ambiguity, so that is the leg reported.)

No dist preflight applies to this ablation: tsc --noEmit reads packages/rest/src directly. The preflight was used for the cross-package measurement above, which does resolve through distablation-dist-preflight.mjs confirmed the canonical literal present in dist/index.js and dist/index.cjs, and --absent confirmed findArgsBase({ $filter gone from all 6 built files.

Negative control — the three call paths

Added to §3 of the pin, driven through the REAL ObjectStackProtocolImplementation normalizer: the option bag engine.find receives is asserted equal for the wire and canonical spelling of each of the three sites (reference resolver, duplicate probe, id recheck). All three pass, alongside §3's existing control that the instrument can tell two option bags apart.

That is the control for callers that route through the normalizer. It is also exactly why the plugin-auth finding above is a finding and not noise: the equality is a property of the normalizer, and that adapter does not use it.

Pin widening

rest-server-canonical-query-ast.test.ts now censuses the package from a table rather than one file, and the two files get different rules for a stated reason:

  • rest-server.ts — the HTTP door. It parses filter / top / skip / sort / select off the caller's own querystring, so a wire spelling outside a server-built query: literal is legitimate there. Unchanged rules, floor of 5 query: slots.
  • import-runner.ts — no door; every query in it is server-built. Its census therefore rejects a wire-dialect key in object-literal position anywhere in the file, not only inside a query: slot. Floor of 3 query: slots.

The whole-file rule is the one that closes the class: these three literals were arguments to a helper and were never in a query: slot, so a slot census structurally could not have found them.

Controls on the census instrument itself, because an empty result is otherwise indistinguishable from a detector that matches nothing:

  • it fires on { $filter: ... }, on a key after a trailing comma across a newline, and on { select: [] };
  • it does not fire on a const filter: declaration typed as a Record of string to any (a type annotation, not a key) or on where: filter (a value reference);
  • the comment stripper leaves the code being censused (the helper signature is still present afterwards, the file is still over 400 lines) and really does drop prose.

The stripper drops comment-ONLY lines and keeps trailing comments — deliberately the conservative direction, so the scan can over-report loudly but never under-report silently. A string-aware tokenizer is the unsafe alternative here: replace(/[BACKTICK-DQUOTE-SQUOTE]/g, '') in import-runner.ts opens a quote state a simple tokenizer never closes, and everything after it would stop being scanned.

§2 gains a live @ts-expect-error for $filter (the alias this card retires); check:test-typecheck compiles that layer, so an unused directive there is TS2578 — it is an assertion, not decoration. §3's picker control is now located by name rather than by PAIRS[3], since inserting rows above it would have silently re-pointed a positional reference at a different row.

Verification

All at 03fdc6ceb7, working tree clean.

check result
pnpm --filter @objectstack/rest typecheck exit 0 (tsc --noEmit + check:test-typecheck: 0 files / 0 errors)
pnpm --filter @objectstack/rest test exit 1 — 2 failed / 182 passed files; 2 failed / 3068 passed / 1 skipped tests. Both failures are the out-of-surface doubles named above. The widened pin passes.
pnpm --filter '@objectstack/rest^...' build exit 0 (dependency closure)
eslint . --no-inline-config (whole repo, not narrowed) exit 0 — population 6383 files read from eslint's own --format json output, 0 errors, 0 warnings, and both touched files are in that population. No narrowing was needed, so no invariance argument is owed; for the record the config enables no type-aware linting for any file (eslint.config.mjs:325-335).
dispatch-gates.mjs --commands then --ran 56 derived, 56 run, 0 NOT-MEASURED, 0 UNRUN
gate outcomes 53 exit 0. 3 runs returned exit 3 = PREREQUISITE NOT MET, not a finding: check:dual-build-cjs-loads and check:type-check-debt both require a whole-repo pnpm build and measured nothing. Declared to CI.
notable green gates check:query-options-erasure (ratchet holds, baseline verified against 9a89a00, no files added), check:where-matcher, check:cross-package-test-inputs, check:test-source-alias, check:published-files, check:type-check-coverage, check:nul-bytes (8373 files, no raw control bytes)
control-character self-sweep grep -naP over the three touched files: no hits

Changeset — measured, and it is required

skip-changeset is not defensible here. @objectstack/rest publishes ["dist","README.md","CHANGELOG.md"], and dist moves:

  • positive control that the search firesablation-dist-preflight.mjs @objectstack/rest 'query: { object: referenceObject, where: { [f]: display }, limit: 2 }' reports hit packages/rest/dist/index.cjs and hit packages/rest/dist/index.js, exit 0;
  • the complement — the same tool with --absent on findArgsBase({ $filter reports the marker absent from all 6 built files, exit 0.

So the published artifact carries the new spelling, and the payload handed to every ImportProtocolLike implementor changes with it. .changeset/import-runner-canonical-query-ast.md declares @objectstack/rest: minor and states the implementor-visible consequence explicitly. A @objectstack/plugin-auth entry is owed with the adapter fix, whenever that is authorised.

Docs drift — re-derived, and it is not zero

Re-derived from a clean worktree (git status --porcelain empty at 03fdc6ceb7). scripts/docs-audit/affected-docs.mjs named 7 pages, and printed its own coverage limit: the sdk bridge reached 60 of 216 client-bound ledger rows, so 156 are unreachable to it.

⚠️ Those 7 rows are wide by construction: all are reached through the same anchor, the route /:object/import bridged from the symbol runImport — and this diff sits INSIDE that route's implementation, so the anchor catches every page about the route. "Read-only" answers whether I may edit a page, never whether the page is falsified. The discrimination, re-derived here at 03fdc6ceb7 (occurrence counts, import as the positive control):

page $filter / $top import (control) verdict
api/client-sdk.mdx 0 10 wide-anchor artifact
api/wire-format.mdx 0 3 wide-anchor artifact
data-modeling/fields.mdx 0 5 wide-anchor artifact
data-modeling/import-mappings.mdx 0 20 wide-anchor artifact
protocol/objectql/state-machine.mdx 0 19 wide-anchor artifact
releases/v12.mdx (release-owned) 0 5 wide-anchor artifact
releases/v17.mdx (release-owned) 1 51 read below — not falsified

The control fires on all seven, so those six zeros are readings and not dead greps. Six pages are on the list only because they name the import route; none of them is edited.

releases/v17.mdx:3282, read out — under the heading #### Protocol & wire changes since rc.6:

A where on a virtual formula field is refused, not answered with zero rows (#8296) — as is an unknown field inside where / $filter / a filter AST (#7534), a dotted fields / $select entry (#7532, which used to widen the response to every field), and a repeated ?filter= (#7390).

That sentence is a claim about what the transport ACCEPTS from a caller: it enumerates the caller-facing spellings the ingress refuses an unknown field in — where, $filter and a raw filter AST side by side, fields / $select, and the ?filter= querystring. It says nothing about what the server EMITS. This diff changes only server-built literals and leaves the door byte-identical — rest-server.ts has zero changed lines, and the whole diff is 3 files (import-runner.ts, the pin, the changeset). So the accepted wire dialect cannot have moved, and declaring those aliases at the door is #16066's half, deliberately not merged in here.

Not falsified. No card filed, no edit — and it is release-owned besides, so it would not have been mine to edit either way.
Hand sweep of content/ for this change's tokens, against a live positive control (objectstack, 358 files):

token files
$filter 8
$top 7
findData 5
QuerySchema 10
import-runner 0
ImportProtocolLike 0

Reading: every $filter / $top page (odata.mdx, query-adapter.mdx, data-api.mdx, query-syntax.mdx, schema-design.mdx, ...) documents the caller-facing dialect at the HTTP door, which this change does not touch — the wire aliases stay accepted for callers, and declaring them there is #16066's spec half, deliberately not merged in here. ImportProtocolLike appears in no page, so the extension point whose payload this diff changes is undocumented. No docs edit is owed by the in-surface diff.

The sweep did catch one thing the tool's list would not have led me to: content/docs/permissions/authentication.mdx:979 documents the matchBy: 'email' | 'phone' upsert contract for POST /api/v1/auth/admin/import-users — the exact behaviour the unmitigated change breaks. That page is not stale because of this diff; it is a published contract that item 1 above must protect. content/docs/releases/ is untouched.

验收备注


Generated by Claude Code

@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/api/client-sdk.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/api/wire-format.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/data-modeling/fields.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/data-modeling/import-mappings.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it), /:object/import/jobs (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/protocol/objectql/state-machine.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it), /:object/import/jobs (route, bridged from symbol runImport — its route source's handler names it))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v12.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))
  • content/docs/releases/v17.mdx (via /:object/import (route, bridged from symbol runImport — its route source's handler names it))

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.

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 13 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 34a76c4800d46f2aec1c136b65ff94e4087435c8packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 34a76c4800d46f2aec1c136b65ff94e4087435c8

⚠️ 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 34a76c4800d46f2aec1c136b65ff94e4087435c8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Collaborator

⛔ BLOCKED — CI red is this PR's, and this PR is not landable alone

Test Core (6/6) failed on 03fdc6ceb7. I read the job log before writing this.

FAIL packages/plugins/plugin-auth/src/admin-import-users.test.ts
  > runAdminImportUsers — upsert > matches by email: updates profile fields only
      AssertionError: expected 2 to be 1        (data.summary.updated, :560)
  > runAdminImportUsers — upsert > matches by phone_number when enabled
      m.find called with { context: {…}, limit: 2, where: {} }
      expected                 objectContaining({ where: { phone_number: '+8613800000009' } })   (:592)
  Test Files 1 failed | 105 passed (106)   Tests 2 failed | 2213 passed (2215)

⚠️ This is NOT a failure that belongs to another PR, and it is not a flake. The diff touches three files, all in packages/rest, and packages/plugins/plugin-auth is not among them — but the rule is "fix and push when it is in code the PR touches or breaks", and this PR breaks it. ⛔ No re-run: the failure is deterministic and its cause is understood.

Why it breaks

Verified by me at source on origin/main:

packages/plugins/plugin-auth/src/admin-import-users.ts:351-357
  const protocol: ImportProtocolLike = {
    // findExisting path: `{ $filter, $top }` against sys_user.
    async findData(args: any) {
      const where = args?.query?.$filter ?? {};
      const limit = args?.query?.$top ?? 2;
      return engine.find(args.object, { where, limit, context: SYSTEM_CTX } as any);
    },

runImport takes an injected ImportProtocolLike; the alias folding the card relied on is a property of ObjectStackProtocolImplementation, which this caller does not use. So the canonical spelling arrives, $filter is undefined, the ?? {} fallback turns a key match into a match-everything, and the duplicate probe stops discriminating. ⇒ An admin user import could update the wrong user.

Nothing is broken on main today — it still emits the wire spelling — so there is no incident. The regression exists only if this lands alone.

What is blocking, exactly

The remaining fix is four files, all outside this card's declared surface: packages/plugins/plugin-auth/src/admin-import-users.ts (a second published package, needing its own changeset) and three packages/rest/src/import-runner-*.test.ts doubles. ⭐ The delivering seat stopped at the fence and reported instead of widening — correct.

The two halves are one atomic seam: shipped apart, main is broken in between, and surviving an interval would need the adapter to tolerate both spellings — the lenient-alias fallback Prime Directive 12 forbids.

I am not widening the card, and I am not arming this PR. Two preconditions are not mine: the card's priority:p3 was graded on "⛔ 无行为后果", which is now falsified, so re-grading is triage's (pm:retriage applied); and the diff is now clause-② yes, so it needs the contract review at CONTRACT_REVIEW_TIER, whose budget is a maintainer line and is currently exhausted.

Full reasoning, the amended clause-② declaration, and the root-cause reading are on the card: #16638 (5589976924).

⭐ Worth keeping from this branch whatever happens to it

  • The typing of findArgsBase's parameter is real value that stands alone, and the three-leg ablation proves it holds ground: reverting one literal to $filter with the helper typed gives TS2353: '$filter' does not exist in type 'QueryInput', while the actual pre-card file at base compiles clean with 0 errors — the pre-card world where that spelling cost no diagnostic.
  • packages/rest/src/import-runner-idempotency.test.ts:52-53 carries the same double and is GREEN for the wrong reason: with $filter undefined its filter degrades to {}, matches every row, and its assertions pass vacuously. That is worth more than the two red doubles.

Generated by Claude Code

Copy link
Copy Markdown
Contributor

Contract review (claude-fable-5-1, isolated seat) — PR #16950 @ 03fdc6c

Verdict: CHANGES REQUIRED

Head reviewed: 03fdc6ceb7ba691e09d9ea8a55003c35c9e01ae7 (merge-base with origin/main: 9a89a00; 16 commits behind origin/main@f36eef55d; none of the three touched files moved on main since the merge-base). Everything below was re-measured from refs/pull/16950/head, not taken from the PR body.

Ruling implemented: yes, within the fence. Card #16638's triage acceptance (os-zhuang, MEMBER — maintainer-side triage) items 1–6 are all present in the diff: the three literals now spell object / where / limit; findArgsBase takes FindDataRequest instead of any; the pin is widened to a per-file census table; the three call paths are added as §3 negative-control pairs; #16066 is not merged in; rest-server.ts and content/docs/releases/ are untouched. The operative ruling on this PR is the PM's (os-project-manager, COLLABORATOR — a seat, not a maintainer), card comment 5589976924, quoted verbatim once:

Held. The card returns to triage with the measurement; the PR stays a draft, explicitly not landable alone.

That hold is correctly implemented: the PR is a draft, carries Part of #16638 and no closing keyword, and is not armed. Triage's premise "⛔ 无运行时差异、⛔ 无线上形状变化、⛔ 无消费者受影响" is falsified by this diff (finding 1); re-grading priority:p3 is triage's, and pm:retriage is on the card.

Governed paths touched: NO — diff is .changeset/import-runner-canonical-query-ast.md (new), packages/rest/src/import-runner.ts, packages/rest/src/rest-server-canonical-query-ast.test.ts. No docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md, content/docs/releases/**.

Clause-②: yes (derivation). Mechanical floor: packages/spec/src/** is not in the diff; no new exported symbol (git grep of packages/rest/src/index.ts at head shows ImportProtocolLike and runImport were already exported); no new key on a published payload — so the floor alone does not fire. The published-surface limb does: ImportProtocolLike is an exported type of published @objectstack/rest (packages/rest/src/index.ts:42), its findData(args: any) declares no dialect, and this diff changes the payload the runner hands every implementor (query.$filter/query.$topquery.object/query.where/query.limit). That is not inferred: a published sibling package's implementor (packages/plugins/plugin-auth/src/admin-import-users.ts:353-356) breaks on this head in CI. dist moves (the PR's preflight reading is consistent with the source change). Comparison: PR body declares yes; the claim comment 5588889126 declared no, amended to yes by the PM in 5589976924; needs:contract-review is on both carriers. No mismatch remains. scripts/pm/check-widening-tells.mjs --declaration yes --diff <mb..head> → exit 0.

Changeset: @objectstack/rest: minor (top-level packages/rest, published, src/** touched). Graded by hand: Clause-② yes + published src/** ⇒ ≥ minormet. No BREAKING banner / ADR-0087 disposition required: no declared accept set narrows (QuerySchema, FindDataRequestSchema and the HTTP door are byte-identical; $filter/$top were never declared on FindDataRequest['query']), so the runner is moving onto the contract it already declares. Gates from the head tree (blob-identical to the checkout copies, run read-only against the two refs): check-changeset-no-major.mjs --base 9a89a00 --head <ref> → "introduces no major bump", exit 0; check-adr-0087-registration.mjs → "adds no declared-breaking changeset (1 non-breaking changeset(s) seen)", exit 0. ⚠️ The changeset's implementor-visible warning is correct prose, but the same PR that lands the adapter fix owes a @objectstack/plugin-auth entry (finding 1).

CI reading (head 03fdc6c, 49 check runs, read once): Test Core aggregate failure — 2 of 6 shards published no positive attestation. Both reds are this PR's, deterministic, not infra:

  • Test Core (3/6)@objectstack/rest#test: Test Files 2 failed | 182 passed, Tests 2 failed | 3068 passed | 1 skipped. import-runner-bulk.test.ts:160 (['created','created','created'] vs ['created','updated','created']) and import-runner-selfref.test.ts:74 — both doubles read args.query?.$filter (bulk :151-152, selfref :45-46).
  • Test Core (6/6)@objectstack/plugin-auth#test: admin-import-users.test.ts:560 (summary.updated expected 1, got 2) and :592 (m.find called with where: {} instead of where: { phone_number: '+8613800000009' }).
  • Everything else green: Build Core, Type Check (workspace / source gates / consumer gates / debt ledger), Lint & Repo Gates, Check Changeset ×3, Governed Surface Queue Guard, Part-of guard, single-writer / same-issue guards, Dogfood ×4, Temporal Conformance. No ci: a shard attestation upload is refused with a 403 on FinalizeArtifact after uploading successfully, so a fully green Test Core shard reds the PR — measured twice on two PRs in 35 minutes #16928 FinalizeArtifact … 403 signature present — both failing shards finalized their artifacts successfully. mergeable_state: unknown at read time. Commit trailers: two commits, no Fixes/Refs/Part of in either message (RULE 2 OK).

Findings

  1. HIGH — Not landable alone; the delivered diff regresses a published sibling. runImport's p is an injected ImportProtocolLike; the alias folding the card relied on lives in ObjectStackProtocolImplementation, which packages/plugins/plugin-auth/src/admin-import-users.ts:353-356 does not use — it reads args?.query?.$filter ?? {}. With this head the duplicate probe becomes where: {} (match-everything): with ≥2 users every row is ambiguous, with exactly one the wrong user is updated (POST /api/v1/auth/admin/import-users, matchBy: 'email' | 'phone' per content/docs/permissions/authentication.mdx:979). Reproduced in CI shard 6/6. The seat stopped at the fence and reported instead of widening — correct. Expectation: the emitter change and the implementor fix are one seam and land in one PR (the PM concurs on mechanics; a two-PR order would need the adapter to accept both spellings, the lenient fallback Prime Directive 12 forbids). That PR reads args.query.where / args.query.limit in the adapter, adds a @objectstack/plugin-auth changeset, and stays draft until the card's surface is widened by whoever owns that decision. main is not broken today; the regression exists only if this lands as-is.

  2. HIGH — CI red in packages/rest is this PR's, and one green double is vacuous. import-runner-bulk.test.ts:151-152 and import-runner-selfref.test.ts:45-46 read $filter and are RED on this head (shard 3/6). import-runner-idempotency.test.ts:52-53 reads the same key, degrades to {} and stays GREEN for the wrong reason — its recheck matches every row. Expectation: all three doubles read args.query.where; the idempotency double gains an assertion that its filter actually narrowed (e.g. the recheck receives the id: { $in: … } it was given), so the test reddens if the payload spelling drifts again.

  3. MEDIUM — Consumer-side patch on a producer defect (contract-first). The class exists because the exported extension point ImportProtocolLike.findData(args: any) (packages/rest/src/import-runner.ts:96, exported at index.ts:42) declares no dialect; every implementor in this repo (three test doubles, the plugin-auth adapter) froze on the spelling it observed. This PR types the runner's emitter (findArgsBase(request: FindDataRequest)) — real, load-bearing, and the pin's whole-file rule holds it (replayed the detector over the base file: ["$filter","$top"]; over head: []) — but leaves the producer contract untyped, so the changeset's "implementor must read where/limit" is prose where a type would be a compile error. packages/runtime/src/action-execution.ts:287 and the two rest-server.ts call sites route through the real protocol and are unaffected. Expectation: the atomic PR (or the PM's separately-filed card — its number is not yet linked from this PR or the card thread) types findData's parameter as ServerScopedDataRequest<FindDataRequest>-shaped (rest-server.ts:290 already defines that alias privately) and exports it; until then the PR body should link the follow-up card so the seam is traceable.

  4. LOW — Pin quality: acceptable, no loosening. it.skipIf(!noDoor) is a per-row conditional (the 1 skipped in rest is the rest-server.ts row, where a wire key is legitimate at the door), not a .skip/.only/.todo; none of the latter present. PAIRS[3] → by-name lookup, toHaveLength(7→8), floors 5/3 — nothing existing loosened. Every changed verdict is pinned: reverting any literal to $filter reddens wireKeysAnywhere and the by-name toContain and (per the typed helper) tsc. I did not re-run the three-leg tsc ablation; it is consistent with FindDataRequest['query'] being the QuerySchema type, on which an excess $filter in an object literal is TS2353. Expectation: none for this PR.

  5. LOW — Card state. Claim 5588889126 present with Container & model: claude-opus-5 (reported as the claim names it; both commits carry Co-Authored-By: Claude Opus 5). needs:contract-review is on the card and the PR. Card labels also carry pm:retriage and priority:p3 — the grade rests on a falsified premise and re-grading is triage's, not this seat's.

Maintainer-only merge: no — no governed path, no !/breaking on a security or contract face (declared contracts unchanged; the runner conforms to them), no maintainer-floor item; the director seat lands it once findings 1–2 are in the same PR and every check is green. The two open holds (scope widening, p3 re-grade) are triage/PM process, not merge-floor conditions.


Generated by Claude Code

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

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants