carlos / platform Public

Download this file

1	# Review brief: the implementation plans for `carlos db` region forward (rounds 1 and 2 answered)
2
3	You are an independent second reader of three implementation plans for the
4	CARLOS platform (this repository). Round 1
5	(`CODEX-FINDINGS-DB-REGION-FORWARD-PLANS.md`) found 24 findings (8 P0, 4 P1,
6	12 P2) and asked for all three plans to be revised. This round checks the
7	revisions. Nothing is built.
8
9	**Plans**, at the commit named in the prompt:
10
11	- `docs/superpowers/plans/2026-10-22-db-region-forward-plan-1-box.md`: the
12	box side and the wire format, shipped dark behind the `db-read-1`
13	capability (17 tasks);
14	- `docs/superpowers/plans/2026-10-22-db-region-forward-plan-2-query.md`: the
15	console and the CLI for `query` and `query --at` (14 tasks);
16	- `docs/superpowers/plans/2026-10-22-db-region-forward-plan-3-pull.md`:
17	`pull` and `pull --at` (10 tasks).
18
19	Each plan now opens with a "Plan review, round 1" section saying where each
20	of its findings landed.
21
22	**Spec:** `docs/superpowers/specs/2026-10-22-db-region-forward-design.md`,
23	rev 6. Rev 6 is rev 5 (accepted in round 5, approved by Paul on 2026-10-22,
24	rulings in §16) plus four clarifications the plan review forced, each marked
25	**(plan review N)**: §5.4 (what a result names when a chunk cannot be
26	confirmed), §5.5 steps 3 and 4 and §5.8 (the audit row first, `cancel` only
27	once a request has ended, and a box's `cancel` read before it elects), §5.7
28	(a retry's reservation), §9.3 (the audit row's order). No mechanism changed.
29	Read `AGENTS.md`, then the spec's status header and the four marked places,
30	then the plans.
31
32	## Fixed: do not re-argue
33
34	- **The spec's design**, rev 6 included. Report a plan that fails to build
35	what the spec says, or builds something the spec rules out.
36	- **Paul's rulings (spec §16).** Plans 4 and 5 are listed, not written.
37	- **The repository's rules** each plan carries as Global Constraints.
38	- **Round 1's findings as disposed below**, unless a fix is wrong or
39	incomplete: then say so as a new finding, naming the round 1 number.
40
41	## Round 1 dispositions
42
43	| # | Sev | Plan, task | Disposition |
44	|---|---|---|---|
45	| 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". |
46	| 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`. |
47	| 3 | P0 | 1: 15 | Fixed. The payload prints one `DBREAD-REPORT ` line; the parent scans for it. |
48	| 4 | P0 | 1: 15 | Fixed. `dbreadServiceFile` is `/etc/systemd/system/carlos-dbread@.service`, errors checked, restored in cleanup through `mustSystemctl`. |
49	| 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. |
50	| 6 | P0 | 2: 4 to 6 | Fixed. `putFixtureObject` reads the etag and replaces with it; `putEdgesFile` and `putBoxStatus` use it. |
51	| 7 | P0 | 2: 6 | Fixed. `TestDBReadAdmissionBounds` moved to Task 7, where the stream holds requests open. |
52	| 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). |
53	| 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. |
54	| 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. |
55	| 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). |
56	| 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`. |
57	| 13 | P2 | 2: 6 | Fixed. Read deadline `preHeader` until admission, then the read's bound. Test `TestDBReadStalledBodyIsCutAtThePreHeaderBound`. |
58	| 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. |
59	| 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. |
60	| 16 | P2 | 1: 15 | Fixed. T12 submits the second read once the first's unit is running. |
61	| 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. |
62	| 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. |
63	| 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. |
64	| 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. |
65	| 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. |
66	| 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. |
67	| 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. |
68	| 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). |
69
70	**Fixed:** 23 in full, 1 in part (11). **Declined:** the part of 11 below.
71
72	## Round 2 dispositions
73
74	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.
75
76	| # | Sev | Plan, task | Disposition |
77	|---|---|---|---|
78	| 25 | P0 | 2: 7 | Fixed. The relay type is `dbReadRelay`. |
79	| 26 | P0 | 3: 5 | Fixed. The replacement keeps plan 1's two timing lines (`held through` for a pull); `TestPullShipsThePlannedFiles` asserts them. |
80	| 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. |
81	| 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. |
82	| 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. |
83	| 30 | P2 | 2: 6, 7 | Fixed. Accelerated windows allow for `issued_at`'s truncation. |
84	| 31 | P2 | 2: 7, 14 | Fixed. `TestDBReadResultWaitsForAChunkTheListingMissed`. |
85	| 32 | P2 | 3: 9, 10 | Fixed. Real files, a valid manifest, a wrong digest. |
86	| 33 | P2 | 2: 6, 14 | Fixed. "ended during the serial read", with a store that refuses a cancelled write. |
87	| 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. |
88	| 35 | P2 | 2: 7, 14 | Fixed. `TestDBReadResultAfterTheDeadlineIsNotRelayed`; the "no witness" note is gone. |
89	| 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. |
90	| 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. |
91	| 38 | P2 | 2: 10, 14 | Fixed. `TestDBReadSetsBothReadDeadlines` watches the connection's read deadlines. |
92
93	## Declined plan findings
94
95	- **11, in part: a cancellation-aware audit insert.** `RecordActivityContext`
96	takes a context, but its claim plumbing ends in `claimRawRecordActivity`'s
97	context-free `s.db.Exec` (`internal/console/store.go:2588`, called from
98	`:2668`), shared by every audited route. Changing it is a change to the
99	claim SQL layer and its census, for one local SQLite insert. The order
100	makes it unnecessary: the row is written while the request is open
101	(checked just before) and before any object a box can see, so a request
102	that ends during the insert leaves the row of an admission and no read.
103	The one bucket write that announced reads after the request ended, the
104	serial bump, is now request-bound.
105
106	## What to check
107
108	Report **new P0, P1 and P2 findings only**; leave P3s out of this round.
109
110	1. **Each fix as built.** Does it do what the disposition says, against the
111	code on `main` (cite `file:line`), and does its test fail without it, by
112	the mutation the plan names? Where a plan says a check has no witness of
113	its own (plan 2 Task 14's note on the `ownCtx` check before the result
114	line), is that true and acceptable?
115	2. **What the fixes broke.** Each fix moved code or tests: the stream's
116	rewrite (`dbfCall`, phase contexts, `stop`), the audit-first admission,
117	the moved tests, the gate store in the MinIO test, the retry's
118	reservation and `settle`, `SelfView`, the frame tap, the stand-in console
119	in plan 3. Do names, signatures and imports still line up across tasks
120	and plans, and does every task still build on the ones before it?
121	3. **Rev 6 against the plans.** Do the four marked clarifications match
122	what the plans build, and does any of them contradict J1 to J5 or
123	another part of the spec?
124	4. **Anything round 1 missed** that is P0 to P2 under the round 1
125	definitions below.
126
127	## Output
128
129	A numbered list of findings, numbered from 25. Each one gives:
130
131	- **severity:** P0 the plan cannot be executed as written (a task that
132	cannot build or whose gate cannot pass); P1 the built code would break a
133	spec invariant or ruling, cross a tenant, or expose box credentials; P2
134	wrong behaviour, a missing spec item, or a test that proves nothing;
135	- the plan and task, and the round 1 number if it reopens one;
136	- the evidence, with `file:line` in the code or `plan:Task N` in the plans;
137	- a concrete fix to the plan.
138
139	Then a verdict per plan: accept, or revise. Do not rewrite the plans. No
140	praise.
141