Skip to content

fix(plugin-security): report the two swallowed tryUpdate refusals outside the catalog seed - #13177

Merged
os-elon merged 2 commits into
mainfrom
claude/issue-12970-tryupdate-refusal-log
Aug 29, 2026
Merged

fix(plugin-security): report the two swallowed tryUpdate refusals outside the catalog seed#13177
os-elon merged 2 commits into
mainfrom
claude/issue-12970-tryupdate-refusal-log

Conversation

@os-elon

Copy link
Copy Markdown
Collaborator

Fixes#12970

Both sites call the shared tryUpdate in permission-set-projection.ts, which answers false on refusal — an answer byte-identical to "nothing to do". That helper already accepts an optional refusal log; neither caller passed one.

The card's premise, re-verified on origin/main at edit time

Confirmed, with one correction the dispatching PM had already measured:

  • tryUpdate (permission-set-projection.ts:141-142) already takes a trailing refusals?: SeedWriteRefusals. True.
  • The reporter the card names, warnSeedWriteRefusals, has 0 hits repo-wide; it does not exist. The real API is createSeedWriteRefusals / reportSeedWriteRefusals in per-organization-catalog.ts. The card's central claim — the channel exists, neither caller passes one — holds; only the reporter's name was wrong.
  • Site 1's suppressor is really the updated > 0 gate in runPermissionSetDriftDiagnostics, confirmed by reading it rather than assuming: the equality gate above it means a steady-state boot writes nothing, and a wholly refused boot produced the same zero.

Site 1 — permission-set-drift.ts

persistPermissionSetDriftDiagnostics counted only writes that landed; runPermissionSetDriftDiagnostics reported only when that count was non-zero. A boot on which every drift write was refused computed the drift correctly, persisted none of it, and printed nothing — indistinguishable from a deployment with no drift.

  • the pass now records refusals and answers a refused count beside updated;
  • refusals are reported once per pass on the durability channel;
  • the gate becomes updated > 0 || refused > 0, so the line that names the drifted sets is no longer hidden behind the counter the refusal suppressed. A steady-state boot (nothing to write, nothing refused) stays exactly as quiet as before — pinned by a counter-direction test.

Site 2 — permission-set-overlay-discard.ts

On the degraded-kernel branch the resync write's result was discarded entirely. On refusal the row was re-read unchanged, so objectGrantsAfter equalled objectGrantsBefore while the info entry still announced a completed "sanctioned operator action": every field individually true, the entry as a whole false.

The result is now read. A refused resync emits one entry stating what did and did not land — the overlay deletion, which had already succeeded, and the refused resync, with the un-healed grant count named as such — instead of the success line, never alongside it.

Emitted-with-the-failure-stated rather than withheld, deliberately: the destructive half of the operator action did land (that leg rethrows, so reaching the audit point means the overlay rows are gone). Withholding the entry would erase the record of a deletion that actually happened, a worse audit defect than the optimistic one this repairs.

The caller contract is untouched, per the dispatch ruling. No fork arose: the audit line can be made fully honest in the log, and healedObjectGrantCount is documented as "what the row carries now", which stays literally true. Only its doc gained a warning that a refused resync leaves it equal to the pre-discard count.

Declared deviation from the suggested route

The suggested route was to report through reportSeedWriteRefusals. These sites reuse the shared accumulator (createSeedWriteRefusals — the shipped cross-dialect classification and the value-free code/errno channel, never a local regex) but not that reporter, because every sentence it prints is about seeding the RBAC catalog: "the catalog is INCOMPLETE", "this pass's seeded count", and a remedy naming the legacy platform-wide unique index on the catalog name column. Neither site seeds anything — site 1 writes two diagnostic columns onto existing rows by id, site 2 re-projects one row. Printing that text over these failures would send an operator to os migrate for a defect that is not there, which is the same "a confident wrong answer is worse than no answer" reasoning that makes other its own class in that function rather than a relabelled unique violation. Each site emits site-appropriate prose and carries the machine facts (object, class, count, driverCodes, columns) in meta. Pinned: the drift line asserts it does not contain "RBAC catalog".

Both lines go through the shared durability helper, which is why logSeedDurabilityFailure is now exported from per-organization-catalog.ts (intra-package only, deliberately not added to index.ts). Re-deriving the if (logger.error) ... else logger.warn spelling locally is what its own doc forbids — "lives in logSeedDurabilityFailure so no site can forget it". Both sites are pinned against a reduced sink with no error.

Log level

AGENTS.md "Degradation log levels", one question — after the degradation, does the system still look normal while something it claims is persisted has not landed? Yes on both: drift_status/drift_detail are what Setup's "Needs Attention" surface reads, and the discard's caller is answered normally with a 200 by ruling while the row keeps its pre-discard grants.

check:durability-log-level does not vouch for either choice, and this is stated in-code at both sites. Its verdict line reads 29 durability-critical catch seam(s), all loud and its summary limb names exactly 3 sites, none of them these — ql.update is not in its DURABILITY_CRITICAL_CALLEES vocabulary. Green here means the sites are outside the gate's reach (NOT MEASURED), never approval.

