amadan
settings-2: Astra's final review findings (REJECT, fixed) and scoped re-review (ACCEPT)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+67
−0
docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-2-R2.md+18 −0docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-2.md+49 −0
CI passed — run details
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-2-R2.md b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-2-R2.md| new file mode 100644 |
| index 000000000..c243619de |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-2-R2.md |
| @@ -0,0 +1,18 @@ |
| +Verdict: ACCEPT |
| + |
| +## Round-1 findings |
| + |
| +1. **ADDRESSED** — [internal/release/materializeheld.go:126](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-2-promotion-records/internal/release/materializeheld.go:126) skips digestless entries on channels carrying digests and continues looking for evidence. The regression at `internal/release/heldauth_test.go:381` checks refusal and no downloads with the flag both off and on. |
| +2. **ADDRESSED** — [internal/release/materializeheld.go:107](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-2-promotion-records/internal/release/materializeheld.go:107) calls `Admit` before checking whether the pointer names the requested version, following R15. Naming-pointer retry behavior remains intact; `ratchet.go:168` remembers observations before persistence. The two-pass replay regression is at `internal/release/heldauth_test.go:418`. |
| +3. **ADDRESSED** — [internal/edge/storedoor.go:451](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-2-promotion-records/internal/edge/storedoor.go:451) grants only the exact five-segment promotion-record shape for the owning account, validating app and version. Listing, writes, and deletes remain denied. `internal/edge/fleetapi_e2e_test.go:628` exercises record-only authorization and digest-mismatch refusal through the actual door and fleet client. |
| +4. **ADDRESSED** — [internal/updatewords/words_authorize.go:43](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-2-promotion-records/internal/updatewords/words_authorize.go:43) now reports an unconfirmed write and advises retrying, without claiming nothing was signed. |
| + |
| +## New findings |
| + |
| +None. No new Critical or Important problem found in the fix diff. |
| + |
| +## Out of scope |
| + |
| +None. |
| + |
| +Read-only source review of `98e3f164..2c3f0ad8`. Reviewed the reported test and mutation evidence; did not rerun tests or modify files. |
| \ No newline at end of file |
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-2.md b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-2.md| new file mode 100644 |
| index 000000000..be23fc33d |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-2.md |
| @@ -0,0 +1,49 @@ |
| +Verdict: REJECT |
| + |
| +1. **Important — [internal/release/materializeheld.go:108](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-2-promotion-records/internal/release/materializeheld.go:108): A digested head admits an undigested history target.** |
| + `r.Admit` checks the head’s digest. If that passes but `p.DigestOf(version)` is empty, `HeldAuthorized` still returns success with no digest binding. |
| + |
| + **Failure:** A signed pointer has head `v2` with a digest and legacy history entry `v1` without one. `v1` has no promotion record. Replace its unsigned manifest and artifacts, then request a held stage of `v1`: the replacement bytes pass even with `CARLOS_REQUIRE_POINTER_DIGEST=true`. This bypasses the protection against undigested historical targets in §4.6. |
| + |
| + **Fix:** Reject a digestless history entry when its channel carries digests; continue looking for valid alternative evidence. Add a mixed-head/history regression test that checks both refusal and absence of downloaded artifacts. The existing ratchet test exercises only wholly digestless pointers. |
| + |
| +2. **Important — [internal/release/materializeheld.go:93](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-2-promotion-records/internal/release/materializeheld.go:93): Verified pointers can escape the ratchet’s memory.** |
| + The version-name check skips `Admit` after successfully verifying a pointer that names another build. §4.6 requires remembering that the box has verified digests for that channel. |
| + |
| + **Failure:** While authorizing `v1` through channel B, the box verifies channel A’s digested pointer for legacy build `v2`, but records no mark. Later, a saved digestless A pointer is replayed alongside substituted `v2` bytes. With the fleet-wide flag off and no promotion record for this legacy build, a held stage of `v2` succeeds. |
| + |
| + **Fix:** Separate observing a verified digested pointer from selecting evidence for this request. Remember every such observation, preserving fail-closed behavior after persistence failures. Replace `TestHeldAuthorizedAdmitsOnlyNamingPointers`’s no-mark assertion with a two-pass replay test; its current assertion protects the wrong behavior. |
| + |
| +3. **Important — [internal/release/promotion.go:124](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-2-promotion-records/internal/release/promotion.go:124): Fleet boxes cannot read promotion records.** |
| + The new reader requests `releases/<acct>/<app>/promoted/<version>`, but `internal/edge/storedoor.go:425` grants no reads under `releases/`. The fleet client converts the resulting 404 into `os.ErrNotExist`, which becomes `ErrNoPromotionRecord`. |
| + |
| + **Failure:** A validly authorized build outside pointer history cannot be staged on a box using the fleet HTTPS store. Explicitly authorizing it again cannot repair this. If a legacy pointer still names it, the reader instead falls back to weaker evidence despite the existing digest-bound record. |
| + |
| + **Fix:** Grant fleet read access to the exact promotion-record shape within its own account. Keep writes and other accounts denied. Add a test through the actual store door and fleet client, covering record-only authorization and digest-mismatch refusal. |
| + |
| +4. **Minor — [internal/updatewords/words_authorize.go:43](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-2-promotion-records/internal/updatewords/words_authorize.go:43): “Nothing was signed” is substantively false after a write error.** |
| + Signing occurs before `Put`, and a failed response does not establish that storage rejected the write. |
| + |
| + **Failure:** Storage commits the authorization but the response times out. The console tells the member nothing was signed, although the build is now authorized. |
| + |
| + **Fix:** Report that authorization could not be confirmed, or verify the stored record before reporting the outcome. This is an accuracy issue, independent of copy style. |
| + |
| +## Rulings |
| + |
| +**R13:** Disagree with checking whether the pointer names the requested version *before* updating the ratchet. That contradicts §4.6’s “once verified” rule and produces finding 2. The distinction between transient failures and definitive refusals should remain. |
| + |
| +## Deferred minors that must be fixed before merge |
| + |
| +None of the ledger’s deferred minors independently warrants blocking merge. Findings 1–3 and their regression coverage do. |
| + |
| +Read-only source review against merge base `412ca787`; no files changed or tests executed. |
| +--- |
| + |
| +**Controller response (fix wave 1, commits 6f79b01d, 0fc74c11, 2c3f0ad8).** All four findings accepted; none rejected. |
| + |
| +- Finding 1 (Ruling R16): a naming pointer whose entry for the version has no digest, on a channel that carries digests, is not held-stage evidence; HeldAuthorized skips it and keeps looking. Cost if wrong: a legacy history build on a now-digested channel needs `carlos releases authorize` before a move can stage it. |
| +- Finding 2 (Ruling R15, revising R13's ordering): HeldAuthorized admits every verified pointer it reads through the ratchet, then selects evidence among those naming the version; R13's retry/skip split stands for naming pointers. Cost if wrong: an unrelated channel's mark is written during a held stage, as adoption would write it. |
| +- Finding 3 (Ruling R17): the fleet store door grants GET of exactly `releases/<sqid>/<app>/promoted/<version>` for the box's own account; no listing, no write, nothing else under `releases/`. Cost if wrong: a box can read its own account's signed build digests (no secret). |
| +- Finding 4 (Ruling R18): the `settings.authorize.write-failed` draft no longer claims nothing was signed. Still a draft under copy review. |
| + |
| +Scoped re-review: CODEX-FINDINGS-SETTINGS-IMPL-2-R2.md (ACCEPT). |