Repository navigation
Two ingresses fold their authoritative id into the payload with the losing spread order — { id, ...data } lets a payload id override the id the ingress resolved #11231
Description
Observation from the #11142 ingress survey (PR #11229). No wrong write was measured — filed as a fragile-pattern finding, unassigned.
Two ingresses spell "fold the id I resolved into the write payload" with the id before the spread, so a caller-supplied data.id would override the id the ingress just resolved:
packages/services/service-settings/src/settings-service-plugin.ts(~line 469, the engine-facadeupdatewrapper):eng.update(objectName, { id, ...data }, driverOpts)—idwas read from the caller'swhere.id, thendataspreads over it. The wrapper passes nowhereto the engine, so the A scalardata.idbeside a DIFFERENT scalarwhere.idsilently drops thewhere.idpredicate — the one unhonoured-predicate shape #11009's refusal deliberately left standing #11142 conflict refusal cannot catch a divergence: the payload is the only id source the engine sees.packages/metadata/src/loaders/database-loader.ts(~line 276,_update):this.engine.update(table, { id, ...data })— same order, same single-source consequence.
The contrast is deliberate elsewhere and documented: the REST batch update arm spells it ql.update(op.object, { ...data, id }, …) with the comment "the operation's id AFTER the spread, so it wins" (rest-server.ts), and protocol.updateData does the same ({ ...request.data, id: request.id }, the #6479 fix). The two sites above are the same fold with the losing order.
Reachability, honestly stated: both sites' current callers appear to never put id inside data (settings rows are addressed by key/namespace; the loader is internal), so this may be unreachable today — that is why this is a finding, not a defect report with a repro. The pattern is still the exact #6479 shape one refactor away: any future caller that passes a row copy (rows carry id) silently retargets the write.
Cheap hardening if triage wants it: flip both to { ...data, id } (three characters each), matching the documented convention at the other two sites.
Refs: #6479, #11142, PR #11229 (survey section).
Generated by Claude Code
Activity
claude commentedon Aug 24, 2026
Finding concentration round (triage seat, session
session_01Kktexqp6uVuFMztvvTMf3V, 2026-08-24, maintainer-invoked): promoted →pm:queue+domain:services, Task (S, two one-line fixes + pins). Align both ingresses with the documented convention ({ ...data, id }— "the operation's id AFTER the spread, so it wins"):service-settings/src/settings-service-plugin.ts:~469andmetadata/src/loaders/database-loader.ts:~276. Cross-domain designation by triage: services seat carries both edits in one PR; declared file surface = those two files + their tests. No behaviour question — the winning convention is already documented inrest-server.ts.Generated by Claude Code
claude commentedon Aug 24, 2026
{ "issue": 11231, "status": "done", "branch": "claude/issue-11231-ingress-id-spread-order", "pr": "https://github.com/objectstack-ai/objectstack/pull/11625", "premise_still_valid": true, "summary": "Premise re-measured on origin/main 2cf5a96cc7 and VALID at BOTH sites — matched lines read, not hit counts: settings-service-plugin.ts:469 `return eng.update(objectName, { id, ...data }, driverOpts);` and database-loader.ts:276 `return this.engine.update(table, { id, ...data });`. Searches for 11231 across issues AND PRs (title and body) returned 0, so no split-out/sibling card had done it. Flipped both to `{ ...data, id }` — the operation's id after the spread, so it wins — each with a comment naming the convention (rest-server.ts batch arm, protocol.updateData #6479) and why neither site can be caught downstream: both pass no `where`, so the payload is the engine's only id source and UPDATE_ID_MISMATCH has nothing to compare. Added one conflicting-id pin per site plus a changeset. Final surface is exactly the declared one: the two source files, two new test files, one changeset.", "tests": "All at the pushed commit 8e8dab829c, exit codes captured before any pipe, verdicts quoted from the tools themselves. RED FIRST (pins run against pristine unfixed origin/main sources, no mutation needed): site1 `Tests 3 failed | 1 passed (4)` with `- \"id\": \"sys_setting_resolved\" / + \"id\": \"sys_setting_claimed\"` (the 1 pass is the multi branch, untouched by this change); site2 `Tests 3 failed (3)` with `expected { kind: 'by-id', id: 'meta_claimed' } to deeply equal { kind: 'by-id', id: 'meta_resolved' }`. Both match the failure signature predicted in writing beforehand. GREEN AFTER: site1 4/4, site2 3/3. ABLATION (trap-guarded, re-confirmed post-commit): both fix lines reverted on disk via an asserted-anchor python edit, mutation proven on disk by anchored grep in BOTH directions (losing form want-1 got-1, fixed form want-0 got-0) — not by an editor exit code; SITE1_EXIT=1 (3 failed | 1 passed), SITE2_EXIT=1 (3 failed); restored via `git -C \"$REPO_ROOT\" checkout --` against ABSOLUTE paths from a `trap ... EXIT INT TERM`, and restoration proven by `git hash-object` == HEAD blob, both non-empty (daaa2b21c1ae..., 761d1a6f1713...), `git status` clean. No rebuild sits between mutation and result: the pins import the SUT relatively from src, never dist, proven by the mutation flipping the outcome with no build step. FULL SUITES: @objectstack/service-settings 29 files / 514 tests passed + `tsc --noEmit` clean; @objectstack/metadata 33 files / 618 tests passed. GATES: 18 families exit 0, including the two this change KIND moves — check:engine-double-contract (`OK — 397 pinned, 133 in the DEBT ledger, 2 exempt`) and check:where-matcher (`292 matcher(s) discovered ... 0 silently-wrong and 0 unjudged ... none new`) — plus check:type-check-coverage, check:test-source-alias, check:cross-package-test-inputs, check:type-source-resolution, check:published-files, check:slot-lookup, check:durability-log-level, check:query-options-erasure, check:nul-bytes, check:changeset-gate-self-tests, check:objectui-changeset, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-ci-filter-parity, check-plugin-teardown-shape. Gate list re-derived from the ACTUAL final change set with `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack` (no hand-built diff). NOT MEASURED, declared: (a) check:type-check-debt --re-measure — needs the whole workspace closure built, does not fit the foreground budget; (b) repo-wide `pnpm lint`; (c) CI convergence — reporting at draft-PR time per the standing contract. One earlier lock acquisition returned `VERDICT queue-timeout (exit 99)` and was treated as NOT MEASURED and re-run, never read as a pass.", "open_questions": [ { "question": "The two new engine doubles sit OUTSIDE check:engine-double-contract's scan scope, so the gate's ratchet does not protect them. Bringing them in costs a row in scripts/engine-double-contract.pinned.json, which is outside the declared file surface. Take the coverage or keep the surface?", "options": [ "A — leave as shipped: doubles declare only `update`, and discovery needs the verb plus >=2 engine siblings, so the gate is green without naming them (measured, not assumed: 379 pinned rows listed exhaustively, neither file among them). They honour the contract by construction, calling assertEngineUpdateDispatch directly, so there is no looseness today; what is missing is only future protection of my own test files.", "B — add `find`/`insert` siblings and run `node scripts/check-engine-double-contract.mjs --write`. I TRIED this: the gate then discovers and pins both and says `New pinned coverage is GOOD and nothing is wrong with your change — the ledger just has to learn about it, or it never protects this file`. It costs 2 rows in a 379-row shared ratchet artifact that every parallel agent adding a double must also write." ], "recommendation": "A as shipped, because the fence said 'those two files plus their tests. Nothing else.' and B writes to a hot shared ledger that parallel dev seats also touch — avoidable conflict surface on a PR whose substance is a three-character flip. But B is the better durable engineering outcome and the gate explicitly blesses it, so this is your call, not mine to take unilaterally. Reverted cleanly (hash-object proof of byte-identical restore); one command to redo if you want it." } ], "out_of_scope_findings": [] }Generated by Claude Code