Skip to content

fix(plugin-audit): declare preserveAudit on the read-audit ledger write so record-view rows keep the VIEW instant - #16996

Merged
os-trump merged 6 commits into
mainfrom
claude/issue-16829-read-audit-preserve-view-instant
Sep 9, 2026
Merged

fix(plugin-audit): declare preserveAudit on the read-audit ledger write so record-view rows keep the VIEW instant#16996
os-trump merged 6 commits into
mainfrom
claude/issue-16829-read-audit-preserve-view-instant

Conversation

@os-trump

@os-trump os-trump commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16829

Clause-②: no

sys_audit_log's record_views rows answer "when did this user look at this record?". Read auditing batches its INSERTs off the request path by design, so buildRow writes created_at: event.viewedAt rather than letting the column's NOW() default stamp a whole batch with one drain timestamp — up to flushIntervalMs after the fact, with read order inside the window destroyed.

persistReadAuditRows wrote that row under { context: { isSystem: true } }, and the module's comment cited that flag as what carried the view instant through. It never was.

Re-measured on this tree, not inherited

Triage confirmed the anchors on origin/main e08892da; re-measured here at b834b48e7 (branch point). Four of five sit where triage left them; one moved by a line.

anchor triage this tree reading
plugin-audit/src/read-audit.ts created_at: event.viewedAt, :512 :512 unchanged
plugin-audit/src/read-audit.ts the ledger insert :460 :460{ context: { isSystem: true } }, only isSystem unchanged
objectql/src/plugin.ts record.created_at = preserveAudit ? (record.created_at ?? now) : now; :1123 :1124 moved by one line; the hook reads preserveAudit, never isSystem
objectql/src/engine-audit-anchor-write.test.ts a system-context write is still exempt :216 :216 — and it calls engine.update unchanged
.changeset/audit-binder-created-at-unconditional-on-create.md present present; objectql/CHANGELOG.md at 17.3.0 carries no entry for it still unreleased

Confirmed independently: sys_stamp_audit_update passes isInsert: false into applyToRecord, whose created_at assignment is inside if (isInsert). That case is green whatever the insert path does, so it never covered the reliance it was cited for.

The change

await engine.insert(
  'sys_audit_log',
  rows as any,
  { context: { isSystem: true, preserveAudit: true } } as any,
);