Evidence

Negative control (ablation). Implementation committed first, then the three source files reverted to the pre-fix base with the tests left at HEAD:

  • mutation confirmed on disk, not by exit code — marker counts reportDriftWriteRefusals 3 to 0, resyncRefused 3 to 0, export function logSeedDurabilityFailure 1 to 0; on-disk hash of the drift module equal to the base blob hash;
  • tests confirmed unmutated (git diff --stat HEAD over both test files empty);
  • no rebuild needed, and proven rather than asserted: the suites import their subject through same-package relative specifiers that vitest resolves to src/*.ts. The mutated run going red with no rebuild is that proof — a dist-resolved suite would have stayed green;
  • direction predicted before running, and matched: 5 failed, 16 passed. The one new test that stayed green is the overlay counter-direction case (a write the store accepts still emits the unchanged success line), which is what shows the new tests are not vacuously red;
  • restore proven by state: git diff HEAD empty, on-disk hash equal to the HEAD blob hash, marker count back to 3. Restore ran under a trap ... EXIT INT TERM with absolute paths.

Suites, at fc6df0b2b (the commit every number below was measured at, and the branch head):

  • pnpm --filter @objectstack/plugin-security testTest Files 89 passed (89), Tests 1625 passed (1625).
  • The two affected files alone: 21 passed (15 pre-existing plus 6 new).
  • pnpm --filter @objectstack/plugin-security typecheck — exit 0.

A NOT MEASURED result reported as such. That typecheck says nothing about the new test code: this package's tsconfig.json excludes **/*.test.ts, and tsc --noEmit --listFiles returns 0 hits for any .test.ts file. Measured separately through a throwaway sibling config at the package's own strictness: 11 pre-existing errors across 6 other test files, 0 in either file this PR touches. The standing gap is filed as #13176 and is not addressed here.

Gates, derived from the actual changed set with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (27 matched families plus the convention-triggered ones), exit codes captured before any pipe. All green:

check:nul-bytes · check:logger-receiver-detach · check:engine-double-contract (OK, 705 pinned) · check:where-matcher (314 matchers, 0 silently-wrong, baseline verified against 1a540e8) · check:objectql-double-limit · check:durability-log-level · check:dispatcher-error-vocabulary · check:cross-package-test-inputs · check:test-source-alias · check:query-options-erasure · check:changeset-gate-self-tests · check:objectui-changeset · check:pm-half-states · check:published-files · check:page-declaration-shape · check:slot-lookup · check:type-source-resolution · check:i18n (OK, 9 packages in sync — required building the CLI first) · check:i18n-stale-fill · check:type-check-coverage · check-adr-0087-registration · check-changeset-no-major · check-ci-filter-parity · check-comment-mask-adoption · check-empty-changeset · check-keyed-text-bounds · check-plugin-teardown-shape · check-shard-attestation · check-undeclared-dep-imports · docs-audit/check-affected-docs.

Both test doubles were extended in place rather than duplicated, because this file pair is pinned 1-update-double-per-file in scripts/engine-double-contract.pinned.json; the injected refusal throws afterassertEngineUpdateDispatch accepts the call shape, which is the real failure order and the only one tryUpdate's catch sees.

NOT MEASURED, named rather than counted as passes:

  • check:type-check-debt — refuses outright: 39 workspace dependencies have no built type entry point in this worktree, and its own message says a number recorded from there measures a different world. Needs the full closure build that lint.yml performs before this step. CI measures it.
  • check-test-completeness — a bare local invocation is a usage error; it takes a turbo test log CI supplies.
  • scripts/pm/check-half-states.mjs — exit 3, PREREQUISITE NOT MET: this container has no usable GitHub credential, so nothing was swept. Unrelated to this tree, and the same measurement is why the duplicate search for @objectstack/plugin-security excludes **/*.test.ts from its only tsc program — 89 test files unchecked, 11 standing type errors behind a green typecheck #13176 went through one targeted MCP query rather than REST.

Out of scope

Untouched by ruling and confirmed untouched in the diff: bootstrap-system-capabilities.ts (its refusal case is already counted and warned) and auto-org-admin-grant.ts (already logs on refusal). Whether the overlay-discard path should propagate to its caller instead of logging is left open by the card and deliberately not decided here.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/api/error-catalog.mdx(via sys_permission_set (literal))
  • content/docs/permissions/authorization.mdx(via sys_permission_set (literal))
  • content/docs/permissions/delegated-administration.mdx(via sys_permission_set (literal))
  • content/docs/permissions/permission-sets.mdx(via sys_permission_set (literal), /security/permission-sets/:id/discard-overlay (route))

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

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

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

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 596b20a6ac26c94e185f0fb36b7b5f30cb56e7d0packageMentionDocs.

Which tree this was computed on

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

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

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

2 participants

@os-elon@claude