# Astra review r4: settings-5-surfaces task 8 (tenant change resolves fresh)

Range 5c57c95d..1b603228. Fixed in 3e24809f.

Verdict: REJECT

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

## Findings

### 1. [P2] Ignoring a legacy decision can allow a failed build to wake

**Where:** [internal/host/buildstate.go:672](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/buildstate.go:672).

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

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.

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.

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

### 2. [P2] Equivalent account identities lose same-tenant settings

**Where:** [internal/registry/registry.go:1169](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/registry.go:1169).

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

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

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

### 3. [P2] The new pre-exec tenant check fails open on a registry read error

**Where:** [internal/host/buildstate.go:527](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/buildstate.go:527).

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

The discarded error predates this commit, but the new isolation check depends on that read succeeding.

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

## Rulings

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

## Deferred minors triage

- **Raw SQL account comparison:** promote to must-fix, finding 2. The consequence can be an unwanted installation, not merely temporarily missing columns.
- **Discarded `beforeExec` registry error:** promote to must-fix, finding 3. An unread tenant must not authorize merging hostname-keyed bookkeeping.

The remaining inspected changes are sound:

- For distinct canonical tenants, both SQL columns take incoming values, including empty values. Exact same-tenant preservation and the claim-ownership guard remain intact.
- Root filters foreign data before both `TargetInput` constructions; `NextConsumed` independently prevents copying foreign bookkeeping.
- The default-settings fresh write uses `publishConsumed` and its existing lock and route-hold recheck. It introduces no unfenced write; publication failure stops installation.
- 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.

## Out of scope

- Existing preservation of `good_version`, `good_digest`, `good_demand`, `serving_version`, and other unrelated columns across tenant changes.
- Cleanup of retired hosts’ consumed files.
- Copy approval and remaining branch-wide execution gates. Reported test/build results were not independently executed and do not alter this inspection verdict.