carlos / platform Public
Review brief: the implementation plans for carlos db region forward (rounds 1 and 2 answered)
You are an independent second reader of three implementation plans for the CARLOS platform (this repository). Round 1 (CODEX-FINDINGS-DB-REGION-FORWARD-PLANS.md) found 24 findings (8 P0, 4 P1, 12 P2) and asked for all three plans to be revised. This round checks the revisions. Nothing is built.
Plans, at the commit named in the prompt:
docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md: the box side and the wire format, shipped dark behind thedb-read-1capability (17 tasks);docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md: the console and the CLI forqueryandquery --at(14 tasks);docs/superpowers/plans/2026-10-22-db-region-forward-plan-3-pull.md:pullandpull --at(10 tasks).
Each plan now opens with a "Plan review, round 1" section saying where each of its findings landed.
Spec: docs/superpowers/specs/2026-10-22-db-region-forward-design.md, rev 6. Rev 6 is rev 5 (accepted in round 5, approved by Paul on 2026-10-22, rulings in §16) plus four clarifications the plan review forced, each marked (plan review N): §5.4 (what a result names when a chunk cannot be confirmed), §5.5 steps 3 and 4 and §5.8 (the audit row first, cancel only once a request has ended, and a box's cancel read before it elects), §5.7 (a retry's reservation), §9.3 (the audit row's order). No mechanism changed. Read AGENTS.md, then the spec's status header and the four marked places, then the plans.
Fixed: do not re-argue
- The spec's design, rev 6 included. Report a plan that fails to build what the spec says, or builds something the spec rules out.
- Paul's rulings (spec §16). Plans 4 and 5 are listed, not written.
- The repository's rules each plan carries as Global Constraints.
- Round 1's findings as disposed below, unless a fix is wrong or incomplete: then say so as a new finding, naming the round 1 number.
Round 1 dispositions
| # | Sev | Plan, task | Disposition | |---|---|---|---| | 1 | P0 | 1: 7, 12, 13 | Fixed. SelfView reads /proc/self/net/dev (parseNetDev); net.Interfaces needs AF_NETLINK, refused by RestrictAddressFamilies=AF_UNIX. Unobserved facts go in HelloReply.Errors; the probe fails on any. Tests TestParseNetDev, the probe's "an observation that failed". | | 2 | P0 | 1: 15; 2: 7 | Fixed. waitFor (internal/host/housekeeper_test.go:139) became dbreadWaitFor; hookStore (internal/console/temphook_test.go:16) became dbReadHookStore. Checked with git grep on origin/main. | | 3 | P0 | 1: 15 | Fixed. The payload prints one DBREAD-REPORT line; the parent scans for it. | | 4 | P0 | 1: 15 | Fixed. dbreadServiceFile is /etc/systemd/system/carlos-dbread@.service, errors checked, restored in cleanup through mustSystemctl. | | 5 | P0 | 1: 17 (2, 3: census) | Fixed. TestZZDBForwardFingerprints computes each @legacy line from the row's seven fields; plans 2 and 3 use it. | | 6 | P0 | 2: 4 to 6 | Fixed. putFixtureObject reads the etag and replaces with it; putEdgesFile and putBoxStatus use it. | | 7 | P0 | 2: 6 | Fixed. TestDBReadAdmissionBounds moved to Task 7, where the stream holds requests open. | | 8 | P0 | 3: 4 | Fixed. The installed-unit pull test is Task 5 Step 5, after the agent's pull; submit already takes the op (plan 1). | | 9 | P1 | 1: 11 | Fixed. flushLocked advances seq only after the create is confirmed. Test TestResultNamesOnlyConfirmedChunks (a chunk store that never confirms; result failed/interrupted, last_seq 1). Spec §5.4 rev 6. | | 10 | P1 | 1: 9 to 11 | Fixed. admission.retry keeps the first attempt's reservation under id#attempt; settle waits for the unit count before the retry dials. Tests TestRetryWaitsForTheFirstWorkerToStop (a lingering unit, space for one), TestAdmissionRetryHoldsTheOldReservation. Spec §5.7 rev 6. | | 11 | P1 | 2: 6 (and 1: 12) | Fixed in part. The audit row is written first, while the request is open and before any box can see the read; after the queue entry, an ended request writes cancel and returns, with no serial bump; the bump runs on the request's context (announceDBRead, not the detached bumpSerial). Boxes read cancel before electing. Tests TestDBReadEndedDuringAdmissionWritesOnlyTheCancel, TestPassSkipsACancelledRequest. Spec §5.5 and §9.3 rev 6. Declined: the audit insert's own cancellation (below). | | 12 | P1 | 2: 7 | Fixed. Phase deadline contexts; every store call through dbfCall (5 s, wait lines while it blocks); the bound checked before the result line; write deadlines clamped to the bound plus 10 s. Tests TestDBReadStalledStoreKeepsTheStreamAndItsBound, TestDBReadSlowRelayEndsAtTheBound. | | 13 | P2 | 2: 6 | Fixed. Read deadline preHeader until admission, then the read's bound. Test TestDBReadStalledBodyIsCutAtThePreHeaderBound. | | 14 | P2 | 1: 10, 12, 13 | Fixed. AgentConfig.Draining, from the box's own edges record, checked before admission and after the jitter. Test TestADrainingBoxDoesNotPick. Nothing on main sets Draining yet; the plan says so. | | 15 | P2 | 1: 11 | Fixed. Every pre-ready transport failure asks for a probe. Tests TestAWorkerThatNeverStartsAsksForAProbe, T13's fail-start case through the installed unit. | | 16 | P2 | 1: 15 | Fixed. T12 submits the second read once the first's unit is running. | | 17 | P2 | 1: 15, 17 | Fixed. The witness reports the interfaces the unit sees and fails on any but loopback (removing PrivateNetwork turns it red); ATTACH targets a real SQLite file. | | 18 | P2 | 2: 6, 7 | Fixed. TestDBReadAdmissionBounds: both slots freed before pickup and again after, the seventeenth read across nine people, a restart under open reads that writes nothing and starts with empty counts. | | 19 | P2 | 2: 12 | Fixed. dbfGateStore holds the real box at publication until the console's request has ended, by disconnect and by the bound; late writes are asserted after the end; the nothing-replicated branch is gone. | | 20 | P2 | 2: 14 | Fixed. An operation-order check (each list preceded by its round's result) with its own mutation row, apart from the chunk-count check. | | 21 | P2 | 3: 1 | Fixed. Plan 1's agent logs planning-and-restore time and confirmed output; plan 2's console logs its relay; plan 3 Task 1 measures from those lines. | | 22 | P2 | 3: 7 | Fixed. checkPullManifest checks sizes, levels, ranges, duplicates and an overflow-safe sum before any file is created. Tests: four new malformed cases, each DBF-13, no panic, no file. | | 23 | P2 | 3: 7, 9 | Fixed. The spool failure and Ctrl-C run dbPull against a stand-in console with TMPDIR scratch and a spool-write seam; Task 9 sends SIGINT to the binary mid-pull. Each asserts no --out and no carlos-db-* left. | | 24 | P2 | 3: 10 | Fixed. TestNewAgentClampsMaxPull calls NewAgent; the malformed-manifest cases assert no file; the record-changed case asserts the worker never received run (frames recorded at the agent's end). |
Fixed: 23 in full, 1 in part (11). Declined: the part of 11 below.
Round 2 dispositions
Round 2 (CODEX-FINDINGS-DB-REGION-FORWARD-PLANS-R2.md) found 14 more (3 P0, 1 P1, 10 P2). There is no plan round 3: the build starts from the tip that answers them, with per-task reviews and a whole-branch review after. Every new top-level identifier in the three plans was checked against origin/main for a same-package declaration; dbStream was the only collision.
| # | Sev | Plan, task | Disposition | |---|---|---|---| | 25 | P0 | 2: 7 | Fixed. The relay type is dbReadRelay. | | 26 | P0 | 3: 5 | Fixed. The replacement keeps plan 1's two timing lines (held through for a pull); TestPullShipsThePlannedFiles asserts them. | | 27 | P0 | 1: 15 | Fixed. The /etc unit is swapped for a /dev/null symlink, LoadState=masked is asserted, and the symlink is removed before the file is restored. | | 28 | P1 | 2: 6 | Fixed as far as checks can: the request's context is checked immediately before each of the four writes, cancel once anything is published; one test case per step. A write already past its check still lands; the next check cancels, and boxes read cancel before electing and every 5 s while running. Spec §5.5 step 3. | | 29 | P2 | 1: 11, 12 | Fixed. A skipped probe stays owed (probeRetry); the failed connection closes before the probe is asked for. Tested through Run with a lingering unit. | | 30 | P2 | 2: 6, 7 | Fixed. Accelerated windows allow for issued_at's truncation. | | 31 | P2 | 2: 7, 14 | Fixed. TestDBReadResultWaitsForAChunkTheListingMissed. | | 32 | P2 | 3: 9, 10 | Fixed. Real files, a valid manifest, a wrong digest. | | 33 | P2 | 2: 6, 14 | Fixed. "ended during the serial read", with a store that refuses a cancelled write. | | 34 | P2 | 2: 10, 11 | Fixed. The edges run in package edge's test binary as a helper process; the real CLI and console in the root package; progress observed at the CLI's stderr and socket, through the local edge too. | | 35 | P2 | 2: 7, 14 | Fixed. TestDBReadResultAfterTheDeadlineIsNotRelayed; the "no witness" note is gone. | | 36 | P2 | 2: 6, 8 | Fixed. CLI refusal with a new row, DBF-32 (pending copy review), and a console 400 for U+FFFD; console, CLI and binary tests. Spec §5.5 step 3 and §10. | | 37 | P2 | 2: 8; 3: 7 | Fixed. dbfChunkBound: a count cap, a size floor, nothing after an empty chunk, checked before memory or the spool keeps a chunk. | | 38 | P2 | 2: 10, 14 | Fixed. TestDBReadSetsBothReadDeadlines watches the connection's read deadlines. |
Declined plan findings
- 11, in part: a cancellation-aware audit insert.
RecordActivityContexttakes a context, but its claim plumbing ends inclaimRawRecordActivity's context-frees.db.Exec(internal/console/store.go:2588, called from:2668), shared by every audited route. Changing it is a change to the claim SQL layer and its census, for one local SQLite insert. The order makes it unnecessary: the row is written while the request is open (checked just before) and before any object a box can see, so a request that ends during the insert leaves the row of an admission and no read. The one bucket write that announced reads after the request ended, the serial bump, is now request-bound.
What to check
Report new P0, P1 and P2 findings only; leave P3s out of this round.
- Each fix as built. Does it do what the disposition says, against the code on
main(citefile:line), and does its test fail without it, by the mutation the plan names? Where a plan says a check has no witness of its own (plan 2 Task 14's note on theownCtxcheck before the result line), is that true and acceptable? - What the fixes broke. Each fix moved code or tests: the stream's rewrite (
dbfCall, phase contexts,stop), the audit-first admission, the moved tests, the gate store in the MinIO test, the retry's reservation andsettle,SelfView, the frame tap, the stand-in console in plan 3. Do names, signatures and imports still line up across tasks and plans, and does every task still build on the ones before it? - Rev 6 against the plans. Do the four marked clarifications match what the plans build, and does any of them contradict J1 to J5 or another part of the spec?
- Anything round 1 missed that is P0 to P2 under the round 1 definitions below.
Output
A numbered list of findings, numbered from 25. Each one gives:
- severity: P0 the plan cannot be executed as written (a task that cannot build or whose gate cannot pass); P1 the built code would break a spec invariant or ruling, cross a tenant, or expose box credentials; P2 wrong behaviour, a missing spec item, or a test that proves nothing;
- the plan and task, and the round 1 number if it reopens one;
- the evidence, with
file:linein the code orplan:Task Nin the plans; - a concrete fix to the plan.
Then a verdict per plan: accept, or revise. Do not rewrite the plans. No praise.