Skip to content

perf(security): batch the curated capability existence read and equality-gate the reconcile - #11537

Merged
os-sam merged 1 commit into
mainfrom
claude/issue-11451-batch-bootstrap-system-capabilities
Aug 24, 2026
Merged

perf(security): batch the curated capability existence read and equality-gate the reconcile#11537
os-sam merged 1 commit into
mainfrom
claude/issue-11451-batch-bootstrap-system-capabilities

Conversation

@os-sam

Copy link
Copy Markdown
Collaborator

Part of #11451

bootstrapSystemCapabilities built its definition set in memory and then issued a per-item SELECT … WHERE name = ? LIMIT 1 for each one, followed by an UPDATE that fired whether or not label/description differed. The curated half's read is now ONE batched $in; the reconcile is equality-gated for both halves.

The derived half's existence read is not batched. That is a decision, not an omission, and the reason is below — filed as #11520, which is a sub-issue of #11451 and remains open, so this PR deliberately does not carry a closing keyword for the card.

The design choice the card asked to be made deliberately

Written down before the code (the reasoning is preserved in the module header of bootstrap-system-capabilities.ts, not just here).

The card's three options conflate two independent questions: how to batch a predicated read (options 1 vs 2), and which halves to batch (option 3). This PR takes option 2's mechanismseed-name-lookup.ts gains an optional equality predicate carried alongside the $in — applied to the curated half only.

Why not option 1 (batch wide, filter in memory)

