J4: attribution backfill

ROADMAP.md

@@ -723,23 +723,107 @@ sees the trail. J1–J4 are the open `BACKLOG.md` evidence entries, scheduled; *
   - **Reconciled:** there is no Admin tab — credential and `.env` editing lives on the ⚙ Setup
     page (`GET`/`POST /api/config`).
 
-- [ ] **J4** Attribution backfill. (S, P2)
-  `vpcopilot audit-backfill` fills only what is provably derivable, policy name to finding
-  via `policies.json`, marks the rest `unknown`, and records the backfill itself.
-  - Acceptance: no entry gets an invented actor or run_id; the backfill writes its own audit
-    record; a second run is a no-op.
-  - **Reconciled:** `audit` is a flat `@app.command()` with no typer sub-app, so `audit backfill`
-    as two words is not addable as written — `audit-backfill` matches the existing kebab
-    convention (`bench-model`, `apply-maluser`, `xc-rm`, `lab-create`). Smaller than it looks:
-    `export.build_audit_events` **already** recovers a finding from the `policies.json` index for
-    legacy entries and already leaves blanks rather than inventing. J4 persists what the exporter
-    derives at read time. **It cannot rewrite `audit.log`**: the log is append-only
-    (`audit.py:26`) and `record()` strips caller-supplied identity (`_STAMPED`, `audit.py:17,21`),
-    enforced by `test_identity_cannot_be_overridden_by_a_caller`. The backfill must write a
-    sidecar (e.g. `<out>/audit-backfill.json`) that `export.build_audit_events` consults beside
-    its existing `by_policy` lookup. Note a second copy of that lookup already exists —
-    `ledger.find_finding_for_policy` (`ledger.py:108`) — consolidate on one rather than adding a
-    third. Needs a `POST /api/audit-backfill` twin per the two-surfaces invariant.
+- [x] **J4** Attribution backfill. (S, P2) — **DONE:** `vpcopilot audit-backfill` +
+  `POST /api/audit-backfill`. `backfill.py` freezes the finding each audit entry belongs to into
+  `<out>/audit-backfill.json`, which `export.build_audit_events` reads beside the log and the
+  evidence bundle ships. Verified against the two real audit logs on disk. 21 tests.
+  - **Acceptance, as met:** no entry gets an invented actor or run_id ✅ — guaranteed by having
+    nowhere to put one: the sidecar carries attribution and nothing else, pinned by a test on its
+    keys; the backfill writes its own audit record ✅ (`audit_backfill`, with identity stamped
+    centrally like every other action); a second run is a no-op ✅ — nothing written, nothing
+    recorded.
+  - **The reconciled note undersold why this matters, and running it on real data showed it.** It
+    framed J4 as persisting what the exporter already derives at read time. But `policies.json` is
+    rewritten by **every** scan, so the derivation does not merely decay — it can go *wrong*. Two
+    outcomes, and the second is the one worth building for:
+    - Re-scan into the same out dir and the mapping is gone; the exporter silently stops attributing
+      those entries.
+    - If the later scan generates a policy with the **same name** for a **different** finding, the
+      live lookup still succeeds and attributes the old entry to the **wrong** finding — a confident
+      wrong answer in the artifact whose entire job is to be trustworthy. That is why the frozen
+      answer **wins over** the live lookup rather than merely filling gaps in it.
+    Both reproduced against the real `out-claude` log, whose four `refine_apply` entries record a
+    policy but no finding: attribution survived a wiped index, and a planted name collision failed to
+    move them.
+  - **`unknown` is sticky, deliberately.** An entry the backfill looked at and could not resolve stays
+    unresolved, so a later `policies.json` cannot supply an answer the backfill already declined to
+    give. "We looked and could not establish this" and "we have not looked" are different facts —
+    the same distinction H2 draws between an unpinned dependency and a clean one.
+  - **The no-op is load-bearing, not tidiness.** The command appends its own `audit_backfill` entry,
+    so without the check every run would see one more entry than the last, decide something had
+    changed, and append again: a log that grows by a line every time anyone asks whether it needs
+    backfilling. (The I1 precedent — a reconcile pass that changes nothing writes nothing.)
+  - **Keyed by index, verified by `(ts, action)`.** The index is stable only because the log is
+    append-only; if it is ever rebuilt or truncated the index still resolves — to a *different*
+    entry. Mismatched rows are dropped and counted rather than moved onto the wrong record, and the
+    count is surfaced, because a non-zero one means the log was rewritten and that is itself worth
+    knowing.
+  - **Verified against the real logs, and the no-op case is the interesting one.** `demo/out` is
+    already fully attributed — every entry carries its `finding_id` — so the backfill resolved
+    nothing and wrote nothing, which is the correct answer and the one a synthetic fixture would not
+    have exercised. `out-claude` had four genuinely unattributed `refine_apply` entries; all four
+    resolved, and a repeat run left `audit.log` byte-identical.
+  - **Found by adversarial review, before shipping** — and the first one nearly shipped a feature
+    that destroyed its own purpose.
+    - **A second run wiped the attribution it had just frozen.** `derive` recomputed every row from
+      the CURRENT `policies.json`, which is precisely what the sidecar is a defence against: run the
+      command, re-scan, run it again, and every resolved row collapsed to `unknown` — permanently,
+      because `unknown` is sticky. The second use of the feature undid the first. **Two reviewers
+      found it independently**, which is the signal worth noting. The sidecar is now monotonic: a row
+      already on disk is carried forward verbatim and never recomputed, exactly like the append-only
+      log it annotates. Forcing a fresh derivation means deleting the sidecar — an explicit act, not
+      a side effect of running a command twice.
+    - **The sidecar was written non-atomically.** `write_text` truncates first, so a crash mid-write
+      left a half-written evidence file — and `load` swallows the resulting `JSONDecodeError`, so the
+      failure mode was losing every frozen attribution without a word. Written to a temp file and
+      `os.replace`d now. The skeptic verifying it went further and **found a weakness in the test
+      that was supposed to cover this**: `test_a_corrupt_sidecar_is_ignored_rather_than_fatal`
+      pinned "does not crash" but not "does not mis-attribute", because its fixture left the live
+      index agreeing with the frozen answer. With a re-scan that reused a policy name, a torn
+      sidecar fell through and produced a confidently wrong attribution — reproduced at 372 wrong
+      exports across 1,887 concurrent reads. So *absent* and *unreadable* are now different answers:
+      no sidecar means nobody froze anything and the live index is the best available guess, while a
+      sidecar that exists and will not parse means the live index is by definition not the one those
+      entries belong to, and the cell is left blank.
+    - **The bundle's caveats said nothing about it.** A derived sidecar that can supply a
+      `finding_id` is exactly what the manifest's caveat list exists to disclose; it now says what
+      the sidecar is, that it carries no identity, and that where it and the log disagree the log is
+      the evidence.
+    - **A corrupt `policies.json` tracebacked out of the CLI and 500'd the console.** Keeping the
+      raise for the apply paths (above) is right; propagating it out of a read-mostly evidence
+      command is not. An unreadable index means nothing can be established, which is a decline with a
+      reason.
+    - **Rich silently ate the `[dry-run]` marker.** `rprint(f"[dim]{m}[/dim]")` reads `[dry-run]` as
+      a markup tag and drops it — so the one word telling an operator that nothing was written was
+      the one word that vanished. Escaped at this command's log sink. The same pattern exists in
+      other commands whose messages contain brackets; only the one whose output actually does was
+      changed here rather than sweeping the CLI.
+    - **The two removed copies disagreed about duplicate policy names** — the exporter's inline dict
+      was last-wins, `find_finding_for_policy` first-wins — so consolidation could not preserve both.
+      First-wins is the deliberate unification: it matches the apply path, the safety-critical
+      consumer. Pinned, with the caveat that `policies.json` should never contain duplicates anyway.
+  - **Found while verifying the reviewers: consolidating the lookup silently changed three
+    behaviours.** Compared against the previous implementation across nine inputs, `policy_index`
+    had altered duplicate-name precedence (last-wins instead of first), filtered falsy `finding_id`s
+    to `None`, and swallowed a corrupt `policies.json` that used to raise. All three reach four
+    apply-path callers, where the corrupt-file case is the sharp one: returning `None` instead of
+    raising means applying a band-aid with weaker probe validation and no explanation. The old
+    semantics are restored exactly and pinned by a parametrized comparison; the exporter keeps its
+    own tolerance at its call site, and the backfill does its own `or None`. A consolidation that
+    quietly changes behaviour is worse than the duplication it removes.
+  - **The limit is stated rather than left to be discovered** (the J1 precedent). A forged sidecar
+    can move `finding_id` and the columns joined *through* it — `title`, `vuln_class`, `severity`,
+    `ledger_state`, `pr_url` — and nothing else: everything the log stamped is unreachable. That adds
+    no trust assumption the export did not already make, because `findings.json` and `ledger.json`
+    already drive those same joins and are the same kind of file in the same directory. Verified end
+    to end: the sidecar is digested in the bundle manifest, a clean bundle verifies at 47 members,
+    and a forged one is caught as `MISMATCH audit-backfill.json`.
+  - **Reconciled, and confirmed accurate:** `audit` is a flat `@app.command()` with no typer sub-app,
+    so `audit-backfill` matches the kebab convention. `export.build_audit_events` already left blanks
+    rather than inventing. The log cannot be rewritten (`audit.py` `_STAMPED`, enforced by
+    `test_identity_cannot_be_overridden_by_a_caller`), hence the sidecar. The duplicate lookup was
+    real — `ledger.find_finding_for_policy` and an inline copy in the exporter — and J4 would have
+    been the third; both now go through `ledger.policy_index`, the one implementation.
 
 - [ ] **J5** Weakness and framework mapping. (M, P2)
   Stamp each finding with a CWE ID and an OWASP API or Web Top 10 category at triage, and

