carlos / platform Public

Download this file

1	# Astra review r4: settings-5-surfaces task 8 (tenant change resolves fresh)
2
3	Range 5c57c95d..1b603228. Fixed in 3e24809f.
4
5	Verdict: REJECT
6
7	Reviewed only `5c57c95d..1b603228`, against the brief, report, prior reviews, rev 11, and R25–R27. **Go cannot run here; this is inspection only. No files modified.**
8
9	## Findings
10
11	### 1. [P2] Ignoring a legacy decision can allow a failed build to wake
12
13	**Where:** [internal/host/buildstate.go:672](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/buildstate.go:672).
14
15	**Scenario:** A same-tenant manual instance has T installed, a readable build state, and an untagged consumed decision recording `ReasonFailed`, `Detail: T`. There is no separate `RefusalNotUp` record, and the swap planner is disabled.
16
17	The new attribution check removes that failure decision. `DecideHeldWake` now returns T with `AskRoot=true` and `Failed=false`. However, [awaitRootDecision](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/activator/wakecheck.go:193) immediately returns when T is already live; it neither requests nor awaits root. The wake proceeds, and pre-exec revalidation also admits T.
18
19	The implementer’s report records the corresponding failed assertion in `TestAFailedInstalledBuildWithNoBuildStateIsHeldFailed`. Automatically tagging fixtures made that test pass, but removed its coverage of the legacy case.
20
21	**Fix:** Make an unattributed held-wake decision actually wait for root’s attributed decision, including when the target is already installed. Add an explicitly untagged failed-decision regression through the wake path with no planner. Do not rely on `AskRoot` alone.
22
23	### 2. [P2] Equivalent account identities lose same-tenant settings
24
25	**Where:** [internal/registry/registry.go:1169](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/registry.go:1169).
26
27	**Scenario:** A legacy row has account `""`, app `km`, and manual or major settings. Re-adding that same app stamps account `bab` and supplies empty settings/build-state columns.
28
29	`RouteAccount` treats both account spellings as `bab`; the consumed file remains same-tenant. The new SQL nevertheless identifies a tenant change and clears both columns. Root can adopt the channel head before reconciliation restores the hold. Restoring settings afterward cannot undo that installation.
30
31	**Fix:** Compare effective account identities consistently with `RouteAccount`, preserving the columns for equivalent legacy identities. Test `""` ↔ `bab` explicitly, alongside genuine account-only changes with empty incoming settings.
32
33	### 3. [P2] The new pre-exec tenant check fails open on a registry read error
34
35	**Where:** [internal/host/buildstate.go:527](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/buildstate.go:527).
36
37	**Scenario:** B owns the host, A’s consumed file remains, and `reg.Get` fails. Its error is discarded, leaving `rt=nil`; the new foreign-file check is skipped. `b.write` can then merge A’s resolution/completion into B’s durable build state. That object write happens before the registry-copy update, so even a subsequent registry error does not prevent contamination.
38
39	The discarded error predates this commit, but the new isolation check depends on that read succeeding.
40
41	**Fix:** Return the registry error before any build-state write. Add an injected read-error regression asserting that the durable object remains unchanged. Keep successful “row absent” handling separate from read failure.
42
43	## Rulings
44
45	- **R26: Partly agree; disagree with the held-wake safety claim.** Preserving untagged bookkeeping in root, pre-exec, and carry avoids losing file-only resolutions and failed completions. Rejecting tagged foreign files is correct. But treating an untagged decision as absent does **not** always hold execution until root responds; finding 1 demonstrates the exception.
46	- **R27: Agree with the explicitly accepted migration cost.** A genuine effective-account change invalidates the old attribution, and a file-only completion or resolution can consequently be lost. Keeping the build-state watermark limits that exposure. This does not cover finding 2: `""` → `bab` leaves the effective tenant unchanged.
47
48	## Deferred minors triage
49
50	- **Raw SQL account comparison:** promote to must-fix, finding 2. The consequence can be an unwanted installation, not merely temporarily missing columns.
51	- **Discarded `beforeExec` registry error:** promote to must-fix, finding 3. An unread tenant must not authorize merging hostname-keyed bookkeeping.
52
53	The remaining inspected changes are sound:
54
55	- For distinct canonical tenants, both SQL columns take incoming values, including empty values. Exact same-tenant preservation and the claim-ownership guard remain intact.
56	- Root filters foreign data before both `TargetInput` constructions; `NextConsumed` independently prevents copying foreign bookkeeping.
57	- The default-settings fresh write uses `publishConsumed` and its existing lock and route-hold recheck. It introduces no unfenced write; publication failure stops installation.
58	- The new tests detect the principal foreign-file and column-carry defects. They do not cover the three findings above. The new default-route scenario also lacks a dedicated move-fence interleaving test, although it uses the existing fenced path.
59
60	## Out of scope
61
62	- Existing preservation of `good_version`, `good_digest`, `good_demand`, `serving_version`, and other unrelated columns across tenant changes.
63	- Cleanup of retired hosts’ consumed files.
64	- Copy approval and remaining branch-wide execution gates. Reported test/build results were not independently executed and do not alter this inspection verdict.