Skip to content

fix(plugin-audit): read-audit reports once per CAUSE, not once per process - #18595

Merged
os-project-manager merged 1 commit into
mainfrom
claude/issue-18247-read-audit-third-copy
Sep 17, 2026
Merged

os-project-manager merged 1 commit into
mainfrom
claude/issue-18247-read-audit-third-copy

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Fixes #18247

reportReadAuditWriteFailure (packages/plugins/plugin-audit/src/read-audit.ts) carried the THIRD independent copy of one defect pair — the pair #15166 fixed in audit-writers.ts and #17452 fixed in auth-event-audit.ts. This PR removes the duplicate; it does not write a fourth implementation. The two helpers #18246 exported for exactly this purpose are imported.

Clause-②: no — no export is added, no error code is added, and nothing is relaxed. Two already-exported helpers are imported and one existing message literal becomes conditional; git diff adds no export in packages/.

The two defects, both live on a seam the repo already declared durability-critical

  1. Its own process-level failureReported boolean. The first failure of ANY cause silenced every later failure of every OTHER cause for the life of the process. Record-view rows are written from a BUFFER off the request path, so there is no in-flight request left to notice, and the shipped record_views list view answers "who viewed this record" with a confident, wrong, SHORT list.
  2. Its own fixed message literal, printing the ADR-0057 §3.6 / OS_TELEMETRY_DB datasource guidance unconditionally — so a fault with nothing to do with datasource routing (an ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED refusal, say) sent the operator to check something that was working.

Its callee persistReadAuditRows is registered in DURABILITY_CRITICAL_CALLEES (scripts/check-durability-degradation-log-level.mjs), so the repo has already declared this write durability-critical. A reporter that switches itself off after one cause is exactly the failure that declaration cannot afford.

What changed

  • The dedupe key is now auditFailureCauseKey, imported from audit-writers.ts rather than re-spelled — a second copy of the key is how this defect reached the second file. The counting unit is a DEGRADATION, and a second cause is a second degradation.
  • The object dimension of the shared key is the ledger (sys_audit_log), not the viewed object. One flush is ONE write carrying rows about MANY audited objects, so there is no single viewed object to name, and picking whichever landed first in the batch would make the key depend on traffic — the one property auditFailureCauseKey exists to deny. The key therefore reduces to the driver's code vocabulary: bounded by construction. (audit-writers.ts passes ctx.object and auth-event-audit.ts its constant sys_session because on those two seams the audited object IS single-valued per write.)
  • The first line now leads with auditFailureCauseSummary(err, detail), so the driver's code and message reach the operator instead of being computed and dropped.
  • The ADR-0057 §3.6 remedy is asked for through the shared isMissingTableError predicate and printed for exactly the missing-table cause it was written for. persistReadAuditRows writes ONE table, so the question is asked about that one — unlike persistAuditTrailRow, which writes the ledger row and its sys_activity mirror and asks about both. Every other cause now gets the driver's own verdict plus the fix that matches it.

What deliberately did NOT change

Evidence

Tests — 7 new pins, packages/plugins/plugin-audit/src/read-audit.test.ts

Four pin the defects, three are the discriminating controls that must NOT move:

pnpm --filter @objectstack/plugin-audit test      -> exit 0   24 files / 353 tests passed
pnpm --filter @objectstack/plugin-audit typecheck -> exit 0   (tsc, tsconfig.scripts.json, check:test-typecheck: 0 files / 0 errors)

Ablation — the pins are capable of failing

Both defects put back (git show of the pre-fix blob onto the path, proven on disk: reportedReadAuditFailureCauses 3 to 0, missingTable 2 to 0, let failureReported = false; 0 to 1, SHORT answer. Fix: confirm 0 to 1; mutated blob f202954d vs HEAD blob ae1e1b9a), then:

