| 1 | Reviewed `56f9ecfa`. All plan line references refer to that commit. |
| 2 | |
| 3 | 1. **P1 — Task 7, Step 3: manual purge omits settlement.** |
| 4 | `docs/superpowers/plans/2026-09-26-temporary-static-sites.md:3327` adds only `lockApp` to `handlePurgeApp`. If trash wrote its marker but failed its row update, manual purge deletes the objects while `PurgeAppRow` silently preserves the live row (`internal/console/ownership.go:472`, `internal/console/store.go:1491`). Subsequent settlement leaves a live row without a marker untouched (plan:1811). This violates the settle-first requirement in spec:65–73. |
| 5 | **Fix:** settle under the mutex, re-read the row, and validate purge prerequisites before destructive teardown. Test manual purge immediately after a failed trash row update. |
| 6 | |
| 7 | 2. **P1 — Task 7, Step 3: destructive handlers retain protection decisions made before acquiring the lock.** |
| 8 | Plan:3312–3331 places locks after the existing control-plane checks (`internal/console/apicli.go:1209`, `internal/console/appsettings.go:991`, `internal/console/ownership.go:335`). Meanwhile, Task 6 gives the control-plane setter the same mutex (plan:2859). A delete request can pass its protection check, wait while the setter marks the app protected, then acquire the lock and trash it anyway. The missing row re-read also allows a delayed trash request to recreate a marker after its row has been purged. |
| 9 | **Fix:** re-check existence and control-plane protection after locking and settling. Add deterministic tests that change these preconditions while a destructive request waits. |
| 10 | |
| 11 | 3. **P2 — Task 8, Step 1: the `ErrNotTrashed` tests bypass the branch they claim to exercise.** |
| 12 | `TestEarlyPurgeNotTrashedBranches` removes the marker before invoking the pass (plan:3502–3525). The settle sweep repairs or removes the row first (plan:3722), so `purgeOne` returns at plan:3816 without reaching the recovery branch at plan:3844. Deleting that branch would leave these tests passing. |
| 13 | **Fix:** use a store hook to remove the marker after initial settlement but before the purge precondition reads it. Assert that `ErrNotTrashed` occurs and that both recovery outcomes work. |
| 14 | |
| 15 | 4. **P2 — Task 6, Step 1: the allowed static-instance test accepts unrelated failures.** |
| 16 | At plan:2579, `TestTemporaryInstanceCreateIsStaticAtThePlatformHost` discards the HTTP status and checks only that the error lacks the keep hint. A 400 or 500 therefore passes, despite further validation before creation (`internal/console/apiinstances.go:972`). |
| 17 | **Fix:** require the successful status and read back the instance record, checking its account, app, platform hostname and static kind. |
| 18 | |
| 19 | 5. **P2 — Task 11, Step 3: an explicitly empty `--ttl` silently selects ordinary deployment.** |
| 20 | `temporaryDeployFlags` detects `--ttl` through its nonblank value (plan:5020–5029); its caller supplies no flag-presence information (plan:5357). Consequently, `carlos deploy --ttl "" ./dist` follows ordinary app resolution instead of refusing an invalid TTL or creating a temporary app. This contradicts spec:442, where `--ttl` implies temporary creation. |
| 21 | **Fix:** pass `flagWasSet(fs, "ttl")`, validate every explicitly supplied value, and test empty and whitespace-only values through the CLI entry point. |
| 22 | |
| 23 | **Verdict: revise.** |
amadan