Both keys, for two different layers. isSystem still carries the readonly-strip exemption the row needs; preserveAudit is the one the stamp hook reads. preserveAudit is the ruled historical-import channel (#3493, reaffirmed by #15964's ruling of 2026-09-06) — 「the door audit left open for reinstating an original timeline, not a bypass of it」 — and a view row's original timeline is the moment of the view, so this use sits inside its declared purpose.

The test is the substantive half, and it was RED first

packages/plugins/plugin-audit/src/read-audit-view-instant-preservation.integration.test.ts — a real ObjectKernel + real ObjectQLPlugin (which is what registers sys_stamp_audit_insert) + real SqliteWasmDriver, read back through the driver's own SQL surface. Shape copied from #16312's packages/runtime/src/notification-migration-audit-preservation.integration.test.ts, not reinvented.

RED before the change — the card's case only:

× a record-detail view is stamped with the VIEW instant, not the flush instant
AssertionError: expected 1788912779588 to be 1551675967891
   Tests  1 failed | 2 passed (3)

1551675967891 is 2019-03-04T05:06:07.891Z, the view instant. 1788912779588 is the drain instant of that run.

GREEN afterTests 3 passed (3); whole package Test Files 23 passed (23) · Tests 326 passed (326).

The two passing cases are anti-vacuity controls, green on both sides on purpose:

  1. a bare isSystem write does NOT keep a supplied created_at — the card's central claim, asserted on the very write path the ledger uses. It is also the proof the real hook is live in this fixture: in a hook-less harness this is the case that goes red first, by name.
  2. context.preserveAudit DOES keep it, on this object and this write path — so a green third case means "the writer declared the channel", not "nothing was ever going to restamp this row".

Why a new file rather than the case that already claimed this

read-audit.test.ts has had a case named records the VIEW instant, not the flush instant since #8992. Its engine is a bare new ObjectQL() over a stub driver — no ObjectQLPlugin, therefore no audit stamp hook at all. It is green before #15964, green with the defect live, and green after this change: an instrument that cannot fail. That is the same shape that let migrate-sys-notification-to-event.test.ts read 23 passed for #16312 while the rows were being restamped.

That case is kept but demoted to what it can actually witness — that the writer puts viewedAt on the row it hands the engine — and both its docblock and the file header are corrected. The header previously claimed a third pin this file cannot make, in the same words the source comment used, so leaving it would have kept the false premise alive in a second place.

The comment repair

The comment at the created_at field cited engine-audit-anchor-write.test.ts's "a system-context write is still exempt" as authority. Both halves were wrong: isSystem exempts a write from the readonly strip and has never been read by the stamp hook, and the cited case is an engine.update. It is replaced with the real mechanism, a pointer to the write that carries it, and a note that what was truly carrying the value was the hook's pre-#15964 line record.created_at = record.created_at ?? now.

Three commits of harness plumbing, each forced by a measured gate failure

⚠️ These are not incidental — each was a red gate, and the remedy each took is the one that gate prescribes.

  1. @objectstack/driver-sqlite-wasm as a devDependency of plugin-audit. No cycle: the driver depends on core / driver-sql / spec, none of them on this package.
  2. check:test-source-alias redNEW unaliased artifact import(s). Anchored alias added to vitest.config.ts. Widening the shrink-only registry is explicitly not the remedy.
  3. check:type-source-resolution redNEW dist-resolved type import(s). paths rule added to tsconfig.json. Its precondition holds: rootDir here is ../.. (packages/) and the source sits at packages/drivers/driver-sqlite-wasm/src, so no TS6059 is owed.
  4. typecheck then red with 4 × TS1470 — the paths rule puts the driver's source into whichever program resolves the specifier, and in the BUILD program it compiles under the root config's CommonJS-bound module setting, where knex-wasm-dialect.ts and wasm-connection.ts use import.meta. Tests now leave that program, exactly as plugin-approvals spells it, and tsconfig.test.json (module: esnext / moduleResolution: bundler — what vitest really runs) keeps them. Coverage is unchanged and re-measured: check:type-check-coverage green, check:test-typecheck 0 files / 0 errors.

Ablation — the case can fail, and the tree came back

Run from the committed fix, under a trap ... EXIT INT TERM, restoring with git checkout HEAD -- ABSPATH (never the bare form, which restores from the index).

HEAD blob        : f202954dad80a99d008e1c703a26abea8765114e
BEFORE  anchorA(preserveAudit present)=1  anchorB(bare isSystem)=0
hash before      : f202954dad80a99d008e1c703a26abea8765114e
AFTER   anchorA(preserveAudit present)=0  anchorB(bare isSystem)=1
hash after       : 082ac545e7e07f2c14f310de5fb06f411582bc94
ON-DISK PROOF    : OK (anchor counts flipped 1/0 -> 0/1; blob differs from HEAD)
ABLATED RUN exit : 1
AssertionError: expected 1788914642949 to be 1551675967891
      Tests  1 failed | 2 passed (3)
hash restored    : f202954dad80a99d008e1c703a26abea8765114e
git diff HEAD    : EMPTY
RESTORE PROOF    : OK (empty git diff HEAD; blob equals the HEAD blob)
RESTORED RUN exit: 0
      Tests  3 passed (3)

Only the card's case moved; both controls stayed green under the mutation, so the ablation is targeted rather than a blanket break. No dist preflight is owed here: the mutated subject is reached by a relative in-package import (./read-audit.js), so vitest resolves it from src — which the RED itself demonstrates.

Gates

Derived from a fresh tree after merging origin/main (fe2b7554a), reconciled mechanically:

Run reconciliation — 70 derived, 70 run, 0 NOT-MEASURED, 0 UNRUN.
✓ dispatch-gates --ran: 70 derived famil(ies) accounted for — 70 run, 0 NOT-MEASURED.

Three gates first came back exit 3 PREREQUISITE NOT MET, which is NOT a pass and was not reported as one: check:dual-build-cjs-loads, check:i18n and check:type-check-debt all read built output. A full workspace build (turbo run build --filter='./packages/*' --filter='./packages/*/*', 72/72 successful) was run and all three re-run to real verdicts — green.

Beyond the derived union: pnpm lint (whole repo, exit 0 — no narrowing claimed and none needed), pnpm --filter @objectstack/plugin-audit typecheck, pnpm --filter @objectstack/plugin-audit test, and the three artifact-roster families whose roster sits in a directory this diff is in (check:authz-resolver, check:error-code-casing, check:filter-alias-parity) — all green, since their silence is not evidence in either direction.

pnpm check:route-envelope was run explicitly (green), per its known absence from the derived union (#16828). It does not in fact apply: every file in this diff contains zero c.json( / res.json( / ctx.json( call sites, measured per file.

⚠️ check:partof-closing-keyword will be RED on this PR, and no author action clears it. The first commit on this branch carries a card-relation trailer in its message, which the contract puts in the PR body only. It was pushed before the mistake was noticed, and this repository forbids amend / rebase / force-push — the gate's own text says so and says the red is then to be READ rather than acted on. Recorded here rather than left for a reviewer to discover. The relation itself is correct and unambiguous: the trailer names this same card and nothing else, and the body above declares it once.

验收备注

Triage's acceptance criteria, carried verbatim:

  1. 用例必须驱动真实的 sys_stamp_audit_insert 钩子 —— ⛔ 不是任何跳过钩子的替身。 → met: real ObjectKernel + ObjectQLPlugin + SqliteWasmDriver, with an explicit anti-vacuity control that goes red if the hook is not live.
  2. 该用例在修复之前必须是红的(⇒ 先写测试、看它红、再修)。 → met: written first, observed red (expected 1788912779588 to be 1551675967891), then the change; both readings above, plus an ablation reproducing the red from the committed state.
  3. 形状可以照抄 —— The sys_notification migration back-dates created_at through the create-side ?? that #15964 removes, so migrated inbox rows and receipts get the migration instant #16312packages/runtime/src/notification-migration-audit-preservation.integration.test.ts。⛔ 不必重新发明。 → met: copied, including its three-reading structure and its two anti-vacuity controls.

And the two constraints the dispatch carried as binding:

  • 两个 key 都要,理由不同 — both present; ⛔ neither swapped for the other.
  • 注释同笔修复 — done, and extended to the sibling suite's docblocks, which carried the identical false premise about the identical field. Left alone they would have handed the next reader exactly what triage said to remove.

Third-site sweep — the extra deliverable

Re-ran (not inherited) the eight-row created_at enumeration whose incompleteness produced this card. Result: no third RELIANT site; exactly two exist and both are known. Posted to #16312 with the method, the positive control, and the four sites that table omitted: #16312 (comment)

⚠️ The method matters more than the count: a textual window around each insert call — the obvious sweep — misses this card's own site, because persistReadAuditRows and buildRow are different functions ~50 lines apart. Reliance is a dataflow fact, not a proximity fact.

Out of scope — noted, not filed


🤖 Generated with Claude Code

https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37


Generated by Claude Code

…te so the row keeps the VIEW instant

`buildRow` writes `created_at: event.viewedAt` on purpose — batching moves the
INSERT off the request path, so `created_at`'s `NOW()` default would stamp a
whole batch with one buffer-drain time. `persistReadAuditRows` wrote that row
under `{ context: { isSystem: true } }` and the module's comment cited that flag
as the mechanism carrying the view instant through.

It never was. `isSystem` exempts a write from the readonly strip; the layer that
decides `created_at` on an insert is `sys_stamp_audit_insert`, which reads
`session.preserveAudit` and has never read `isSystem`. What was actually
carrying the value was the hook's pre-#15964 line, `record.created_at =
record.created_at ?? now` — client-preferred on every insert, no flag required.
#15964 closed that accident, so the ordinary branch now stamps `now`: the flush
instant, on every row in the batch.

Both context keys are kept, for different layers: `isSystem` still carries the
readonly-strip exemption the row needs, `preserveAudit` is what the stamp hook
reads. `preserveAudit` is the ruled historical-import channel (#3493,
reaffirmed by #15964's ruling of 2026-09-06) and a view row's original timeline
is the moment of the view, so this use is inside its declared purpose.

The comment at the `created_at` field is repaired in the same change. It cited
`engine-audit-anchor-write.test.ts`'s "a system-context write is still exempt"
as authority — but that case calls `engine.update`, and `sys_stamp_audit_update`
never writes `created_at` in any branch (`if (isInsert)` guards it), so it is
green whatever the insert path does. The insert path had no pin at all, and the
citation is why nobody re-checked it.

New pin: `read-audit-view-instant-preservation.integration.test.ts` — a real
`ObjectKernel` + `ObjectQLPlugin` + `SqliteWasmDriver`, so the real
`sys_stamp_audit_insert` hook runs. Measured RED before the fix (stored
1788912779588, expected 1551675967891) and green after, with two anti-vacuity
controls that pass on both sides. `read-audit.test.ts`'s docblocks are corrected
too: its engine is a bare `new ObjectQL()` that registers no stamp hook, so its
VIEW-instant case could never have pinned the engine half it claimed.

Fixes #16829

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…new integration test

`check:test-source-alias` and `check:type-source-resolution` both went red on
the new @objectstack/driver-sqlite-wasm devDependency: without an alias the
integration test would resolve the driver through `exports` (dist/), making a
test whose whole point is a real driver into a verdict about build state.

Both gates prescribe the same self-serve remedy and refuse the alternative
(widening their shrink-only registries). The tsconfig `paths` precondition
holds: rootDir here is `../..` (packages/) and the redirected source sits at
packages/drivers/driver-sqlite-wasm/src, so no TS6059 is owed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…ource lands in the ESM one

The driver paths rule put driver-sqlite-wasm's source into whichever program
resolves the specifier. In the BUILD program that source compiles under the root
config's CommonJS-bound module setting, where knex-wasm-dialect.ts and
wasm-connection.ts are 4 x TS1470 — a verdict about the driver's module
semantics billed to this package.

Tests now leave that program, exactly as plugin-approvals spells it, and
tsconfig.test.json (module esnext / moduleResolution bundler — what vitest
really runs) keeps them. Coverage is unchanged: check:type-check-coverage reads
the pair, and the typecheck script names both.

Measured after: typecheck 0 errors, check:test-typecheck 0 files / 0 errors,
check:type-check-coverage / check:type-source-resolution / check:test-source-alias
all green, suite 23 files / 326 tests passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
@github-actions github-actions Bot added the size/m label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-audit, touching 2 documentable anchor(s). ⚠️ 2 changed file(s) yielded no anchor (packages/plugins/plugin-audit/tsconfig.json, packages/plugins/plugin-audit/vitest.config.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

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))

3 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.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
  • 2 changed file(s) yielded no anchor (packages/plugins/plugin-audit/tsconfig.json, packages/plugins/plugin-audit/vitest.config.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 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; 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 — 8 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 fe2b7554a527c27f92582f8259d46c52ac87d4a1packageMentionDocs.

Which tree this was computed on

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

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

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

os-trump commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Stand-down record for the two remaining reds — neither is this PR's, and one of them is now a repo-wide condition

domain:services PM seat (#6021). Posting this so a reviewer meets the reasoning rather than the absence.

Check Changeset — cleared, with no push

It wanted the Clause-② declaration in the PR body, not merely in the prose. The line was added (Clause-②: no, transcribed verbatim from the claim comment on #16829 — the claiming seat's judgement, not the dev's) and the gate went failure 00:58:00 → success 01:04:07 → success 01:05:13 with no commit and no re-run, exactly as its own diagnostic said it would.

⛔ The changeset was not touched. The gate names the temptation explicitly — "dropping the changeset, or regrading the package to dodge this message, changes what ships in order to quiet a gate" — and @objectstack/plugin-audit: patch is correct on the merits: nothing that works stops working; a wrong timestamp becomes a right one. Note also the gate's own arithmetic: with every moved package at patch, a yes would have refused under #16361, so no is both true and the only self-consistent answer.

Validate Package Dependencies — NOT this PR's. Measured, not asserted.

The failing step is only Audit dependencies for known vulnerabilities (OSV-Scanner); every step that reads this diff passed, pnpm install --frozen-lockfile included.

Two independent readings establish it:

  1. The lockfile's registry contents did not move. Every registry package tuple extracted at three heads — 1ea349f0e (main's last scheduled validate-deps run, which succeeded, 2026-09-08 03:03), fe2b7554a (this PR's merge base) and c7f481346 (this head) — gives 2748 tuples, byte-identical at all three, diff exit 0 both ways. OSV-Scanner resolves from the lockfile, so its input is provably the input that scanned green.
  2. All seven flagged packages sit at identical versions at all three heads. This PR's entire lockfile delta is 3 lines, all one workspace link: entry.

⇒ The same package set scanned green ~22 hours ago and red now, with no lockfile movement. The advisory database moved, not this repository.

⚠️ One reading that could not be taken, stated rather than hidden: a direct api.osv.dev query to date the advisory publications was denied by the egress proxy (CONNECT 403). That is NOT MEASURED — the advisory IDs come from the CI log, and the conclusion above rests on the lockfile identity, ⛔ not on anything retrieved from OSV.

No fix is ported here, and deliberately so. The remedy is version bumps across next (2 Critical), sharp, nodemailer, hono (at two versions), vitest and @vitest/mocker — a dependency-security sweep, which is a different act from a p1 audit-timestamp repair and needs its own review. Filed as #16999, which also records the consequence this PR happens to be the first to see: nothing in the lockfile moved, so main's next scheduled run will go red on its own and every PR from now on inherits this red.

Part-of PR must not also close its card — unrepairable, read rather than acted on

The first commit carries Fixes #16829 in its message; the contract puts the card relation in the PR body only. It was pushed before the dev noticed, and amend/rebase/force-push are forbidden here — the gate's own text says so and says the red is then to be read, not acted on. It is absent from the required-context registry and does not decide whether this PR merges. The residue is a correct trailer naming the right card.

⚠️ This one is my fault upstream of the dev: I told the round I had removed card trailers from the dispatch template and then did not put that instruction into this dispatch. The two dispatches still in flight have since received it.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants