| 1 | # Review brief: S3 change index, whole-branch code review |
| 2 | |
| 3 | You are the final, independent reviewer of branch `s3-polling-336` before |
| 4 | it is merged. Read cold and adversarially. Verify against the code, cite |
| 5 | `file:line` for every claim. |
| 6 | |
| 7 | **Range:** `git diff 5f341f33..<commit named in the prompt>` (5f341f33 is the |
| 8 | merge base with main; the branch already contains main up to it). The same |
| 9 | diff with commit list and stat is in |
| 10 | `.superpowers/sdd/2026-10-21-s3-change-index/review-5f341f33..26b9e850.diff`. |
| 11 | |
| 12 | **Spec (binding):** `docs/superpowers/specs/2026-10-21-s3-change-index-design.md` |
| 13 | (rev 3 plus the §3.2 `schedule-status` row and the §4.4 confirm-in-bucket |
| 14 | note). **Plan:** `docs/superpowers/plans/2026-10-21-s3-change-index.md`. |
| 15 | **Execution ledger (rulings, deferred minors, carried findings):** |
| 16 | `.superpowers/sdd/2026-10-21-s3-change-index/progress.md`. |
| 17 | |
| 18 | ## Fixed rulings, do not re-argue |
| 19 | |
| 20 | Spec §1 R1 to R4, and every line in the ledger that starts with `Ruling:`. |
| 21 | Report only mechanisms that fail to honour them, or that break something. |
| 22 | |
| 23 | ## What matters most |
| 24 | |
| 25 | The feature is dormant until `CARLOS_CHANGE_INDEX=read` is set; writers |
| 26 | always bump. In priority order: |
| 27 | |
| 28 | 1. **Disarmed equivalence.** With the variable unset, is every box, edge, |
| 29 | console and CLI path behaviourally identical to main, apart from the |
| 30 | writers' best-effort bump of `control/changes.json`? Any new read, |
| 31 | goroutine, changed store type, changed error, or changed timing. |
| 32 | 2. **Destructive decisions on stale data, when armed.** Reconcile and |
| 33 | sidecar removals, trash removals, moves steps, seed-sweep snapshot |
| 34 | deletes, orphan-run deletes, claim install and retire. Each must still |
| 35 | be proved against the live bucket, or by a snapshot that is provably at |
| 36 | least as fresh as the walk it replaces. |
| 37 | 3. **Lost changes beyond the 30-minute backstop.** Any write a reader acts |
| 38 | on that can be missed indefinitely: scopes, gates, the cert mirror, the |
| 39 | home map, the console sweeps, the fast tick's last-acted map. |
| 40 | 4. **Concurrency.** Lock order `w.pass` then `reconcileMu` then |
| 41 | `RecordView.mu` (a leaf); the watch's poll slot; the edge and agent |
| 42 | sharing one watch; anything that can stall the health heartbeat. |
| 43 | 5. **Cost.** The point of the work. Any steady-state loop that still reads |
| 44 | per app or per box when nothing changed, once armed. |
| 45 | 6. **Census and root.** Task 12's overlays |
| 46 | (`internal/provision/testdata/plan14-root-reads-s3-change-index.tsv`, |
| 47 | `internal/provision/s3_change_index_census_test.go`): are the classes |
| 48 | right, and is root's new read-increment-CAS of `control/changes.json` |
| 49 | (via `runSystemClaims`) acceptable? |
| 50 | |
| 51 | Also triage the ledger's `minor (deferred)` lines: name any that must be |
| 52 | fixed before merge, and why. One is flagged by the controller as likely: |
| 53 | RecordView's index poll runs under the caller's unbounded context, and a |
| 54 | lock give-up leaves `ReadGate` readers trusting a frozen observation. |
| 55 | |
| 56 | ## Output |
| 57 | |
| 58 | Numbered findings, each with severity (P1 = correctness, data loss or an |
| 59 | outage; P2 = a goal failure or significant gap; P3 = minor), `file:line` |
| 60 | evidence, and the change you would require. Then the deferred-minor triage |
| 61 | (must-fix list). End with a verdict: accept, or reject with the P1s that |
| 62 | block. |
| 63 | |
amadan