Skip to content

perf(plugin-security): batch capability + overlay boot seeds, skip no-op writes - #11470

Merged
os-sam merged 7 commits into
mainfrom
claude/issue-11096-batch-capability-and-overlay-boot-seeds
Aug 23, 2026
Merged

perf(plugin-security): batch capability + overlay boot seeds, skip no-op writes#11470
os-sam merged 7 commits into
mainfrom
claude/issue-11096-batch-capability-and-overlay-boot-seeds

Conversation

@os-sam

Copy link
Copy Markdown
Collaborator

Fixes#11096
Fixes#11097

What

Both bootstrapDeclaredCapabilities and reconcilePermissionSetProjection were
read-then-write reconcilers over a set known in full before their loop started —
the exact shape #10946 removed from the permission-set and position seeders next
door. Each now:

  1. pays ONE batched { name: { $in: [...] } } existence read for the whole
    set (buildExistingByName, from Boot seeds permission sets and positions one at a time: 4 sequential DB round trips each, 2 of them an unconditional UPDATE #10946), instead of one SELECT per item;
  2. skips the UPDATE when the stored row already matches what a re-seed
    would write (an equality test, not a presence test) — the write only fires
    on real drift.

bootstrapDeclaredCapabilities gets its own capabilityRecordDiffers (covering
exactly the columns it writes: label/description/scope).
permission-set-projection.ts reuses the existing recordDiffersFromBody for
facets, plus a new customizedDiffers term — customized is provenance, not
definition, so recordDiffersFromBody deliberately excludes it and it needs its
own comparison against the same managed_by:'package' rule the write uses.
projectPermissionMutation's in-memory evaluator-registry sync is now gated on
seeded + updated + unchanged > 0, not just seeded + updated > 0 — that sync
isn't a database round trip, and gating it on "a write happened" would leave a
steady-state boot enforcing a stale registry entry.

⚠️ One resulting-state change, required by the write-skip itself — not a rider

upsertEnvPermissionSet's INSERT now stamps customized: false unconditionally
(previously !!customized, the caller's raw opinion). This is a behaviour
change beyond the write count, and it is called out here deliberately per
the dispatch instructions for this card.

Why it's in this PR rather than filed separately (it was filed separately
first — #11460 — then closed as superseded once this
was measured): the UPDATE branch's rule (existing.managed_by === 'package' ? !!customized : false) already forces customized: false on every subsequent
call for a non-package row. If the INSERT stamps !!customized instead, a
fresh managed_by:'admin' row is born disagreeing with its own update rule.
Before this PR, every boot rewrote every record unconditionally, so that
disagreement self-corrected invisibly on the very next boot. Once writes are
equality-gated — the whole point of this PR — that disagreement stops being
invisible: it becomes a REAL, PERMANENT one-boot-late corrective UPDATE
after every such creation, forever, not a one-time historical artifact.
Measured, not asserted: reverting this one line back to !!customized
fails #11097 — env overlay reconciliation: round trips > does not grow the steady-state round-trip count and #11097 — drift STILL reconciles > only the DRIFTED overlay is written (both start seeing a real UPDATE on the boot
immediately after any overlay-backed admin row is created).

Net effect: today, a brand-new environment-authored permission set with no
package baseline can be observed customized: true for the one boot between
its creation and the next reconciliation pass. After this PR it is never
observed true. Small, narrow, and in the "more correct" direction — but a
real resulting-state change, so this PR is not a pure round-trip/write-
count fix and is labelled needs:contract-review for that reason.

Tests

