Uh oh!
There was an error while loading. Please reload this page.
fix(security): make a refused RBAC catalog write boot-visible instead of a silent seed of zero - #12967
Conversation
…ng a seed of zero Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
…ed, classified) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
…ac-seeder-silent-unique-violation
…imit contracts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 6 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 75682f39fca8fa5ed0bf80854d867d849cff4ab5 && git checkout 75682f39fca8fa5ed0bf80854d867d849cff4ab5
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin db39dfc1c9b09681474475cea6e77537ab2c89ab b8651025a4ba82c2baefc6a31ec4ff066a727d56 && git checkout -B drift-repro db39dfc1c9b09681474475cea6e77537ab2c89ab && git merge --no-ff b8651025a4ba82c2baefc6a31ec4ff066a727d56
node scripts/docs-audit/affected-docs.mjs --json db39dfc1c9b09681474475cea6e77537ab2c89ab
|
os-litant
commented
Aug 28, 2026
Review — ACCEPT-pending, one patch round in flight. ⛔ Not a REWORK.PM seat
|
| Fence | Reading |
|---|---|
⛔ No packages/spec | 0 files ⇒ clause-② no holds |
| ⛔ Nothing new exported from the package index | plugin-security/src/index.ts not in the changed set |
⛔ legacyUniqueReplacements untouched | schema-drift.ts not in the changed set |
| ⛔ No governed surface | no docs/adr/**, no content/docs/releases/** |
| ⛔ WARN, never THROW | only logger?.warn?.() in the new code; :503 carries a ## Why it WARNS and does not throw section |
| ⛔ Shipped classifier, no local regex | :86 imports isUniqueViolationError / uniqueViolationColumn from @objectstack/types; :83-84 explicitly rejects a local 23505 / ER_DUP_ENTRY regex |
| ⭐ Three copies did not become four | one shared helper in per-organization-catalog.ts, beside warnOrganizationLessRows, aggregated one line per object per class |
Three gate findings were real and fixed in the diff, never baselined — the new test double ignored the caller's limit, read a combinator as a field name, and declared update() unpinned. The engine-double-contract row added is a PIN, not an exemption.
The patch round: the log level, which is my pin's error
The dev raised this rather than quietly picking a side, which is the right call and the reason it is one patch round and not a rework.
My pin read "WARN, never THROW". Those are two claims and I should have separated them. "Never THROW" stands — a rethrow would fail the boot on every deployment carrying the legacy index. "warn" was me over-specifying a level against a written repo rule. AGENTS.md → Degradation log levels decides it with one question:
After the degradation, does the system still look "normal" from the outside, while something it claims is persisted has not actually landed? Yes →
error.
A boot that logs RBAC catalog seeded at info over zero landed rows is that shape exactly. So: option C — error for the unique-violation class, warn for the other. That is the split the rule's own "do not over-apply it" paragraph asks for, not an over-application.
check:durability-log-level is not evidence the level was fine. The gate's header calls itself "deliberately narrow": it only inspects a try/catch whose try calls a durability-critical operation from its declared vocabulary. The seeder's ql.insert is evidently outside that vocabulary ⇒ the pass means NOT MEASURED for this site, not "level approved". Worth stating, because a reader would otherwise take the green as endorsement.
⭐ #4420 — the accident that gate exists for — is this defect's exact shape on a different table: "the durable suspended-run store was attached to a table that was never created, every write failed into a warn nobody read, and each restart silently dropped every in-flight approval. The system reported itself healthy the whole time."
The patch must widen SeedLogger with an optional error and a warn fallback (sql-driver.ts:4320logDurabilityFailure is the correct spelling in this tree) — a bare logger.error?.() against a reduced sink would silently drop the loudest line in the change.
Out-of-scope finding, worth reading
#12970 — two more swallowed tryUpdate refusals outside the catalog seed. permission-set-drift.ts drops a refused drift-diagnostic write and then gates its whole report behind updated > 0, so a boot where every write is refused prints nothing and reads as "no drift"; and permission-set-overlay-discard.ts logs "overlay discarded (sanctioned operator action)" with before/after counts that are equal — an audit record asserting an operator action that never landed. Both now have a channel to fix, since this PR's shared helper already takes an optional refusal log.
Generated by Claude Code
…ue violations, warn for the rest The AGENTS.md degradation rule decides this: a boot that logs 'RBAC catalog seeded' at info over zero landed rows is the shape whose answer is error. A non-unique refusal is a retrying outage and stays warn, per the same section's 'do not over-apply it'. SeedLogger gains an OPTIONAL error carrying the kernel Logger arity, and every emission routes through logSeedDurabilityFailure so the warn fallback cannot be forgotten — never logger.error?.() (silence against a reduced sink) and never (a ?? b)() (detached receiver). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
…llback is guaranteed by the type
With both `warn` and `error` optional, `{}` satisfied SeedLogger and every
value of the type was permitted to print nothing — a contract that permits
silence, which no call-site spelling can repair. `error` stays optional so
reduced sinks remain representable.
Pinned by reading the declaration's AST rather than a `@ts-expect-error`:
measured, zero test files in this package reach any tsc program, so an
expect-error here would evaluate never.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5…level The earlier AST-reading pin used import.meta.url, which is TS1470 under this package's CommonJS target and pushed TEST_DEBT 11 -> 12 on a shrink-only ratchet. Replaced with a type-level pin that needs no filesystem: the re-measure program DOES compile test files (it lifts the tsconfig test exclusion), so a @ts-expect-error here is enforced rather than phantom. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
… AST assertion check:type-check-coverage refuses a @ts-expect-error in a file no tsc program the typecheck script runs compiles, and PHANTOM_PIN_DEBT is closed to new entries — it named this file when the pin was written that way. Replaced with a runtime assertion over the declaration's own AST, seeded from __dirname (import.meta is TS1470 under this package's CommonJS resolution). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
…ac-seeder-silent-unique-violation # Conflicts: # packages/plugins/plugin-security/src/bootstrap-platform-admin.ts
Uh oh!
There was an error while loading. Please reload this page.
Fixes#12923
The defect
The five RBAC catalog seeders answered a refused write with
null/false, which isbyte-for-byte the answer for "nothing to do": the
seededcounter never incremented andthe pass returned normally. On a deployment still enforcing a platform-wide unique
index on the name column — the shape that predates per-organization materialization —
every per-organization INSERT is refused that way, so the boot log read as a successful
seed of zero rows. Measured on a deployed plane and undetected for weeks: an empty
Setup (no positions, no permission sets, no capabilities) under a clean log.
Re-verified independently on this branch's merge base rather than inherited:
bootstrap-declared-positions.tstry { return await ql.insert(...); } catch { return null; }/catch { return false; }bootstrap-builtin-positions.tsbootstrap-platform-admin.tspermission-set-projection.tsbootstrap-declared-permissions.tsandbootstrap-declared-capabilities.tsboth importEach file declares a
SeedOptions.loggerin the same file, and neithertryInsertnortryUpdateaccepted or used it.The outer handler was not missing — it was disarmed
security-plugin.tsalready wrapsseedCatalogForOrganizationin atry/catchthatwarns, and that
catchis unreachable for this failure class: the refusal isconverted to
nullthree call layers below, so theawaitresolves normally and thehook logs "RBAC catalog seeded" at
infoover a seed of nothing. Adding another outertry/catchfixes nothing. The signal has to survive the inner helper, which is wherethis change is.
The shape follows the honesty precedent already in that file: the resync path returns
success: falsewith a reason string whenapplied === 0rather than a cheerfulzero.
The change
per-organization-catalog.ts— the existing shared home for this family's cross-cuttingconcerns — gains
createSeedWriteRefusals()/reportSeedWriteRefusals(), besidewarnOrganizationLessRowsand built on the same aggregate-then-report discipline. Eachseeder's
catchnow records before it answers, and the pass reports one actionableline per object per class, beside its counts. The three copies did not become four.
Two classes, deliberately never merged:
(
os migrate plan, where the legacy index surfaces as areplace_unique_indexoperation, then
os migrate apply);migration repairs it. Sending an operator to
os migratefor an unreachable databaseis a confident wrong answer.
Classification is the shipped cross-dialect predicate (
isUniqueViolationError/uniqueViolationColumnfrom@objectstack/types, the same pairobjectql/src/engine.tsimports), never a local
23505/ER_DUP_ENTRYregex.Diagnosis only — loud, never fatal. A rethrow would turn a silent degradation into a
boot failure on every deployment carrying the legacy index. Counts, accept/reject
behaviour and the healthy-path logs are unchanged; a pass that refuses nothing stays
silent, and nothing new is exported from the package index.
The two classes take different LEVELS, and the split is the rule's own
AGENTS.md "Degradation log levels" decides this with one question — after the
degradation, does the system still look normal from the outside while something it claims
is persisted has not actually landed?
unique-violationreacheserror. Yes, exactly: the boot goes on to log "RBACcatalog seeded" at
infoover zero landed rows, nothing else fails, and the losssurfaces later to somebody who cannot connect it back to this boot. That is the
founding incident of this very rule ([automation/approvals] 进程重启后审批决策静默失效:挂起 flow run 仍只存内存(#1518 标记 COMPLETED 但 17.0.0-rc.1 未生效),approve 落库却永不推进且零报错 #4420) on a different table — the durable
suspended-run store was attached to a table that was never created, every write failed
into a
warnnobody read, and each restart silently dropped every in-flight approval;the system reported itself healthy the whole time. Per the rule, the line owes both
halves in its first sentence, and it carries them: the consequence (the catalog did
not land, and the deployment will go on looking healthy) and the fix (the migrate
remedy).
otherstayswarn. No: a refusal that is not a unique violation is typically aplain outage that retries on the next boot and on organization creation, and the next
person to open Setup finds it. Escalating it would be the over-application the same
section warns about — it is what trains everyone to skim
error, which is what made[automation/approvals] 进程重启后审批决策静默失效:挂起 flow run 仍只存内存(#1518 标记 COMPLETED 但 17.0.0-rc.1 未生效),approve 落库却永不推进且零报错 #4420's
warnunreadable in the first place.SeedLoggergains an optionalerrorcarrying the kernelLoggerarity(
message, cause?, meta?, matching the siblingProjectionLoggerandLogger.errorin@objectstack/spec/contracts, so meta cannot land in the cause slot).Every emission goes through one named helper,
logSeedDurabilityFailure, so no site canforget the fallback:
logger?.error?.(...)— that prints nothing against a host that injected areduced sink, silently dropping the loudest line in this change;
(logger.error ?? logger.warn)(...)— that evaluates to a bare function and callsit with
this === undefined, which@objectstack/core's class-basedObjectLoggerdoes not survive.
Both prohibitions and the exact
if/elsespelling are the measured conclusions alreadyrecorded on
SqlDriver.logDurabilityFailure; this mirrors them.warnalsooptional on
SeedLogger,{}satisfied the type and every value of it was permitted toprint nothing, so the fallback was a convention rather than a guarantee and no call-site
spelling could repair it.
warnis now non-optional;errorstays optional soreduced sinks remain representable. (⛔ Not the other two repairs: making
errorrequiredforecloses those hosts, and a required
infowould let a lost write be reported atinfo— the reassuring half-truth the degradation rule exists to remove.) Verified rather than
assumed: the package source typechecks clean with
warnrequired, so every one of thefive seeders' construction sites already carried it — no
ascast was added anywhere, andnothing was widened.
SeedLogger,logSeedDurabilityFailureand the reporter remain unexported from thepackage index —
src/index.tsis byte-identical toorigin/main.The reporter is renamed
warnSeedWriteRefusalstoreportSeedWriteRefusals, becausea function named
warn*that emitserrorfor one of its two classes misdescribesitself. Mechanical: five call sites and the tests.
On echoing driver text — what I found, since the card asked
Two corrections to the framing I was dispatched with, both measured in-tree:
driver-fault-redaction.tsgoverns the server LOG, not the REST boundary. Its ownheader: "This is a server LOG. The rethrown error is untouched and every HTTP boundary
is unaffected." A boot log is that boundary. A SQL driver builds its message by
prefixing the fully bound statement — every value inlined — to the database's
diagnostic, so printing it from the seeder would re-open exactly the exposure that
redaction was written to close. The warning therefore reads only the value-free
code/errnochannel, and a test plants a canary in the bound statement and assertsit cannot reach the log through this line.
uniqueViolationColumndoes not answer "name the colliding index" — by ruling. Itreturns
undefinedwhenever the dialect named an index rather than a column(maintainer ruling, 2026-08-08: an index name mistaken for a column is worse than no
answer), which is the usual case for this defect. It is still called, so SQLite's
column-naming spelling is reported when it appears, and nothing is guessed when it
does not.
So the colliding index is named where it already is, safely: the query engine logs every
one of these refusals at ERROR with
redactBoundStatementapplied, and that redactiondeliberately keeps the identifier-bearing tail so an operator debugging a duplicate
can read the index name. The seeder's aggregate line points at those entries instead of
re-deriving them. That leaves one open question for the maintainer, recorded in the dev
report rather than guessed at here: whether a shipped index-name accessor should exist at
all is a contract decision sitting next to the 2026-08-08 ruling, and I did not invent one.
I also checked for a ruling against boot-time warnings.
types/src/unique-scope-install-gate.tscarries one — "Never a boot-time warning (#4884 discipline)" — and it does not reach
here: that rule refuses a gate derived from declarations, which would fire on every
boot of every deployment forever. This line is evidence-driven and fires only when a write
was actually refused, so a healthy deployment stays silent. Pinned by a test.
driver-sql'slegacyUniqueReplacementsis untouched — it is the already-shippedoperator-side half of this story, the counterpart rather than the target.
Merged with
main— both changes keptdb39dfc1c(#13065, verified platform owner bypasses the Layer 0 org wall) landed onmainmid-round and touchesbootstrap-platform-admin.ts, the one file this card alsoedits. Exactly one conflicted file;
scripts/engine-double-contract.pinned.jsonauto-merged and was not hand-edited.
The conflict was purely import adjacency — both sides added an import after
claim-seed-ownership.js— so both sides are kept. TheirmatchesDeclaredOwnerEmailimport and its call site are intact and the inlinetoLowerCase()compare is not restored (0 occurrences), so nothing about thatsecurity change is un-landed here. Verified in both directions rather than by eye:
origin/mainshows only this card's additions;three lines.
Merged with a merge commit — never a rebase, amend or force-push, so any other checkout of
this branch stays valid. The branch now merges cleanly with
origin/main(
git merge-treeproduces a tree with no conflict), and the derived change set is stillexactly this card's 10 paths, now against merge base
db39dfc1c.Tests
New pins in
seed-write-refusal.test.ts(24 cases). Error spellings are copied from theclassifier's own live-server fixtures, never invented:
the true count, not 40;
migrate remedy is absent from it;
and the sentence says there is none) stays in the
otherclass;The level split is pinned both ways, and so is the fallback:
errorsink and neverwarn, through the realseeder as well as through the helper;
warnand nevererror;warnstill receives the unique-violation message whole,remedy included, through the fallback — the case a bare
logger?.error?.(...)wouldanswer with silence;
errorline carries both halves the rule asks for (the consequence and the fix);undefinedand the detail travels in meta, since a summary of Nrefusals has no single cause.
And the guarantee itself is pinned, by reading the declaration's own AST:
warnisnon-optional,
errorandinfoare optional, the pin fails loudly if it ever stopsfinding
SeedLogger, and the reduced sink is not merely representable but actuallyserved. It survives removal of
check:optional-error-sink-contract, which is the point —that gate found the hole, but the property belongs to this module.
@ts-expect-error. Measured, and it cost two attempts.This package's
tsconfig.jsonexcludes**/*.test.ts, andtsc --noEmit --listFilesreports zero plugin-security test files in the program its
typecheckscript runs.check:type-check-coveragerefuses exactly that shape by name — "carries a@ts-expect-errordirective but no tsc program thetypecheckscript runs compiles it …replace the pin with a runtime assertion", with
PHANTOM_PIN_DEBTclosed to new entries— and it refused this file when the pin was first written that way. The runtime assertion
is the remedy the gate itself names. It is seeded from
__dirnamerather thanimport.meta.url, because undermodule: NodeNextthis package resolves as CommonJSwhere
import.metais TS1470 — which pushed the shrink-onlyTEST_DEBTratchet from 11to 12 before it was fixed. Both were caught locally by re-measuring, not in review.
Ablation
Two ablations, both on the merged tree, both confirming the mutation on disk by
git hash-object(never an editor's exit code) and restoring withgit checkout HEAD -- ABSOLUTE_PATHunder atrap ... EXIT INT TERMwith an absoluterepo root, verified by hash equality plus an empty
git diff HEADand an emptygit status --porcelain.1. The repair itself — revert the two recording
catchbodies inbootstrap-declared-positions.tsto their pre-fix form. Re-run on the merged tree,because a reading taken on a different tree is not a reading about this one:
The same five cases go RED as before the merge — the four that drive
bootstrapDeclaredPositionsplus the "reaches the durability channel through the realseeder" pin written to prove the split is not helper-only. The green count moved 14 to 19
only because the pin file grew from 19 cases to 24. The cases exercising the helper
directly, the built-in-position pass (a different file) and the green path stay GREEN. No
rebuild was required and none is claimed: the test reaches the mutated file by
relative import inside the same package, so vitest resolves it from
src/, not fromany
dist/— and had the resolution path been otherwise the ablation would have stayedgreen, which is the reading that would have voided it.
2. The type guarantee — put the
?back onSeedLogger.warn. The merge left bothablation subjects byte-identical (
per-organization-catalog.tsandbootstrap-declared-positions.tshave the same blob hashes before and after), but thiswas re-measured on the merged tree anyway rather than re-quoted:
So the pin is precisely targeted (one case, not a cascade), and the same mutation
reproduces the CI failure locally — which is how this fix was verified to be the one that
closes it rather than merely adjacent to it.
ablation reported a clean
11and no red at all. The script was measuring throughtsc,which was the right instrument for the
@ts-expect-errorshape this pin used to haveand the wrong one for the runtime AST assertion it has now. The stale instrument, not
the tree, produced the green. Fixed and re-run above.
Gates
All run on the merge commit
b8651025a, exit codes captured before any pipe.@objectstack/plugin-securityvitestTest Files 89 passed (89)·Tests 1619 passed (1619)typechecktsc --noEmit && tsc --noEmit -p tsconfig.scripts.json, exit 0pnpm lint(repo-wide, not narrowed)check:optional-error-sink-contract✓ optional-error sink contract: every sink declaring an optional error guarantees a warn channel (1 baselined, shrink-only)— the 1 baselined entry is pre-existing, not this diffcheck:engine-double-contractOK — 705 pinned, 134 in the DEBT ledger, 3 exempt— this card's double is one of the 705 pins, not a baseline exemptioncheck:where-matcherwhere-matcher conformance holds: 314 matcher(s) discovered, 314 answer the combinator battery correctly or refuse it loudlycheck:objectql-double-limitObjectQL double limit conformance holds: 291 double(s) graded, 91 apply the caller's bound or refuse it loudlycheck:type-check-coverageOK — 65/78 workspace packages type-checked (plus the root)check:type-check-debt(--re-measure)OK — 31 ledger entr(ies) re-measured in 243.7s, 1570 raw tsc error(s) total, none above its recorded number·surplus: nonecheck:i18nOK (9 package(s) — all bundles in sync, no undeclared authoring keys)check:cross-package-test-inputs,check:test-source-alias,check:type-source-resolution,check:published-files,check:slot-lookup,check:page-declaration-shape,check:nul-bytes,check:query-options-erasure,check:i18n-stale-fill,check:changeset-gate-self-tests,check:objectui-changeset,check:pm-half-statescheck-adr-0087-registration,check-changeset-no-major,check-empty-changeset,check-ci-filter-parity,check-comment-mask-adoption,check-plugin-teardown-shape,release-rehearsal-clone --self-testNOT MEASURED, not folded into the green list:
check:durability-log-levelexits 0 over this file, and that is NOT an endorsementof the level. The gate is deliberately narrow: it judges a
catchwhosetrycalls anoperation named in its declared
DURABILITY_CRITICAL_CALLEESvocabulary. That vocabularyis 18 specific function names (
syncSchema,writeRecord,persistAuditTrailRow,…) and
ql.insertis not among them, so these seeder seams sit outside the gate'sreach. Its green here means not measured for this site, never level approved. The
level was chosen against the AGENTS.md rule directly, not against this gate.
scripts/pm/check-half-states.mjs— exit 3,PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential. Its own text: "Nothing was swept ... it isno reading at all." A board gate, unrelated to this diff.
Would adding these seams to that vocabulary be cheap and shrink-only? Measured: no, so it
is not done here. The vocabulary matches callee names, and the gate's own header
already excludes
find/findOne/countas "too generic to declare repo-wide".insertis in that category: 156 non-test
.insert(call sites acrosspackages/. And 15 non-testfiles outside this card's five seeders still declare the same swallowing
catch { return null; }shape, so every one of them would redden at once — with nowhereto park them, because
scripts/durability-degradation.baseline.jsonis shrink-only anddeliberately empty, its header stating that an entry means "a REAL degradation that is
not yet fixed". Admitting 15 would assert 15 real unfixed degradations nobody has reviewed.
Widening the gate is its own card; flagged rather than attempted.
Four gate findings during the run were real and are fixed in the diff rather than
baselined: the new test double ignored the caller's
limit, read a combinator as a fieldname, and declared
update()without routing throughassertEngineUpdateDispatch; andSeedLoggerdeclared an optionalerrorwith no guaranteed fallback channel. Theengine-double-contractledger entry added is a pin ("pinned": 1), not a baselineexemption, and nothing was added to any shrink-only baseline.
Three prerequisite refusals were hit and cleared rather than reported as results, because
"could not run" is not a pass:
check:i18n(workspace CLI not built) andcheck:type-check-debttwice — first for an unbuilt@objectstack/service-knowledge, then,after the merge, for
@objectstack/plugin-securityitself, which the^...dependency-closurebuild deliberately excludes (
--re-measure cannot run: 1 workspace dependenc(ies) … still have a type entry point OLDER than their own sources). Each was rebuilt and re-run to a realreading, and only the real readings are quoted above.
Generated by Claude Code