carlos / platform Public

Download this file

1	# Astra review: settings-5-surfaces (round 1)
2
3	Range 8916558d..561c7dfb. Fixed in 40e7782e (wave 1); see -R2 and -R3.
4
5	Verdict: REJECT
6
7	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.
8
9	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.
10
11	## Findings
12
13	### 1. [P1] Tenantless platform reports can cross tenants after hostname reassignment
14
15	**Where:** [internal/console/apiinstances.go:391](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/apiinstances.go:391).
16
17	**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.
18
19	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.
20
21	**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.
22
23	**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.
24
25	### 2. [P2] Rate limiting bypasses the fast-track owner’s byte-identical 404
26
27	**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).
28
29	**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.
30
31	**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.
32
33	**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.
34
35	### 3. [P2] An inherited major hold disappears from the editable settings display
36
37	**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).
38
39	**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.
40
41	**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.
42
43	**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.
44
45	### 4. [P3] Named-unit service records lose the required explanation
46
47	**Where:** [internal/console/browserupdates.go:73](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/console/browserupdates.go:73).
48
49	**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.
50
51	**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.
52
53	**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.
54
55	### 5. [P3] The new documentation lead-in bypasses the copy inventory
56
57	**Where:** [docs/site/cli.md:202](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/docs/site/cli.md:202).
58
59	**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.
60
61	**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.
62
63	**Fix:** Add the lead-in to `updatewords` and the export/pin coverage, or remove it.
64
65	### 6. [P3] Both new commands expose an em dash in help
66
67	**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).
68
69	**Scenario:** `carlos instances set -h` or `carlos instances update -h` prints the copied `--account-id` description containing an em dash.
70
71	**Why it matters:** These new surfaces violate the explicit no-em-dash constraint. Reusing existing wording explains its origin but leaves that constraint unsatisfied.
72
73	**Fix:** Put a compliant shared description through the copy review and use it in both commands. Preserve the separately pinned zero-downtime literals.
74
75	## Deferred minors triage
76
77	- **Must fix before merge:** Task 5’s kind-before-unit entry, finding 4; Task 6’s unreviewed lead-in, finding 5.
78	- **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.
79	- **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.
80
81	The cross-tenant and exhausted-budget cases need regression coverage beyond the deferred ledger entries.
82
83	## Rulings
84
85	- **R1: Agree.** A separate plan-5 record preserves existing approvals.
86	- **R2: Agree.** The authorized draft workflow is appropriate; approval remains required before shipping.
87	- **R3: Agree.** Preserve the pinned app-set literals and append new usage separately.
88	- **R4: Agree.** Concatenation preserves the approved retry fragments.
89	- **R5: Agree.** `resolveMode` supplies the established mode check and conflict handling.
90	- **R6: Agree.** Sharing the channel helper preserves proposal handling without resolving the session twice.
91	- **R7: Agree.** Explicit values distinguish clearing an override from an effective no-op.
92	- **R8: Agree.** The response describes fleet-wide enforcement, not one box.
93	- **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.
94	- **R10: Agree.** Kept settings remain read-only on the page; member API/CLI authority is unchanged.
95	- **R11: Agree.** Default behavior should not produce an enforcement warning.
96	- **R12: Agree.** Reuse the approved named-unit explanation, including service-kind records.
97	- **R13: Agree.** A CLI hint is appropriate while no browser channel control exists.
98	- **R14: Agree.** Installing the approval pin early makes the pending review explicit.
99	- **R15: Agree.** Render-time baselines protect untouched controls from stale-form overwrites.
100	- **R16: Agree.** The browser rate-limit message belongs in the review inventory.
101	- **R17: Agree.** Existing approval and documentation pins make another test file unnecessary; the final execution gate remains outstanding.