fix(plugin-security): retire seven dead { records } find-result limbs, and stop the permission-set loader inventing an empty page - #15838
Conversation
…bs, and stop the permission-set loader inventing an empty page Six of the seven `Array.isArray(x) ? x : x.records` blocks in this plugin carried an unreachable envelope limb over an engine `find()` result. The limb is removed on a MEASUREMENT rather than on the declared type: `IDataEngine.find` says `Promise<any[]>`, but a declared type is not proof here — this repo also carries a `find()` that resolves a `QueryResult` envelope and never an array. A real `ObjectQL` over a real `SqlDriver` was booted and each seam driven through the shipped function that owns it; every one answered a bare array with no own `records` key, on a populated page and an empty one alike. Each of the six keeps its existing disposition for a non-array. Removing a dead limb must not quietly convert a seam that gaps into one that invents an empty. The seventh block is the opposite defect and is repaired in the opposite direction. `SecurityPlugin`'s `sys_permission_set` loader swallowed a thrown read into `[]` and mapped an unreadable result to `[]` too, so three distinct facts left by one door. This is the enforcement plane: "no permission sets" silently withdraws grants that exist while every request still looks normal, and the swallow made `PermissionEvaluator.resolvePermissionSets`' own "db lookup failed" warn unreachable — the diagnostic this repo had already built for exactly this loss. The read fault now propagates and an unreadable page is refused with `DATABASE_ERROR`; a page carrying a non-row refuses too, where the trailing filter used to drop it in silence. Enforcement is unchanged in both directions — an unanswered read still grants nothing — but it is now sayable. Seven pins, one per block, each driving its own seam against the real engine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
…ugin-security-dead-limbs
…er predicates, and re-anchor the rotted system-context rows Two gate repairs on the #15598 pins, neither a behaviour change. `check:engine-double-contract`: the pin file's `observed()` recorder forwards every verb to a real `ObjectQL`, but a seam that merely forwards is exactly the shape that reads as "not a double" and then admits a call the real engine would refuse. Its `update`/`findOne`/`delete` now open with the producer's own predicates (`assertEngineUpdateDispatch` / `assertEngineFindOnePredicate` / `assertEngineDeleteDispatch` from `@objectstack/metadata-core`), and the ledger learns about the newly pinned double — the ratchet grows, it is not weakened. `check:system-context-census`: line rot, not a finding. The loader repair added a net 66 lines to `security-plugin.ts` (77 added / 11 deleted), and every rotted anchor the census reported was off by exactly 66. Repaired with the gate's own `--fix`; only line numbers in `content/docs/permissions/system-context.mdx` change, no prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
📓 Docs Drift CheckThis PR changes 1 package(s): 8 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 4 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 14 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 93dc262c158434fab60e692acefde48c615beccb && git checkout 93dc262c158434fab60e692acefde48c615beccb
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin cee3961759160ed72aacb407f15288cbc018d2eb 8101c7dcb99faeeb70060f4159653e5906ac3702 && git checkout -B drift-repro cee3961759160ed72aacb407f15288cbc018d2eb && git merge --no-ff 8101c7dcb99faeeb70060f4159653e5906ac3702
node scripts/docs-audit/affected-docs.mjs --json cee3961759160ed72aacb407f15288cbc018d2eb
|
Clause-② contract review — PR #15838 (card #15598) — verdict: PASSTier: Subject: head 1 · The refusal — enforcement measured in both directions (weighted first)Method: a scratch vitest probe (unique name, deleted afterwards) boots the real
2 · The two authorities
3 · Census — 7, zero delta, right anchor
4 · Driven, and the attribution
5 · The two red-first gates — repaired, not loosened
6 · The disclosed defective proof — real mechanismReproduced on GNU grep 3.11: a two-line HEAD anchor passed as one 7 · #15840's restraintRight to leave; one reason under-stated and one grade under-called. None of the three has a consumer contract of the #2565 kind, so nothing settles the card — but two declare a disposition: 8 · Honesty audit
Verdict — PASSEnforcement is unchanged on every input the shipped engine produces and moves only fail-closed on two inputs it cannot produce, both declared; both cited authorities exist and say what is claimed (one with a scope caveat); both red-first gates were repaired by growing the ledger and re-anchoring numbers, not by loosening anything. Requested before undraft, PM's call and not blocking the direction: sharpen the changeset sentence per §1. Non-blocking notes: recorder Generated by Claude Code — reviewer for the Generated by Claude Code |
…eachable input, fail-closed on the two that are not Clause-② review measured `PermissionEvaluator.resolvePermissionSets` through this PR's own `dbLoaderFor(ORG)` over a real ObjectQL/SqlDriver, at HEAD and at the merge-base blob, on five engine conditions. A healthy page, a thrown read and `undefined` resolve identically before and after — every result the shipped engine actually produces. An envelope and a page carrying a non-object element do NOT: they granted at base and refuse at HEAD. Both are fail-closed and both are unreachable on the measured engine, which is what the eleven pins establish, so this is a declared narrowing rather than a discovered move — but "unchanged in both directions" is an unqualified claim about behaviour and it sits in the changeset, which feeds release notes. The reviewer's wording replaces it verbatim. ⛔ No source change: the refusal itself was reviewed and passed exactly as it stands. Changeset and PR body carry identical wording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
…ugin-security-dead-limbs
…ugin-security-dead-limbs # Conflicts: # content/docs/permissions/system-context.mdx
Conflict resolved and re-armed — with the one blob that does not match, verified rather than waivedThis PR was not queued, it was conflicted: GitHub reported Head is now
|
Fixes #15598
Seven
{ records }union-normalizer blocks inpackages/plugins/plugin-security, on enginefind()results. Six carried a dead envelope limb. The seventh is the opposite defect and is repaired in the opposite direction.Census, re-derived — 7, matching the card
Re-measured on
origin/main5315098dfrather than trusted from the card (whose count came from the #15094 census taken 2026-09-04 on a tree that has since moved). Method:grep -rn '\.records' packages/plugins/plugin-security/src --include=*.ts, tests excluded, then each hit resolved to a distinct enclosing normalizer block by hand — 9 grep lines, 6 files, 7 blocks (seed-name-lookup.tscarries two blocks, and two blocks each span two grep lines). Diff against the card's 7: none. After this change the same scan returns zero hits in this package's source.The
.recordsanchor is load-bearing rather than incidental:normalize-managed-by.ts:55wasif (Array.isArray(rows?.records)) return rows.records;— a guard-clause spelling that a ternary-anchored regex misses.The #14843 standard: every block driven, not inferred
IDataEngine.finddeclaresPromiseofany[], and that is not proof — #13706 is this repo's ownfind()that resolves an envelope and never an array. So a realObjectQLwas booted over a realSqlDriver(better-sqlite3:memory:) and each block driven through the shipped function that owns it, with a recorder that forwardsfindverbatim and captures what came back. Attribution is by the source line that issued the call, not by my labels:seed-name-lookup.tsreadNamePagebuildExistingByName(batched)seed-name-lookup.ts:266Array, len 1seed-name-lookup.tsperItemIndexseed-name-lookup.ts:320Array, len 1bootstrap-declared-permissions.tsdefaultLookupupsertPackagePermissionSetwith no oracle (the ADR-0086 P2 publish-materializer path)bootstrap-declared-permissions.ts:86Array, len 0auto-org-admin-grant.tstryFindreconcileOrgAdminGrantauto-org-admin-grant.ts:151Array, len 0claim-seed-ownership.tsidsFromclaimSeedOwnership, bulk write refused for its per-row-hook budget (the only path that reaches it)claim-seed-ownership.ts:345Array, len 1normalize-managed-by.tstryFindnormalizeManagedByVocabnormalize-managed-by.ts:53Array, len 1security-plugin.tsloaderSecurityPlugin.start()against the real engine, then its own loadersecurity-plugin.ts:1284Array, len 1Every seam answered
[object Array], no ownrecordskey, elements all objects — on a populated page and an empty one alike. Block 7 was driven with the plugin's own middleware really registered on the engine, since a middleware is what could rewrite the result.Where a seam sits behind a trigger, only the trigger is shaped — the batched read is refused, or the bulk write is refused. The read under test always reaches the real engine.
The six dead limbs
The envelope limb is removed and each block keeps its existing disposition for a non-array. Removing dead code must not quietly convert a seam that gaps into one that invents an empty answer, so
seed-name-lookup(both) andbootstrap-declared-permissionsstill answerunknown/null, andauto-org-admin-grant/claim-seed-ownership/normalize-managed-bystill answer[]exactly as before.The seventh — repaired in the opposite direction
security-plugin.ts'ssys_permission_setloader mapped three different facts onto one value: the read succeeded on an empty catalog; the read threw (catch { rows = [] }); the read resolved something unreadable (?? []). Only the first is an answer.This is the enforcement plane, and its own neighbouring comment already names the failure — dropping a row here "revokes standing access with no signal at the moment of loss". The swallow produced that loss for a whole page at once.
The refusal is not this seam's invention; its consumer already declared the handling.
PermissionEvaluator.resolvePermissionSetscatches a throwing loader, keeps the request fail-closed, and names the failure in a warn — because "without the warn, a transient DB error makes custom permission sets silently vanish and the resulting 403s are undiagnosable" (#2565). The loader swallowing its own read failure made that warn unreachable: the diagnostic this repo had already built could never fire. The read fault now propagates, and an unreadable page refuses withPermissionSetReadUnansweredError(DATABASE_ERROR, ADR-0112's existing vocabulary — nopackages/specchange). A page carrying a non-row refuses too, where the trailing filter used to drop it in silence.Enforcement is unchanged for every result the shipped engine produces; an envelope or a non-row element now refuses (fail-closed) where the old code read through it. An unanswered read still grants nothing; what changes is that the loss is now sayable. Measured in both directions by the Clause-② review, driving
resolvePermissionSetsthrough this PR's owndbLoaderFor(ORG)at HEAD and at the merge-base blob on five engine conditions: a healthy page, a thrown read andundefinedresolve identically before and after (the warn count on a thrown read going 0 to 1); an envelope and a page carrying a non-object element granted at base and refuse at HEAD. Both of those are fail-closed and both are unreachable on the measured engine — which is exactly what the eleven pins establish — so this is a declared narrowing, not a discovered move. The changeset carries this same sentence verbatim.#2565 is the binding consumer contract for this seam, and it holds verbatim. The 2026-08-11 store-fault ruling (#7505) is precedent for the direction only — its own scope line is
readRowByIdand its callers, so it is not the ruling of record here.Pins, and the mutation that proves each one discriminates
Seven blocks, eleven pins in
engine-find-bare-array.pin.test.ts, each driving its own seam against the real engine. Every pin was then mutated and confirmed red — nine legs, each proving the mutation reached disk by exact substring count before running, and each restoring viagit checkout HEAD -- ABSOLUTE_PATHand proving the file byte-identical to its HEAD blob afterwards:readNamePagereadsrows.recordsonlyperItemIndexreadsrows.recordsonlypresentdefaultLookupreadsrows.recordsonlyunreadable: 0/seeded: 1tryFindreadsrows.recordsonlygrantedidsFromreadsrows.recordsonlytryFindreadsrows.recordsonlymanaged_byrewrittenrows.recordsArray.isArray(rows) ? rows : rows?.records ?? []try/catch { rows = [] }restoredM7c is the direct evidence for the direction: reinstating the swallow makes the repo's own diagnostic unreachable again. It was independently reproduced by the review.
No mutation crosses a package boundary — every mutated file is
plugin-security's own source, imported by the pins through a relative specifier, so nodist/stands between the mutation and the subject and no rebuild leg is owed.Verification
pnpm --filter @objectstack/plugin-security test— 97 files / 1827 tests pass (exit 0)pnpm --filter @objectstack/plugin-security typecheck— exit 0, andcheck:test-typecheckreports 0 files / 0 errors, so the new pins really are type-checkednode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack— re-derived a second time after the diff grew, which raised it from 58 to 93 families. 93/93 exit 0, exit codes captured by redirect, never through a pipe.check:engine-double-contract(the pin's forwarding recorder is an engine double — itsupdate/findOne/deletenow open with the producer's own predicates, and the ledger gains two rows: the ratchet grows, it is not weakened) andcheck:system-context-census(pure line rot — this change adds a net +66 lines tosecurity-plugin.ts, 77 added / 11 deleted, and every rotted anchor was off by exactly 66; repaired with the gate's own--fix, line numbers only, no prose).check:dual-build-cjs-loads,check:i18nandcheck:type-check-debtfirst answered exit 3 = PREREQUISITE NOT MET = NOT MEASURED; the full build closure was then built (turbo run build, 71/71 successful) and all three re-run exit 0.$RUNNER_TEMP, matrix-shard expansions) — three shard-attestation invocations, two test-completeness invocations,cross-package-test-inputs' CI invocation, and half-state-patrol's. CI measures them.⛔ No gate was widened — #15094 ruled against it (precision 0.446 / 0.857 measured). ⛔ No
packages/specchange. ⛔ Nothing outsideplugin-securityexcept the two ledger/anchor files the gates themselves rewrote.Found, not fixed
Three of the six dead-limb blocks (
auto-org-admin-grant.tstryFind,claim-seed-ownership.tsidsFrom,normalize-managed-by.tstryFind) still answer[]for a non-array, and theircatcharms turn a failed read into[]too — a smaller instance of the same invention the seventh block was repaired for. Left alone deliberately: each is a behaviour change with its own blast radius, and this card's face is the dead limbs plus the one site it names. Filed as #15840 rather than ridden in here; the review has since measured that one of the three persists a withdrawal rather than gapping passively, and the PM is regrading that card accordingly.Generated by Claude Code
🤖 Generated with Claude Code
https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y