docs/AUDIT.md

@@ -568,7 +568,7 @@ back-fills it.
 |---|---|
 | No `run_id` / `actor` / `host` / `tool_version` | Those cells export **blank**. They are not inferred from the current environment — a guess in an audit trail is worse than a gap. |
 | `open_pr` wrote `finding`, not `finding_id` | Resolved via the `finding` key. Current builds write **both** (`pr.py`), so old and new logs read the same way. |
-| `apply_*` recorded no finding at all | Resolved through the `policies.json` index: `entry.policy` → `policy_name` → `finding_id`. Works only for entries that name a `policy`, and only while that scan's `policies.json` is still in the dir. Otherwise blank. |
+| `apply_*` recorded no finding at all | Resolved through the `policies.json` index: `entry.policy` → `policy_name` → `finding_id`. Works only for entries that name a `policy`, and only while that scan's `policies.json` is still in the dir — **`vpcopilot audit-backfill` freezes it before that expires** (§9.1). Otherwise blank. |
 | No `namespace` | Blank. The `lb` is still there; the tenant/namespace is not recoverable after the fact. |
 | No `kept` | Older entries (and any action that omits it) fall through to `passed`/`failed` rather than `kept`. Absence of `kept` is not evidence of rollback. |
 | Mixed success keys (`passed` / `config_enabled` / `enabled`) | Coalesced into one `outcome` (§5) — including the seeded demo dataset, which uses its own mix. |
