| 1 | # Astra review r5: settings-5-surfaces task 8 (tenant change resolves fresh) |
| 2 | |
| 3 | Range 1b603228..3e24809f. N1 fixed in 8837f20a. |
| 4 | |
| 5 | Verdict: REJECT |
| 6 | |
| 7 | 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. |
| 8 | |
| 9 | ## r4 findings |
| 10 | |
| 11 | 1. **ADRESSED — legacy failed decision can allow a wake.** |
| 12 | [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. |
| 13 | |
| 14 | 2. **NOT ADDRESSED — effective-account equivalence remains incomplete.** |
| 15 | [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. |
| 16 | |
| 17 | 3. **ADDRESSED — pre-exec registry errors fail open.** |
| 18 | [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. |
| 19 | |
| 20 | ## Rulings |
| 21 | |
| 22 | - **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. |
| 23 | - **R29: Agree with the requirements.** The pre-exec implementation satisfies them. The SQL implementation does not yet normalize every invalid account consistently with `RouteAccount`. |
| 24 | |
| 25 | ## New findings in the fix diff |
| 26 | |
| 27 | ### N1. [P2] SQL accepts NUL-containing accounts that Go normalizes to `bab` |
| 28 | |
| 29 | **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. |
| 30 | |
| 31 | **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. |
| 32 | |
| 33 | 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). |
| 34 | |
| 35 | 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. |
| 36 | |
| 37 | **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. |
| 38 | |
| 39 | ## Out of scope |
| 40 | |
| 41 | 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. |
amadan