amadan
db-region-forward: Astra's plan review, round 1 findings, unedited
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+101
−0
docs/superpowers/specs/CODEX-FINDINGS-DB-REGION-FORWARD-PLANS.md+101 −0
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-DB-REGION-FORWARD-PLANS.md b/docs/superpowers/specs/CODEX-FINDINGS-DB-REGION-FORWARD-PLANS.md| new file mode 100644 |
| index 000000000..ca887d372 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-DB-REGION-FORWARD-PLANS.md |
| @@ -0,0 +1,101 @@ |
| +Reviewed `c267f5f5` against the accepted spec and existing code. This was a static review; no files changed or proposed implementation tests ran. |
| + |
| +1. **P0 — Plan 1, Tasks 7, 12 and 13: the installed worker cannot pass readiness.** |
| + [`SelfView` calls `net.Interfaces()`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:2695), but the planned service permits only `AF_UNIX` at line 5527. Linux Go enumerates interfaces through [`NetlinkRIB`](/usr/local/go/src/net/interface_linux.go:17), which opens an [`AF_NETLINK` socket](/usr/local/go/src/syscall/netlink_linux.go:61). Enumeration therefore fails, and the probe rejects the empty interface list at plan line 5248. Fixtures override `Self`, hiding this failure. |
| + **Fix:** obtain network-namespace evidence without opening a prohibited socket, propagate observation errors, and test the default implementation inside the installed unit. |
| + |
| +2. **P0 — Plan 1, Task 15; Plan 2, Task 7: new test helpers collide with existing declarations.** |
| + Plan 1 defines [`waitFor`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:6595), already declared with a different signature in [`housekeeper_test.go:139`](internal/host/housekeeper_test.go:139). Plan 2 defines [`hookStore`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:1552), already declared in [`temphook_test.go:16`](internal/console/temphook_test.go:16). Both affected test packages fail to compile. |
| + **Fix:** give the new helpers feature-specific names and update every call and receiver. |
| + |
| +3. **P0 — Plan 1, Task 15: successful payload reports are parsed as invalid JSON.** |
| + The child runs `m.Run()` at line 6278, then [`payload` decodes its last stdout line](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:6478). Successful `m.Run()` prints [`PASS` after the test finishes](/usr/local/go/src/testing/testing.go:2484), including with verbosity disabled. |
| + **Fix:** use a dedicated report pipe or a distinguished JSON record instead of the final test-runner output line. |
| + |
| +4. **P0 — Plan 1, Task 15: readiness tests remove the wrong service file.** |
| + The test manipulates [`/run/systemd/system/carlos-dbread@.service`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:6662). Its installer calls `ensureSystemUnits("/etc", ...)`, whose real implementation writes beneath [`<etcDir>/systemd/system`](internal/host/systemunits.go:67). Removing the `/run` file leaves the installed service available, so the missing-service assertion fails. |
| + **Fix:** manipulate the actual installed file, check every operation, and restore it through cleanup. Exercise masking at the effective installed path too. |
| + |
| +5. **P0 — Plan 1, Task 17: the census fingerprint bootstrap cannot execute.** |
| + [Line 6937](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:6937) instructs loading an unfinished overlay into a fresh inventory to compute fingerprints. The loader requires valid fingerprints before accepting declarations ([lines 413–421](internal/provision/plan14_root_reads_test.go:413)); `@remove` also requires an existing base entry ([line 470](internal/provision/plan14_root_reads_test.go:470)). |
| + **Fix:** calculate fingerprints directly from the seven-field declaration rows, then validate the completed overlay over main’s layered inventory. Update the procedure inherited by plans 2 and 3. |
| + |
| +6. **P0 — Plan 2, Tasks 4–6: evidence fixtures attempt create-only updates.** |
| + [`putEdgesFile`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:576) always passes an empty `ifMatch`. The fixture seeds `uk-1` at line 753; tests at lines 786 and 1010 then write that key again. [`DirStore.Put` explicitly rejects this](internal/release/store.go:421). |
| + **Fix:** make the edges-fixture helper conditionally replace an existing object using its etag. Keep protocol writes create-only. |
| + |
| +7. **P0 — Plan 2, Task 6: the admission gate depends on Task 7’s implementation.** |
| + [`TestDBReadAdmissionBounds`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:1059) opens two requests and expects a third to receive 429. Task 6’s stream stub at line 1298 immediately returns, releasing both earlier requests’ counts. The third receives 200. |
| + **Fix:** move this test and its passing gate into Task 7, or provide an interim stream that remains open until cancellation. |
| + |
| +8. **P0 — Plan 3, Task 4: its installed-unit pull test precedes the agent support it requires.** |
| + The [acceptance test at line 466](docs/superpowers/plans/2026-10-22-db-region-forward-plan-3-pull.md:466) opens output as `StreamLTX`. At this task boundary, the agent still uses Plan 1’s `StreamRows` writer. Task 5 introduces the stream selection at lines 653–660. |
| + **Fix:** move the installed-agent pull acceptance test and run after Task 5, while retaining the isolated worker tests in Task 4. |
| + |
| +9. **P1 — Plan 1, Task 11: a result can advertise an unconfirmed chunk.** |
| + [`flushLocked`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:4549) increments `seq` before confirming the create. On failure, `attempt` returns an interrupted result carrying that sequence at line 4779; `serve` still publishes it at line 4620. This contradicts [spec §5.4](docs/superpowers/specs/2026-10-22-db-region-forward-design.md:259): every chunk must be confirmed before the result is written. |
| + **Fix:** distinguish an incomplete output protocol from a publishable result. Retain ambiguous chunk bytes for confirmation and publish no result referencing an unconfirmed sequence. Add a fault test where chunk confirmation fails but result creation would succeed. |
| + |
| +10. **P1 — Plan 1, Tasks 9–11: retry reuses a reservation while the previous worker may still hold scratch.** |
| + The [retry loop](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:4664) immediately starts another attempt after a vanished file. Closing the previous connection does not establish unit termination. The next `grow` overwrites the reservation under the same request ID at line 3229, and `dial` does not reconcile outstanding units. This breaks J5 and can consume the probe’s reserved connection. |
| + **Fix:** retain each attempt’s reservation until its unit stops, or await confirmed retirement before retrying. Test with deliberately delayed teardown and space sufficient for only one attempt. |
| + |
| +11. **P1 — Plan 2, Task 6: admission can write after request cancellation.** |
| + The handler calls [`s.bumpSerial(ctx)` followed by `auditActivity`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:1286). Existing `bumpSerial` deliberately uses [`context.WithoutCancel`](internal/console/serialbump.go:35); the audit path uses a context-free [`Exec`](internal/console/store.go:2588). A disconnect can therefore be followed by serial and audit writes, although J1 permits only the best-effort cancel afterwards. |
| + **Fix:** provide request-bound announcement and audit operations for this route, including cancellation-aware underlying writes. Test cancellation between the queue create and these operations. |
| + |
| +12. **P1 — Plan 2, Task 7: the request bounds do not constrain blocking work.** |
| + [`streamDBRead`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:1800) uses bare `r.Context()` for storage calls. Pickup expiry is checked after GET; owner expiry is checked after result GET, LIST and the entire chunk relay. A result can even be sent before checking an already-expired deadline at lines 1861–1865. Blocked storage also prevents the required progress lines. |
| + **Fix:** use deadline-bound contexts for both phases, bound storage operations, check deadlines during relay, and clamp write deadlines to the request bound. Add stalled-store and slow-relay tests. |
| + |
| +13. **P2 — Plan 2, Task 6: the 20-second pre-header timeout does not bound request-body reading.** |
| + The handler extends the socket deadline at [line 1130](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:1130), then creates a separate timeout context. `readJSONLimit` still blocks directly on [`r.Body`](internal/console/authcli.go:1149). A slowly delivered body can exceed both the specified 20 seconds and backhaul’s header timeout. |
| + **Fix:** enforce a short socket/body deadline during admission, propagate its context throughout admission, and extend the read deadline for streaming afterwards. Add a stalled-body test. |
| + |
| +14. **P2 — Plan 1, Tasks 10, 12 and 13: draining boxes can win reads.** |
| + [`consider`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:3994) checks the envelope and admission but never draining state; host wiring supplies no such state. Console filtering cannot exclude a draining contender when another eligible box exists or evidence is incomplete. This contradicts [spec §7.1](docs/superpowers/specs/2026-10-22-db-region-forward-design.md:613). Existing edges records already carry [`Draining`](internal/host/edgesfile.go:319). |
| + **Fix:** supply authoritative draining state to the loop and check it before election, including after jitter. Test a draining contender beside a healthy one. |
| + |
| +15. **P2 — Plan 1, Task 11: some worker-start failures do not trigger the required probe.** |
| + [`askProbe`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:4723) runs for dial and header-write failures, but pre-ready file-send/read failures merely return `interrupted` at lines 4741–4748. Socket activation can accept the connection before service startup fails, leaving capability advertised until the periodic probe. |
| + **Fix:** detect successful worker startup explicitly, or request a probe after every pre-ready transport failure once its connection closes. Test this through a read against a failing installed service. |
| + |
| +16. **P2 — Plan 1, Task 15: the disk-admission test assumes submission order is queue order.** |
| + [Lines 6620–6625](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:6620) enqueue both reads before starting the agent, then assume the second remains unpicked. Same-second queue entries sort by random ID; the real store [sorts keys lexicographically](internal/release/store.go:575). The test can fail with correct admission behavior. |
| + **Fix:** observe the first worker running before submitting the second, or identify the losing request from the actual election. |
| + |
| +17. **P2 — Plan 1, Tasks 15 and 17: T3 does not prove the `PrivateNetwork` mutation.** |
| + The [installed-unit witness](docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md:6235) attempts TCP connections. Removing `PrivateNetwork` leaves `RestrictAddressFamilies=AF_UNIX`, so those attempts still fail. A textual unit-file assertion is not the required runtime witness. |
| + **Fix:** assert actual network-namespace evidence from inside the unit using a method compatible with its restrictions. Also use a valid SQLite fixture for the ATTACH witness; querying a plain-text “registry” file fails even when readable. |
| + |
| +18. **P2 — Plan 2, Tasks 6–7: T15 does not cover both admission counts or restart.** |
| + The [disconnect test](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:1466) closes one request before pickup and one after, then succeeds when one replacement fits. It does not prove both slots were released. The admission test covers only a person’s third request, and no restart case implements [spec T15](docs/superpowers/specs/2026-10-22-db-region-forward-design.md:963). |
| + **Fix:** separately fill and disconnect both person slots before and after pickup; exercise the seventeenth console request across multiple people; restart the console during open reads and verify no recovery state remains. |
| + |
| +19. **P2 — Plan 2, Task 12: T16 can pass without late output.** |
| + The [disconnect case](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:3338) waits only for cancel and result objects, without establishing publication after handler return. The timeout case explicitly accepts `nothing-replicated` at line 3382, which need exercise neither timeout nor late chunks. |
| + **Fix:** pause the real agent immediately before publication, force and observe handler termination, then release chunk/result writes. Assert operation ordering for both timeout and disconnect under the strict versioned-MinIO store. |
| + |
| +20. **P2 — Plan 2, Task 14: the result-before-list mutation requirement is weakened.** |
| + The [mutation table](docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md:3679) combines listing first with removing the last-sequence wait, explicitly acknowledging that listing first alone stays green. The [accepted spec](docs/superpowers/specs/2026-10-22-db-region-forward-design.md:975) requires these mutations independently. |
| + **Fix:** add an operation-order witness that rejects LIST before result GET, independently of chunk-completeness checks. |
| + |
| +21. **P2 — Plan 3, Task 1: the required throughput measurement depends on telemetry nobody builds.** |
| + [Line 99](docs/superpowers/plans/2026-10-22-db-region-forward-plan-3-pull.md:99) requires worker-reported restore/query durations and journal timestamps for successful pickup/result publication. Plan 1’s worker messages at lines 2382–2400 contain no phase timings, and its successful `serve` path at lines 4604–4625 emits no such measurements. The prescribed relay rate cannot be calculated from the supplied implementation. |
| + **Fix:** add the necessary phase instrumentation before the measurement, or define a reproducible measurement using observations the implementation actually exposes. |
| + |
| +22. **P2 — Plan 3, Task 7: malformed file sizes can panic the CLI.** |
| + [`openDBPull`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-3-pull.md:1090) checks only the total size, file-list presence and final transaction. A sealed summary containing sizes `-1` and `N+1` can satisfy a total of `N`; line 1132 then slices plaintext with a negative bound. |
| + **Fix:** validate individual sizes, coordinates, duplicate paths and overflow-safe totals before creating files. Add malformed-manifest tests that require DBF-13, no panic and no output copy. |
| + |
| +23. **P2 — Plan 3, Tasks 7 and 9: the cleanup tests do not exercise cleanup.** |
| + [`TestForwardedPullLeavesNothingWhenTheSpoolFails`](docs/superpowers/plans/2026-10-22-db-region-forward-plan-3-pull.md:906) checks only that `readDBForward` returns an error. The Ctrl-C test at line 925 uses an already-cancelled context and a memory sink. Neither invokes `dbPullForward` or checks directory removal. Its comment promises a binary cleanup test in Task 9, but that task supplies none. |
| + **Fix:** run the real pull path, fail after some spool files exist, and interrupt an active binary pull. Assert no `--out` file and removal of the specific spool directory. |
| + |
| +24. **P2 — Plan 3, Task 10: several listed mutations cannot turn their named tests red.** |
| + In the [mutation table](docs/superpowers/plans/2026-10-22-db-region-forward-plan-3-pull.md:1471), removing `NewAgent`’s clamp cannot affect `TestPullCapOnlyLowers`, which calls only `dbReadsSettings`. Removing the file-size sum check still fails later with the same error through incomplete file splitting. Sending `run` before check B need not create a bucket chunk because the agent can refuse before starting relay. |
| + **Fix:** test `NewAgent` directly; assert rejection of invalid manifests before file creation; and observe worker commands to prove no `run` occurs before successful check B. |
| + |
| +**Plan 1: REVISE. Plan 2: REVISE. Plan 3: REVISE.** |
| + |
| +**REVISE** |
| \ No newline at end of file |