New bootstrap-seed-round-trips.test.ts (+ additions to
permission-set-projection.test.ts) pin, against a counting fake driver:

  • batched read: expect(ql.calls.find).toBe(1) with the exact $in where
    clause, on a steady-state rebuild;
  • no-op skip: steady-state round trips flat at [1,1,1,1] for 1/5/20/40
    items, updated: 0;
  • drift still reconciles (the counter-direction, load-bearing pin): a
    changed label/description/scope/facet/customized flag still gets its
    UPDATE, and the healed value is asserted. Ablated on this branch (mutate →
    confirm-on-disk via git hash-object → run → restore byte-identically,
    trap … EXIT INT TERM): forcing capabilityRecordDiffers to always false
    fails 4 tests; forcing the permission-set write-skip condition to always
    take the "unchanged" branch fails 18 tests. Both restores verified
    byte-identical to the pre-mutation hash.
  • a read that cannot answer is not "none exist": a throwing/non-array read
    falls back to the per-item read and never blind-inserts;
  • a name declared twice in one batch still gets its loud skippedForeign
    refusal (the oracle's remember() write-back).

bootstrap-seed-round-trips.test.ts's own purpose, per its header, is a
database-round-trip count, not value/identity round-tripping — it does not
assert row-id stability. That's separately pinned in the pre-existing
permission-set-projection.test.ts (row.id === 'ps_env' / 'ps_pkg', "id
stable — junction FKs stay valid"), unmodified by this PR; the round-trip
file's own drift assertions additionally re-read the specific named row's
content after each update, which transitively catches a wrong-id write (the
fake driver's update dispatches strictly by id via assertEngineUpdateDispatch).

Clause-② (accept/reject surface) — YES, not the dispatch's initial "no"

Judged against the dispatch instructions' own test ("does the resulting
projected state differ from today in any case — not just the number of writes
taken to reach it?"): yes, for the reason above. Labelled
needs:contract-review; not flipped ready, not enqueued, auto-merge not
armed — left for maintainer review per standing policy for this grading.

Generated by Claude Code


Generated by Claude Code

…ehaviour
upsertEnvPermissionSet's INSERT was changed to always stamp
customized:false, replacing main's !!customized. That's a real,
if narrow, provenance-flag correctness fix (a brand-new admin-owned
row can otherwise be born badged "customized" for one boot cycle) —
but it's a different defect class from #11096/#11097's round-trip
and no-op-write scope, it changes the resulting projected state
(not just the write count), and no test in this diff pins it.
Revert to keep this PR a strictly behaviour-preserving perf fix, and
update the changeset to match. The adjacent bug is being filed as a
separate, unassigned issue.
…uired, not a rider
Correction to the previous commit on this branch: reverting
upsertEnvPermissionSet's INSERT-path customized fix back to main's
!!customized turned out to be wrong, confirmed by an actual test run.
The UPDATE branch's rule (`managed_by === 'package' ? !!customized :
false`) already forces `customized: false` on every subsequent call
for a non-package row. Stamping the caller's raw opinion on INSERT
instead means a fresh managed_by:'admin' row disagrees with its own
update rule the moment it's created. Before #11096/#11097 every boot
rewrote every record unconditionally, so this was invisible. Once
writes are equality-gated, that disagreement is measured as real
drift: `does not grow the steady-state round-trip count` and `only
the DRIFTED overlay is written` both failed with `!!customized` on
insert — a real corrective UPDATE recurs one boot after every
overlay-backed admin row is created, forever, not once historically.
So this insert-side fix is required for the round-trip claim to hold
on this path; it isn't separable from it. It IS a resulting-state
change beyond a pure write-count reduction (a freshly created record
is never transiently observed customized:true), called out explicitly
in the changeset per that distinction. Verified: all 122 tests in
bootstrap-declared-capabilities.test.ts, bootstrap-seed-round-trips.
test.ts and permission-set-projection.test.ts pass with the fix
restored; 3 fail without it.
@github-actions

github-actionsBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-security, touching 10 documentable anchor(s).

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

  • content/docs/api/error-catalog.mdx(via sys_permission_set (literal))
  • content/docs/permissions/authorization.mdx(via bootstrapDeclaredCapabilities (symbol), sys_capability (literal), sys_permission_set (literal))
  • content/docs/permissions/capabilities.mdx(via bootstrapDeclaredCapabilities (symbol), sys_capability (literal))
  • content/docs/permissions/delegated-administration.mdx(via sys_permission_set (literal))
  • content/docs/permissions/permission-sets.mdx(via sys_capability (literal), sys_permission_set (literal))

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

  • content/docs/releases/v12.mdx(via sys_permission_set (literal))
  • content/docs/releases/v13.mdx(via sys_permission_set (literal))
  • content/docs/releases/v15.mdx(via sys_capability (literal), sys_permission_set (literal))
  • content/docs/releases/v17.mdx(via sys_capability (literal), sys_permission_set (literal))

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
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 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 — 14 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 f31ea1c4f1b48c4ebeb8b132d3d71cb232c23f18packageMentionDocs.

Which tree this was computed on

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

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

check:type-check-debt (the TEST_DEBT ratchet, tsc run over the
package's test-inclusive program) reported plugin-security going
11 -> 12 (+1). Root cause: declaredCaps's return type was inferred
from its body, narrowing scope to the literal 'platform' (from
`scope: 'platform' as const`). The "declared SCOPE changed" drift
fixture then does `upgraded[1] = { ...upgraded[1], scope: 'org' as
const }`, assigning a 'org'-scoped object into an array slot typed
to only ever hold 'platform' -- TS2322.
Same shape DeclaredSet already solves one property over for
permission sets (see its docstring): give the generator function an
explicit return type wide enough for the upgrade fixtures to widen
into, rather than letting scope infer down to a literal. Added
`interface DeclaredCapability` with `scope: 'platform' | 'org'` and
annotated declaredCaps's return type; dropped the now-redundant
`as const` on the steady-state value.
Reproduced with the exact generated project
scripts/check-type-check-coverage.mjs's --re-measure builds for a
TEST_DEBT package (extends the package's own tsconfig, drops the
**/*.test.ts exclude, adds the same typeRoots) -- 12 errors before
this commit, 11 after, matching the frozen TEST_DEBT entry exactly.
`pnpm --filter @objectstack/plugin-security typecheck` cannot see
this class of error at all: the package is in TEST_DEBT precisely
because its own typecheck script excludes its tests from tsc.
@os-sam
os-sam marked this pull request as ready for review August 23, 2026 22:34
@os-sam
os-sam added this pull request to the merge queueAug 23, 2026
Merged via the queue into main with commit c6c895cAug 23, 2026
38 checks passed
@os-sam
os-sam deleted the claude/issue-11096-batch-capability-and-overlay-boot-seeds branch August 23, 2026 22:48
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

2 participants

@os-sam@claude