# Review brief: S3 change index, whole-branch code review

You are the final, independent reviewer of branch `s3-polling-336` before
it is merged. Read cold and adversarially. Verify against the code, cite
`file:line` for every claim.

**Range:** `git diff 5f341f33..<commit named in the prompt>` (5f341f33 is the
merge base with main; the branch already contains main up to it). The same
diff with commit list and stat is in
`.superpowers/sdd/2026-10-21-s3-change-index/review-5f341f33..26b9e850.diff`.

**Spec (binding):** `docs/superpowers/specs/2026-10-21-s3-change-index-design.md`
(rev 3 plus the §3.2 `schedule-status` row and the §4.4 confirm-in-bucket
note). **Plan:** `docs/superpowers/plans/2026-10-21-s3-change-index.md`.
**Execution ledger (rulings, deferred minors, carried findings):**
`.superpowers/sdd/2026-10-21-s3-change-index/progress.md`.

## Fixed rulings, do not re-argue

Spec §1 R1 to R4, and every line in the ledger that starts with `Ruling:`.
Report only mechanisms that fail to honour them, or that break something.

## What matters most

The feature is dormant until `CARLOS_CHANGE_INDEX=read` is set; writers
always bump. In priority order:

1. **Disarmed equivalence.** With the variable unset, is every box, edge,
   console and CLI path behaviourally identical to main, apart from the
   writers' best-effort bump of `control/changes.json`? Any new read,
   goroutine, changed store type, changed error, or changed timing.
2. **Destructive decisions on stale data, when armed.** Reconcile and
   sidecar removals, trash removals, moves steps, seed-sweep snapshot
   deletes, orphan-run deletes, claim install and retire. Each must still
   be proved against the live bucket, or by a snapshot that is provably at
   least as fresh as the walk it replaces.
3. **Lost changes beyond the 30-minute backstop.** Any write a reader acts
   on that can be missed indefinitely: scopes, gates, the cert mirror, the
   home map, the console sweeps, the fast tick's last-acted map.
4. **Concurrency.** Lock order `w.pass` then `reconcileMu` then
   `RecordView.mu` (a leaf); the watch's poll slot; the edge and agent
   sharing one watch; anything that can stall the health heartbeat.
5. **Cost.** The point of the work. Any steady-state loop that still reads
   per app or per box when nothing changed, once armed.
6. **Census and root.** Task 12's overlays
   (`internal/provision/testdata/plan14-root-reads-s3-change-index.tsv`,
   `internal/provision/s3_change_index_census_test.go`): are the classes
   right, and is root's new read-increment-CAS of `control/changes.json`
   (via `runSystemClaims`) acceptable?

Also triage the ledger's `minor (deferred)` lines: name any that must be
fixed before merge, and why. One is flagged by the controller as likely:
RecordView's index poll runs under the caller's unbounded context, and a
lock give-up leaves `ReadGate` readers trusting a frozen observation.

## Output

Numbered findings, each with severity (P1 = correctness, data loss or an
outage; P2 = a goal failure or significant gap; P3 = minor), `file:line`
evidence, and the change you would require. Then the deferred-minor triage
(must-fix list). End with a verdict: accept, or reject with the P1s that
block.