carlos / platform Public

settings-5: Astra's findings (r1 REJECT, r2 REJECT, r3 ACCEPT)

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

claude 1 week ago 86329f0ed689c5841b9fd99fb022b150c25f3c1a parent 5179064

3 files changed, +172 −0

docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R2.md +40 −0
docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R3.md +31 −0
docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5.md +101 −0

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

new file mode 100644

index 000000000..c66d2c0be

--- /dev/null

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

@@ -0,0 +1,40 @@

+# Astra re-review r2: settings-5-surfaces

+Range 561c7dfb..40e7782e. N1 fixed in 51790645 (wave 2).

+Verdict: REJECT

+Reviewed `561c7dfb..40e7782e` against rev 11 and R18–R21 by inspection. No files modified. **Go cannot run here; I did not run tests.** I inspected the regression tests and implementer’s reported results.

+## Round-1 findings

+1. **F1 — NOT ADDRESSED.** The original stale-snapshot join is fixed, but the new producer attribution can mislabel an older tenant’s decision; see N1 below. The console correctly strips tenantless reports to `Enforced` and rejects mismatches at [apiinstances.go:387](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/apiinstances.go:387). The additive fields survive the private snapshot copy at [snapshot.go:1076](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/status/snapshot.go:1076); public fleet output excludes them. Platform precedence remains intact. An `Enforced`-only view exposes no update details through the list, CLI reason, or page.

+2. **F2 — ADDRESSED.** The fast-track owner 404 now precedes `allowWrite` at [browserupdates.go:254](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/browserupdates.go:254). `requireFreshAuth` remains last at line 302. The exhausted-budget regression compares the refusal with an unknown host’s response and checks for no install request.

+3. **F3 — ADDRESSED.** [browserupdates.go:99](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/browserupdates.go:99) preserves the explicit override while calculating the inherited note separately. [app_instance.html:48](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/templates/app_instance.html:48) retains both input and `was_major` semantics. Coverage includes clearing an override with enforcement unavailable.

+4. **F4 — ADDRESSED.** The named-unit check precedes kind exclusion at [browserupdates.go:82](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/browserupdates.go:82). The service-kind regression checks the explanation and absence of forms; ordinary services remain excluded.

+5. **F5 — ADDRESSED.** The lead-in enters the draft inventory at [words_surfaces.go:77](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/updatewords/words_surfaces.go:77), is pinned by [words_docs_test.go:19](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/updatewords/words_docs_test.go:19), and maps to “Docs and roll notes.”

+6. **F6 — ADDRESSED.** Both commands use the shared, em-dash-free draft at [instances_updates.go:106](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/instances_updates.go:106) and line 208. Export mapping and help-output regression coverage are present; the separately pinned app-set literal is unchanged.

+## New findings in the fix diff

+### N1. [P1] Current route attribution can disclose a previous tenant’s stored decision

+**Where:** [internal/host/updatesreport.go:66](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/updatesreport.go:66), consumed by [internal/console/apiinstances.go:643](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/apiinstances.go:643).

+**Scenario:** A hostname moves from tenant A to tenant B on the same box. Before root replaces its previous decision, `updatesReport` reads A’s hostname-keyed consumed file, labels the entry with B’s current route account/app, and copies A’s desired build, reason, detail, baking build and completion information into it.

+[Consumed has no account/app identity](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/release/buildstate.go:87); `ReadConsumed` validates only schema and hostname. With claims disabled, [route retirement deletes the registry row](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/provision/claimassembly.go:40), leaving that decision file.

+The console consequently accepts A’s details as B’s. This creates a new disclosure path for rollout-free fleet reports, which the pre-fix join refused. The regression fixtures assume attribution is already correct and do not exercise this transition.

+**Fix:** Bind persisted decisions to their account/app when written, and suppress decision-derived report fields unless that identity matches the route. Treat legacy unattributed decisions conservatively. Add a regression that retains A’s consumed file, replaces the route with B’s, generates the actual report, and checks list/page disclosure.

+## Out of scope

+- Paul’s plan-5 copy approval remains pending. The missing-record-only `TestApprovedCopyIsApplied` failure is expected and does not affect this verdict.

+- Full execution gates and the reported sandbox-blocked host socket tests remain outstanding; this review does not independently validate their results.

\ No newline at end of file

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

new file mode 100644

index 000000000..503cb7ee8

--- /dev/null

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

@@ -0,0 +1,31 @@

+# Astra re-review r3: settings-5-surfaces

+Range 40e7782e..51790645.

+Verdict: ACCEPT

+Reviewed `40e7782e..51790645` against rev 11 and rulings R22–R23 by inspection. No files modified. **Go cannot run here; I did not run tests or builds.** I inspected the tests and implementer’s reported evidence.

+## Round-2 finding

+**N1 — ADDRESSED within the ruled scope.**

+- **Persisted attribution:** [buildstate.go:113](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/release/buildstate.go:113) adds optional Account/App fields without changing schema validation or legacy decoding. `DecidedFor` requires both identities to be non-empty and match the route.

+- **Every production write:** [updatetarget.go:274](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/release/updatetarget.go:274) stamps identity after copying any previous record. Both production publication paths—`adopt.go:2197` and `adopt.go:2229`—use this constructor; the fence preserves those fields.

+- **Updates report:** [updatesreport.go:61](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/updatesreport.go:61) rejects legacy or mismatched attribution. Its early exit suppresses Desired, Reason, Detail, Baking, Completed, CompletedFailed and Pending, including the build-state watermark fallback. Major remains sourced solely from the current route’s settings, as R22 permits.

+- **Control socket:** [updatesreport.go:130](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/updatesreport.go:130) applies the same identity check before adding Desired/Reason, satisfying R23.

+- **Regression coverage:** [updatesreport_test.go:211](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/updatesreport_test.go:211) exercises reassignment, legacy attribution, matching attribution and partial/mismatched identities. [adoptsettings_test.go:951](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/release/adoptsettings_test.go:951) exercises both root write paths.

+## New findings in the fix diff

+None at [P1], [P2] or [P3].

+Root’s install decisions and request bookkeeping are unchanged. No platform-specific dependencies or build constraints were introduced; `release` already depends on `registry`. Inspection found no cross-platform build regression. The implementer reports successful Darwin, Windows and Linux-arm64 builds; I did not independently execute them.

+## Out of scope

+The parked plan-4 notes are **correctly outside this fix’s scope**:

+- Root and the held-wake logic still trust previous-tenant completion/resolution data. `NextConsumed` can retain that bookkeeping when stamping the new identity. This remains a substantive follow-up, explicitly excluded by R22; acceptance does not certify its correctness.

+- In-place registry Upsert preserves empty settings/build-state columns at [registry.go:1162](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/registry.go:1162). That behavior predates this diff.

+- Copy approval and remaining execution gates, including sandbox-blocked socket tests, remain separate from this inspection verdict.

\ No newline at end of file

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

new file mode 100644

index 000000000..163bc43a2

--- /dev/null

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

@@ -0,0 +1,101 @@

+# Astra review: settings-5-surfaces (round 1)

+Range 8916558d..561c7dfb. Fixed in 40e7782e (wave 1); see -R2 and -R3.

+Verdict: REJECT

+Reviewed `git diff 8916558d..561c7dfb` and the seven commits in `git log 8916558d..561c7dfb`, against the spec, plan, index, and ledger. No files changed. Go cannot run in this sandbox; tests were reviewed by inspection. The missing plan-5 copy record is expected and is not a finding.

+The core extraction preserves the inspected JSON responses, validation order, CAS handling, and single audit/serial bump per successful write. The CLI sends only supplied settings and applies them before the channel move. The approved zero-downtime strings in changed files remain present byte-for-byte.

+## Findings

+### 1. [P1] Tenantless platform reports can cross tenants after hostname reassignment

+**Where:** [internal/console/apiinstances.go:391](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/apiinstances.go:391).

+**Scenario:** A platform box with `CARLOS_SWAP` off reports tenant A’s update state for a hostname, without a rollout report. The hostname is subsequently retired and assigned to tenant B while that snapshot remains fresh. B’s record now passes the list’s ownership check, but `update()` accepts A’s report because `platform` is true. B receives A’s desired build, reason, detail, and request information. The page can display that report too.

+The freshness allowance is fifteen minutes. Current hostname ownership does not establish ownership when a snapshot was generated. The documented duplicate-record race with host claims disabled provides another way to reach this ambiguity.

+**Why it matters:** Plan 5 Review Focus 3 requires another tenant’s report never to appear. R9’s platform fallback does not establish that property; hostname uniqueness at one moment is insufficient.

+**Fix:** Require positive account/app attribution for per-host update details, independent of whether the rollout scheduler runs. Add that attribution to the report additively, or suppress tenantless details until it exists. Test hostname reassignment with a still-fresh, rollout-free platform snapshot through both list and page.

+### 2. [P2] Rate limiting bypasses the fast-track owner’s byte-identical 404

+**Where:** [internal/console/browserupdates.go:239](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/browserupdates.go:239), [internal/console/browserupdates.go:283](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/browserupdates.go:283).

+**Scenario:** An ordinary member exhausts the shared write budget, then posts `fast_track=1` for an existing instance. `updatesFormGate` returns a rate-limit flash and 303 before the owner check runs. The same request for a nonexistent record returns the plain 404.

+**Why it matters:** The plan’s form security constraint requires unauthorized fast track to receive the byte-identical owner 404. The current result depends on budget state and record existence. Fresh authentication remains correctly behind the owner check, but that alone does not preserve the required refusal.

+**Fix:** Check fast-track ownership after membership/record resolution and before rate limiting; keep fresh authentication last. Add an exhausted-budget case to the non-owner test.

+### 3. [P2] An inherited major hold disappears from the editable settings display

+**Where:** [internal/console/browserupdates.go:90](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/browserupdates.go:90), [internal/console/templates/app_instance.html:48](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/templates/app_instance.html:48).

+**Scenario:** The app defaults to major 2 and grants `major` in `instance_control`. A stable instance has no explicit major. `MajorInput` is empty, while `MajorShown` correctly contains 2. The editable branch renders only `MajorInput`, so “Stay on major version” appears empty despite an active hold. Clearing an explicit override back to the app default produces the same misleading display.

+**Why it matters:** Spec §3.2 makes the inherited hold effective, and plan 5’s goal is that members can see their settings. A box status sentence is not a substitute: it can be absent or describe another reason.

+**Fix:** Show the effective inherited major and its source alongside the override control. Preserve the explicit-value baseline and empty-to-inherit semantics. Test inherited defaults and clearing an override while the fleet gate is closed.

+### 4. [P3] Named-unit service records lose the required explanation

+**Where:** [internal/console/browserupdates.go:73](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/browserupdates.go:73).

+**Scenario:** A valid record has `Kind: "service"` and `Unit: "console.service"`. The kind check returns before the named-unit check, so the page has no Updates section. The shared API core instead returns the named-unit explanation.

+**Why it matters:** Spec §4.5 and Review Focus 4 require a named-unit route’s page to explain that settings do not apply, without a form. The current test covers only instance-kind named units.

+**Fix:** Handle a successfully read named-unit record before excluding other kinds. Add a service-kind named-unit case; retain the absence of a section for ordinary services.

+### 5. [P3] The new documentation lead-in bypasses the copy inventory

+**Where:** [docs/site/cli.md:202](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/docs/site/cli.md:202).

+**Scenario:** Paul reviews every exported plan-5 draft. The new sentence introducing the app policy example is never included because it exists only in the documentation. `TestDocsContainDrafts` checks the following paragraph, not this sentence.

+**Why it matters:** The plan’s copy constraint requires every new member-facing sentence to enter the review inventory. This is separate from the explicitly permitted missing approval record.

+**Fix:** Add the lead-in to `updatewords` and the export/pin coverage, or remove it.

+### 6. [P3] Both new commands expose an em dash in help

+**Where:** [instances_updates.go:106](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/instances_updates.go:106), [instances_updates.go:208](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/instances_updates.go:208).

+**Scenario:** `carlos instances set -h` or `carlos instances update -h` prints the copied `--account-id` description containing an em dash.

+**Why it matters:** These new surfaces violate the explicit no-em-dash constraint. Reusing existing wording explains its origin but leaves that constraint unsatisfied.

+**Fix:** Put a compliant shared description through the copy review and use it in both commands. Preserve the separately pinned zero-downtime literals.

+## Deferred minors triage

+- **Must fix before merge:** Task 5’s kind-before-unit entry, finding 4; Task 6’s unreviewed lead-in, finding 5.

+- **Resolve during the pending copy pass:** Task 2’s “default updates none” output, which means automatic updates. Also correct the drafts claiming that `default-channel` applies to existing instances and that the refusal lists blocking boxes: the implementation applies that channel only at creation and exposes a count, respectively.

+- **May remain deferred:** Exporter duplication/complexity; report line counts; duplicate guards and error prefixes; old-console and missing-record guidance; host trimming; comment/test organization; manifest-error coverage; reason-code drift coverage; unused completion data; unknown-reason punctuation; duplicate step-up budget consumption; extra reads; section placement; and last-writer-wins behavior for a control the member actually changed.

+The cross-tenant and exhausted-budget cases need regression coverage beyond the deferred ledger entries.

+## Rulings

+- **R1: Agree.** A separate plan-5 record preserves existing approvals.

+- **R2: Agree.** The authorized draft workflow is appropriate; approval remains required before shipping.

+- **R3: Agree.** Preserve the pinned app-set literals and append new usage separately.

+- **R4: Agree.** Concatenation preserves the approved retry fragments.

+- **R5: Agree.** `resolveMode` supplies the established mode check and conflict handling.

+- **R6: Agree.** Sharing the channel helper preserves proposal handling without resolving the session twice.

+- **R7: Agree.** Explicit values distinguish clearing an override from an effective no-op.

+- **R8: Agree.** The response describes fleet-wide enforcement, not one box.

+- **R9: Disagree with the tenantless platform exception.** Platform precedence and refusing tenantless fleet reports are sound; current record ownership cannot attribute a historical platform report. See finding 1.

+- **R10: Agree.** Kept settings remain read-only on the page; member API/CLI authority is unchanged.

+- **R11: Agree.** Default behavior should not produce an enforcement warning.

+- **R12: Agree.** Reuse the approved named-unit explanation, including service-kind records.

+- **R13: Agree.** A CLI hint is appropriate while no browser channel control exists.

+- **R14: Agree.** Installing the approval pin early makes the pending review explicit.

+- **R15: Agree.** Render-time baselines protect untouched controls from stale-form overwrites.

+- **R16: Agree.** The browser rate-limit message belongs in the review inventory.

+- **R17: Agree.** Existing approval and documentation pins make another test file unnecessary; the final execution gate remains outstanding.

\ No newline at end of file