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 now admits untagged consumed files while excluding tagged foreign files. This restores the failure information needed to refuse an installed failed target. buildstateastratest.go:90 covers tagged and untagged decisions; tenantchange_test.go:183 exercises the real activator wake without a planner or refusal record, asserting no live-link capture, generation, or durable write.

1. NOT ADDRESSED — effective-account equivalence remains incomplete. 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.

1. ADDRESSED — pre-exec registry errors fail open. internal/host/buildstate.go:527 returns the registry error before entering b.write. 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, used for both settings columns at line 1205. The equivalence cases in 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 and GLOB implementation.

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.