fix: review round 1 -- re-running the agent doubled findings; report render was unstable

Two defects found by probing the shipped code rather than re-reading the diff.

1. `make agent` twice on one date -- an ordinary thing to do -- appended the
   same logical finding to ops.finding_log twice, and the report counted rows
   rather than findings. One open finding rendered as "2 still open". A diff
   whose counts are wrong is worse than no diff, because the counts are read as
   a measurement. `_latest_per_key` collapses to one row per finding_key per
   run; `make monitor` gets the same treatment.

2. The diff sections were rendered in set-iteration order, so regenerating a
   report reshuffled its lines with no content change. Reports are committed
   artifacts: an unstable render puts noise in every git diff, and a diff that
   is usually noise stops being read. Sorted by finding key, with a test that
   renders twice and compares.

Also added a test that an unescaped pipe in detail text cannot split a Markdown
table cell -- it was already handled, but nothing pinned it.

Reviewed: 279 tests pass (275 before), ruff clean. reports/daily-2026-07-01.md
re-rendered under the stable ordering; counts are unchanged (6 new, 0 cleared,
2 still open), only line order. Verified byte-stable across two regenerations.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199ePw5w34FyfGAB41mkv3G

Author: rubinder

docs/ai-sdlc/decisions/0007-finding-history-in-iceberg-not-in-the-incident-files.md

@@ -91,6 +91,27 @@ have to agree, or "first seen" describes something different from the file on
 disk. The filename also stopped rendering `None` for breaches that carry no
 column.
 
+## Review round: two defects the tests did not reach
+
+**1. Running `make agent` twice in one day doubled the findings.**
+`ops.finding_log` is append-only, and re-running the agent on the same date is
+an entirely ordinary thing for a human to do. The report counted *rows*, not
+findings, so a second run turned one open finding into "2 still open". A
+day-over-day diff whose counts are wrong is worse than no diff at all, because
+the counts are read as a measurement. `_latest_per_key` now collapses to one row
+per `finding_key` per run; the same applies to `make monitor`.
+
+**2. The report was not byte-stable across regenerations.**
+The diff sections were built from set differences and rendered in set-iteration
+order, so simply regenerating a report reshuffled its lines. These are committed
+artifacts: an unstable render produces a git diff on every regeneration that has
+nothing to do with what changed, and a diff that is usually noise is a diff
+nobody reads. Now sorted by finding key, with a test that renders twice and
+compares.
+
+Both were found by probing the shipped code, not by re-reading the diff. Neither
+would have failed a test that only asked "does the report render?"
+
 ## Scope note
 
 `drift-demo --day N` was added because the report needed something to narrate.

reports/daily-2026-07-01.md