Tests  4 failed | 29 passed (33)
  x reports a SECOND, DIFFERENT cause at error - a new cause is a new degradation
      AssertionError: expected [ { level: 'error', ... } ] to have a length of 2 but got 1
  x carries the underlying code and message in the first line it prints
      AssertionError: expected 'Read-audit write FAILED - 1 record-vi...' to match /ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED/
  x prints the datasource remedy for the cause it is the remedy FOR, and not for others
      AssertionError: expected 'Read-audit write FAILED - 1 record-vi...' not to match /OS_TELEMETRY_DB/
  x [#9657] the `warn` fallback is per-cause too - a sink with no `error` hears the second fault
      AssertionError: expected [ { level: 'warn', ... } ] to have a length of 2 but got 1

The three controls stayed GREEN on both sides, which is the half that matters: "still degrades a REPEAT of an already-reported cause to debug", "keys on the error CODE, never its message" (200 batches, 200 distinct messages, one code, one line) and "folds a fault carrying NO code into ONE bucket". Deleting the dedupe outright would redden those three.

Restore leg: git checkout HEAD -- PATH (naming HEAD, never a bare checkout), blob back to ae1e1b9a, git diff HEAD for the path empty, git status --porcelain clean. No ablation artifact is left in the tree.

Gates — node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, reconciled with --ran

63 derived families, all run with the exit code captured to disk before reading; reconciliation reports 63 derived, 60 run, 3 NOT-MEASURED, 0 UNRUN.

  • 60 green.
  • 3 NOT MEASURED — check:dual-build-cjs-loads, check:i18n, check:type-check-debt all exited 3, the code these gates use for PREREQUISITE NOT MET: each needs a full pnpm build (57 packages have no dist/ in this worktree, which built only plugin-audit's dependency closure). CI builds, so CI measures them. Not read as green and not as red.
  • pnpm check:durability-log-level — run although this card's derivation does not name it, because the diff sits in a catch guarding a registered durability-critical callee. Green: 36 durability-critical catch seam(s), all loud, rethrowing or propagating to the caller. The gate has no objection to this change.
  • pnpm check:cross-package-test-inputs exited 1, and the finding is not this diff's. It names packages/cli/test/init-created-files-summary.e2e.test.ts descending packages/spec/dist/ — no path of mine. Mechanism: coversDirectory/the walked-root radius answer with readdirSync against the real filesystem, and packages/spec/dist/ is a gitignored build artifact that exists here only because the dependency-closure build created it. Control: the same gate on a checkout with no packages/spec/dist exits 0 (OK: 29 package(s) read outside themselves, all declared). Reported below rather than ridden in.

Lint — the whole population, not a narrowing

node --stack-size=4000 node_modules/eslint/bin/eslint.js . --no-inline-config --format json  -> exit 0
files in eslint population: 6803      files with findings: 0

Run at 6195b00 (the final commit), on a clean tree.

Acceptance notes

  • reportOverflow in the same file was examined and is NOT this class. Its report has no cause dimension at all — the buffer overflowing is one condition, it takes no err, and its remedy text is already cause-agnostic. A cause key there would key on nothing. Noted, not filed; successor: whoever next touches this batcher.
  • packages/services/service-settings/src/config-change-audit.ts:157 stays out, and my reading agrees with the card's. Its callee is a bare eng.insert that no register names, its first line already carries Cause: plus the real detail, and its remedy text is already cause-agnostic. An observation, not a contract violation.
  • check:cross-package-test-inputs reverses its verdict on a gitignored build artifact (see Gates above) — reported to the PM with dedupe words for the filing seat, not filed from here and not fixed here.

Generated by Claude Code

…ocess

`reportReadAuditWriteFailure` carried its own process-level
`failureReported` boolean and its own fixed message literal — the third
independent copy of the pair #15166 fixed in `audit-writers.ts` and
#17452 fixed in `auth-event-audit.ts`, on a seam already registered in
`DURABILITY_CRITICAL_CALLEES`.

The dedupe key is now `auditFailureCauseKey`, imported rather than
re-spelled (a second copy of the key is how this defect reached the
second file), and the ADR-0057 §3.6 / `OS_TELEMETRY_DB` guidance is
printed for the missing-table cause it is the remedy for, asked through
the shared `isMissingTableError` predicate.

Claude-Session: https://claude.ai/code/session_01WmBwEiWPff9JZPd5BSGNeH
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/deployment/production-readiness.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))
  • content/docs/kernel/runtime-services/audit-service.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))
  • content/docs/permissions/record-view-auditing.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))
  • content/docs/permissions/system-context.mdx (via installReadAuditWriter (symbol, a top-level function))
  • content/docs/plugins/packages.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))
  • content/docs/protocol/kernel/config-resolution.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))
  • content/docs/ui/setup-app.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))

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

  • content/docs/releases/index.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))
  • content/docs/releases/v14.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))
  • content/docs/releases/v17/17-0.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))
  • content/docs/releases/v17/17-1.mdx (via sys_audit_log (literal, a string literal in installReadAuditWriter))

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 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 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 — 9 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 e0d05538c0d0275728d25acfafbab259e23d0710 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5bfd82d87d854fd6607d7c7ba09bc61062893f20 — the merge of head 6195b0000124648378e738018ae74704acde21ed into base e0d05538c0d0275728d25acfafbab259e23d0710, 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 5bfd82d87d854fd6607d7c7ba09bc61062893f20 && git checkout 5bfd82d87d854fd6607d7c7ba09bc61062893f20
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e0d05538c0d0275728d25acfafbab259e23d0710 6195b0000124648378e738018ae74704acde21ed && git checkout -B drift-repro e0d05538c0d0275728d25acfafbab259e23d0710 && git merge --no-ff 6195b0000124648378e738018ae74704acde21ed

node scripts/docs-audit/affected-docs.mjs --json e0d05538c0d0275728d25acfafbab259e23d0710

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

@os-project-manager
os-project-manager marked this pull request as ready for review September 17, 2026 07:49
@os-project-manager
os-project-manager added this pull request to the merge queue Sep 17, 2026
Merged via the queue into main with commit a6a1de4 Sep 17, 2026
36 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-18247-read-audit-third-copy branch September 17, 2026 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] plugin-audit: a THIRD copy of both #15166 defects lives in read-audit.ts — process-wide boolean plus an unconditional datasource remedy

2 participants