Repository navigation
fix(security): RBAC catalog seeder swallows unique-violation write failures — 'seeded 0' reported as success while a legacy index vetoes every row #12923
Description
Activity
Claimed →
pm:dispatchedPM seat
domain:services, sessionsession_0194kbQJxUvv2yvsGRtuXpP5. Dev branch:claude/issue-12923-rbac-seeder-silent-unique-violation.Clause-② graded no, conditional on the scope pin below: the repair adds a boot-visible warning and changes no accept/reject behaviour, and nothing new is exported from the package index.
⚠️ If the fix ends up throwing instead of warning, that grading is void — see ⛔ below.The card names its evidence in
cloud#1695, which this lane cannot readSo the defect was located independently, in this repo, on
origin/main@944d798c5. It is where the card said it would be, and the mechanism is one layer deeper than the card states.MEASURED — the swallow is three copies of one helper, and there is no logging in any of them
File Lines Code packages/plugins/plugin-security/src/bootstrap-declared-positions.ts37-42 try { return await ql.insert(…); } catch { return null; }/catch { return false; }packages/plugins/plugin-security/src/bootstrap-builtin-positions.ts76-81 byte-similar copies of both packages/plugins/plugin-security/src/bootstrap-platform-admin.ts115-129 same shape, multi-line Each file declares a
SeedOptions.loggerin the very same file and neithertryInsertnortryUpdatetakes it or uses it. A refused INSERT becomesnull, indistinguishable from "nothing to do", andseededsimply never increments (bootstrap-declared-positions.ts:164,bootstrap-builtin-positions.ts:129,bootstrap-declared-capabilities.ts:319,bootstrap-declared-permissions.ts:237).⭐ The correction to the card: the outer handler is not missing, it is disarmed
The card reads as "the seeder's catch swallows it". The truer statement, and the one the fix must act on:
packages/plugins/plugin-security/src/security-plugin.ts:3552-3559already has the right handler —try { await seedCatalogForOrganization(organizationId); ctx.logger.info?.('[security] RBAC catalog seeded for a newly created organization', { … }); } catch (e) { ctx.logger.warn?.('[security] catalog seeding for a new organization failed (retried on next boot)', { … }); }
That
catchis unreachable for this failure class.tryInsertconverts the throw into anullthree call-layers down, so theawaitresolves normally and:3554logs "RBAC catalog seeded" — atinfo, as a success — over a seed of zero rows. ⇒ The bug is not an absent handler; it is an inner barecatchthat eats the signal the outer one was written to receive. Anyone "fixing" this by adding another outer try/catch will change nothing.⭐ The precedent is in the same file
security-plugin.ts:3196-3208already does exactly what this card asks for, on the resync path: whenapplied === 0it returnssuccess: falsewith a reason string rather than a cheerful zero, under a comment whose own word is "honesty". That is the shape to follow — this card is that treatment applied to the boot seed path.⛔ Reuse the shipped classifier — do not invent one
packages/types/src/unique-violation.tsalready exportsisUniqueViolationError(:215) anduniqueViolationColumn(:389), andpackages/objectql/src/engine.ts:118already imports both.uniqueViolationColumnis the piece that answers the card's "naming the colliding index" — perpackages/objectql/src/driver-fault-redaction.ts:68, MySQL'sfor key '…'names an INDEX. Hand-rolling a23505/ER_DUP_ENTRYregex here would re-derive a tested, shipped, cross-dialect classifier.⛔ Scope pin: WARN, never THROW
The card is explicit — "should surface as a boot-visible warning naming the colliding index and the migrate remedy — never a silent zero." A repair that rethrows would turn a silent degradation into a boot failure on every deployment carrying the legacy pre-#8556 index — a far larger behaviour change than this card authorises, at the maintainer floor (it decides whether a deployment boots), and it would flip the clause-② grading. Loud is the ask. Fatal is not.
Generated by Claude Code
os-dev-report
{ "issue": 12923, "status": "done", "branch": "claude/issue-12923-rbac-seeder-silent-unique-violation", "pr": "https://github.com/objectstack-ai/objectstack/pull/12967", "premise_still_valid": true, "summary": "Re-verified the premise independently on my own merge base: the swallowing helper is in FIVE places, not three — the three named local copies (bootstrap-declared-positions, bootstrap-builtin-positions, bootstrap-platform-admin) plus the EXPORTED pair in permission-set-projection.ts that bootstrap-declared-permissions and bootstrap-declared-capabilities both import, which is what the card's own counter-site evidence already implicated. Added createSeedWriteRefusals() / warnSeedWriteRefusals() to per-organization-catalog.ts beside warnOrganizationLessRows; each seeder's catch now RECORDS before it answers, and each pass emits one aggregated line per object per class beside its counts. Two classes kept apart: a unique violation is named as a deployment-schema defect with the os migrate plan / os migrate apply remedy, and anything else gets its own line and never inherits that remedy. Classification is the shipped isUniqueViolationError / uniqueViolationColumn from @objectstack/types, no local regex. WARN, never THROW; no new exports from the package index; the three copies did not become four. TWO CORRECTIONS TO THE DISPATCH FRAMING, both measured and both written into the PR body: (1) driver-fault-redaction.ts governs the SERVER LOG, not the REST boundary — its header says so verbatim — so a boot log IS that boundary and echoing driver text there would re-open the #8682 exposure; the warning therefore reads only the value-free code/errno channel and a test plants a canary in the bound statement to prove it cannot leak. (2) uniqueViolationColumn does NOT answer 'name the colliding index' — by the 2026-08-08 maintainer ruling it returns undefined precisely when the dialect named an index, which is the usual case here. The colliding index is instead named where it already is safely: the query engine logs every one of these refusals at ERROR with redactBoundStatement applied, and that redaction deliberately KEEPS the identifier-bearing tail; the aggregate line points at those entries rather than re-deriving them. I also checked for a ruling against boot-time warnings and found one in types/src/unique-scope-install-gate.ts ('Never a boot-time warning', #4884 discipline) — it does not reach here because it refuses a DECLARATION-derived gate that would fire on every boot forever, whereas this line is evidence-driven and a healthy deployment stays silent (pinned by a test). driver-sql legacyUniqueReplacements untouched.", "tests": "ALL on the final commit d745e36af, clean tree, exit codes captured BEFORE any pipe. (a) pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 -> 'Test Files 88 passed (88)' / 'Tests 1599 passed (1599)'. (b) package typecheck ('tsc --noEmit && tsc --noEmit -p tsconfig.scripts.json') exit 0 — NOTE it does NOT cover the new test file: this package's tsconfig excludes '**/*.test.ts', so the test file's type correctness is measured by vitest and by check:type-check-debt, not by that green. (c) pnpm lint repo-wide, NOT narrowed, exit 0 with no findings. (d) Gate families re-derived with 'node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack' from my ACTUAL changed set (10 paths) — the FIRST derivation warned STALE TREE (3 commits behind, 2 files it derives from had changed), so I merged origin/main and re-derived clean before running anything. Green with their own judgment lines: check:where-matcher '312 matcher(s) discovered, 312 answer the combinator battery correctly or refuse it loudly'; check:objectql-double-limit '289 double(s) graded, 89 apply the caller's bound or refuse it loudly'; check:engine-double-contract exit 0 after registering the new PIN via --write; check:type-check-coverage 'OK — 65/78 workspace packages type-checked'; check:type-check-debt --re-measure 'OK — 31 ledger entr(ies) re-measured in 295.4s, 1570 raw tsc error(s) total, none above its recorded number'; check:i18n 'OK (9 package(s) — all bundles in sync)'; plus check:durability-log-level, 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-states, check-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-test all exit 0. (e) NOT MEASURED, not folded into the green list: 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 is no reading at all' — a board gate unrelated to this diff. check:i18n and check:type-check-debt BOTH first returned PREREQUISITE NOT MET on an unbuilt workspace (i18n: CLI not built; type-check-debt: '--re-measure cannot run: 1 workspace dependenc(ies) ... no built type entry point on disk -- @objectstack/service-knowledge'); I built and re-ran both to real readings, and only the real readings are quoted. (f) THREE gate findings were real and are FIXED IN THE DIFF, never baselined: my new test double ignored the caller's limit, read a combinator as a field name, and declared update() without assertEngineUpdateDispatch. The engine-double-contract ledger row added is a PIN ('pinned': 1), not a baseline exemption. (g) ABLATION: reverted the two recording catch bodies in bootstrap-declared-positions.ts to their pre-fix form on disk. NO REBUILD REQUIRED AND NONE CLAIMED — the test reaches the mutated file by RELATIVE import inside the same package, so vitest resolves it from src/ and no dist/ is on the path; had the resolution path been otherwise the ablation would have stayed GREEN, which is exactly the reading that would have voided it. Mutation confirmed ON DISK by git hash-object, never by an editor's exit code: PRE_HASH 26db884c766606f97dc8e86cdb7b29a2482f0e96 (equal to the HEAD blob, asserted before mutating) -> POST_HASH ce6f2e296336d94bc9f8746fd2d84d6031a9a1d7, with both anchors counted in both directions (deleted-marker 1 -> 0, injected-marker 0 -> 1) and the script aborting the reading if any of those failed. Direction PREDICTED BEFORE RUNNING and matched exactly: 'ABLATED_VITEST_EXIT=1 · Tests 4 failed | 10 passed (14)' — the four cases driving bootstrapDeclaredPositions go RED; the ten covering the helper directly, the built-in-position pass (a different file) and the green path stay GREEN. One honest note: the canary/leak case asserts an ABSENCE and therefore cannot go red under this ablation. RESTORE leg: git checkout HEAD -- ABSOLUTE_PATH under trap ... EXIT INT TERM with an absolute repo root resolved by git rev-parse --show-toplevel, verified by hash equality with the HEAD blob PLUS an empty git diff HEAD and an empty git status --porcelain.", "mcp_calls": "5 — create_pull_request 1, search_issues 2 (one positive control, one dedup), issue_write 1, add_issue_comment 1. Card body and comments were read through the zero-quota public-repo payload channel; the repo-scoped REST probe returned 403 for this seat and gh is absent, so dedup went through one targeted MCP search after a known-hit control query returned #12923 as its top result.", "open_questions": [ { "question": "Should this line be `warn` or `error`? AGENTS.md's degradation-log-level rule points at `error` for this exact shape ('a write that claims to persist does not, while the system still looks normal from the outside'), but the card, the dispatch scope pin and the clause-2 grading all say WARNING. I implemented `warn` as pinned and did not quietly pick the other side.", "options": [ "A. Keep `warn` (shipped). Matches the card's wording, the scope pin, and the house style of the sibling warnOrganizationLessRows in the same file; SeedLogger declares only info/warn today, so no sink widening is needed.", "B. Promote to `error`, widening SeedLogger with an optional `error` that falls back to `warn` when the host injected a reduced sink (the #9657 shape — `logger.error?.()` with no fallback is never correct).", "C. Split: `error` for the unique-violation class (a catalog that did not land is a durability degradation) and `warn` for the other class." ], "recommendation": "A for this PR, because the pin is explicit and clause-2 depends on it; then C as a follow-up if the maintainer agrees, since the unique-violation class is precisely the AGENTS.md `error` shape — persisted state and runtime state disagree while the boot looks healthy — and the other class is often a plain outage that retries. B alone would over-apply the level to both classes, which is the 'trains everyone to skim error' failure the same section warns about." }, { "question": "Nothing shipped can name the colliding INDEX to a caller, and the card asks for it by name. uniqueViolationColumn refuses index names by the 2026-08-08 ruling; the only place the index survives is the free-text tail that redactStatementFromMessage preserves, and that function lives in @objectstack/objectql — a devDependency of plugin-security, not exported from objectql's index. Should an index accessor exist?", "options": [ "A. Ship nothing new (what this PR does): point the operator at the query engine's own already-redacted 'Insert operation failed' entries, which keep the identifier. Zero new contract.", "B. Add `uniqueViolationIndex(error)` to @objectstack/types beside uniqueViolationColumn — gated on isUniqueViolationError exactly as the column extractor is, answering the DIFFERENT question 'which index', never converting one into a column. New public export, new api-surface entry.", "C. Export redactStatementFromMessage from @objectstack/objectql's index and promote objectql to a runtime dependency of plugin-security, so callers can safely render the driver tail themselves." ], "recommendation": "A for this card — it is complete, adds no contract, and does not guess. B is the right long-term shape if operators find the pointer indirect, but it is a public-surface decision sitting directly beside a maintainer ruling that deliberately withheld index names from that module, so it belongs to the maintainer and not to this lane. C is the worst of the three: it adds a runtime package edge to publish one string formatter." } ], "out_of_scope_findings": [ "filed as #12970: two more swallowed `tryUpdate` refusals in plugin-security 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 discards the write's result entirely on the degraded-kernel branch, then logs '[security] package-declared permission set 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 tryUpdate already takes an optional refusal log." ] }
Generated by Claude Code
os-dev-report
Patch round 2 — supersedes the report above. Change: the log level is now SPLIT per class.
{ "issue": 12923, "status": "done", "branch": "claude/issue-12923-rbac-seeder-silent-unique-violation", "pr": "https://github.com/objectstack-ai/objectstack/pull/12967", "premise_still_valid": true, "summary": "Patch round implementing option C on the log-level question. `unique-violation` now reaches `error`, `other` stays `warn`. The AGENTS.md degradation rule decides it: a boot that logs 'RBAC catalog seeded' at info over zero landed rows is exactly the 'still looks normal from the outside while something it claims is persisted did not land' shape, and the non-unique class is a retrying outage that the same section's 'do not over-apply it' paragraph keeps functional. `never THROW` is untouched and still the fence — nothing rethrows. SeedLogger gains an OPTIONAL `error` carrying the kernel Logger arity (`message, cause?, meta?`), matching the sibling ProjectionLogger and `Logger.error` in @objectstack/spec/contracts — I checked the arity deliberately, because declaring `(m, meta)` would have put the meta object in the cause slot where a Logger neither reads nor serializes it. Every emission routes through ONE named helper, `logSeedDurabilityFailure`, spelled `if (logger?.error) logger.error(msg, undefined, meta); else logger?.warn?.(msg, meta)` — mirroring SqlDriver.logDurabilityFailure, whose doc records why neither `logger.error?.(...)` (silence against a reduced sink) nor `(a ?? b)(...)` (detached receiver; @objectstack/core's ObjectLogger is class-based and reaches for `this.writeErrorLike`) is acceptable. The error line now carries both halves the rule demands in its first sentence: the consequence, including the explicit 'THE DEPLOYMENT WILL GO ON LOOKING HEALTHY', and the fix. ONE THING I DID BEYOND THE INSTRUCTION, flagged for your objection: I renamed `warnSeedWriteRefusals` to `reportSeedWriteRefusals`, because after the split a function named `warn*` that emits `error` for one of its two classes misdescribes itself. Mechanical — five call sites plus tests, no behaviour in it. I also declared the new optional `error` member on the two seeders whose inline SeedOptions.logger types were narrow (`{info, warn}`), so the channel the seeder reaches for is visible in its own options rather than only inside the reporter; the other three already route through ProjectionLogger, which had the right shape already. Everything else is unchanged from the delivered PR.", "tests": "ALL on the final commit eddfb237f, clean tree, exit codes captured BEFORE any pipe. (a) pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 -> 'Test Files 88 passed (88)' / 'Tests 1604 passed (1604)'; the pin file itself is 19 cases, up from 14. (b) package typecheck exit 0 — still NOT covering the test file (tsconfig excludes '**/*.test.ts'), same caveat as before. (c) pnpm lint repo-wide, NOT narrowed, exit 0 no findings. (d) THE THREE PINS YOU ASKED FOR, all passing: a unique violation reaches the `error` sink and never `warn` (asserted through the real seeder as well as the helper); a non-unique refusal reaches `warn` and never `error`; and a host that injected ONLY `warn` still receives the unique-violation message whole, remedy included — the case a bare `logger?.error?.(...)` answers with silence. Plus two more I added: the error line contains both the consequence and the fix, and the cause slot stays `undefined` with the detail in meta (a summary of N refusals has no single cause). (e) Gate families re-derived from the actual changed set (still the same 10 paths, no stale-tree warning this round) and re-run: check:engine-double-contract, check:where-matcher '312 matcher(s) discovered, 312 answer the combinator battery correctly or refuse it loudly', check:objectql-double-limit '289 double(s) graded, 89 apply the caller's bound or refuse it loudly', check:type-check-coverage 'OK — 65/78 workspace packages type-checked', check:type-check-debt --re-measure 'OK — 31 ledger entr(ies) re-measured in 286.5s, 1570 raw tsc error(s) total, none above its recorded number', check:i18n 'OK (9 package(s) — all bundles in sync)', plus 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-states, check-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-test — all exit 0. (f) NOT MEASURED: scripts/pm/check-half-states.mjs exit 3 PREREQUISITE NOT MET (no valid GitHub credential), unrelated board gate. check:i18n and check:type-check-debt again returned PREREQUISITE NOT MET first because the RECREATED worktree had no built CLI/closure; built and re-ran both to real readings, and only those are quoted. (g) ABLATION RE-RUN on the patched tree — I did not carry the previous round's numbers forward, because they described a test file that no longer exists. Same script, same discipline: HEAD_BLOB 10e2875383892727a2ea1df65c102aca25bc0332 == PRE_HASH (asserted equal before mutating) -> POST_HASH 98c8c518f4f757060be3ba7b5d6ca507471f0bdc, both anchors counted in both directions (deleted 1->0, injected 0->1), aborting the reading on any mismatch. Result 'ABLATED_VITEST_EXIT=1 · Tests 5 failed | 14 passed (19)', predicted before running and matched exactly: the four seeder-driven cases from round one PLUS the new 'reaches the durability channel through the real seeder' pin — which is precisely the one written to prove the split is not helper-only, so its going red is the reading that matters here. Restored via git checkout HEAD -- ABSOLUTE_PATH under trap ... EXIT INT TERM with an absolute repo root; verified by hash equality with the HEAD blob plus empty git diff HEAD and empty git status --porcelain.", "mcp_calls": "8 total for the whole card — 5 in round one, 3 in this patch round (update_pull_request 1, add_issue_comment 1, plus this one). No searches were needed this round.", "open_questions": [ { "question": "Your point 3, answered with measurements rather than an opinion: should these seams be added to `DURABILITY_CRITICAL_CALLEES` so the gate actually covers them? I did NOT do it unasked, and I do not think it is cheap or shrink-only.", "options": [ "A. Leave the gate alone (what I did). The PR body states plainly that the gate's green over this file means NOT MEASURED for this site, never 'level approved'.", "B. Add `insert`/`update` to the vocabulary. Measured cost: the vocabulary is 18 SPECIFIC function names and the gate's own header already excludes `find`/`findOne`/`count` as 'too generic to declare repo-wide'; `.insert(` has 156 non-test call sites across packages/, so it is squarely in that excluded category.", "C. Add the seeder-local helper names (`tryInsert`/`tryUpdate`). Measured cost: 15 non-test files outside this card's five seeders still declare the same `catch { return null; }` shape and would all redden at once — and there is nowhere to park them, because scripts/durability-degradation.baseline.json is shrink-only and DELIBERATELY EMPTY, its header stating an entry means 'a REAL degradation that is not yet fixed'. Admitting 15 would assert 15 real unfixed degradations nobody has reviewed." ], "recommendation": "A now, and a dedicated card for the widening if you want the coverage. B is refused by the gate's own stated design; C is a real repair programme (15 files) wearing the costume of a one-line vocabulary edit, and doing it inside this card would either redden CI or grow a baseline whose whole point is being empty. #12970, which I filed last round, is already two of those 15." }, { "question": "The rename `warnSeedWriteRefusals` -> `reportSeedWriteRefusals` is the one thing I did that you did not ask for. Flagging rather than burying it.", "options": [ "A. Keep the rename (shipped). A `warn*` name that emits `error` for one class is the kind of misdescription this repo pays for later.", "B. Revert to the old name — one mechanical sed across five call sites and the tests." ], "recommendation": "A, but this is entirely yours and B costs one command. Nothing else in the diff depends on the name." } ], "out_of_scope_findings": [ "filed as #12970 in round one, unchanged and still open: two more swallowed `tryUpdate` refusals in plugin-security 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 discards the write's result entirely on the degraded-kernel branch, then logs a 'sanctioned operator action' audit line with before/after counts that are equal — an audit record asserting an action that never landed. Note these are two of the 15 files named in the vocabulary-widening measurement above." ] }
Generated by Claude Code
os-dev-report
Patch round 3 — supersedes the reports above. Change:
SeedLogger.warnis now non-optional, plus the pin, plus a correction to my own round-2 reasoning.{ "issue": 12923, "status": "done", "branch": "claude/issue-12923-rbac-seeder-silent-unique-violation", "pr": "https://github.com/objectstack-ai/objectstack/pull/12967", "premise_still_valid": true, "summary": "Fixed the CI red exactly as the gate specified: dropped the `?` from `warn` on SeedLogger. `error` stays optional; I did not make `error` required (the gate names that as falsified option C) and did not satisfy it with a required `info`. VERIFIED RATHER THAN ASSUMED, as you asked: the package source typechecks clean with `warn` required, so all five seeders' construction sites already carried `warn` — no `as` cast was added anywhere, nothing was widened, and no call site turned out to be unable to supply it. Only one value in the tree could not satisfy the new type: my own test's `reportSeedWriteRefusals({}, ...)` case, which is now correctly unrepresentable — I deleted that case rather than casting around it, because its whole subject moved into the type. THE PIN TOOK THREE ATTEMPTS AND THE TWO FAILURES ARE THE INTERESTING PART. (1) I first wrote it as an AST read seeded from `import.meta.url`; that is TS1470 under this package's CommonJS resolution and pushed the shrink-only TEST_DEBT ratchet 11 -> 12. (2) I then switched to `@ts-expect-error`, reasoning that the ratchet's re-measure program DOES compile test files (it lifts the tsconfig test exclusion) so the directive would be evaluated. `check:type-check-coverage` refused it by name: it judges against the program the `typecheck` SCRIPT runs, not the re-measure program, and PHANTOM_PIN_DEBT is closed to new entries — its own remedy text says 'replace the pin with a runtime assertion'. (3) Final shape: a runtime AST assertion over the declaration, seeded from `__dirname`. Zero added tsc errors, ratchet back at 11. I am flagging attempt (2) explicitly because I asserted in the round-2 PR body that no program compiles these tests — that claim was too strong and is now corrected in the body: the package's own typecheck does not, the ratchet's re-measure does, and the gate cares about the former.", "tests": "ALL on the final commit bbb160f8d, clean tree, exit codes captured BEFORE any pipe. (a) pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 -> 'Test Files 88 passed (88)' / 'Tests 1609 passed (1609)'; the pin file is 24 cases. (b) package typecheck exit 0. (c) pnpm lint repo-wide, NOT narrowed, exit 0 no findings. (d) THE GATE YOU NAMED, run explicitly: `node scripts/check-optional-error-sink-contract.mjs` exit 0, judgment line '✓ optional-error sink contract: every sink declaring an optional `error` guarantees a `warn` channel (1 baselined, shrink-only)'. Its census now places SeedLogger in the '29 declare it optional beside a REQUIRED warn' bucket; the 1 remaining baselined entry is PRE-EXISTING and not this diff — I checked, it is not mine. (e) check:type-check-debt --re-measure 'OK — 31 ledger entr(ies) re-measured in 361.5s, 1570 raw tsc error(s) total, none above its recorded number' with 'surplus: none'; and I independently reproduced the test-inclusive count at 11 (== the recorded TEST_DEBT), my file contributing ZERO. (f) Every other derived family re-run and green: check:engine-double-contract, check:where-matcher '312 matcher(s) discovered, 312 answer the combinator battery correctly or refuse it loudly', check:objectql-double-limit '289 double(s) graded, 89 apply the caller's bound or refuse it loudly', check:type-check-coverage 'OK — 65/78 workspace packages type-checked', check:i18n 'OK (9 package(s) — all bundles in sync)', check:durability-log-level, 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-states, check-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-test — 26 commands, all exit 0. (g) NOT MEASURED: scripts/pm/check-half-states.mjs exit 3 PREREQUISITE NOT MET (no valid GitHub credential); check:i18n and check:type-check-debt again needed a build first in the recreated worktree and were re-run to real readings. (h) SECOND ABLATION, on the pin itself, because a pin nobody has seen fail is decoration: put the `?` back on SeedLogger.warn, HEAD_BLOB 16868ccfc38f9eb2a57f0c7d39a617bb8c008673 == PRE_HASH -> POST_HASH 19618bb86b63c3aa264928f3d78e0afdb2d2bed8, markers 1->0 and 0->1, mutation confirmed on disk. Result: EXACTLY ONE case red ('declares `warn` NON-optional — so no value of the type can be silent'), and in the same run `check:check-optional-error-sink-contract` went red reproducing YOUR CI message verbatim ('`error` is optional and `warn` is optional too — every value of this type may print nothing'). So the pin is precisely targeted rather than a cascade, and I verified locally that this fix is the one that closes the CI failure rather than merely adjacent to it. Restored via git checkout HEAD -- ABSOLUTE_PATH under trap ... EXIT INT TERM with an absolute repo root; hash equality plus empty git diff HEAD and empty git status --porcelain. The first ablation (the repair itself) was also re-run on this tree: 'Tests 5 failed | 14 passed', same predicted direction.", "mcp_calls": "10 total for the card — 5 in round one, 3 in round two, 2 in this round (update_pull_request, add_issue_comment). No searches needed this round.", "open_questions": [ { "question": "A correction I owe you, not a request: my round-2 PR body claimed 'no tsc program compiles these tests'. That is wrong as stated and I have fixed the body. The package's own `typecheck` script does not compile them; `check:type-check-coverage --re-measure` does, by lifting the tsconfig exclusion into a temp project. The distinction matters because the two gates disagree about what counts.", "options": [ "A. Leave it as documented in the PR body (done): the pin is a runtime assertion, and the body now states which program does what and why the gate refused the directive.", "B. Additionally graduate plugin-security's tests into a real tsc program (drop the exclusion, or add a sibling tsconfig.test.json named by the typecheck script — the #5286 route). That is what would make `@ts-expect-error` pins legitimate in this package at all." ], "recommendation": "A for this card. B is genuinely worth doing — it is the difference between 11 frozen test-layer errors being debt and being invisible, and it would unlock type-level pins for every future author in this package — but it surfaces all 11 existing errors into the typecheck script on the PR that does it, so it is its own card with its own budget, not a rider here." } ], "out_of_scope_findings": [ "filed as #12970 in round one, unchanged and still open: two more swallowed `tryUpdate` refusals in plugin-security 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 discards the write's result entirely on the degraded-kernel branch, then logs a 'sanctioned operator action' audit line with before/after counts that are equal — an audit record asserting an action that never landed." ] }
Generated by Claude Code
os-dev-report
Patch round 4 — supersedes the reports above. Change: merged
origin/main, resolved the one conflicted file keeping both sides. No code change of my own this round.{ "issue": 12923, "status": "done", "branch": "claude/issue-12923-rbac-seeder-silent-unique-violation", "pr": "https://github.com/objectstack-ai/objectstack/pull/12967", "premise_still_valid": true, "summary": "Merged origin/main into the branch and resolved the single conflicted file, bootstrap-platform-admin.ts. The conflict was purely import adjacency — db39dfc1c (#13065) and this card both added an import immediately after './claim-seed-ownership.js' — so BOTH sides are kept verbatim. I did not drop their matchesDeclaredOwnerEmail import or its call site, and I did not restore the inline String(u.email ?? '').trim().toLowerCase() compare: it is at 0 occurrences in the resolved file, so nothing of #13065 is un-landed. Rather than eyeballing that, I proved it in BOTH directions: the resolved file diffed against origin/main shows ONLY this card's additions, and diffed against my pre-merge head shows ONLY #13065's three lines (the import, a three-line comment, and the swapped predicate). scripts/engine-double-contract.pinned.json auto-merged and was NOT hand-edited — I verified it still parses and that my pin row (seed-write-refusal.test.ts, verb update, pinned 1) survived among its 647 entries. Merge commit, never a rebase/amend/force-push, so any other checkout of this branch stays valid. The merge moved pnpm-lock.yaml, so I reinstalled --frozen-lockfile, cleared packages/runtime/.objectstack and rebuilt before trusting any gate — the stale-artefact discipline, not optional after a merge. The branch now merges cleanly with current origin/main (git merge-tree produces a tree with no conflict), and the derived change set is still exactly this card's 10 paths, now against merge base db39dfc1c.", "tests": "ALL on the merge commit b8651025a, clean tree, exit codes captured BEFORE any pipe. (a) vitest -> 'Test Files 89 passed (89)' / 'Tests 1619 passed (1619)' — up from 88/1609 because #13065 brought its own test file; both changes coexist. (b) package typecheck exit 0. (c) pnpm lint repo-wide, NOT narrowed, exit 0 no findings. (d) check:optional-error-sink-contract re-run explicitly as asked, since the merge touches the file it judges: exit 0, '✓ optional-error sink contract: every sink declaring an optional `error` guarantees a `warn` channel (1 baselined, shrink-only)'. (e) Every derived family re-run on the merged tree — 25 gate commands, all exit 0: check:engine-double-contract 'OK — 705 pinned, 134 in the DEBT ledger, 3 exempt'; check:where-matcher '314 matcher(s) discovered, 314 answer the combinator battery correctly or refuse it loudly'; check:objectql-double-limit '291 double(s) graded, 91 apply the caller's bound or refuse it loudly'; check:i18n 'OK (9 package(s) — all bundles in sync)'; check:type-check-coverage 'OK — 65/78 workspace packages type-checked'; 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' with 'surplus: none'; plus check:durability-log-level, 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-states, check-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-test. (f) ABLATION 1 RE-RUN ON THE MERGED TREE as you asked: HEAD_BLOB 10e2875383892727a2ea1df65c102aca25bc0332 == PRE_HASH, POST_HASH 98c8c518f4f757060be3ba7b5d6ca507471f0bdc, both anchors 1->0 and 0->1, mutation confirmed on disk. 'ABLATED_VITEST_EXIT=1 · Tests 5 failed | 19 passed (24)' — the SAME five cases red as before the merge; the green count moved 14 -> 19 only because the pin file grew from 19 to 24 cases in round 3, not because anything about the ablation changed. (g) ABLATION 2: I did NOT simply re-quote it. Both ablation subjects are byte-identical across the merge (I checked the blob hashes: per-organization-catalog.ts and bootstrap-declared-positions.ts are unchanged), but I re-ran it anyway — and that caught a real instrument bug, reported below.", "mcp_calls": "12 total for the card — 5 round one, 3 round two, 2 round three, 2 this round (update_pull_request, add_issue_comment).", "open_questions": [ { "question": "Not a question — a self-caught near-miss you should know about, because it is the exact failure class this lane keeps recording. Re-running ablation 2 on the merged tree first reported a clean 11 errors and NO red at all, which reads as 'the pin does not fire'. The tree was fine; my ablation SCRIPT was stale. It measured through `tsc`, which was the correct instrument for the `@ts-expect-error` shape the pin had in round 3's first attempt, and the wrong one for the runtime AST assertion the pin has now. A green produced by a stale instrument is indistinguishable from a green produced by working code.", "options": [ "A. What I did: recognise the 11 as an instrument reading rather than a result, rewrite the ablation to drive vitest plus the sink gate, re-run, and record the near-miss in the PR body so a reviewer sees why the number moved.", "B. Have accepted the 11 and reported ablation 2 as 'unchanged by the merge' — which would have been literally true about the subject file and false about the measurement." ], "recommendation": "A, done. Corrected reading: 'ABLATED_VITEST_EXIT=1 · Tests 1 failed | 23 passed (24)', exactly one case red ('declares `warn` NON-optional'), and in the same run check:optional-error-sink-contract went red reproducing the CI message verbatim. The transferable lesson is narrower than 'ablations go stale': an ablation script encodes an ASSUMPTION ABOUT THE PIN'S MECHANISM, so changing a pin's mechanism invalidates its ablation even when the subject file, the mutation and the hashes are all untouched. Nothing in the hash discipline catches that — every hash check passed." } ], "out_of_scope_findings": [ "filed as #12970 in round one, unchanged and still open: two more swallowed `tryUpdate` refusals in plugin-security 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 discards the write's result entirely on the degraded-kernel branch, then logs a 'sanctioned operator action' audit line with before/after counts that are equal — an audit record asserting an action that never landed." ] }
Generated by Claude Code
MERGED — verified by content on
origin/main@901355c3bPR #12967 landed. ⛔ Read out of the merged tree, never off the merge event or head-SHA ancestry.
The repair is there:
per-organization-catalog.tsreturns 6 hits forreportSeedWriteRefusals/logSeedDurabilityFailure.The type guarantee that was CI-red is there — the whole point of the last round:
:89 info?: (m: string, meta?) => void; :106 warn: (m: string, meta?) => void; ← non-optional :126 error?: (m: string, error?: Error, meta?) => void;⇒ No value of
SeedLoggercan be silent.errorstays optional so reduced sinks remain representable, and the fallback is now guaranteed by the type rather than by convention.And the change this branch had to merge across survived intact. #13065 (
verified platform owner bypasses the Layer 0 org wall) landed on main mid-flight and touched the same file. On main now:matchesDeclaredOwnerEmail2 hits, the inlinetrim().toLowerCase() === wantedcompare 0 hits — nothing of that security change was un-landed to make this one apply.pm:dispatchedstripped, assignee released.Corrections this card produced, recorded because two were mine
My dispatch order asserted two things that were wrong, both caught by measurement and both consequential:
-
driver-fault-redaction.tsgoverns the server LOG, not the REST boundary. A boot log is the surface Undeclared fields still reach the driver: beforeInsert hooks run and an auto-number is consumed before the request is refused, and the whole INSERT with its values is logged at ERROR (17.0.0 GA) #8682 closes. Had the order been followed, the warning would have re-opened that exposure. The shipped line reads only the value-freecode/errnochannel, with a canary test proving the bound statement cannot reach it. -
uniqueViolationColumndoes not name the colliding index — by the 2026-08-08 ruling it returnsundefinedprecisely when the dialect named an index, which is the usual case here. A warning built on my framing would have named nothing in exactly this defect's shape. -
My scope pin said "WARN, never THROW", conflating two claims. Never-throw stands. The level did not: AGENTS.md → Degradation log levels puts a boot that logs "RBAC catalog seeded" over zero landed rows squarely in the
errorclass. Shipped as the split —errorfor unique-violation,warnfor the rest.
⚠️ And the count: the swallowing helper was in five places, not the three my order named. The exported pair inpermission-set-projection.tswas the miss.⭐ One instrument lesson worth carrying
Re-running the type-guarantee ablation on the merged tree first reported zero red, which reads as "the pin does not fire". The tree was fine; the ablation script was stale — it measured through
tsc, the right instrument for the@ts-expect-errorshape the pin had briefly, and the wrong one for the runtime AST assertion it ended up with. Corrected reading: exactly one case red, and the sink gate reproducing the CI message verbatim.The transferable form is narrower than "ablations go stale":
An ablation script encodes an assumption about the PIN'S MECHANISM. Changing a pin's mechanism invalidates its ablation even when the subject file, the mutation and every hash are untouched.
⚠️ Every hash check passed.PRE_HASHequalled the HEAD blob,POST_HASHdiffered, both marker counts moved in both directions. That discipline proves the mutation reached the disk; it says nothing about whether the right thing was measured. A green from a stale instrument and a green from working code are indistinguishable.Out of scope, filed not fixed
- The durability log-level gate cannot see the
catch { return null; }seeder family — 15 files outside #12923's five, and neither widening path is cheap #12981 — the durability log-level gate cannot see thiscatch { return null; }family: 15 more non-test files carry it, and neither widening path is cheap (insertis in the vocabulary's own excluded "too generic" class at 144 non-test call sites; the baseline is shrink-only and deliberately empty). - plugin-security: two more swallowed
tryUpdaterefusals outside the catalog seed — drift diagnostics vanish silently, and the overlay-discard audit line reports an action whose write was refused #12970 — two more swallowedtryUpdaterefusals outside the catalog seed, including an audit line asserting a "sanctioned operator action" whose before/after counts are equal. - Not filed, recorded here so it is not lost:
plugin-security'stsconfig.jsonexcludes**/*.test.ts, so its 11 test-layer type errors are invisible to thetypecheckscript and live only in the ratchet's re-measure. Graduating those tests into a real tsc program would unlock type-level pins for every future author in this package — and would surface all 11 on the PR that does it, so it wants its own card and its own budget.
Generated by Claude Code
-
The capability-loader-silent-fail class, measured end to end on cloud#1695: with a pre-#8556 platform-wide unique index present, every per-organization catalog INSERT is refused, the seeder's catch swallows it, and the boot log reads as a successful seed of zero rows — for weeks, on a deployed plane. The framework's own
legacyUniqueReplacementsdoc (driver-sql schema-drift.ts) describes the operator-side half; the seeder side must be LOUD: a unique-violation during catalog seeding is a deployment-schema defect and should surface as a boot-visible warning naming the colliding index and the migrate remedy — never a silent zero. Evidence, repro recipe and the exact log lines are in cloud#1695's diagnosis comment. Part of objectstack-ai/cloud#1653 chain.