@@ -84,16 +84,16 @@ _Assembled from `ops.agent_runs`, `ops.finding_log`, `ops.monitor_results` and `
 Compared with `2026-06-30`: **6 new**, **0 cleared**, **2 still open**.
 
 ### New
-- `[breaking]` [silver.transactions_arrival_gap] 1 missing period(s): 2026-07-01
+- `[breaking]` [bronze_txn_row_count] 5 is 0% of trailing median 250000
 - `[additive]` column 'settlementDays' present in table but not declared in contract
 - `[breaking]` latest snapshot added 5 rows, below 50% of trailing median 250,000
-- `[breaking]` [silver.stock_prices_arrival_gap] 1 missing period(s): 2026-07-01
-- `[breaking]` [bronze_txn_row_count] 5 is 0% of trailing median 250000
 - `[breaking]` [gold.forecast_training_set_arrival_gap] 1 missing period(s): 2026-07-01
+- `[breaking]` [silver.stock_prices_arrival_gap] 1 missing period(s): 2026-07-01
+- `[breaking]` [silver.transactions_arrival_gap] 1 missing period(s): 2026-07-01
 
 ### Still open
-- `[additive]` column 'merchantCategoryCode' present in table but not declared in contract (open 2d (since 2026-06-29))
 - `[renaming]` column 'checkNumber' was renamed to 'check_reference' (field id 20); the contract still declares the old name (open 1d (since 2026-06-30))
+- `[additive]` column 'merchantCategoryCode' present in table but not declared in contract (open 2d (since 2026-06-29))
 
 ## Open incidents
 

src/ops/report.py

@@ -71,6 +71,21 @@ def _as_date(value) -> date | None:
     return value.date() if hasattr(value, "date") else value
 
 
+def _latest_per_key(rows: list[dict]) -> list[dict]:
+    """One row per `finding_key`, last write wins.
+
+    `ops.finding_log` is append-only, so running `make agent` twice on the same
+    date -- an entirely ordinary thing for a human to do -- writes the same
+    logical finding twice. Counting rows instead of findings then reports "2
+    still open" where one finding exists, and the diff's whole value is that
+    its counts mean something.
+    """
+    latest: dict[str, dict] = {}
+    for row in rows:
+        latest[row["finding_key"]] = row
+    return list(latest.values())
+
+
 def load_snapshot(engine, as_of: date) -> ReportSnapshot:
     """Select everything the report needs for `as_of`. The only engine call."""
     runs = [_as_date(r["run_at"]) for r in _rows(
@@ -84,8 +99,9 @@ def load_snapshot(engine, as_of: date) -> ReportSnapshot:
     for row in findings:
         row["run_at"] = _as_date(row["run_at"])
 
-    today = [f for f in findings if f["run_at"] == as_of]
-    prior = [f for f in findings if f["run_at"] == previous] if previous else []
+    today = _latest_per_key([f for f in findings if f["run_at"] == as_of])
+    prior = _latest_per_key(
+        [f for f in findings if f["run_at"] == previous]) if previous else []
 
     # First time each key was ever seen, across the whole log.
     first_seen: dict[str, date] = {}
@@ -105,9 +121,13 @@ def load_snapshot(engine, as_of: date) -> ReportSnapshot:
                      default=None)
         monitors_today = [m for m in monitor_rows if m["run_at"] == newest]
 
+    monitors_today = list({m["monitor"]: m for m in monitors_today}.values())
+
     monitor_runs = sorted({m["run_at"] for m in monitor_rows})
     current_run = monitors_today[0]["run_at"] if monitors_today else None
     prior_runs = [r for r in monitor_runs if current_run and r < current_run]
+    # Same dedupe reasoning as `_latest_per_key`: a second `make monitor` on
+    # one date must not turn one metric into two.
     previous_monitors = {
         m["monitor"]: m["metric"]
         for m in monitor_rows if prior_runs and m["run_at"] == prior_runs[-1]
@@ -329,9 +349,16 @@ def _diff_section(snapshot: ReportSnapshot) -> list[str]:
 
     today = {f["finding_key"]: f for f in snapshot.findings}
     prior = {f["finding_key"]: f for f in snapshot.previous_findings}
-    new = [today[k] for k in today.keys() - prior.keys()]
-    cleared = [prior[k] for k in prior.keys() - today.keys()]
-    still = [today[k] for k in today.keys() & prior.keys()]
+    # Sorted, not set-iteration order. These are committed artifacts: a report
+    # whose lines shuffle between runs produces a git diff on every
+    # regeneration that has nothing to do with what changed, and a diff that is
+    # usually noise is a diff nobody reads.
+    def _ordered(keys, source):
+        return [source[k] for k in sorted(keys)]
+
+    new = _ordered(today.keys() - prior.keys(), today)
+    cleared = _ordered(prior.keys() - today.keys(), prior)
+    still = _ordered(today.keys() & prior.keys(), today)
 
     lines.append(f"Compared with `{snapshot.previous_run}`: "
                  f"**{len(new)} new**, **{len(cleared)} cleared**, "

tests/test_report.py

@@ -304,3 +304,62 @@ def test_findings_written_by_the_agent_are_keyed_consistently(engine):
         # and the incident slug is derived from the same key
         assert actions.incident_slug(item["finding"]).endswith(
             __import__("hashlib").sha256(key.encode()).hexdigest()[:8])
+
+
+def test_running_the_agent_twice_in_one_day_does_not_double_the_findings(engine):
+    """`ops.finding_log` is append-only and re-running `make agent` is
+    ordinary. Counting rows instead of findings reported "2 still open" for a
+    single finding -- and a diff whose counts are wrong is worse than no diff,
+    because it is read as a measurement."""
+    maintenance.evolve_day(engine, 1)
+    graph.run(engine=engine, dry_run=True, as_of=D2)
+    graph.run(engine=engine, dry_run=True, as_of=D2)   # a human re-runs it
+
+    snapshot = report.load_snapshot(engine, D2)
+    keys = [f["finding_key"] for f in snapshot.findings]
+    assert len(keys) == len(set(keys)), f"duplicate findings in one run: {keys}"
+
+    graph.run(engine=engine, dry_run=True, as_of=D3)
+    text = report.build_report(report.load_snapshot(engine, D3))
+    assert "**0 new**, **0 cleared**, **1 still open**" in text
+
+
+def test_running_monitors_twice_in_one_day_does_not_double_the_rows(engine):
+    runner.persist(engine, runner.run_monitors(engine, D3))
+    runner.persist(engine, runner.run_monitors(engine, D3))
+    graph.run(engine=engine, dry_run=True, as_of=D3)
+
+    snapshot = report.load_snapshot(engine, D3)
+    names = [m["monitor"] for m in snapshot.monitors]
+    assert len(names) == len(set(names)), "monitor rows duplicated for one run"
+
+
+def test_a_pipe_in_a_detail_string_does_not_break_the_table():
+    """Detail text is machine-generated but carries column names and values;
+    an unescaped `|` silently splits a Markdown table cell."""
+    text = report.build_report(_snapshot(findings=[
+        _finding("k", D3, kind="staleness", detail="a | b | c")]))
+    row = next(ln for ln in text.splitlines()
+               if ln.startswith("| `breaking`") and "b" in ln)
+    assert "\\|" in row, "the pipe in the detail text was not escaped"
+    # Escaped pipes still contain a `|` character, so strip them before
+    # counting the structural ones: four cells means five separators.
+    assert row.replace("\\|", "").count("|") == 5, (
+        f"pipe leaked into the table structure: {row}")
+
+
+def test_the_report_is_byte_stable_across_regeneration(engine):
+    """Reports are committed artifacts. Set-iteration order made the diff
+    sections shuffle between runs, producing a git diff on every regeneration
+    that had nothing to do with what changed."""
+    maintenance.evolve_day(engine, 1)
+    graph.run(engine=engine, dry_run=True, as_of=D2)
+    maintenance.evolve_day(engine, 4)
+    graph.run(engine=engine, dry_run=True, as_of=D3)
+
+    first = report.build_report(report.load_snapshot(engine, D3))
+    second = report.build_report(report.load_snapshot(engine, D3))
+    assert first == second
+
+    # and stable across a fresh load, not just a repeated render
+    assert report.build_report(report.load_snapshot(engine, D3)) == first