amadan
settings-5: Astra's task 8 findings (r4 REJECT, r5 REJECT, r6 ACCEPT)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+133
−0
docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R4.md+64 −0docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R5.md+41 −0docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R6.md+28 −0
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R4.md b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R4.md| new file mode 100644 |
| index 000000000..331172548 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R4.md |
| @@ -0,0 +1,64 @@ |
| +# 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. |
| \ No newline at end of file |
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R5.md b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R5.md| new file mode 100644 |
| index 000000000..6c7baf598 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R5.md |
| @@ -0,0 +1,41 @@ |
| +# Astra review r5: settings-5-surfaces task 8 (tenant change resolves fresh) |
| + |
| +Range 1b603228..3e24809f. N1 fixed in 8837f20a. |
| + |
| +Verdict: REJECT |
| + |
| +Reviewed only `1b603228..3e24809f`, by inspection. **Go cannot run here; I ran no Go commands and modified no files.** The implementer’s results and controller’s additional passing tests are reported evidence, not independently reproduced. |
| + |
| +## r4 findings |
| + |
| +1. **ADRESSED — legacy failed decision can allow a wake.** |
| + [internal/host/buildstate.go:681](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/buildstate.go:681) now admits untagged consumed files while excluding tagged foreign files. This restores the failure information needed to refuse an installed failed target. [buildstate_astra_test.go:90](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/buildstate_astra_test.go:90) covers tagged and untagged decisions; [tenantchange_test.go:183](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/tenantchange_test.go:183) exercises the real activator wake without a planner or refusal record, asserting no live-link capture, generation, or durable write. |
| + |
| +2. **NOT ADDRESSED — effective-account equivalence remains incomplete.** |
| + [internal/registry/registry.go:1124](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/registry.go:1124) fixes `""` ↔ `bab` and the tested invalid spellings. However, its SQL validity predicate disagrees with `controlname.ValidAccount` for embedded NUL characters. The same-tenant column-loss defect remains for those inputs; see N1. |
| + |
| +3. **ADDRESSED — pre-exec registry errors fail open.** |
| + [internal/host/buildstate.go:527](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/buildstate.go:527) returns the registry error before entering `b.write`. [tenantchange_test.go:75](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/host/tenantchange_test.go:75) closes the registry and checks that the durable object’s etag remains unchanged. Successful row absence remains distinct: `(nil, nil)` passes the error check, skips tenant filtering and held revalidation, and retains the previous write behavior. |
| + |
| +## Rulings |
| + |
| +- **R28: Agree**, within the explicitly accepted legacy attribution window. Restoring legacy reads preserves failure and resolution information; the new regression directly covers the failed-installed-target escape. Tagged foreign files remain excluded. |
| +- **R29: Agree with the requirements.** The pre-exec implementation satisfies them. The SQL implementation does not yet normalize every invalid account consistently with `RouteAccount`. |
| + |
| +## New findings in the fix diff |
| + |
| +### N1. [P2] SQL accepts NUL-containing accounts that Go normalizes to `bab` |
| + |
| +**Where:** [internal/registry/registry.go:1125](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/registry.go:1125), used for both settings columns at line 1205. The equivalence cases in [updatesettings_test.go:155](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/updatesettings_test.go:155) omit embedded NULs. |
| + |
| +**Scenario:** An existing row has account `"abc\x00x"`, app `km`, and manual settings. It is upserted as account `"bab"` with empty incoming settings and build-state columns. |
| + |
| +Go’s anchored account regexp rejects `"abc\x00x"`, so both routes have effective account `bab`. SQLite’s `length` stops at the NUL, and its GLOB matcher likewise stops there: the SQL predicate accepts the three-letter prefix and returns the original account value. SQLite equality compares the complete value, so the upsert incorrectly detects a tenant change and clears both columns. This can remove the manual hold before reconciliation restores it. These semantics follow from [SQLite’s NUL documentation](https://sqlite.org/nulinstr.html) and [GLOB implementation](https://sqlite.org/src/doc/trunk/src/func.c). |
| + |
| +The release-side test correctly compares the two Go implementations, but cannot catch this SQL discrepancy. The finite SQL test cases therefore do not establish the claimed equivalence. |
| + |
| +**Fix:** Explicitly reject embedded NULs in the SQL validity predicate, for example with `instr(col, char(0)) = 0`. Add SQL/Go equivalence cases containing NULs and an upsert regression proving `"abc\x00x"` ↔ `"bab"` preserves both columns. |
| + |
| +## Out of scope |
| + |
| +Pre-existing preservation of unrelated route columns, retired consumed-file cleanup, accepted migration behavior outside this diff, copy approval, and branch-wide landing gates. None determines this verdict. |
| \ No newline at end of file |
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R6.md b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R6.md| new file mode 100644 |
| index 000000000..1f70aec3d |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-SETTINGS-IMPL-5-R6.md |
| @@ -0,0 +1,28 @@ |
| +# Astra review r6: settings-5-surfaces task 8 (tenant change resolves fresh) |
| + |
| +Range 3e24809f..8837f20a. |
| + |
| +Verdict: ACCEPT |
| + |
| +Reviewed only `git diff 3e24809f..8837f20a`. **I cannot run Go here; I ran no Go commands and modified no files.** |
| + |
| +## r5 finding |
| + |
| +**N1 ADDRESSED.** [registry.go:1127](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/registry.go:1127) adds `instr(col, char(0)) = 0`. SQLite’s `instr` searches using the value’s byte length, so it detects embedded NULs without stopping at them. [SQLite implementation](https://sqlite.org/src/doc/trunk/src/func.c) |
| + |
| +By inspection, the combined predicate accepts exactly 3–16 lowercase ASCII letters and maps every other account string to `bab`, matching `controlname.ValidAccount` and `release.RouteAccount`. The same expression applies to both stored and incoming accounts. |
| + |
| +The tests pin the reported failure: |
| + |
| +- [updatesettings_test.go:160](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/updatesettings_test.go:160) adds seven SQL/Go equivalence cases covering leading, embedded, trailing and repeated NULs, including one after a 16-letter prefix. |
| +- [updatesettings_test.go:124](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/settings-5-surfaces/internal/registry/updatesettings_test.go:124) adds six upsert transitions, including `"abc\x00x"` ↔ `"bab"`; line 132 asserts that both settings columns survive. |
| + |
| +These are regression cases, not exhaustive enumeration. The general equivalence follows from the predicate. “Fix round 2” reports red-before/green-after results; I have not independently reproduced them. |
| + |
| +## New findings in the fix diff |
| + |
| +None found. |
| + |
| +## Out of scope |
| + |
| +Changes outside `3e24809f..8837f20a`, broader branch correctness, and landing gates. |
| \ No newline at end of file |