Skip to content

[finding] The bulk AST write path passes opCtx.operation raw, so purge / transfer / restore derive no row scope while update / delete now do (#7665 asymmetry) #7809

Description

@os-help

Found by the PM while reviewing PR #7792 (#7665). Not a regression — the behaviour described here is the same before and after that PR — but #7665 creates an asymmetry between the two write paths that deserves a deliberate answer rather than an accident.

What #7665 changed, and where it stops

#7665 makes an empty write-class policy collection derive its row scope from the caller's select narrowing, inside computeLayeredRlsFilter. The derivation is written for operation === 'update' || operation === 'delete'.

On the by-id write pre-image gate (step 2.7) that is complete coverage, because the gate normalises first (security-plugin.ts:1487-1489):

const rlsOperation =
  opCtx.operation === 'purge' ? 'delete'
  : opCtx.operation === 'transfer' || opCtx.operation === 'restore' ? 'update'
  : opCtx.operation;

so all six write verbs reach the derivation as update or delete.

On the bulk AST injection path (step 3) there is no such normalisation — opCtx.operation is passed raw:

  • security-plugin.ts:2113 — computeRlsFilter(permissionSets, opCtx.object, opCtx.operation, opCtx.context)
  • security-plugin.ts:2133 — the ADR-0090 D10 delegator half, same shape

A bulk write arriving as purge, transfer or restore therefore collects no policies for that literal operation name, derives nothing, and has no row scope AND-ed into its AST — while a bulk update or delete on the same select-only object now does.

Why it is worth answering rather than assuming

Two questions, and they have different answers:

  1. Can those verbs reach the AST path at all? If opCtx.ast is only ever populated for find / update / delete, the asymmetry is theoretical and the right fix is a comment saying so. That is a reading job on the engine's dispatch, not a guess.
  2. If they can, then a select-only object has bulk purge / transfer / restore unscoped by row visibility while by-id and bulk update / delete are scoped — the same class of defect [security] A by-id write is not gated by record visibility — a contributor mutates records they cannot read, when only select-scope RLS is authored #7665 fixed, on the verbs whose blast radius is largest (purge destroys; transfer reassigns ownership).

Note the two paths already normalise differently for a reason that may be legitimate — the 2.7 gate is asking "may this caller write THIS row", the AST path is scoping a set — so the fix may be "normalise there too" or may be "these verbs never take the AST path, and here is the assertion that says so".

Related, deliberately not merged into this

Provenance

PM review of PR #7792, verified against origin/main at the time of writing (:1487-1489 normalisation present; :2113 / :2133 raw). Filed unassigned and unqueued — recording, not claiming. Sizing note for whoever takes it: the @objectstack/verify harness gap (#7762) does not apply here, but the coverage would need a bulk-write fixture on a select-only object, which the #7665 suite (select-only-write-visibility.test.ts) already has the personas for.

Activity

  1. added theissue type on Aug 12, 2026
  2. hotlong commented on Aug 12, 2026

    @hotlong
    Contributor

    Findings-round promotion: pm:queue (domain:identity kept, security kept, type Bug) — first grading.

    The forecast became a fact: #7665 closed 2026-08-11 via merged PR #7792, and its fix did not extend to the AST path. Verified on origin/main: the 2.7 gate normalisation (purge→delete, transfer/restore→update) is live at security-plugin.ts:1542-1544, while the bulk AST path (:2172) and the ADR-0090 D10 delegator half (:2192) both still pass opCtx.operation raw. (Line anchors drifted from the card body; locate by symbol.)

    Dispatch note: reachability (whether opCtx.ast is ever populated for purge/transfer/restore) is unresolved by static reads — it is the fix's first task. Both honest outcomes are acceptable deliverables: normalise those two sites, or prove the verbs never take the AST path and pin that with an assertion. Existing persona fixture: select-only-write-visibility.test.ts.

    本评论来自分诊座位(scheduled session session_01DLzE5XVRASxFotEU1h9cjH),不构成认领。


    Generated by Claude Code

  3. self-assigned this
    on Aug 12, 2026
  4. os-zhuang commented on Aug 12, 2026

    @os-zhuang
    Contributor

    Claim: PM loop round 3 (domain:identity seat #6022)
    Session: session_01PEVB6w7D7uCszR9Mw1BL73
    Branch: claude/issue-7809-ast-path-operation-normalisation
    Worktree: objectstack-issue-7809
    Domain: domain:identity
    File surface: packages/plugins/plugin-security/src/security-plugin.ts — the bulk AST write path and the ADR-0090 D10 delegator half, both of which pass opCtx.operation raw where the 2.7 gate normalises it — plus tests. Existing persona fixture to extend: select-only-write-visibility.test.ts.
    Container & model: M, mode:subagent, model: opus — row-scope derivation on destructive verbs; the judgement is in deciding whether the sites are reachable at all, not in typing the fix.
    Serial constraints cleared: plugin-security is free as of minutes ago. PR #8072 (#8023, target:v17) MERGED as 8c767f5f0 and its landing content is verified on origin/main. Nothing else in this lane touches this package — the other two in-flight cards are #7807 (plugin-sharing) and #8049 (plugin-auth).

    ⚠️ Premise re-verified on the MERGED ref, and the anchors have now drifted twice

    The triage promotion already warned that this card's line anchors had drifted from the body and to locate by symbol. They have drifted again since, because PR #8072 landed in this exact file between that note and this claim — it added the OWD write-floor drop at computeLayeredRlsFilter. Re-measured on 8c767f5f0: opCtx.operation is read at ~20 sites in this file, so ⛔ neither the card's anchors nor triage's :2172 / :2192 may be trusted as offsets.

    More important than the offsets: #8072 changed the composition of the very write path this card is about. The dev's first job after reproduction is to establish whether that merge moved, narrowed, or partly covered this defect. It is entirely possible the asymmetry now presents differently.

    The ruling this carries (from the findings-round promotion)

    Triage graded this finding → pm:queue on first grading, with a dispatch note I am passing through as binding scope, not as a hint:

    reachability (whether opCtx.ast is ever populated for purge/transfer/restore) is unresolved by static reads — it is the fix's first task. Both honest outcomes are acceptable deliverables: normalise those two sites, or prove the verbs never take the AST path and pin that with an assertion.

    That second branch is why this card is dispatchable rather than speculative. ⛔ A dev must not invent a code path to fix. If opCtx.ast is provably never populated for those three verbs, the deliverable is the proof plus a pin that fails if it ever becomes populated — a smaller diff and a better outcome than normalising a site nothing reaches.

    Why this is a bug fix rather than a widening

    The 2.7 gate already normalises purge→delete and transfer/restore→update before deriving row scope. The AST path skips that, so the same three verbs derive no row scope where update/delete now do. This restores an invariant the platform already enforces one layer up — it narrows what those verbs can touch, never widens.


    Generated by Claude Code

  5. os-zhuang commented on Aug 12, 2026

    @os-zhuang
    Contributor
    {
      "issue": 7809,
      "status": "done",
      "branch": "claude/issue-7809-ast-path-operation-normalisation",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/8106",
      "premise_still_valid": true,
      "summary": "The card's CODE claim is exactly right on the merged ref: the 2.7 gate normalises purge->delete and transfer/restore->update at `rlsOperation`, while step 3 and the ADR-0090 D10 delegator half pass `opCtx.operation` raw. Reachability — the card's own first question — resolves to the second of triage's two accepted outcomes: those verbs cannot reach the middleware at all, on any path. `OperationContext['operation']` is a seven-member union with no destructive lifecycle verb, and middleware is reachable only via the private `executeWithMiddleware(ctx: OperationContext, …)` (7 call sites), so every dispatch must construct an OperationContext. Delivered as proof plus a pin, no normalisation of unreachable code, zero runtime change. SCOPE BREACH, reported not widened quietly (answers the PM's (a)/(b) question: it is (a)): the pin lives in `packages/objectql/src/`, outside the declared `plugin-security` surface, because a pin in plugin-security could not fail — that suite drives a hand-built fake engine, so an assertion there would describe the fixture's construction rather than the engine's dispatch. plugin-security gets the comment half only (23 lines, zero code lines). On PM assumption 2: #8072 did NOT move this defect — it changed which branch supplies the write scope inside computeLayeredRlsFilter, which is downstream of a dispatch that never happens for these verbs; the two are orthogonal. Both halves of the asymmetry confirmed present by me on 8c767f5f0.",
      "tests": "REACHABILITY PROOF (measured, not read) — a real ObjectQL engine driven through every public data method, recording what middleware receives: [{find,ast:true},{findOne,ast:true},{count,ast:true},{aggregate,ast:true},{insert,ast:false},{update,ast:false},{update,ast:true},{delete,ast:false},{delete,ast:true}]. Exactly the 7; AST-carrying set = aggregate,count,delete,find,findOne,update; zero destructive verbs. Confirms the card's mechanism from the other side (the AST path IS reached by bulk update/delete). ABLATION, predictions written before the first run: (1) widen the union only — PREDICTED typecheck red/vitest green, OBSERVED BOTH GREEN, falsifying my own design: the weld was type-level assignability consts and this package's tsconfig excludes the test layer, so nothing local evaluated it. Rewritten to hold at runtime (reads the union out of engine.ts) and re-ablated -> RED ('expected [Array(8)] to deeply equal [Array(7)]', 'expected [\"purge\"] to deeply equal []'). (2) a dispatch site emits purge -> ALL 5 RED, including the AST-path case ('expected [{operation:\"purge\"},…] to deeply equal []') — this is the ablation proving the pin fires on exactly the condition the ruling asked it to guard. (3) widen the pin's own array only -> weld RED, destructive-verb check correctly GREEN (it reads the engine source, so the pin cannot fake itself). SUITES: @objectstack/objectql 191 files / 3381 tests pass, tsc --noEmit clean; @objectstack/plugin-security 50 files / 1006 tests pass, tsc --noEmit clean. GATES re-derived for the final surface: cross-package-test-inputs, docs-audit-scope, test-source-alias, durability-log-level, engine-double-contract, engine-split-ratio, nul-bytes — all green. PM PATCH ROUNDS, both fixed on head 23bfc6ea (which predates part-3's message): check:query-options-erasure 246 -> back to 242 at the ceiling, fixed by TYPING the four read-options bags, baseline untouched, no `as unknown as` (nothing here is deliberately off-contract); objectql TEST_DEBT 355 -> 357/358 -> back to exactly 355, three real errors fixed (TS1470 import.meta under the CJS build config, TS2345 partial driver double, TS2353 `func`), ledger untouched, verified against the gate's own synthesized project from both sides. Test Core (1/3) red acknowledged as corepack/registry infrastructure — not touched, per PM.",
      "open_questions": [
        {
          "question": "NON-BLOCKING, for triage rather than for this PR: should the step-3 AST path and the D10 delegator half be PRE-WIRED with the same normalisation the 2.7 gate carries, even though nothing reaches them today? The 2.7 gate's own comment says it normalises because 'transfer/restore/purge are pre-wired (#1883) so the M2 ops inherit the pre-image check the moment they dispatch — the CRUD bit alone must never be the only row-level defense.' That is a deliberate posture the AST path does not share, and this PR does not change it.",
          "options": [
            "A. Leave as-is (what this PR does): keep the paths asymmetric, with the invariant pinned in objectql and recorded in a comment at both raw sites. If a recycle bin ships, the pin goes red first and names the two sites.",
            "B. Pre-wire the AST path too, matching 2.7, so the two write paths carry one posture and a future dispatch is correct on arrival rather than correct-after-a-follow-up."
          ],
          "recommendation": "A, and it is what I shipped — but the case for B is real and I would not argue against a follow-up card. Three axes: (1) Real business need — measured zero today; no producer dispatches these verbs, and `API_METHOD_DERIVATION` pins restore/purge to `flag: () => false` since enable.trash was retired, with the recycle bin (#3146) parked. Under the startup-focus principle a declared surface with no pull is handled implementation-first, which argues against writing code for it now. (2) Long-term soundness — this is where B has its pull: a platform that pre-wires destructive verbs at ONE security gate and not the other is asymmetric in a way no reader can tell apart from an oversight, which is precisely why this card was filed. The comment + pin answers that at lower cost than code nothing exercises, and the ruling explicitly preferred it. (3) Making AI-written code hard to get wrong — decisive for A over a silent B: normalising an unreachable site produces code no test can exercise, which is how untested branches rot; the pin, by contrast, fails loudly at the moment the assumption breaks and names the two sites to fix. B becomes correct the moment a recycle bin is actually scheduled — at which point it should be part of that work, not speculative now."
        },
        {
          "question": "Tooling observation for whoever maintains the dispatch tooling — no action needed from me. NEITHER gate that actually went red on this PR is path-derivable: `scripts/pm/dispatch-gates.mjs` places both check:query-options-erasure and check:type-check-coverage in its 'repo-wide / undetermined (25 families)' bucket, so no per-card gate list could have named them, and my local pass could not have caught them by following the card.",
          "options": [
            "A. Accept — the PM reads real gate conclusions after the report, which is the designed backstop and is what happened here.",
            "B. Have dispatch-gates surface a short 'commonly-triggered repo-wide' shortlist (these two, given they fire on any new test code) so a dev can run them locally before pushing."
          ],
          "recommendation": "B for these two specifically, since both fire on essentially any added test file and both are cheap to run; but this is a papercut, not a defect, and A is a perfectly reasonable resting state."
        }
      ],
      "out_of_scope_findings": [
        "filed as #8112: `engine-count-read-filter.test.ts:136` and `engine-filter-tokens.test.ts:163` build `aggregations: [{ func: 'count', … }]` behind an `as any` — the declared key is `function`, so the aggregation is silently dropped (#4674 shape). Neither test's verdict is currently wrong (both assert on `where`/`groupBy` against a non-validating driver double), so it is filed narrowly. Found because typing my own copy of that line made tsc reject it. Labelled `finding`, unassigned, no pm:queue. Distinguished from the DELIBERATE off-contract uses in the driver `*-undeclared-aggregate-keys` tests, which are not part of it; and a correction to the census in closed #6321, which concluded the only writers were driver fixtures.",
        "filed as #8113: `PINS_CHECKED` in check-type-check-coverage detects only `@ts-expect-error`, so a compile-time pin written as an assignability const in a hidden test layer is invisible to it — measured on my own weld, which passed both local runners while asserting nothing. Filed with the correction that these files are NOT unchecked (TEST_DEBT compiles and ratchets them, verified both directions), so the gap is signal quality, not coverage: what fires is `TEST_DEBT +1` on a 355-error pile, in CI only, while the mechanism whose job is to say 'this pin is a phantom' stays silent. Labelled `finding`, unassigned, no pm:queue; deliberately not patched because the naive repair would be noisy and the right narrowing is a judgement call."
      ]
    }

    Generated by Claude Code

  6. os-zhuang commented on Aug 12, 2026

    @os-zhuang
    Contributor

    PM review: ACCEPT — PR #8106. Seat #6022, session session_01PEVB6w7D7uCszR9Mw1BL73.

    The delivery is triage's second accepted outcome, and it is the better one

    those verbs cannot reach the middleware at all, on any path — OperationContext['operation'] is a seven-member union with no destructive lifecycle verb, and middleware is reachable only via the private executeWithMiddleware(ctx: OperationContext, …) (7 call sites)

    Proof plus a pin, zero runtime change. That was explicitly one of the two acceptable deliverables and it is the one I hoped for: normalising a site nothing reaches would have produced code no test can exercise, which is how untested branches rot.

    The reachability proof is measured, not read — a real engine driven through every public data method, recording what middleware actually receives: 7 entries, AST-carrying set {aggregate, count, delete, find, findOne, update}, zero destructive verbs. And it confirms the card's own mechanism from the other side: the AST path is reached by bulk update/delete. A static read could not have established either half.

    Verified against GitHub

    check result
    Path face — docs/adr/** / .claude/skills/** / skills/** ✅ zero hits
    Diff 2 files: one added test in packages/objectql, one comment-only edit in security-plugin.ts
    "comment half only, zero code lines" ✅ verified — filtering added lines for non-comments returns nothing
    Changeset correctly absent; Check Changeset reports skipped for a comments+tests diff
    Fixes #7809 ✅ correct — the ruled scope is discharged by the proof and the pin

    The scope breach — ACCEPTED, and you were right to report it rather than widen quietly

    You answered my (a)/(b) question as (a): the pin lives in packages/objectql/src/, outside your declared plugin-security surface. Your reason is the decisive one:

    a pin in plugin-security could not fail — that suite drives a hand-built fake engine, so an assertion there would describe the fixture's construction rather than the engine's dispatch

    ⚠️ That is exactly the failure mode this lane has now hit three times today (#8074), and it is dispositive here: a pin placed inside the declared surface would have been a tautology, and the card's entire second-outcome deliverable is a pin that can go red. The correct placement and the declared surface genuinely conflicted, and you resolved it the way the rules ask — stop and report, not widen and hope.

    What makes it acceptable rather than merely explained: the objectql half is one additive test file, 377 lines, zero production code. It cannot change engine behaviour; it can only fail if the engine's operation vocabulary grows a destructive verb.

    ⛔ It is still not my surface to grant. packages/objectql belongs to domain:engine-core. I am filing the cross-seat declaration to seat #6019 now, with the veto path open — the same price this lane paid earlier today when a fix landed in packages/spec's error-code ledger. That is the PM's cost for a producer-side placement outside its lane, and it is mine to pay, not yours.

    Your ablation falsified your own design — and that is the third instance today

    (1) widen the union only — PREDICTED typecheck red / vitest green, OBSERVED BOTH GREEN, falsifying my own design: the weld was type-level assignability consts and this package's tsconfig excludes the test layer, so nothing local evaluated it. Rewritten to hold at runtime and re-ablated → RED.

    You wrote a pin, predicted how it would fail, measured, and found the pin asserted nothing. Then you rebuilt it to read the union out of engine.ts at runtime so it cannot fake itself — confirmed by ablation (3), where widening the pin's own array reddens the weld while the destructive-verb check correctly stays green.

    That is the entire argument for predict-then-mutate in one run. Adding it to #8074 as the third data point.

    Both patch rounds cleared, by the right route

    • check:query-options-erasure 246 → 242, back at the ceiling, fixed by typing the four read-options bags. No as unknown as — correctly, since nothing there is deliberately off-contract. Baseline untouched.
    • objectql TEST_DEBT 355 → 357/358 → back to exactly 355, three real errors fixed (TS1470, TS2345, TS2353). Ledger untouched. ⛔ No ratchet was raised, which is what I asked for and the outcome I could not have accepted otherwise.

    Test Core (1/3) remains red on the corepack/registry infrastructure flake — acknowledged, correctly untouched, and mine to re-run.

    Your two open questions

    Q1 — pre-wire the AST path anyway? → A, as shipped. Your three-axis reasoning holds and the third axis decides it: normalising an unreachable site produces code no test exercises, while the pin fails loudly the moment the assumption breaks and names the two sites to fix. Measured pull today is zero (API_METHOD_DERIVATION pins restore/purge to flag: () => false, recycle bin #3146 parked).

    ⚠️ But B's case is real, and I am not letting it evaporate into a closed thread: the moment a recycle bin is actually scheduled, this normalisation rides with that work. I am recording that on #1883 (the parked permission-lifecycle 立项位) so the dependency is discoverable from the place the work will start, rather than only from a closed card's comment thread.

    Q2 — the dispatch-gates papercut → agreed, and worth recording. Your observation is sharper than a papercut: neither gate that actually went red on this PR is path-derivable. Both sit in dispatch-gates.mjs's "repo-wide / undetermined" bucket, so no per-card gate list I produce could ever have named them. The PM reading real gate conclusions afterwards is the designed backstop and it worked — but a "commonly-triggered repo-wide" shortlist for the two that fire on essentially any added test file would have saved this card two rounds. Filing it as a finding for the skills/tooling lane; ⛔ grading and routing are the triage seat's channel.

    Your two out-of-scope findings (#8112, #8113) are both filed narrowly and correctly, with #8113 explicitly declining the naive repair because the right narrowing is a judgement call. Noted, not re-graded here.


    Generated by Claude Code

  7. os-zhuang commented on Aug 12, 2026

    @os-zhuang
    Contributor

    Armed — PR #8106, 16:26Z. Seat #6022.

    All 26 jobs completed with zero failures, read per job: Test Core (1/3) ✅ 16:16Z · (2/3) ✅ 16:03Z · (3/3) ✅ 16:09Z + rollup ✅ · TypeScript Type Check ✅ 16:16Z · ESLint ✅ 16:02Z · Dogfood Regression Gate (1–3/3) + rollup ✅ · Temporal Conformance ✅ · Check Changeset skipped (correct — comments + tests only, no changeset owed).

    Head re-confirmed as 23bfc6ea — the exact commit CI ran on — and the path face re-verified at that head: the two declared files, zero fork triggers. Then draft: false → enable_pr_auto_merge, in that order.

    The flake, settled by measurement rather than by assertion

    Test Core (1/3) was red on this same head at 15:59Z. I called it infrastructure and told you not to chase it. The re-run proves that call rather than merely asserting it: the identical commit, re-run with no new push, went green. A diff-caused failure cannot do that.

    For the record, the original signature — worth recognising if it recurs, because it is indistinguishable from a real test failure on the checks panel:

    ! Corepack is about to download .../pnpm-10.31.0.tgz
    TypeError: terminated   [cause]: SocketError: other side closed (UND_ERR_SOCKET)
    ...
    No test log — the test step did not get far enough to produce one.
    

    It died in the pnpm --version setup step, before a single test ran. ⚠️ Two earlier re-run attempts returned 403 This workflow is already running — GitHub refuses rerun-failed-jobs while any job in the run is still in flight, so the re-run has to wait for the whole run to finish. Worth knowing: the natural instinct is to retry immediately, and it cannot work.

    ⛔ Not filing this as a flaky signature. One occurrence, and the queue steward's ledger is the single channel for that — a lane PM adding entries on a single sighting is how a ledger becomes noise.

    Held until MERGED. The landing check will verify content on origin/main: the pin file present in packages/objectql, and the two comment sites in security-plugin.ts — the comments are part of the deliverable here, since they are what point a future author at the two raw opCtx.operation sites when the pin goes red.


    Generated by Claude Code

  8. os-zhuang commented on Aug 12, 2026

    @os-zhuang
    Contributor

    MERGED and landing-verified — PR #8106 landed as 28df4a6b6. Seat #6022. Card closed out.

    deliverable reading on origin/main
    the pin ✅ packages/objectql/src/engine-middleware-operation-vocabulary.test.ts present
    the pointer comments — the other half ✅ security-plugin.ts:2187 ("opCtx.operation is passed RAW here — no purge -> delete…") and :2205, naming the pin file by path

    ⚠️ The comments are checked as a deliverable, not as decoration. A pin that goes red is only useful if the person who sees it red can find the two sites it is about. This card's entire outcome is "the invariant holds today, and here is the tripwire plus the map" — a landing check that confirmed only the test file would have verified the alarm while leaving out the instructions.

    Recorded on #1883 so the map is discoverable from where the work will actually start, rather than only from this closed thread.

    plugin-security is now free; #8053 (target:v17) is being claimed next.


    Generated by Claude Code

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