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

docs/memory/feature-flows/audit-trail.md

@@ -469,10 +469,14 @@ When enabled:
 
 Verify with `POST /api/audit-log/verify?start_id=1&end_id=100`:
 ```json
-{"valid": true, "checked": 100, "first_invalid_id": null}
+{"valid": true, "status": "verified", "checked": 100, "skipped_unhashed": 0,
+ "total_in_range": 100, "hash_chain_enabled": true, "first_invalid_id": null}
 ```
 
-Entries written before hash chain was enabled are skipped during verification.
+Entries written before hash chain was enabled are skipped during verification
+(surfaced as `skipped_unhashed`, `status: "verified_partial"`). An all-unhashed
+range returns `valid: null, status: "unverifiable"`; an empty range returns
+`valid: null, status: "empty_range"` — never a vacuous `valid: true` (#1984).
 
 ### Export
 

src/backend/models.py

@@ -1981,8 +1981,21 @@ class AuditCalendarResponse(BaseModel):
 class AuditVerifyResponse(BaseModel):
     """Hash chain verification result."""
 
-    valid: bool
-    checked: int
+    # TRI-STATE (#1984). True = verified intact, False = mismatch/tampering,
+    # None = UNVERIFIABLE (nothing in the range carried a hash). It was a plain
+    # `bool`, so "no integrity data exists" was indistinguishable from
+    # "verified" — and that is the default state of every install which never
+    # enabled hashing.
+    valid: Optional[bool] = None
+    # verified | verified_partial | tampered | unverifiable | empty_range
+    status: str = "unverifiable"
+    checked: int = 0
+    # Entries skipped for carrying no hash. Non-zero alongside
+    # `verified_partial` marks the permanent unhashed prefix of a chain that
+    # was enabled midway.
+    skipped_unhashed: int = 0
+    total_in_range: int = 0
+    hash_chain_enabled: bool = False
     first_invalid_id: Optional[int] = None
 
 

src/backend/services/platform_audit_service.py

@@ -203,36 +203,87 @@ def enable_hash_chain(self, enabled: bool = True) -> None:
     async def verify_chain(self, start_id: int, end_id: int) -> Dict[str, Any]:
         """Verify hash chain integrity between two row IDs (inclusive).
 
+        `valid` is deliberately TRI-STATE (#1984):
+
+          * ``True``  — entries were hashed and the chain checks out
+          * ``False`` — a hash mismatch: tampering or corruption
+          * ``None``  — **unverifiable**: there was nothing to check
+
+        The third state is the fix. Skipping an unhashed entry is right on its
+        own (a chain enabled midway legitimately has an unhashed prefix), but
+        when EVERY entry was skipped the old code still answered ``valid: True``
+        — so an install that never enabled hashing, which is the default,
+        reported its audit log verified-intact across thousands of rows. One
+        answer for three different states: verified, empty, and "no integrity
+        data exists". An operator asking "was this tampered with?" during an
+        incident got a green tick meaning "unanswerable".
+
+        ``None`` rather than ``False``: ``False`` claims tampering, which is an
+        equally wrong and considerably louder lie. A caller doing a plain
+        truthiness test degrades to "not verified" — the safe direction.
+
         Returns:
-            {"valid": bool, "checked": int, "first_invalid_id": int | None}
+            {"valid": bool | None, "status": str, "checked": int,
+             "skipped_unhashed": int, "total_in_range": int,
+             "hash_chain_enabled": bool, "first_invalid_id": int | None}
         """
         entries = db.get_audit_entries_range(start_id, end_id)
+        base: Dict[str, Any] = {
+            "checked": 0,
+            "skipped_unhashed": 0,
+            "total_in_range": len(entries),
+            "hash_chain_enabled": self._hash_chain_enabled,
+            "first_invalid_id": None,
+        }
+
         if not entries:
-            return {"valid": True, "checked": 0, "first_invalid_id": None}
+            # Distinct from "rows exist but none are hashed": the caller asked
+            # about a range holding nothing, which is not a statement about
+            # integrity in either direction.
+            return {**base, "valid": None, "status": "empty_range"}
 
         checked = 0
+        skipped = 0
         for i, entry in enumerate(entries):
             if not entry.get("entry_hash"):
-                # Entry was written before hash chain was enabled — skip
+                # Written before hash chain was enabled — skipped, and now
+                # COUNTED, so the verdict below can tell "some" from "none".
+                skipped += 1
                 continue
             expected = self._compute_hash(entry)
             if entry["entry_hash"] != expected:
                 return {
-                    "valid": False,
-                    "checked": checked + 1,
+                    **base, "valid": False, "status": "tampered",
+                    "checked": checked + 1, "skipped_unhashed": skipped,
                     "first_invalid_id": entry["id"],
                 }
             if i > 0 and entry.get("previous_hash"):
                 prev = entries[i - 1]
                 if prev.get("entry_hash") and entry["previous_hash"] != prev["entry_hash"]:
                     return {
-                        "valid": False,
-                        "checked": checked + 1,
+                        **base, "valid": False, "status": "tampered",
+                        "checked": checked + 1, "skipped_unhashed": skipped,
                         "first_invalid_id": entry["id"],
                     }
             checked += 1
 
-        return {"valid": True, "checked": checked, "first_invalid_id": None}
+        if checked == 0:
+            # Rows exist; none carry a hash. THE reported bug (#1984).
+            return {
+                **base, "valid": None, "status": "unverifiable",
+                "skipped_unhashed": skipped,
+            }
+
+        return {
+            **base,
+            "valid": True,
+            # Named apart so a partially-hashed range cannot pass for a fully
+            # verified one — the unhashed prefix is permanent on any install
+            # that enabled hashing later.
+            "status": "verified_partial" if skipped else "verified",
+            "checked": checked,
+            "skipped_unhashed": skipped,
+        }
 
     @staticmethod
     def _compute_hash(entry: Dict[str, Any]) -> str:

src/frontend/src/stores/auditLog.js

@@ -61,7 +61,11 @@ export const useAuditLogStore = defineStore('auditLog', {
     // #941 v2 — dashboard expansion
     stats: null,              // { total, by_event_type: {...}, by_actor_type: {...} }
     statsLoading: false,
-    verifyState: 'idle',      // idle | verifying | valid | invalid | error
+    // #1984: 'unverifiable' is a REQUIRED state, not a nicety. The backend
+    // answers valid=null when nothing in range was hashed; without this the
+    // binary `data.valid ? 'valid' : 'invalid'` below rendered that as a
+    // green tick (before) or a tamper alarm (after) — both wrong.
+    verifyState: 'idle',      // idle | verifying | valid | unverifiable | invalid | error
     verifyResult: null,       // { checked, first_invalid_id?, range?: [start, end] }
     activePreset: '24h',      // '1h' | '24h' | '7d' | '30d' | 'all' | 'custom'
     exporting: false,
@@ -401,8 +405,10 @@ export const useAuditLogStore = defineStore('auditLog', {
       const authStore = useAuthStore()
       if (!authStore.isAuthenticated) return
       if (this.entries.length === 0) {
-        this.verifyState = 'valid'
-        this.verifyResult = { checked: 0, range: null }
+        // #1984: was 'valid' with checked:0 — the same vacuous affirmation the
+        // backend made, asserted client-side without even asking.
+        this.verifyState = 'unverifiable'
+        this.verifyResult = { checked: 0, status: 'empty_range', range: null }
         return
       }
       const ids = this.entries.map((e) => Number(e.id)).filter((n) => !isNaN(n))
@@ -426,10 +432,20 @@ export const useAuditLogStore = defineStore('auditLog', {
         const data = r.data || {}
         this.verifyResult = {
           checked: Number(data.checked) || 0,
+          skipped_unhashed: Number(data.skipped_unhashed) || 0,
+          total_in_range: Number(data.total_in_range) || 0,
+          hash_chain_enabled: Boolean(data.hash_chain_enabled),
+          status: data.status || null,
           first_invalid_id: data.first_invalid_id ?? null,
           range: [startId, endId],
         }
-        this.verifyState = data.valid ? 'valid' : 'invalid'
+        // Tri-state, NOT truthiness. `valid === null` means "nothing was
+        // hashed, so integrity is unknowable" — reporting that as either
+        // verified or tampered is a different wrong answer (#1984).
+        this.verifyState =
+          data.valid === true ? 'valid'
+          : data.valid === false ? 'invalid'
+          : 'unverifiable'
       } catch (e) {
         this.verifyState = 'error'
         this.verifyResult = null

src/frontend/src/views/enterprise/Audit.vue

@@ -381,14 +381,31 @@ const detailsJson = computed(() => {
                 store.verifyState === 'valid',
               'bg-red-100 text-red-700 dark:bg-red-900 dark:text-red-200':
                 store.verifyState === 'invalid',
+              'bg-amber-100 text-amber-800 dark:bg-amber-900 dark:text-amber-200':
+                store.verifyState === 'unverifiable',
               'bg-yellow-100 text-yellow-700 dark:bg-yellow-900 dark:text-yellow-200':
                 store.verifyState === 'error',
             }"
           >
             <span v-if="store.verifyState === 'idle'">Hash chain · not verified</span>
             <span v-else-if="store.verifyState === 'verifying'">Verifying…</span>
             <span v-else-if="store.verifyState === 'valid'">
-              ✓ Valid · {{ store.verifyResult?.checked || 0 }} entries
+              ✓ Valid · {{ store.verifyResult?.checked || 0 }} entries<template
+                v-if="store.verifyResult?.skipped_unhashed"
+              >
+                · {{ store.verifyResult.skipped_unhashed }} unhashed (not covered)</template>
+            </span>
+            <!-- #1984: previously rendered as a green "✓ Valid · 0 entries" —
+                 a verified verdict over a range nothing had been hashed in. -->
+            <span v-else-if="store.verifyState === 'unverifiable'">
+              <template v-if="store.verifyResult?.status === 'empty_range'">
+                — Nothing in range to verify
+              </template>
+              <template v-else>
+                ⚠ Unverifiable · no hashes on
+                {{ store.verifyResult?.total_in_range || 0 }} entries — hash
+                chain was never enabled
+              </template>
             </span>
             <span v-else-if="store.verifyState === 'invalid'">
               ✗ Tamper detected · first invalid id #{{ store.verifyResult?.first_invalid_id }}

tests/registry.json

@@ -1492,6 +1492,19 @@
         "credentials"
       ],
       "description": "A global GitHub PAT rotation must actually reach running agents (#1967). The #211 auto-propagation reported success while updating nothing, for two independent reasons, both caused by the global path and the per-agent path (#1264) having drifted apart. (1) The eligibility gate was .env-shaped: _propagate_to_agent passed add_if_missing=False, so an agent with no /home/developer/.env - which is EVERY agent provisioned from a GitHub template, since those ship .env.example - returned skipped_no_pat; on such a fleet 100% skipped and the endpoint still answered success:true. (2) .env is not where git authenticates from: clones are created as https://oauth2:<PAT>@github.com/... and that URL is persisted in .git/config on the workspace volume, so rewriting .env changes nothing for the running git process - only git_service.update_remote_pat restores fetch/push before a restart, and only the per-agent path called it. Observed: agents on a revoked token for 11-13 days with nothing in the UI saying so. Covers both halves: an agent with a git config but no .env is now updated (add_if_missing derived per agent, not hard-coded), and the live remote is re-templated with remotes_updated reported SEPARATELY from updated (updated alone overstates the fix - an agent whose .env was rewritten but whose remote was not is still broken for git until restart, which would repeat this issue's own mistake of reporting success for a partial effect). Critically also pins what the fix must NOT be: the issue suggests add_if_missing=True unconditionally, which would spray the global token into every running container including agents that never touched GitHub - the module docstring says the original gate existed to prevent exactly that - so eligibility moved to the git config and a non-GitHub agent keeps the conservative behaviour, with an explicit no-regression test that an agent carrying GITHUB_PAT for the gh CLI with no Trinity-managed repo is STILL rotated (the new gate is a union with the old, not a replacement). Plus: per-agent-PAT agents still skipped and never contacted, stopped agents neither counted nor contacted, one failing agent does not stop the fleet, a git-config read that raises degrades to the conservative gate instead of taking down the rotation, a zero-reach rotation WARNs in the platform log (the failure was silent for weeks; a Settings panel nobody is watching during an incident is not sufficient), and two structural guards that both paths share one _apply_pat_to_agent body and that add_if_missing=False is not reintroduced - the drift IS the bug."
+    },
+    {
+      "file": "unit/test_1984_audit_verify_unverifiable.py",
+      "feature": "#1984",
+      "added": "2026-08-04",
+      "categories": [
+        "backend",
+        "frontend",
+        "unit",
+        "security",
+        "audit"
+      ],
+      "description": "An unhashed audit chain must not report as verified (#1984). POST /api/audit-log/verify answered valid:true, checked:0 for a log in which no entry carried a hash - found by probing a live instance: 1,162 real audit entries, zero hashes, green tick in the UI. valid:true was returned for three materially different states (chain verified, range empty, no integrity data at all) and the last is the DEFAULT for every install that never enabled hashing, so the verdict was independent of the input - the class 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: None rather than False, because False claims tampering - an equally wrong and much louder lie - while a caller doing a truthiness test degrades to 'not verified', the safe direction. Covers: rows-with-no-hashes yields unverifiable not valid; unverifiable is falsy but is NOT False and does not set first_invalid_id; empty_range is its own state (both it and no-hashes report checked:0, which was previously the only signal and made them indistinguishable); an intact chain still verifies and tampering is still detected (no regression); a partially hashed range is named verified_partial with skipped_unhashed exposed, so the permanent unhashed prefix of a late-enabled chain cannot pass for full coverage; the response reports hash_chain_enabled at all, mirroring what monitoring/status already does with enabled:false beside a stale summary; and the response model accepts a null valid (it was `bool`, which would have 500'd the fix at the router). The FRONTEND is covered in the same file deliberately: 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 merely-unhashed log. Pins the tri-state branch, the new store state, the removal of the store's own vacuous 'valid' for an empty list (asserted client-side without even calling the API), and an amber badge so the state does not fall through to 'Verify failed'. Source assertions strip // comments before matching - the inverse of the #1941 trap, since this fix's own comments quote the old code while explaining why it went."
     }
   ]
 }

tests/test_audit_log_unit.py

@@ -996,10 +996,11 @@ def test_verify_chain_valid(audit_service, audit_ops):
 
 
 def test_verify_chain_empty_range(audit_service, audit_ops):
-    """verify_chain on empty range returns valid=True, checked=0."""
+    """verify_chain on an empty range is unverifiable (#1984), not vacuously valid."""
     service, _, _ = audit_service
     result = asyncio.run(service.verify_chain(999999, 999999))
-    assert result["valid"] is True
+    assert result["valid"] is None
+    assert result["status"] == "empty_range"
     assert result["checked"] == 0