fix(security): an unhashed audit chain no longer reports as verified (#1984) by dolho · Pull Request #1985 · Abilityai/trinity

dolho · 2026-08-04T11:47:23Z

Closes #1984 — a bug I found by probing the live instance for siblings of the #1966–#1971 batch.

Problem, as observed on real data

$ psql -c "SELECT count(*), count(entry_hash) FROM audit_log;"
 1162 | 0

$ curl -X POST ".../api/audit-log/verify?start_id=1&end_id=1162"
{"valid":true,"checked":0,"first_invalid_id":null}

And in the UI: a green "✓ Valid · 0 entries" badge — the tick sitting directly beside the zero that contradicts it.

valid: true was returned for three materially different states:

State	Old answer
Chain verified intact	`valid: true`
Range empty	`valid: true`
Nothing hashed at all	`valid: true`

The third is the default for every install that never enabled hashing — so the verdict was effectively independent of the input. That is the class learnings.md already names from #1941: "when a check reports the SAME verdict for every input, that is the signal that it is broken."

Skipping an unhashed entry is correct on its own — a chain enabled midway legitimately has an unhashed prefix. The defect was the aggregate verdict when every entry was skipped.

Fix

valid becomes tri-state: True verified · False mismatch · None unverifiable.

None and not False, deliberately. False claims tampering — an equally wrong and considerably louder lie that would page someone about a breach that did not happen. And a caller doing a plain truthiness test degrades to "not verified", which is the safe direction.

status carries the precise verdict (verified / verified_partial / tampered / unverifiable / empty_range), and skipped_unhashed makes a late-enabled chain's permanent unhashed prefix visible rather than silently averaged over — a partially-hashed range can no longer pass as fully verified.

The response now also reports hash_chain_enabled, which is precisely what made checked: 0 uninterpretable.

Trinity already solves this correctly one endpoint over. GET /api/monitoring/status faces the identical situation — a summary produced by a loop that is default-OFF (#1121) — and returns enabled: false beside a stale last_check_at so the reader can interpret the payload. This is that pattern applied where it was missing.

The frontend had to change too

This is the part a backend-only fix would have got wrong. The store did:

this.verifyState = data.valid ? 'valid' : 'invalid'

So valid: null would have rendered as "✗ Tamper detected" — trading a false green for a false red, which is worse. It now branches on the tri-state and renders an amber "Unverifiable · no hashes on N entries" badge.

The store also carried its own copy of the bug: an empty entry list set verifyState='valid', checked:0 without calling the API at all. Also gone.

Verification

tests/unit/test_1984_audit_verify_unverifiable.py — 16 checks, 15 of which fail against the pre-fix tree. 74 green across all audit-related suites.

Scope of what I actually verified, stated precisely:

Live pre-fix behaviour: confirmed over HTTP against the running instance (above).
Post-fix behaviour: unit tests, using the live shape (rows present, zero hashed) as the fixture.
I did not redeploy your instance to test the fixed code in place.

Two of my own mistakes are recorded in the test file rather than quietly fixed:

Source assertions now strip // comments before matching. This fix's own comments quote the old data.valid ? … : … while explaining why it went, and an unstripped search failed on the explanation — the exact inverse of the bug(ci): nightly unit-suite reports a false merge conflict on every PR — shallow fetch on both sides leaves no common ancestor #1941 trap, where a comment made a naive search pass vacuously.
The first fixtures were partial dicts; _compute_hash reads several fields with [] rather than .get, so the hasher raised KeyError and it looked like a code bug. There is now one complete row builder.

Acceptance criteria

A range containing rows but no hashes does NOT return valid: true
The response distinguishes "verified intact", "nothing hashed", and "empty range"
A genuinely intact hashed chain still returns valid: true
A test asserts the unhashed case — the current default state of every install that never enabled the chain

🤖 Generated with Claude Code

…1984) `POST /api/audit-log/verify` answered `valid: true, checked: 0` for an audit log in which no entry carried a hash. Found by probing a live instance: 1,162 real audit entries, zero hashes, and a green "✓ Valid · 0 entries" badge in the UI — the tick sitting directly beside the zero that contradicts it. `valid: true` was returned for three materially different states — chain verified, range empty, and *no integrity data exists at all*. The last is the DEFAULT for every install that never enabled hashing, so the endpoint's verdict was effectively independent of its input: the class `docs/memory/learnings.md` already names from #1941. Skipping an unhashed entry is correct on its own — a chain enabled midway legitimately has an unhashed prefix. The defect was the aggregate verdict when EVERY entry was skipped. `valid` is now tri-state: True verified, False mismatch, **None unverifiable**. `None` rather than `False` deliberately — `False` claims tampering, which is an equally wrong and considerably louder lie, and a caller doing a plain truthiness test degrades to "not verified", the safe direction. `status` carries the precise verdict (`verified`, `verified_partial`, `tampered`, `unverifiable`, `empty_range`) and `skipped_unhashed` makes a late-enabled chain's permanent unhashed prefix visible instead of silently averaged over. The response also reports `hash_chain_enabled`, which is what made `checked: 0` uninterpretable. Trinity already does this correctly one endpoint over: `/api/monitoring/status` returns `enabled: false` beside a stale summary so the reader can interpret it. **The frontend had to change too.** The store did `data.valid ? 'valid' : 'invalid'`, so a backend-only fix would have turned a false green into a false RED — a tamper alarm for a log that is merely unhashed. It now branches on the tri-state and renders an amber "Unverifiable" badge. The store also had its own vacuous path, asserting `verifyState='valid'` for an empty list without calling the API at all; that is gone. tests/unit/test_1984_audit_verify_unverifiable.py — 16 checks, 15 of which fail against the pre-fix tree. Closes #1984

Resolve tests/registry.json (rebuild: dev entries + this PR's entry). Also sync the two stale surfaces the review found: - tests/test_audit_log_unit.py::test_verify_chain_empty_range still asserted the old vacuous valid=True empty-range contract this PR removes — it is not collected by PR CI (unit/ only) but breaks tests/run-core.sh on dev. - docs/memory/feature-flows/audit-trail.md verify example updated to the tri-state response shape.

vybe

Validated via /validate-pr — the fix itself is exemplary (honest tri-state semantics, 16-check named regression test across model/service/store/view; verified an all-unhashed range can never report valid:true). The one blocker from validation — tests/test_audit_log_unit.py::test_verify_chain_empty_range still asserting the old vacuous empty-range contract (invisible to PR CI which runs unit/ only, but breaks tests/run-core.sh on dev) — I fixed on the branch in the merge commit, plus the stale audit-trail.md response example. Full CI green post-rebase.

…m the DB (#2015) (#2026) * fix(audit): persist the hash-chain toggle and read the chain head from the DB (#2015) `enable_hash_chain` set `self._hash_chain_enabled` and wrote nothing; nothing restored it at boot. So every backend restart silently switched the integrity control back off — and restarts are routine, to the point that CLAUDE.md documents users re-logging in after one. An install could sit unhashed indefinitely with the feature still presenting as available, and a range spanning the restart returns #1985's `verified_partial` — `valid: true` for something mostly unverifiable. The chain HEAD carried the same defect one level down. `self._last_hash` made the chain a property of one PROCESS: with more than one worker each kept its own head and wrote `previous_hash` values pointing into a different worker's sequence, so `verify_chain`'s link check would report an untampered log as **tampered** — the "equally wrong and considerably louder lie" its own docstring warns about. Both are now DB-backed: - the flag lives in `system_settings` and is resolved live on each write. Uncached, for the reason `settings_service._resolve_bool_flag` records: a cache lets a worker keep hashing after an admin flipped the toggle. But fail-CLOSED, unlike those flags — they fail open because an exception would zero every flag in the UI, whereas this one decides whether an integrity record is written, and a settings-read failure is not a reason to claim one exists. `verify_chain`'s `unverifiable` state describes that result exactly. - the head is read by `db.create_audit_entry_chained` inside the INSERT's own transaction, so a concurrent append cannot land between the read and the write. The hashing policy stays in the service (it decides which fields are covered) and is injected; the atomicity belongs to the db layer. `audit_retention_service` gated on the private attribute through `getattr(..., False)` — a default that would have degraded silently to "never warn" the moment it moved. Updated, and the tree grepped for others. 21 tests, mutation-verified. The atomicity guard walks the AST and requires the SELECT and the INSERT to be in one `with` block with no head read outside it; an earlier version asserted substrings of `ast.dump` and passed when I hoisted the read out of the transaction. 449 tests green across audit / retention / settings. Closes #2015 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(audit): serialise the hash-chain append; one transaction was not enough The move from a per-process head to a DB head was right, but the atomicity the writer depended on was not there, and the docstring asserted it was. SQLite pysqlite defers the real BEGIN until it sees DML, so the head SELECT ran in autocommit and the transaction opened at the INSERT. SQLAlchemy's 'BEGIN (implicit)' log line is its own bookkeeping marker, not a statement sent to SQLite. PostgreSQL READ COMMITTED, and a bare SELECT ... ORDER BY id DESC LIMIT 1 takes no lock and cannot lock rows that do not exist yet. Both let two appenders read the same head and both insert — with zero errors, which is the part that matters: it forks silently, writing rows that look fine, and verify_chain later reports that untampered log as tampered. That is the same false positive this change exists to remove, made intermittent instead of deterministic. The append now takes its lock BEFORE reading the head: * PostgreSQL - pg_advisory_xact_lock, cross-connection and cross-host, released at COMMIT/ROLLBACK so no unlock path can be missed (same primitive as db/alembic_runner.py, in its transaction-scoped form; distinct key, so the two never wait on each other). * SQLite - BEGIN IMMEDIATE, taking the RESERVED write lock before the read, so a second appender blocks at BEGIN and then reads the committed hash. It fails CLOSED, against this codebase's usual fail-open default: an unusable lock raises rather than appending unserialised. A failed audit write is loud and recoverable; a forked chain is neither. Measured with the reviewer's harness (6 threads, table pre-seeded), and again across PROCESSES, which is the real --workers 2 + scheduler shape: sqlite threads before: 142/245 broken (58.0%), 46 forked heads, 0 errors after: 0 broken, 0 forked, 0 errors sqlite processes before: 89/160 broken, 40 forked, 0 errors after: 0 broken, 0 forked, 0 errors postgres threads before: 221/245 broken (90.2%), 63 forked heads, 0 errors after: 0 broken, 0 forked, 0 errors (PostgreSQL 16 in a throwaway container; SQLite's connect timeout of 30s is what keeps the cross-process run free of SQLITE_BUSY.) Tests: two behavioural ones over a real SQLite file (every link points at its predecessor; no head is consumed twice) and one structural one asserting the lock is the FIRST statement in the transaction — ordering is the whole property, and asserted over the AST because the docstring explains the locking at length. All three fail with the lock call removed; 24 pass with it. Related to #2015 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(audit): add the DatabaseManager pass-through for the chained writer (#2015) CI's regression diff caught this as a HEAD-only failure: test_database_facade_delegation::test_every_db_call_resolves_on_databasemanager - db.create_audit_entry_chained() called from src/backend/services/platform_audit_service.py The service calls `db.create_audit_entry_chained(entry, self._compute_hash)`, but `DatabaseManager` had no method of that name — only the ops class did. So the hash-chained write path raised AttributeError the first time the chain was switched on: the integrity control failing precisely when enabled, which is the failure mode this PR exists to remove, one layer down. Verified beyond the guard, against a real database: enable_hash_chain(True), then a real `platform_audit_service.log(...)` — the row lands with a populated entry_hash. Previously that raised. facade guard + hash-chain suite: 27 passed Related to #2015 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: trinity-ability <trinity-ability@users.noreply.github.com>

…analysis of #1985 and #1979 (#2018) * test(edge-cases): audit hash chain + PAT propagation edge/property analysis /edge-cases pass over the audit-chain (#1985) and PAT-rotation (#1979) features merged to dev. 84 passing cases plus 6 strict-xfails, each naming the issue it pins: - #2015 — enabling the audit hash chain is in-memory only, so a backend restart silently turns the integrity control off. #1985 made verify_chain honest about unhashed ranges; this is why ranges keep going unhashed. - #2016 — a duplicated GITHUB_PAT line survives `count=1` and wins under the agent's last-wins parser, so the agent keeps the revoked token while the rotation reports `updated`. - #2017 — a backslash in the token raises re.error (the line is an re.sub replacement), and the .env writer escapes a quote the reader never unescapes. The PAT contract is stated as a Hypothesis round-trip property against a copy of the agent's own .env parser, since what the agent reads back is the only definition of a successful rotation. It holds for every realistic single-line .env; the xfails are the inputs where it does not. Product code deliberately unchanged — findings are reported, fixing is a separate decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(2018): make the #2015 alarms fire when the bug dies, not go quiet Both alarms for finding 1 were inert against the very fix they guard (#2026). 1. The strict xfail asserted `svc_b._hash_chain_enabled`. #2026 deletes that attribute when the flag moves to `system_settings`, and `xfail` treats the resulting AttributeError as an expected failure exactly like the assertion failure it replaces — so the marker would keep reporting 'BUG: enabling the audit hash chain is in-memory only' against a codebase where that is no longer true. `strict=True` exists to go loud when the bug dies; this went quiet, permanently. Now asserts the PUBLIC seam (`svc_b.hash_chain_enabled`), so the marker flips to XPASS(strict). 2. The backstop read only `routers/audit_log.py`. #2026 puts the write in the SERVICE (`db.set_setting`) and leaves the router a thin passthrough, so the router-only check passes with the fix in place. It now also inspects the setter the route delegates to, resolved through the import rather than a fixed filename — over the AST, because the fix's own docstring explains the persistence it adds and a substring scan matches that prose. 3. `test_enabling_is_reflected_in_the_verdict` forced only the private attribute, which is the third failure on the merged tree. It now forces both seams (`raising=False` on each), so it reads the same answer before and after the flag moves. Verified on both trees: this branch alone (bug present): 28 passed, 1 xfailed merged with #2026 (bug fixed): XPASS(strict) + backstop failure — both loud, telling you to retire the finding; the verdict test passes Related to #2015 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(2018): retire the finding-2 xfail — #2016 is fixed on dev CI's regression diff flagged this as a HEAD-only failure: [F] test_pat_propagation_properties.TestKnownGaps::test_a_duplicated_pat_line_still_rotates It is an XPASS(strict), not a broken test. The marker asserted finding 2 — that `count=1` replaced only the FIRST GITHUB_PAT line while the agent's .env parser is last-wins, so a duplicated line left the agent authenticating with the REVOKED token while the rotation reported `updated`. `31ba8d98` (#2016 via #2025) levels every occurrence on dev, so the strict marker went loud exactly as designed: the bug died and the alarm fired instead of going quiet. Retired to a plain regression test that now guards the fix instead of the defect, with the history in its docstring. The sibling markers stay: finding 3 (backslash parsed as an re.sub group reference) and finding 4 (writer escapes a quote the reader never unescapes) are still live on dev — their fixes are #2024 and #2030, both still open — and both correctly report XFAIL here. 47 passed, 5 xfailed (this file: 19 passed, 4 xfailed) Also merged latest dev. Related to #2015 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(2018): retire the #2015 xfail — #2026 landed #2026 merged to `dev` (b0126f3), so `enable_hash_chain` now persists to `system_settings` and the finding-1 marker would flip to XPASS(strict), turning `dev` red the moment this branch lands. Per obasilakis's review the marker belongs to whoever merges second; #2026 went first, so it falls here. - `test_enabling_the_hash_chain_survives_a_restart` drops the xfail and keeps its assertion verbatim — the behaviour it describes is now the correct one, so it becomes the named regression test for #2015. - `test_the_enable_route_persists_nothing` → `..._persists_durably`, with both halves inverted: the router still delegates, and the AST check now requires a real `set_setting` call in the service rather than forbidding one. Kept as an AST assertion for the original reason — a docstring describing persistence must not be able to satisfy it. The #2017 marker is deliberately left alone: #2024 is not merged, so that bug is still live on `dev` and the marker is doing its job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: trinity-ability <trinity-ability@users.noreply.github.com> Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com>

dolho and others added 2 commits August 4, 2026 14:46

vybe approved these changes Aug 4, 2026

View reviewed changes

vybe merged commit 10ed465 into dev Aug 4, 2026
23 checks passed

This was referenced Aug 5, 2026

bug(audit): enabling the hash chain does not survive a backend restart — the integrity control silently turns itself off #2015

Closed

fix(audit): persist the hash-chain toggle and read the chain head from the DB (#2015) #2026

Merged

Navigation Menu

Uh oh!

fix(security): an unhashed audit chain no longer reports as verified (#1984) - #1985

fix(security): an unhashed audit chain no longer reports as verified (#1984)#1985
vybe merged 2 commits into
devfrom
fix/1984-audit-verify-unverifiable

dolho commented Aug 4, 2026

Uh oh!

vybe left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

dolho commented Aug 4, 2026

Problem, as observed on real data

Fix

The frontend had to change too

Verification

Acceptance criteria

Uh oh!

vybe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants