# 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.