Skip to content

feat(objectql,plugin-audit): refuse an unscoped multi-UPDATE on the shape, not by accident (#9974) - #10093

Merged
os-elon merged 4 commits into
mainfrom
claude/issue-9974-unscoped-multi-update-refusal
Aug 20, 2026
Merged

feat(objectql,plugin-audit): refuse an unscoped multi-UPDATE on the shape, not by accident (#9974)#10093
os-elon merged 4 commits into
mainfrom
claude/issue-9974-unscoped-multi-update-refusal

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Fixes#9974

Implements the maintainer's option A ruling of 2026-08-19: extend #9719's whole-operation dispatch to beforeUpdate's predicate path, so the sys_comment refusal declared at #4630 fires on the shape — no id and no where — regardless of what the rows say.

Deletion leaves a trace — you can tell who deleted what. An overwrite leaves none: the old value is gone on the spot and there is nothing to restore from. And "forgot to say which ones" is exactly the mistake generated code makes most often.

Implementation shape: generalized the existing flag, did not add a sibling

The card allowed either a dispatchUnscopedMultiUpdate sibling or generalizing the flag. I generalized: dispatchUnscopedMultiDeletedispatchUnscopedMultiWrite, valid on beforeUpdate and beforeDelete, with one dispatch helper taking event as a parameter.

Why this one leaves less duplicated logic:

  • A sibling means two booleans on HookEntry, two in registerHook's options, two in each consumer's duck-typed engine seam, and two registration asserts — for one mechanism whose only difference is the event name.
  • The flag is already per-registration and per-event, so a single name still expresses "delete only" exactly: sys_attachment declares it on its beforeDelete registration and nowhere else, and its accept set is unchanged. There is a dedicated pin for that (a beforeDelete-only declaration does not leak the dispatch onto update).
  • It matches the repo's own recorded preference at the adjacent seam — the delete() by-id 仍允许 beforeDelete 重定向 input.id —— 与 update() 侧的 ADR-0058 Amendment II.1 不对称,需定夺是否一并退休 #6752 ruling quoted in engine.ts: "one rule across both verbs beats two individually-correct rules an author has to memorize."

The rename is compiler-enforced and mechanical; @objectstack/service-storage is a rename-only follow.

The three pins were REPLACED, not relaxed

comment-access-hooks.test.ts, describe block now unscoped multi-UPDATE (no id, no where) — #4630 through the wired engine (#9974):

limbbeforenow asserts
caller authored every rownot refused, whole table rewrittenrejects with RECORD_NOT_ACCESSIBLE/403 + Refusing an unscoped multi-update of comments; bodies unchanged
empty table (zero match)resolved, nothing ransame refusal, on the shape
a row the caller may not touch is sweptrefused with Cannot update comment c2: …same shape refusal, plus an explicit assertion that the per-row wording is gone

Limb 3's message mattered as much as its verdict, so it is pinned in both directions: the new sentence must be present andCannot update comment c2 must be absent — asserting only the former would pass on a message that still led with the row.

The block keeps its neighbours as controls: the per-row refusal still fires on a scoped update, where: {} still resolves, system context still bypasses, and an empty table with where: {} still resolves (so the empty-table refusal measures the shape, not emptiness).

Reverse verification

Direction predicted up front: RED on the three limbs. Ablation = delete the update() dispatch site, rebuild @objectstack/objectql, prove it left dist/ (plugin-audit consumes objectql's built artifact — it is in KNOWN_UNALIASED_TEST_IMPORTS), then run.

node scripts/ablation-dist-preflight.mjs @objectstack/objectql \
'dispatchUnscopedMultiWriteHooks("beforeUpdate"' --absent
✓ marker absent from all 14 built files

Ablated run — Tests 4 failed | 39 passed, and the failures reproduce the card's measured table verbatim:

  • limb 1 → AssertionError: promise resolved "2" instead of rejecting (two rows rewritten)
  • limb 3 → expected Error: Cannot update comment c2: only its… to deeply equal ObjectContaining{…} (the per-row message)

The fourth was the where: null spelling, which is the same shape check. Every control stayed green, so the failures are the dispatch and not the harness.

Restore leg rebuilt too, and proven present (✓ marker present in 4 built files) before re-reading green.

Caller sweep — behaviour change, so the blast radius was surveyed

Structural, not a line grep: brace-matched the enclosing object literal around every multi: true and resolved the enclosing call name, so a comment describing a call is not counted as one. Shorthand { where, multi: true } and spread options are treated as scoped.

node sweep.mjs # 4663 source files walked
  • 22 unscoped multi: true update call sites — 21 in test files, 1 is prose inside a string literal (packages/spec/scripts/liveness/proof-registry.mts:287).
  • 0 in examples/, 0 in the dogfood apps. The showcase's only bulk write is a delete_record flow node carrying filter: { status: 'closed' }, and the app already pins "never declares multi: true without a bounding filter".
  • 0 in packages/ non-test source.

Positive controls, so the zero is evidence rather than a failed search: the same regex returns 22 hits against the file I authored, and the same run classified 150scoped multi-update call sites — a non-empty complement proving the classifier discriminates instead of returning empty for everything.

Decisive narrowing: the dispatch reaches only registrations that declare the flag, and exactly one object declares it on beforeUpdate (sys_comment). There is no programmatic sys_comment bulk write anywhere in repo source. Nothing in this repo newly refuses, so nothing needed scoping in this PR.

Checks run locally, with their real results

Gate set derived from the actual change set via node scripts/pm/dispatch-gates.mjs (no paths passed — the script reads the merge base itself), then run at 81c380d51:

checkresult
@objectstack/objectql test221 files, 3921 passed
@objectstack/plugin-audit test18 files, 300 passed
@objectstack/service-storage test24 files, 362 passed
objectql + plugin-audit typecheckpass (service-storage defines no typecheck script — its DTS build is the equivalent, and passes)
check:engine-double-contractOK — 325 pinned, no new double
check:where-matcherOK — "baseline key set verified against 4a7b360: no files added" (the renamed test file adds no key)
check:type-check-debt (--re-measure)OK — 33 entries, "none above its recorded number"; no ledger raised
check:type-check-coverage, check:query-options-erasureOK
check:slot-lookup, check:durability-log-level, check:stack-collection-mapsOK
check:test-source-alias, check:type-source-resolution, check:cross-package-test-inputsOK
check:changeset-gate-self-tests, check:objectui-changeset, check-empty-changeset, check-changeset-no-major, check-adr-0087-registrationOK
check:engine-split-ratio, check:nul-bytesOK
check:i18nfirst run exit 1 — prerequisite not met, "Nothing was checked" (CLI unbuilt in a fresh worktree). Built @objectstack/cli, re-ran: OK, 9 packages in sync

The check:i18n line is recorded as it happened rather than as a green, because a prerequisite failure is NOT MEASURED, not a pass.

Out-of-scope finding

Filed as #10091, unassigned, not fixed here: sys_attachment has no beforeUpdate guard at all — insert and delete are gated, update is not, while the sys_comment kit derived from it gates all three. Unrelated to this card's mechanism (that object declares the flag on delete only, deliberately), so it gets its own card rather than widening this PR.

Scope

No new configuration surface, no opt-out flag, no public API beyond the refusal itself. Objects whose guards do not declare the flag are untouched — pinned by an object whose registration does NOT declare the flag keeps exactly today's dispatches — and today's ACCEPT.

Draft on purpose: not merging, not arming auto-merge.


Generated by Claude Code

…hape (#9974)
Extend #9719's whole-operation dispatch to `beforeUpdate`'s predicate path,
per the maintainer's option-A ruling of 2026-08-19, and generalize the flag
(`dispatchUnscopedMultiDelete` -> `dispatchUnscopedMultiWrite`) instead of
adding a sibling: it is already per-registration and per-event, so a
delete-only guard still says "delete only".
The #4630 refusal in `resolveTargetRows` now fires on the SHAPE — no id and
no `where` — on both write verbs. Previously an unscoped multi-update was
refused only when it happened to sweep a row the caller lacked rights to,
with the per-row message, and resolved silently when the caller owned every
row.
The three MEASURED-GAP pins are REPLACED with refusal assertions, not
relaxed. Objects whose guards do not declare the flag are unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM
…ur change
A caller entitled to every row loses a call that works today. Stated as a
narrowing of the accept set with the call-site fix named, not as "restoring"
a guard, so the release notes read honestly for whoever is affected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM
@github-actions

github-actionsBot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/objectql, @objectstack/plugin-audit, @objectstack/service-storage, touching 18 documentable anchor(s).

19 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json e61ee68320da9136fedbfe2609b0d21f97b587b4.

4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: ObjectQL (symbol, 61 pages)
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 21 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 e61ee68320da9136fedbfe2609b0d21f97b587b4packageMentionDocs.

Which tree this was computed on

This run read content/docs from 4c320d6f76aebe9069c246281c03ec352bd75b9c — the merge of head eb1ea4bed1c4a6c42e18c8cb401f88d275dbf95c into base e61ee68320da9136fedbfe2609b0d21f97b587b4, 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 4c320d6f76aebe9069c246281c03ec352bd75b9c && git checkout 4c320d6f76aebe9069c246281c03ec352bd75b9c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e61ee68320da9136fedbfe2609b0d21f97b587b4 eb1ea4bed1c4a6c42e18c8cb401f88d275dbf95c && git checkout -B drift-repro e61ee68320da9136fedbfe2609b0d21f97b587b4 && git merge --no-ff eb1ea4bed1c4a6c42e18c8cb401f88d275dbf95c
node scripts/docs-audit/affected-docs.mjs --json e61ee68320da9136fedbfe2609b0d21f97b587b4

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 20, 2026
@os-elonClaude

Copy link
Copy Markdown
CollaboratorAuthor

CI note — Test Core (3/3) red on attempt 1, green on attempt 2, same commit

Attempt 1 failed with @objectstack/example-showcase#test exiting 1. Attempt 2, re-run on the identical commit 81c380d51 with no code change, passed (job 96288897889, run attempt 2).

Not attributed to this change, and not on "it went away" alone — the new code path provably never executes in that suite. I instrumented the built @objectstack/objectql artifact to log every entry into the update-side whole-operation dispatch, with a positive control:

suitedispatch entries recorded
plugin-auditcomment-access-hooks.test.ts (positive control)9 — all object=sys_comment flagged=1
example-showcase, entire suite (337 tests)0

Supporting evidence: the showcase suite is green locally 10/10 (standalone ×3, under CI's TZ=UTC + NODE_OPTIONS, serial --maxWorkers=1, and ×5 repeat); the CI shard-3 turbo invocation reproduces green locally (74/74 tasks), as do shards 1+2 (85/85), so the whole local farm is green; the showcase references none of this PR's symbols (0 hits for sys_comment, sys_attachment, dispatchUnscoped*, HookTargetRebind), and only sys_comment declares the flag on beforeUpdate; and main between this branch's merge base and the base CI merged with added only .claude/skills/** and one qa-checklist JSON — no code, so CI's merge commit is code-identical to what ran green here.

The exact failing assertion is unrecoverable: GitHub truncates the attempt-1 job log to a tail that ends before the showcase task's own output, and attempt 2 replaced the job. CI's own completeness guard was green on attempt 1 (13 package(s), 4308 test(s) declared and all 4308 accounted for), which by that step's own documentation means a real assertion failure rather than a dead worker — i.e. a non-deterministic test, not lost cases.

One candidate mechanism, offered as a lead rather than a diagnosis: examples/app-showcase/test/approval-resume-relation-expand.test.ts polls the async inbox on a 5-second wall-clock deadline (deliveredInbox) and returns short on timeout, after which the caller asserts toHaveLength(1) — so on a loaded runner "slow" is reported as "wrong". I could not reproduce a failure there even at 3× CPU oversubscription, so it is not filed as a defect; it is named so the next occurrence has somewhere to start.

Method note

The local gate union for this PR came from dispatch-gates.mjs, which derives families from changed paths — and examples/app-showcase is not a changed path, so no derivation could have named it, while Test Core runs the whole monorepo regardless. For a PR that changes a published symbol other packages consume, the local check set has to include the consumers, not just the changed packages. That gap is closed for this PR by the three shard runs above.

⛔ Nothing in this round touched the three replaced pins; they remain refusal assertions.


Generated by Claude Code


Generated by Claude Code

@os-elonClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT, enqueueing

domain:engine seat, PM session session_019yDEhPBC3tcGkW9bkce1HM. Head 81c380d51, unchanged across the CI round.

The red was sent back, and came back with a cause rather than a disappearance

I returned this PR red and told it that "flake" is not a root cause. What came back is the strongest possible form of the answer, and it is worth naming because it is not the usual one:

The dev instrumented the built objectql artifact to log every entry into the update-side whole-operation dispatch, and measured 9 entries during plugin-audit's pins (all object=sys_comment, flagged=1) against 0 entries across all 337 showcase tests. That is not "it passed on re-run" — it is the changed code path never executes in the package that failed, which is a claim that survives whether or not the failure ever reproduces.

And the control caught a broken probe. The first instrumentation attempt logged 0 hits everywhere — because it patched an entry point vitest does not load (it misses the .mjs). The positive control is what exposed that; without it, a zero from a probe that could not fire would have read as proof. That is the seat's standing rule (一个不能因正确理由失败的零,不是证据) doing its job inside the dev's own work.

My lead was disproved, correctly. I suggested a duck-typed seam still spelling dispatchUnscopedMultiDelete. There is no live reference to that name anywhere in the tree — only comments and changesets. Recorded so it is not re-inherited.

Verified independently

checkresult
branch tip still 81c380d51✅ nothing was changed to obtain green
files in diff10, unchanged; 0dist/, log, or instrumentation artifacts leaked
per-file divergence vs main0 on every file (positive control: commits on other paths ≠ 0)
merge-tree conflicts0
debt ledgernot in the diff at all
every load-bearing job's own conclusionTest Core 1/3 · 2/3 · 3/3 · aggregate, Temporal Conformance, Build Core, Lint & Repo Gates, TypeScript Type Check, Dogfood ×3 + aggregate, Dogfood Verify CLI, Console Pin Freshness, Check Changeset, all three claim guards — all success

Also confirmed: the earlier rename is pre-release, not breaking. Both #9719/#9798 changesets are still unconsumed on main (.changeset/unscoped-multi-delete-refusal.md declaring @objectstack/objectql: minor), so dispatchUnscopedMultiDelete never shipped. Declining to edit another card's changeset and declaring the rename in its own was the right call — a cross-card write is the conflict class the repo's changeset rule exists to prevent.

And the ablation stands: reverting the update-side dispatch reproduces the card's table verbatim — limb 1 resolving instead of rejecting (whole table rewritten), limb 3 falling back to the per-row Cannot update comment c2: message — with dist-preflight proving both the mutation and the restore reached the artifact. The three MEASURED GAP pins are replaced with refusal assertions, not weakened.

On not filing a flaky-test card

The dev declined to file one because it could not reproduce the failure and filing would assert a cause it did not have. That was the right judgement and I am not overriding it.

But the candidate it named is worth a card under a different claim, and I have filed one — see below. The distinction matters: "this test is flaky" is unproven, while "this helper reports a timeout as a wrong value" is a structural fact readable from the source, needing no reproduction at all.

One method note carried forward

The dev's local check set came from dispatch-gates.mjs deriving families from the changed paths — and examples/app-showcase is not a changed path, so the union could not name it, while Test Core runs the whole monorepo regardless. Same class as #9873: the union is not exhaustive. For a PR that changes a symbol other packages consume, the local check set has to include the consumers, not just the changed packages.

Enqueueing.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32324427332 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Test Core (3/3) — 失败步骤: Run this shard's tests

    �[90mstdout�[2m | src/webhook-secret-at-rest.test.ts�[2m > �[22m�[2mfail-closed and re-arm, extended to headers (#7986 × #7799/#8022)�[2m > �[22m�[2mdrops the subscription rather than delivering it wi
    �[90mstdout�[2m | src/webhook-secret-at-rest.test.ts�[2m > �[22m�[2mfail-closed and re-arm, extended to headers (#7986 × #7799/#8022)�[2m > �[22m�[2mdrops the subscription rather than delivering it wi
    �[90mstdout�[2m | src/webhook-secret-at-rest.test.ts�[2m > �[22m�[2mfail-closed and re-arm, extended to headers (#7986 × #7799/#8022)�[2m > �[22m�[2mre-arms with its headers when the CryptoProvider re
    �[90mstdout�[2m | src/webhook-secret-at-rest.test.ts�[2m > �[22m�[2mfail-closed and re-arm, extended to headers (#7986 × #7799/#8022)�[2m > �[22m�[2mre-arms with its headers when the CryptoProvider re
    �[41m�[1m FAIL �[22m�[49m src/dev-plugin-security-enforcement-warning.test.ts�[2m > �[22m[#10036] the "nothing is enforced" warning must fire when SecurityPlugin.start() bailed�[2m > �[22mbail #1 (no �[41m�[1m FAIL �[22m�[49m src/dev-plugin-security-enforcement-warning.test.ts�[2m > �[22m[#10036] the "nothing is enforced" warning must fire when SecurityPlugin.start() bailed�[2m > �[22mbail #2 (eng
    �[41m�[1m FAIL �[22m�[49m src/dev-plugin-security-enforcement-warning.test.ts�[2m > �[22m[#10036] the "nothing is enforced" warning must fire when SecurityPlugin.start() bailed�[2m > �[22mdoes not fir
    

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 3 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 在其他 PR 的同类评论里搜同名测试;出现过 ⇒ flaky 实锤,开 issue 修/隔离那条测试。修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32324797163 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Test Core (3/3) — 失败步骤: Run this shard's tests

    �[90mstdout�[2m | src/webhook-secret-at-rest.test.ts�[2m > �[22m�[2mfail-closed and re-arm, extended to headers (#7986 × #7799/#8022)�[2m > �[22m�[2mdrops the subscription rather than delivering it wi
    �[90mstdout�[2m | src/webhook-secret-at-rest.test.ts�[2m > �[22m�[2mfail-closed and re-arm, extended to headers (#7986 × #7799/#8022)�[2m > �[22m�[2mdrops the subscription rather than delivering it wi
    �[90mstdout�[2m | src/webhook-secret-at-rest.test.ts�[2m > �[22m�[2mfail-closed and re-arm, extended to headers (#7986 × #7799/#8022)�[2m > �[22m�[2mre-arms with its headers when the CryptoProvider re
    �[90mstdout�[2m | src/webhook-secret-at-rest.test.ts�[2m > �[22m�[2mfail-closed and re-arm, extended to headers (#7986 × #7799/#8022)�[2m > �[22m�[2mre-arms with its headers when the CryptoProvider re
    �[41m�[1m FAIL �[22m�[49m src/dev-plugin-security-enforcement-warning.test.ts�[2m > �[22m[#10036] the "nothing is enforced" warning must fire when SecurityPlugin.start() bailed�[2m > �[22mbail #1 (no �[41m�[1m FAIL �[22m�[49m src/dev-plugin-security-enforcement-warning.test.ts�[2m > �[22m[#10036] the "nothing is enforced" warning must fire when SecurityPlugin.start() bailed�[2m > �[22mbail #2 (eng
    

历史信号:

  • ⚠️本 PR 过去 24h 已在队列失败 1 次(不含本次)。 内容未变而反复失败 ⇒ 高度怀疑 flaky 测试或与同组 PR 的语义冲突,重排不解决。
  • 过去 24h 队列共有 4 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 在其他 PR 的同类评论里搜同名测试;出现过 ⇒ flaky 实锤,开 issue 修/隔离那条测试。修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

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

Labels

documentationImprovements or additions to documentationsize/xlteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decision: should an unscoped multi: true UPDATE get the whole-operation dispatch that #9719 gave DELETE? (the update half split out of #9798)

2 participants

@os-elon@claude