@@ -582,6 +582,70 @@ cells come out blank:
 vpcopilot export --out demo/out --output /tmp/demo-evidence.zip
 ```
 
+### 9.1 Attribution backfill (J4)
+
+The `policies.json` fallback above has an expiry date, and it is easy to miss: **every scan rewrites
+`policies.json`**. Re-scan into the same out dir and the mapping from an old entry's policy name back
+to its finding is gone — the exporter quietly stops attributing those entries. Worse, if the new scan
+generates a policy with the *same name* for a *different* finding, the lookup still succeeds and
+attributes the old entry to the **wrong** finding: a confident wrong answer, in the artifact whose
+entire job is to be trustworthy.
+
+```sh
+vpcopilot audit-backfill --out out            # freeze what is derivable right now
+vpcopilot audit-backfill --out out --dry-run  # report it, write nothing
+```
+
+It writes `<out>/audit-backfill.json`, a sidecar the exporter reads beside the log, and ships it in
+the evidence bundle so a reviewer receives both together.
+
+Four properties are the whole point:
+
+- **`audit.log` is never edited.** It is append-only and `record()` strips caller-supplied identity;
+  a backfill that could rewrite an entry would destroy the property that makes the log worth keeping.
+  The only thing appended is the backfill's own `audit_backfill` record.
+- **Nothing is invented.** The sidecar carries **no `actor`, no `run_id`, no `host`** — there is
+  nowhere to put one. It maps an entry to a finding and nothing else; identity stays on the log.
+- **The frozen answer wins over the live lookup**, because after a re-scan the live index describes a
+  different run.
+- **`unknown` is sticky.** An entry the backfill looked at and could not resolve stays unresolved,
+  so a later `policies.json` cannot supply an answer the backfill already declined to give. "We
+  looked and could not establish this" is a fact worth keeping.
+
+A second run writes nothing and records nothing. That matters more than tidiness: the command appends
+its own entry, so without the check each run would see one more entry than the last, decide something
+had changed, and append again.
+
+The sidecar is keyed by the entry's index in the append-only log and verified against its
+`(ts, action)`. If the log is ever rebuilt or truncated, mismatched rows are **dropped and counted**
+rather than silently moved onto the wrong entry. It is written atomically (temp file plus rename),
+because a half-written evidence file that will not parse is worse than none.
+
+**Absent and unreadable are different answers.** No sidecar means nobody has frozen anything, so the
+exporter falls back to the live policy index as it always did. A sidecar that is present and will
+*not* parse means somebody did freeze attribution and it is now unreadable — and the live index is,
+by definition, not the one those entries belong to. The cell is left blank rather than guessed.
+
+#### What a forged sidecar could do — and what it could not
+
+The log is append-only; a sidecar is an ordinary file. So state the limit plainly rather than leave
+a reviewer to work it out.
+
+A hand-edited `audit-backfill.json` can change an entry's `finding_id`, and through it the fields
+joined *from* that id: `title`, `vuln_class`, `severity`, `ledger_state`, `pr_url`. It cannot change
+anything the log itself stamped — `ts`, `actor`, `host`, `run_id`, `action`, `outcome`, `lb`,
+`tool_version` all come straight from `audit.log` and are unreachable from the sidecar.
+
+**This adds no trust assumption that the export did not already make.** `findings.json` and
+`ledger.json` already drive exactly those joined columns, and they are the same kind of file in the
+same directory — anyone who can forge the sidecar can forge those and get the same result. The
+sidecar is digested in the bundle manifest like every other member (§6), so tampering *after* export
+is caught by `export --verify`; tampering *before* export is a question about who has write access to
+the run directory, which no artifact in it can answer.
+
+The one thing that survives all of it is the log: append-only, identity stamped centrally, and
+shipped verbatim in the bundle. If the sidecar and the log ever disagree, the log is the evidence.
+
 ---
 
 ## See also