carlos / platform Public

settings-3: Astra's final review (r1 reject, r2 accept)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude 2 weeks ago 9dba061034931e8db1ce9d6aaaaeeeeef1d44f95 parent d9164c9

2 files changed, +83 −0

docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-3-R2.md +36 −0
docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-3.md +47 −0

diff --git a/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-3-R2.md b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-3-R2.md

new file mode 100644

index 000000000..8ba5eb534

--- /dev/null

+++ b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-3-R2.md

@@ -0,0 +1,36 @@

+# Astra re-review r2: settings-3-console-settings

+Verdict: ACCEPT

+Reviewed `df3def0f..d9164c9e` read-only against the binding spec and R11. Inspected regression tests and reported RED/GREEN output; tests were not rerun. No files changed.

+## Round-1 findings

+### 1. A missing registry lets the fleet gate pass without checking fleets

+ADDRESSED.

+- `internal/console/apisystemgeoip.go:267` leaves `fleetsListed` false for unset, missing or unreadable registries; it becomes true only after successful fleet listing at line 287.

+- `internal/console/updatesgate.go:59` blocks whenever that evidence is absent. Both enabling doors use this gate (`internal/console/apiupdates.go:241` and `:547`); clearing bypasses it.

+- GeoIP’s existing completeness flags and verdict remain unchanged; only the settings gate consumes the new fields.

+- `internal/console/updatesgate_test.go:187` covers both doors refusing enables without writes and accepting clears across missing/unset registry, unhealthy fleet reports and incomplete heartbeat reads.

+### 2. An unchanged off-stable major blocks a full-state clear

+ADDRESSED.

+- `internal/console/apiupdates.go:178` distinguishes an unchanged major from a newly set or changed one. Lines 202–208 still refuse changed majors off stable and mask unchanged stale majors during validation.

+- The separate enable calculation at line 238 still gates newly enabled manual mode, changed pins and changed majors. Resending an unchanged major cannot bypass it.

+- `internal/console/apiupdates_test.go:339` covers full-state clears with the fleet gate open and closed, preservation of the stale major, and refusal of a changed major.

+### 3. The control socket loses the channel for default settings

+ADDRESSED.

+- `internal/instanceupdates/control.go:89` fills a missing channel from the fetched route without modifying storage; an explicit settings channel retains precedence.

+- The fallback uses the same `reg.Get(host)` result at line 74. `Mount` passes its fixed host at line 48, so request parameters cannot select another host’s channel.

+- `internal/instanceupdates/control_test.go:166` and `:183` cover empty-column fallback, unchanged storage and explicit-channel precedence.

+## New findings in the fix diff

+None.

\ No newline at end of file

diff --git a/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-3.md b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-3.md

new file mode 100644

index 000000000..ff7e71419

--- /dev/null

+++ b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-3.md

@@ -0,0 +1,47 @@

+# Astra review: settings-3-console-settings

+Verdict: REJECT

+Reviewed `70f8119f…df3def0f` read-only. Tests were inspected, not rerun. No files changed.

+## Findings

+### 1. [P1] A missing registry lets the fleet gate pass without checking fleets

+**Where:** [internal/console/updatesgate.go:25](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-3-console-settings/internal/console/updatesgate.go:25), [internal/console/apisystemgeoip.go:258](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-3-console-settings/internal/console/apisystemgeoip.go:258).

+**Scenario:** The deployment has active fleet boxes, but the console’s registry mount is absent or its registry path is unset. `geoipInventory` returns successfully with both completeness flags still true, before listing fleets or reading their heartbeats. If the platform boxes report the new protocol, `updatesEnforced` returns true. Both settings doors then accept protected settings despite unchecked fleet boxes.

+**Why it matters:** Spec §4.4 requires every expected box to report protection. An unavailable fleet inventory is incomplete evidence, not evidence of zero fleets. Members can enable settings that older fleet binaries ignore.

+**Fix:** Make the settings inventory fail closed when the registry is unavailable. Permit a platform-only inventory only with affirmative evidence that no fleets need checking. Test missing and unset registry paths, active fleet boxes with old or missing reports, and incomplete heartbeat reads through both enabling doors. Preserve clearing behavior.

+### 2. [P2] An unchanged off-stable major blocks a full-state clear

+**Where:** [internal/console/apiupdates.go:175](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-3-console-settings/internal/console/apiupdates.go:175), [internal/console/apiupdates.go:197](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-3-console-settings/internal/console/apiupdates.go:197).

+**Scenario:** An instance has `major: 2` and a pin, then moves from `stable` to `beta`. The retained major is correctly ignored. A client clears the pin while resending the unchanged major: `{"major":2,"pin":""}`. `majorSet` becomes true merely because the number is present, and the handler returns 400 before calculating changes.

+**Why it matters:** The pin remains set. This misses the combination of spec §3.1’s ignored off-stable major, §5’s clearing behavior, R4’s preservation of stale majors, and Task 6’s explicit guarantee that unchanged fields beside a clear do not prevent clearing.

+**Fix:** Distinguish a changed major from an unchanged value. Apply the off-stable refusal to newly set or changed majors; mask an unchanged stale major during validation. Add full-state clearing tests on an off-stable record, including with the fleet gate closed.

+### 3. [P2] The control socket loses the channel for default settings

+**Where:** [internal/instanceupdates/control.go:80](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-3-console-settings/internal/instanceupdates/control.go:80), [internal/release/updatesettings.go:38](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-3-console-settings/internal/release/updatesettings.go:38).

+**Scenario:** An ordinary instance follows `beta` with no update overrides or app policy. Its settings column correctly remains `""`. Parsing that column produces only `Updates: "auto"`, and `RegistryReader` returns it without recovering `rt.Channel`. Authenticated `GET /updates` therefore reports `"channel":""`.

+**Why it matters:** Spec §5.1 requires the socket to return the box’s settings, including the channel defined in §3.1. This affects the default, untouched instance population.

+**Fix:** Populate the reported channel from the route when decoding the empty column. Keep the stored column empty to preserve upgrade compatibility. Extend the empty-column reader test with a nonempty route channel and assert the returned channel.

+## Deferred minors triage

+- **Must fix:** Task 5’s “registry absent yields a platform-only inventory that looks complete” entry, ledger line 61. This is finding 1.

+- **Fix alongside it:** Task 5’s incomplete-read and fleet-keyed coverage gaps, ledger line 63. These tests must demonstrate refusal through both enabling doors.

+- The remaining deferred entries do not independently block merge on the evidence reviewed. Findings 2 and 3 need regression tests beyond those listed in the ledger.

+## Rulings

+No numbered ruling needs overturning. Finding 2 completes R4 and Task 6’s clearing fix. R8 remains consistent with the settled preference model; I am not requesting channel-level passkey or approval requirements for an instance install request.

\ No newline at end of file