Both harms are measured, not argued, against the double in bootstrap-system-capabilities.test.ts that deliberately models three shipped-driver behaviours (limit orders by id ascending per #4363, null is IS NULL, insert enforces (COALESCE(organization_id, '__global__'), name)):

  1. It returns the wrong row. With two organizations holding a curated name, an unpredicated buildExistingByName resolves that name to aaa_org_jiamanaged_by: 'admin' — not the platform's row. Reconciling it is bootstrapSystemCapabilities reconciles an arbitrary row when a curated capability name exists in more than one organization — find(..., limit: 1) has no ORDER BY, so the platform's own row can be left unseeded #8470 exactly: an organization's authored copy overwritten every boot while the platform's own row is never addressed.
  2. It truncates. The unscoped page cap is limit: names.length, and post-fix(platform-objects,plugin-security,driver-sql): scope sys_user_preference and sys_capability uniqueness per organization (#8323) #8461 a name can carry a row per organization, so 10 rows match a request for 8 names and the two highest-id rows fall off. Two curated names then read absentwhile their platform rows are present in the same fixture — and absent sends the curated half to its insert branch, where the unique key refuses the write and the seeder reports a blockedCurated collision, every boot, on a healthy install.
  3. It would not even avoid widening the shared file.ExistingByNameIndex.get() returns ONE row chosen by arrival order, so filtering on managed_by/organization_id needs an all-rows accessor — which hands every caller its own spelling of "which row is mine", the shape The Layer-0 tenant wall's strict equality annihilates the driver's platform bucket: #2734's fix is defeated on every walled read, and the org-less RBAC catalog reads ZERO for every principal #10103 repaired.

Keeping the predicate in the query makes the result a provable singleton per name, which is the property sys-capability.object.ts's own index comment names: "the seeder now scopes its curated lookup to managed_by: 'platform' + organization_id: null, i.e. exactly the bucket this key part keeps a singleton". That is what makes the page cap exact rather than merely usually-large-enough.

Why not option 3's reasoning, even though the derived half is untouched

Not because it is the smaller half — it is the half that grows. Its lookup is cross-organization by construction, and derivedRowIsOurs, skippedAuthored and #8751's platformStampedInOrg are all computed from the lowest-id row installation-wide. Narrowing it to the platform bucket answers a different question: it stops counting an organization's platform-stamped row whenever our own bucket row also exists, and it starts seeding the bucket in the case #8552 ruled must be left alone ("adopting or backfilling it was rejected"). Batching it unnarrowed needs an unbounded read. Both are decisions above this card. #11520 carries the full analysis and the options so the next attempt does not re-derive it.

Worth recording: on a stock installation the derived set is EMPTY — the platform's own permission sets grant only names already in KNOWN_CAPABILITIES, and package-declared names are excluded via materializedCapabilityNames. It grows only for an app whose defaultPermissionSets grant capability strings that nothing declares.

Fence: every existing caller of seed-name-lookup.ts is unaffected, by measurement

The widening is additive and opt-in: ...(equals ?? {}) spreads nothing when no predicate is given, so a caller that passes none emits the exact key set it emitted before — not those keys plus undefined-valued ones, which toEqual would have quietly accepted. The three pre-existing pins that assert ql.wheres[0]equals{ name: { $in: … } } (permission sets, positions, declared capabilities) are green, and a new test pins the key set explicitly (Object.keys(…)['name']) so the undefined-key loophole is closed rather than assumed. All four call sites' suites pass: 189 tests across 6 files.

The predicate also rides the per-item degradation read. A fallback that dropped it would ask a wider question than the batched read it stands in for — and for this caller, wider is not "slower but the same", it is a different row.

Fence: the #8470 narrowing is still LOAD-BEARING, not merely still present

Ablation, the way #11470 proved its customized: false flip. CURATED_LOOKUP relaxed to {}, confirmed on disk in both directions (injected form present ×1, removed predicate body ×0), restored byte-identically afterwards (git hash-object = 382ee3d6… = the HEAD blob; git status clean). The suites resolve the seeder through a relative import, so no rebuild is interposed between the edit and the run.

Predicted in writing before running (RED, more failures) and observed exactly that — 85 passed unmutated, 9 failed ablated:

  • seven named [#8470] tests, including "reconciles the platform row and only the platform row (adverse — the org row sorts FIRST)" and "is unaffected by the NUMBER of organizations holding the name";
  • #11451 … › the seeder itself is unharmed by the fixture that breaks the unpredicated read;
  • #11451 … › the batched read carries the #8470 predicate IN the query, with no other keys.

The two tests that call buildExistingByName directly use a literal in the test file rather than the seeder's constant, and stayed green — as intended, since they measure the module, not the seeder's wiring.

Fence: no round-trip pin was moved

bootstrap-seed-round-trips.test.ts gains a new #11451 describe; every pre-existing pin is untouched. The new block states the derived half's residue as 1 + derived reads rather than hiding it, so a later card that batches it moves these numbers deliberately.

⭐ Pins identity, not only counts: the rebuild is asserted to re-read and re-write the same row ids (no offsetting "one dropped, one inserted" behind a constant), and a drifted curated row's UPDATE is asserted to land on that row's id.

Behaviour changes worth review

  • CapabilitySeedResult gains unchanged and unreadable. Reporting "wrote nothing because nothing differed" separately from "wrote nothing because the writes stopped working" is what stops the round-trip count being satisfiable by an implementation that quietly stopped reconciling.
  • An unreadable database now declines instead of guessing. Hoisting a read changes what a failure means: per item a failed read fell through to an insert the unique index refused, for that one name; batched, one failure speaks for the whole set. This also retires a misdiagnosis — an unreadable database used to make this half attempt an insert per curated name and report a blockedCurated collision for each, describing a blocking row nobody ever saw.

What was NOT measured

⚠️No speedup, and no slope. The hosted bootstrap-curve.mjs rig lives in objectstack-ai/cloud and its axes are permission sets / positions / objects — not this one. Nothing here measures wall time, and a test that did would measure the machine it ran on. The only defensible claim is the round-trip count and the identity of the rows each leg touches, both pinned in-repo.

Also not measured: real-driver behaviour. Every number above comes from in-memory doubles. The truncation finding is a property of the double's modelling of limit + ORDER BY id, which its header documents as measured against SqlDriver on better-sqlite3 — I did not re-run that measurement.

Verification (all at 968f91714c, the head of this branch)

  • tsc --noEmit for @objectstack/plugin-securityTSC_EXIT=0
  • vitest run over the four buildExistingByName callers' suites plus both capability suites — Test Files 6 passed (6) · Tests 189 passed (189)
  • pnpm lint (repo-wide eslint . --no-inline-config) — exit 0, no findings. The whole farm, not a narrowed run.
  • Gate union re-derived from the merge-base changeset with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (the script confirmed the --repo assertion holds against this checkout's remote). All green, quoting each gate's own verdict line:
    • check:engine-double-contractOK — 390 pinned, 133 in the DEBT ledger, 2 exempt
    • check:test-source-aliasOK — 72 packages with tests scanned
    • check:cross-package-test-inputsOK: 14 package(s) read outside themselves, all declared
    • check:query-options-erasure✓ ratchet holds: 67 unswept non-test site(s) … none new
    • check:slot-lookup✓ ratchet holds: 107 unswept site(s) … none new
    • check:type-check-coverageOK — 65/78 workspace packages type-checked
    • check:changeset-gate-self-tests, check:objectui-changeset, check:published-files, check:type-source-resolution, check:ci-filter-parity, check:plugin-teardown-shape, check:nul-bytes, ADR-0087 / no-major / empty-changeset — all exit 0
    • check:type-check-debt needs the whole workspace built, so its ledger entry for this package was reproduced directly instead: tsc --noEmit with the **/*.test.ts exclusion lifted reports 11 raw errors, exactly what TEST_DEBT['@objectstack/plugin-security'] records, and none of the 11 diagnostics name a file this PR touches. The probe tsconfig was removed and the tree verified clean. CI runs the ledger-wide invocation.

Release-notes input is the changeset (.changeset/batch-curated-capability-existence-read.md); no content/docs/releases/** edit, and no packages/spec/** edit was needed.

Filed, not fixed here


Generated by Claude Code

…the reconcile (#11451)
`bootstrapSystemCapabilities` issued one `SELECT ... WHERE name = ? LIMIT 1`
per curated definition and then an `UPDATE` that fired whether or not
`label`/`description` differed. The curated half now costs ONE batched `$in`
read, and the reconcile is equality-gated for both halves.
The #8470 predicate (`managed_by: 'platform'` + `organization_id: null`)
travels INSIDE the batched query rather than filtering its answer: post-#8461
a name can carry a row per organization, so the wide question returns an
unbounded set against a page capped at one row per name, and a truncated page
reads as "absent", which inserts.
The derived half keeps its per-item read - its question is cross-organization
by construction and its counters derive from the lowest-id row installation-
wide. Filed as #11520 rather than decided here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/permissions/authorization.mdx(via sys_capability (literal))
  • content/docs/permissions/capabilities.mdx(via sys_capability (literal))
  • content/docs/permissions/permission-sets.mdx(via sys_capability (literal))

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

  • content/docs/releases/v15.mdx(via sys_capability (literal))
  • content/docs/releases/v17.mdx(via sys_capability (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 cd932772d3ea155573e1c9f6a7feaa187721be9bpackageMentionDocs.

Which tree this was computed on

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

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

@os-samClaude

Copy link
Copy Markdown
CollaboratorAuthor

Seat review (domain:services) — fences verified from the diff, not from the report

The fence I set explicitly: no round-trip pin may move

bootstrap-seed-round-trips.test.ts has zero removed lines — additions only. So every pre-existing pin is untouched, structurally, and the new #11451 block sits alongside them. Verified from the merge base rather than read off the PR body.

Five changed files, and zero under packages/spec/**, content/docs/releases/**, docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md.

The shared-file widening genuinely cannot touch existing callers

This was the fence's hard part — seed-name-lookup.ts has four callers. The claim holds and is readable in the diff:

where: { name: { $in: names }, ...(equals ?? {}) },

equals is optional and ...(equals ?? {}) spreads nothing when absent, so a caller that passes no predicate emits the identical key set — not the same keys plus undefined-valued ones, which toEqual would have accepted silently. The PR closes that loophole deliberately with a Object.keys(…)['name'] pin rather than trusting toEqual. That is the right instinct: the weaker assertion would have passed either way.

Two things I want on the record because they are easy to miss

1. The predicate rides the degradation read too.perItemIndex — the fallback for a driver without $in — takes and applies the same predicate. The reasoning in the comment is the part that matters:

A fallback that dropped it would ask a WIDER question than the batched read it is standing in for — and for the caller that needs one, wider is not "slower but the same": it is a different row.

A fallback that silently answers a different question is precisely the shape that makes a degradation path dangerous, and most implementations would have left it unpredicated.

2. The precondition is stated and assigned. The new docblock says the predicate must keep the result a singleton per name, explains why (readNamePage caps the page; a truncated page reads as absent, which inserts), shows the curated predicate discharges it by construction via ADR-0120 D3's (COALESCE(organization_id, '__global__'), name), and then closes the door on the obvious worry:

Narrowing can only SHRINK a page, so passing a predicate never makes truncation likelier than the unpredicated read it replaces.

⇒ This widening cannot make #11518 worse. Given that this PR's own investigation is what found #11518, saying so explicitly rather than leaving it inferred is the right call.

The design choice was made the way the card asked

The card demanded the choice be written down before the code, and it is — in the module header, not only the PR. It also reframed the card's options correctly: they conflate how to batch a predicated read (1 vs 2) with which halves to batch (3), so the answer is option 2's mechanism at option 3's scope. Option 1 was rejected on measured grounds, not preference — it returns the wrong row (#8470 exactly), it truncates, and it would not even have avoided widening the shared file.

NOT MEASURED, correctly

No speedup and no slope — the hosted rig lives in objectstack-ai/cloud on different axes. The only claims made are the round-trip count and the identity of the rows each leg touches. ⭐ Pinning identity rather than only counts is what stops two offsetting errors holding a count constant, and it is applied here deliberately.

Also unmeasured and declared: real-driver behaviour (every number comes from in-memory doubles).

Landing

Clause-② no: no packages/spec/** path; the surface changes are additive fields on an internal seed result plus a tightening (an unreadable database now declines instead of attempting an insert the unique index refuses). Inside seat discretion.

Part of #11451, not Fixes — correct, since #11520 keeps the derived half open. Flipping ready and arming auto-merge once CI is green; merge queue only. Test Core (1/6) was the last job still running at review time, nothing red.


Generated by Claude Code

@os-sam
os-sam marked this pull request as ready for review August 24, 2026 02:59
@os-sam
os-sam added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit c33f185Aug 24, 2026
32 checks passed
@os-sam
os-sam deleted the claude/issue-11451-batch-bootstrap-system-capabilities branch August 24, 2026 03:13
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@os-sam