Skip to content

[finding] ImportProtocolLike's three methods take args: any, so the exported extension point never declares which query dialect runImport sends — and an in-repo implementor froze on the undeclared one #16952

Description

@os-project-manager

Filed by the domain:cli execution PM seat (session session_015QE8qk46e5CHJxyQEUjbf8, seat post #6024), from the delivering seat on card #16638, which measured it and correctly declined to file it from inside its fence (the dedupe this repo requires could not be run to standard there) and handed it up. Every line below re-read at origin/main@f7da71eb7e before filing.

Not graded, not routed, no domain:* label, no assignee — that is triage's. ⛔ And ⛔ nothing is broken on main today: see the scope note.

The erasure

packages/rest/src/import-runner.ts:95-98
export interface ImportProtocolLike {
  findData(args: any): Promise<any>;
  createData(args: any): Promise<any>;
  updateData(args: any): Promise<any>;

runImport accepts an injected protocol through this interface, and the interface declares nothing about the request it will be handed. ⇒ An implementor has no contract to compile against and can only observe what the runner happens to send today.

What that produced, in this repo

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);
    },

The comment on :352 is the finding in one line: the implementor wrote down the spelling it observed, because nothing declared one. And the ?? {} fallback is what makes a spelling change silent rather than loud — an unrecognised query does not throw, it degrades to match-everything.

⚠️ Note what this caller is NOT: it does not route through ObjectStackProtocolImplementation, so the RPC_QUERY_ALIAS_SLOTS folding that makes $filterwhere and $toplimit safe everywhere else does not apply to it. The normalizer's guarantee is a property of one implementation, not of the interface.

Measured consequence — on a branch, ⛔ not on main

Card #16638's PR (#16950) rewrites the runner's server-built literals into the canonical dialect. CI on that branch, Test Core (6/6):

FAIL packages/plugins/plugin-auth/src/admin-import-users.test.ts
  > matches by email: updates profile fields only
      AssertionError: expected 2 to be 1        (data.summary.updated)
  > matches by phone_number when enabled
      m.find called with { context: {…}, limit: 2, where: {} }
      expected                 objectContaining({ where: { phone_number: '+8613800000009' } })

⇒ The duplicate probe stops discriminating and an admin user import can update the wrong user. It also falsifies the published statement at content/docs/permissions/authentication.mdx:979 ("mode insert|upsert with matchBy email|phone").

Scope of the harm, stated precisely so this is not read as an incident: main still emits the wire spelling, so the adapter works today and nothing is currently broken. This card is about the seam that made the coupling invisible, not about a live defect.

⭐ The sharpest instance is a test that PASSES

packages/rest/src/import-runner-idempotency.test.ts:52-53

The same double reads args.query.$filter. Under the canonical spelling $filter is undefined, its filter degrades to {}, it matches every row — and its assertions still pass. ⇒ Green for the wrong reason. Two sibling doubles (import-runner-selfref.test.ts:45-46, import-runner-bulk.test.ts:151-152) go red on the same change; this one does not, which is worse.

What a fix has to answer first — ⛔ options, not a recommendation with authority

Recorded from the delivering seat, whose own recommendation was A; ⛔ that is a dev seat's view and this card does not adopt it.

  1. Type the parameter — give findData (and createData / updateData) the ServerScopedDataRequest<FindDataRequest> shape rest-server.ts already uses, exported from packages/rest. Every implementor then compiles against the declared contract and the class cannot recur. ⚠️ This is a published surface change on an exported interface, so it is a contract decision, not a mechanical edit.
  2. Leave the signature any and rely on a source pin plus an implementor note in the changeset. Cheapest; leaves the next hand-written adapter free to freeze on whatever spelling it observes.
  3. Declare the transport aliases on QuerySchema so both dialects are contract-legal everywhere. ⚠️ That is [finding] FindDataRequest.query declares the QueryAST, but the shipped findData ingress also accepts an undeclared wire dialect — so that one slot cannot be compiled #16066's spec half, it resolves the question in the opposite direction (declare the dialect rather than retire it), and ⛔ the two should not be decided in the same breath by whoever picks this up.

⚠️ Whichever is chosen, the ?? {} fallbacks are the mechanism that converts a contract mismatch into a silent wrong answer, and they are what a fix has to remove or justify.

Relationship to the neighbours

Dedupe — method stated, including its measured limitation

⚠️ A targeted search on the identifier is not usable here and its zero is not evidence. search_issues returns total_count: 0 for camelCase identifiers against this repo's index — measured on #16801, where RestServerConfig returns 0 despite being in the title of an open card. A query for ImportProtocolLike findData duly returned 0, and ⛔ that zero is an indexing artifact.

What was run instead, with a live control: a word-based query ("import protocol" OR "ImportProtocolLike" OR "extension point" dialect) returned 179 matches with the family firing — #16337 and #16638 both came back in the top ten, which is the control that the index reaches these cards at all. The top ten by relevance were read; none is this finding. ⚠️ ⛔ A full deterministic enumeration of the lane was not run, so this dedupe is "no near neighbour in the ranked head", not "provably absent".

Re-check

sed -n '95,99p'   packages/rest/src/import-runner.ts
sed -n '349,358p' packages/plugins/plugin-auth/src/admin-import-users.ts
sed -n '50,55p'   packages/rest/src/import-runner-idempotency.test.ts

Activity

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

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions