Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 164 additions & 0 deletions .agents/code-reviews/m17-slice2a-linux-vm-spike.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
# Code review — M17 slice 17.2a (Linux VM spike)

**Reviewed:** commit `66bf73c` on `m17-slice2a-linux-vm-spike`, against
[`.agents/plans/m17-slice2a-linux-vm-spike.md`](../plans/m17-slice2a-linux-vm-spike.md).

**Stats:**

- Files Modified: 3
- Files Added: 2
- Files Deleted: 0
- New lines: 1532
- Deleted lines: 21

## What was reviewable here

_(Superseded at the triage gate — see Resolution below, and the prp-review pass after it: the diff
now carries three comment-only source edits.)_ At review time the diff was **docs-only** — 0 source
files, which is itself an acceptance criterion (D-M17-5: the
slice measures, it does not fix; verified with
`git status --porcelain | grep -E '\.(ts|tsx|mjs|js|json|sql|yml|toml|rs)$'` → 0 matches). The usual
review axes (logic errors, injection, N+1, memory) have no surface. What a measurement slice can get
**wrong** is different, and that is what was checked:

1. **Factual accuracy** — every quoted number re-derived from the run that produced it.
2. **Citation correctness** — every `file:line` opened and confirmed.
3. **Internal consistency** — the same figure across report, incidents, milestone plan and SUMMARY.
_(Gap this pass missed, found by prp-review: I checked `file:line` citations and numeric figures
but **not incident cross-references**, and two `INC-2026-10`/`INC-2026-11` mixups survived. A
grep for `file:line` shapes cannot catch an ID pointing at the wrong entry — check both.)_
4. **The honesty gates** — tier labels, unmeasured-before-measured, no incident-log inflation.
5. **Secret hygiene** — the §3 privacy rule, on a report written from a run that minted live tokens.

### Checks that passed

- **Secrets:** 0 occurrences in committed docs of the run's ingest token, push-receiver token,
pairing code, `APP_DB_PASSWORD` or admin password. The one token line quoted in the report is
`<redacted>`. No hostname/PII (`Living-Room` → 0 hits).
- **Citations, all 10 opened and confirmed:** `file-watcher.ts:136`, `build-sea.mjs:32/:39/:80/:123/:144`,
`cli.ts:631`, `vitest.config.ts:7`, `cursor-store.ts:11-13`, `capture-engine.ts:329-340`. The last
one is *stronger* than the report claims — `outcome` is read only under `if (outcome.changed > 0)`
and `opts.onSuccess?.(opts.connector)` runs unconditionally after the loop, so INC-2026-12's
mechanism is visible in the cited lines rather than merely asserted.
- **Cross-doc links:** all 5 resolve.
- **Markdown structure:** the milestone slice table is 5 columns on every one of its 11 rows
(17.2a/17.2b insertion did not break it).
- **Severity tally:** 5 med + 1 low across INC-2026-08…13, matching the report's headline claim.
- **Known-defect rule:** no new ID duplicates INC-2026-02…05; INC-2026-11 explicitly cites
INC-2026-05 as a *difference*, which is the rule's prescribed shape.
- **`--force` discipline:** the report reports `tsc -b --force` (9.47 s) as the trustworthy number
and flags the 204 ms incremental run as the false-green shape, per CLAUDE.md.

---

## Resolution (triage gate, 2026-08-09)

The maintainer chose **"Fix everything"** — all 4 findings fixed, including finding 2's source edit,
against my recommendation to defer it. Consequence, recorded rather than left to be inferred: **the
diff is no longer docs-only.** `apps/collector/src/connectors/cursor-store.ts` now carries a
comment-only change, and the execution report's Meta section was corrected accordingly (it had
claimed 0 source files). No executable product code changed, and no defect this slice **measured**
was fixed — INC-2026-08…12 all stand.

| # | Severity | Issue | File | Disposition | What was done | Status |
| --- | -------- | ---------------------------------------------- | ----------------------------------------------------- | ----------- | -------------------------------------------------------------------------------------------------------------- | ------ |
| 1 | Medium | "1120 fd samples" conflates rounds with observations | `cleanroom-linux-x86_64-2026-08-09.md:287` (+ plan, SUMMARY) | Fix | Restated as "160 rounds at 4 Hz over 40 s, 1120 fd observations (7/round)" at all 4 sites | Fixed |
| 2 | Medium | jsdoc addresses its debt to the now-split "17.2" | `apps/collector/src/connectors/cursor-store.ts:12` | Fix | → "17.2b", plus two sentences separating confinement (verified by 17.2a) from convention (still not measured) | Fixed |
| 3 | Low | "Real credentials / archive modified" unscoped | `cleanroom-linux-x86_64-2026-08-09.md:54` | Fix | Label scoped to "Real **Windows** credentials / archive" | Fixed |
| 4 | Low | capture counts given without their inputs | `cleanroom-linux-x86_64-2026-08-09.md:45` | Fix | New §4b derives the counts and **declares the `raw` behaviour unexamined**. First attempt got the order wrong (see below); corrected to a stepwise table | Fixed on the second pass |

**Re-review pass 2 found one defect in the fixes themselves**, which is the whole reason the loop
exists. Finding 4's first version said the seeded file "was later appended" **after** the three extra
files were written. The actual order is the reverse — append at step 3, extra files at step 4 — and
the order is load-bearing, because step 2→3 is the INC-2026-05 evidence. Rewritten as a stepwise
table carrying the archive counts observed at each point, which also exposes something the prose had
hidden: **step 4 added 3 files and 6 lines, moved `events` by +6, and moved `raw` not at all.** That
is now declared unexamined and flagged for 17.2b rather than left invisible. Pass 3 is clean.

## Findings

```
severity: medium
file: .agents/research/cleanroom-linux-x86_64-2026-08-09.md
line: 287
issue: "1120 fd samples" conflates sampling ROUNDS with path OBSERVATIONS, overstating temporal coverage 7x
detail: The producing loop is `for i in $(seq 1 160)` with `sleep 0.25` — 160 rounds over 40 s at
4 Hz — and each round appends every currently-open `/`-prefixed fd path. The reported 1120 is
`wc -l` of the accumulated file, and 1120/160 = exactly 7 paths per round. So there were 160
samples in time, not 1120. The report leans on this figure as the strength of its read-side
evidence ("Holds — 1120 fd samples") in a document whose thesis is that summaries are not
evidence, and it already carries a caveat that a read-and-close inside one 250 ms window can be
missed. Stating the number correctly makes that caveat MORE load-bearing, which is the honest
direction. The figure has propagated to three further sites.
suggestion: Rephrase as "160 sampling rounds over 40 s at 4 Hz, yielding 1120 open-fd observations"
at :287, and shorten the table cells at :48 and :92 to "160 rounds / 1120 fd observations".
Same edit in `.agents/plans/m17-cross-platform-collectors.md:144` and `SUMMARY.md:351`.
```

```
severity: medium
file: apps/collector/src/connectors/cursor-store.ts
line: 12
issue: the jsdoc addresses its measurement debt to "17.2", a slice number this commit split
detail: The comment reads "to be hardware-verified in 17.2 — NOT yet measured." This commit splits
17.2 into 17.2a (✅ DONE) and 17.2b (blocked on procurement), and 17.2a verified the CONFINEMENT
half while explicitly NOT discharging the CONVENTION half. A reader who opens this file, sees
"17.2", and checks SUMMARY finds a 17.2a marked done — and concludes the debt is discharged. That
is precisely the misreading the report spends a paragraph warning against ("the closure of
INC-2026-01 must not be read as closure of this"), except the warning lives in the report and the
stale pointer lives at the site where the decision would actually be made.
NOTE FOR TRIAGE: fixing this edits product source, and "No product code changed (D-M17-5)" is an
acceptance criterion of this slice. A one-word reference correction is arguably documentation
rather than a fix, but it is the user's call, not mine.
suggestion: Either (a) change "17.2" to "17.2b" in the jsdoc and note in the execution report that
the only source edit was a slice-number reference, not a behaviour change; or (b) leave it and
accept that the plan + report carry the boundary. Option (a) puts the correction where the
misreading happens.
```

```
severity: low
file: .agents/research/cleanroom-linux-x86_64-2026-08-09.md
line: 54
issue: the headline row "Real credentials / archive modified — No" is unscoped, while the run DID create a real collector home
detail: The row is accurate as written (it covers the Windows `credentials.json` hash, the archive's
machine/token identity and org/user counts, all verified identical). But the run also created
`~/.420ai` in the real WSL home — a real collector home — via INC-2026-08. Row 56 states the breach
two lines later and the isolation table's last row marks it ❌, so the document as a whole is
honest. The risk is a reader who quotes row 54 alone.
suggestion: Scope the label: "Real **Windows** credentials / archive modified". Costs four
characters and removes the only row that can be quoted misleadingly.
```

```
severity: low
file: .agents/research/cleanroom-linux-x86_64-2026-08-09.md
line: 45
issue: the capture evidence reports outputs (3 raw / 16 events) without stating the inputs that produced them
detail: The report's Level-3 claim is "capture works end-to-end", and counts > 0 is exactly what the
plan required, so the conclusion is sound. But the counts are given with no account of the input —
four session files were seeded (one pre-`watch` and static, three written mid-run), each two lines
of the repo's `sample-session.jsonl` fixture. Without that, `raw = 3` from four files reads as an
unexplained shortfall, and a reader cannot sanity-check the numbers at all. In a report that
states "summaries are not evidence" and quotes its own commands verbatim elsewhere, this is the
one result presented without its derivation.
suggestion: Add one line to the "what WAS measured" section or the §5 finding: which fixture, how
many files, how many lines, and that per-file raw-record accounting was not part of the
measurement — so the shortfall is declared unexamined rather than left to be inferred.
```

---

## Not raised, and why

- **The `git log -S` evidence for INC-2026-11.** `-S` finds when a *string* was added and can be
fooled by reformatting. It is paired here with `git log --until=2026-08-02 -1` showing the file
untouched between 2026-06-20 and 2026-08-02, which closes the gap. The report also correctly
refuses to claim 16.0's actual cause, marking it "undiagnosed" and offering the glob-mismatch
candidate as a candidate. Appropriately hedged; no finding.
- **`≈ 64 s` time-to-first-capture.** Re-added from the timing table (1.13 + 30.15 + 0.12 + 1.52 +
9.54 + 0.70 + 2.07 + ~6 + 0.06 + 0.02 + 0.51 + 0.36 + ~12 = 64.2), isolation-only steps excluded
as 16.0 did. Consistent, and hedged by four honesty caveats.
- **The two operator errors and the failed intermediate `repo-health` run** are disclosed in the
report rather than smoothed over, including the discriminator (`ECONNREFUSED` ≠ `28P01`) that
rules out a leak. This is the behaviour the process wants; no finding.
Loading
Loading