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

[dolho]

Summary

Enabling the audit hash chain set an instance attribute and wrote nothing, and nothing restored it at boot. 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.

Found by the /edge-cases pass over #1985. That PR made the verdict honest about unhashed ranges; this is why ranges kept going unhashed.

The fix

The flag persists in system_settings and is resolved live on each write. Uncached, for the reason settings_service._resolve_bool_flag already records — a cache lets a worker keep hashing after an admin flipped the toggle.

But fail-closed, unlike those flags. They fail open deliberately, because an exception there would zero every feature flag in the UI. 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 then describes the result exactly.

The head is read by db.create_audit_entry_chained inside the INSERT's own transaction, so a concurrent append can't 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, and importing the service from a db module would invert the layering.

One consumer had to move with it. audit_retention_service gated on getattr(platform_audit_service, "_hash_chain_enabled", False) — a default that would have degraded silently to "never warn" the moment the attribute moved. Updated, and the tree grepped for others (there were none).

Acceptance criteria

AC	Where
Enablement survives a restart	system_settings + live resolve · test_enabling_survives_a_restart
State readable, so verify_chain reflects the install not the process	hash_chain_enabled property · test_every_worker_sees_the_same_answer
_last_hash DB-derived so multi-worker can't produce a false tampered	create_audit_entry_chained · test_two_instances_produce_ONE_chain
Regression test: enable → new process → still enabled	first test in the file

Test plan

• 21 tests. A fresh PlatformAuditService() over a shared settings store is the process after a restart — and two instances over one store is exactly the --workers 2 relationship, which is what makes both halves testable without a container
• End-to-end: two instances appending alternately to one table verify as one valid chain; before this the links crossed sequences and reported tampered
• Nine stored-value parsings, fail-closed on a settings error, and a pin that neither _hash_chain_enabled nor _last_hash exists on the instance — every behavioural test above would still pass for a single long-lived process, so only that assertion expresses the property
• Mutation-verified: flag back in the instance → 6 fail; fail-open → 1; always-plain-writer → 2; head read in a separate transaction → 1
• 449 tests green across audit / retention / settings, including bug(security): audit-log verify returns valid:true with checked:0 — an unhashed chain reports as intact #1984's suite unchanged

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(with_node) and passed when I hoisted the read out of the transaction — third time today that a textual structural assertion has been satisfiable by the thing it was meant to catch.

Note on scope

The verify_chain boundary I documented during the edge-case pass is unchanged and still worth knowing: _compute_hash covers event_id/type/action/actor_id/target_id/timestamp/details/previous_hash, so actor_ip, actor_email, endpoint, source and mcp_key_id remain editable after the fact with the range still reporting verified. Widening the hashed set would invalidate every existing hash, so it needs its own decision — not folded in here.

Closes #2015

[obasilakis]

The persistence half of this is right and I have no objection to it: the flag moves to system_settings, is resolved live per write rather than cached, and fails closed — the reasoning in the hash_chain_enabled docstring for why this one diverges from the fail-open feature flags is correct and worth having written down. Removing _last_hash is also the right direction. audit_retention_service.py correctly follows the rename, and platform_audit_service is a module-level singleton so the property access there resolves.

Blocking on the second half. The move from a per-process head to a DB head is right, but the atomicity the new writer depends on is not actually there, and the log-integrity claim rests entirely on it.

Blocking — the SELECT and the INSERT are not in one transaction

db/audit.py::create_audit_entry_chained:

with get_engine().begin() as conn:
    head = conn.execute(
        select(audit_log.c.entry_hash)
        .where(audit_log.c.entry_hash.isnot(None))
        .order_by(audit_log.c.id.desc())
        .limit(1)
    ).scalar()
    entry["previous_hash"] = head
    entry["entry_hash"] = compute_hash(entry)
    conn.execute(self._insert_stmt(entry))

and the docstring:

The SELECT and the INSERT share one transaction so a concurrent append cannot land between them and orphan the link; SQLite serializes writers, so the second append blocks and then reads the first one's hash.

Neither backend gives you that:

• SQLite — pysqlite defers the actual BEGIN until it sees a DML statement. The SELECT therefore executes in autocommit and the transaction only opens at the INSERT. SQLAlchemy logging shows BEGIN (implicit) before the SELECT, but that is SQLAlchemy's own bookkeeping marker, not a BEGIN 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. Two sessions read the same head and both insert.

db/engine.py sets no isolation_level and registers no BEGIN event listener, so nothing compensates.

Measured against the real function — six threads, table pre-seeded so this is not a cold-start artifact:

backend=sqlite  rows=241  errors=0
BROKEN LINKS: 77  (32.0% of rows)
FORKED heads (same previous_hash on >1 row): 39   max fan-out=5

errors=0 is the part that concerns me most — it forks silently, writing rows that look fine. And docker-compose.prod.yml:356:

command: uvicorn main:app --host 0.0.0.0 --port 8000 --workers 2 ...

Two processes, which is precisely the multi-worker scenario this PR's own docstring identifies as the original defect:

with more than one worker each kept its own head, wrote previous_hash values pointing into a different worker's sequence, and verify_chain's link check reported the untampered result as tampered — the loud false positive its own docstring warns against.

Net effect: that false positive is not eliminated, it is made intermittent. Arguably a harder failure to diagnose than the deterministic version, because the log now verifies clean most of the time.

Within a single worker there is no race — create_audit_entry_chained is a synchronous call with no await inside it, so no other coroutine interleaves. The exposure is strictly cross-process: the two uvicorn workers, plus the scheduler container writing the same database.

Suggested fix, roughly in order of preference:

1. Serialize the append explicitly. On PostgreSQL, pg_advisory_xact_lock(<fixed key>) as the first statement in the transaction — the codebase already uses this shape in db/alembic_runner.upgrade_to_head(). On SQLite, open with BEGIN IMMEDIATE (an after_begin event listener, or isolation_level=None plus an explicit emit) so the write lock is taken before the head is read.
2. Or make the link self-correcting rather than read-then-write — derive previous_hash from the row's own predecessor at verify time instead of at insert time.

Either way the test wants to become a real concurrency test — spawn N threads against one table and assert every row's previous_hash equals its predecessor's entry_hash. That is the assertion the current structural test stands in for.

On that test: test_the_head_is_read_inside_the_insert_transaction is well built for what it does — I mutated it by splitting the read into a separate transaction and it correctly went red, and the docstring is honest that the first draft using ast.dump substring matching was insufficient. But it can only ever assert the code's shape, and the shape is what turns out not to be sufficient here. Its docstring says "Pins the atomicity, which no behavioural test can observe" — the probe above is that behavioural test, so the premise is worth revisiting.

Also — durability changes who should be allowed to flip this

@router.post("/hash-chain/enable")
async def enable_hash_chain(
    enabled: bool = Query(True, ...),
    _admin: User = Depends(require_admin),
):

require_admin calls _reject_connector_principal and then checks role != "admin". It does not call reject_agent_principal, and get_current_user resolves an agent-scoped MCP key to its owner carrying the owner's role — so on a default admin-owned install any non-ephemeral agent's injected TRINITY_MCP_API_KEY satisfies this gate.

That was survivable while the flag was in-memory: an agent turning it off lost effect at the next restart. This PR makes it durable, so the same call now silently disables the audit log's tamper-evidence for the life of the install — including the record of the agent's own actions.

docs/memory/architecture.md states the rule this hits:

Role ≠ human (#1644, #1816) ... An endpoint whose blast radius is operator-scale ... needs reject_agent_principal in addition to the role gate ... The trigger to revisit an existing gate is a change in what the endpoint does — escalating a handler's destructiveness silently re-prices every principal that could already reach it.

Adding reject_agent_principal(current_user) to the route is a one-liner and fits the precedent set by POST /api/settings/retention/acknowledge (#1644) and POST /api/system-agent/restart (#1816).

Confirmed clean

• _insert_stmt shared by both writers so the plain and chained paths cannot drift on columns — good factoring, and the reason for injecting compute_hash rather than importing the service (layering) is sound.
• verify_chain's tri-state valid from #1984/#1985 is untouched and now reads the live property; the unverifiable state does describe the fail-closed outcome accurately, as the docstring claims.
• architecture.md updated in the same PR — matches what the code does, aside from the atomicity sentence, which will need a trim alongside the fix.
• Scope: audit db + service + retention service + architecture + tests. Nothing unrelated.
• 21 tests pass on the branch. No credential values anywhere in the diff.

Merge-ordering note

This PR removes _hash_chain_enabled, which #2018 reaches into from two tests. Both need updating and neither belongs to you by default, but landing this without them turns the suite red:

• test_enabling_is_reflected_in_the_verdict — monkeypatch.setattr(svc, "_hash_chain_enabled", True, raising=False) binds a dead attribute (raising=False hides it), then asserts hash_chain_enabled is True. Plain failure.
• test_enabling_the_hash_chain_survives_a_restart — asserts svc_b._hash_chain_enabled, which now raises AttributeError; the xfail swallows it, so it stays XFAIL and keeps reporting the bug as live forever. I have asked for that one on #2018 directly, since the marker is inert regardless of who fixes it.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.