stack/agents/filbert/work/queue-a1-review-2026-09-26.md

# Queue A1 (#1508): Filbert's code review

Reviewer: Filbert, 2026-09-26. Requested by Darkwing, assigned by Sage.

Candidate: `agents/darkwing/work/queue-a1/`, measured against section 8 of
`agents/filbert/work/queue-as-data-plan-2026-09-26.md` (sha256 `282fabbb`)
and Sage's A1/A2 split.

| File | sha256 |
|---|---|
| `build.md` | `a1125ebd` |
| `build-manifest.sha256` | `4319695a` (20 paths) |
| `build.patch` | `419804f2` (20 new files, nothing else) |

**Verdict: changes requested.** The code holds up well: the 624-case matrix
comparison, the lock, the write path and `queue-commit.sh` are sound. Three
things need a revision: one test gap against 8.14 and two recorded-state
defects. All three are small. Notes N1 to N16 don't block.

## 1. What I ran

- **Where:** a `git clone --shared` at `/tmp/fb-qa1`, base `3a209eea` plus
  `build.patch`. It has its own `.git` config and hooks, so nothing ran
  against the canonical `.git`. All 20 pins verify there, and they still
  did after every mutation run.
- **Suites** in the clone: queue 19 checks (node --test 95/95, skip line
  for the missing `queue.json`), config 24, task 90, foundation 43,
  conductor 17, release 14, auth 15, discord 63, extension-package 18.
  Same as `build.md`.
- **Five more full queue runs**: 95/95 each. One of a review agent's seven
  runs showed 94/95. I couldn't reproduce it. Another agent was mutating
  `queue.mjs` in place in the same clone at the time, which may explain
  it.
- **My mutations**, each run in a private copy and then reverted:
  - M1, `H=` moved after the step-1 canary: `commit.test.mjs` fails 1.
  - M4, `env -u NODE_TEST_CONTEXT` dropped from step 4: fails 2.
  - Owner check dropped on unblock: **survives, 95/95** (R1).
  - Owner check dropped on block: caught. Dropped on in-review→back:
    caught.
  - The three fsync removals in N3: **each survives, 95/95**.
  - An unreadable unlock gate during `acquire`: the lock stays behind (N2).
- Two review agents read store/lock/io and queue/cli against the spec. I
  include only what I reproduced or confirmed by reading.

## 2. Sage's five conditions

1. **Nothing runs against the canonical `.git`: met.** The tests build
   repositories under `os.tmpdir()` with a scratch `HOME` and
   `GIT_CONFIG_NOSYSTEM`, and they clear `GIT_DIR`, `GIT_WORK_TREE`,
   `GIT_COMMON_DIR` and `GIT_INDEX_FILE`. I snapshotted the canonical
   `.git` before and after: hooks (samples only), `mosaic-queue*` files
   (none), `core.hooksPath` (unset), `.git/config` sha, refs and HEAD.
   Nothing changed.
2. **No QUEUE, AGENTS or TOOLS edits: met.** The patch holds only new
   files. `git diff HEAD` on `docs/plans/QUEUE.md`, `AGENTS.md` and
   `docs/TOOLS.md` is empty. `docs/plans/BRIEF-TEMPLATE.md` is new and
   listed in 8.1.
3. **`test-queue.sh` green with no `queue.json` in HEAD: met.** It prints
   `skip  queue verify: HEAD has no docs/plans/queue.json (before the
   genesis commit)`.
4. **H recorded before the canary, with a test: met.** `H=` is at line
   138, before `guard_check "$H" step1` at 140. The test at
   `commit.test.mjs:225` moves HEAD at the first hook run. M1 is caught.
5. **The fault layer is reachable only from tests: met.** `io`, `proc`,
   `hook`, `now`, `readOrder`, `lockWaitMs`, `lockStepMs`, `env` and `cwd`
   enter only through `opts` in `makeCtx`. `cli.mjs` calls `run(argv)`
   with no options, and neither `queue-commit.sh` nor `test-queue.sh`
   passes any. No flag or environment variable selects a fault.

## 3. Darkwing's four questions

1. **`NODE_TEST_CONTEXT`: the fix is right.** Node 26 under a parent
   runner: a failing nested `node --test` exits 1 plain, and 0 with
   `NODE_TEST_CONTEXT=child` or `child-v8`. Both nested sites clear it,
   and M4 shows the test catches the regression. The same blind spot
   exists in `scripts/test-foundation.sh:76` and
   `scripts/test-discord.sh:142`. Nothing runs those under a node runner
   today, so it is latent (N13).
2. **git 2.55 and `index.lock`: the reasoning holds for plain `commit
   -e` only.** I checked all three forms with a paused editor:
   - `git commit -e`: no `index.lock` while the editor is open;
   - `git commit -a -e`: `index.lock` held;
   - `git commit -e -- path`: `index.lock` held.

   With `-a` or paths, step 8 sees the lock and exits 3. Everything stays
   safe: the queue commit is in, and the paused commit loses at its
   update-ref either way. Two follow-ons: the comment at
   `commit.test.mjs:170` ("holds index.lock") contradicts line 174 (N14),
   and the test's `res.code === 0` pins 2.55's behaviour. A git that held
   the lock would give 3 and fail the test, not the code.
3. **H before the canary: met**, see condition 4.
4. **The `verify-commit` test deferred to D: agreed.** 8.12 uses
   `review verify-commit` only for review-gated source. D adds the verb,
   so the prospective-tree test belongs with it.

## 4. Required changes

**R1. The matrix tests miss refusals that 8.14 requires.** 8.14 asks for
"tests for every transition and refusal in 8.7". With `ownerOrPriv(row,
by, "unblock")` removed, the suite passes 95/95, so a non-owner can unblock
and no test notices. A review agent found two more survivors of the same
kind: letting the owner move waiting-on-jason→in-progress, and a narrower
queued→blocked owner check. The code is correct: the agent compared all
624 state × target × actor cases with 8.7 and found no mismatch. The
coverage isn't there, though. A table-driven test over state × target ×
actor class (owner, other seat, sage, jason, with gate owner jason or not)
would close it once and for all.

**R2. `review.issue` can be frozen at null.** `queue.mjs:464` sets `issue:
row.review ? row.review.issue : (row.issues[0] ?? null)`. A row with no
issues opens round 1 with `issue: null`. After `set N issues 1508`, round 2
keeps `null` while the row's first issue is #1508. D will read this field
to decide where to post. Either refuse `move in-review` when `issues` is
empty, or take `row.review?.issue ?? row.issues[0] ?? null`. The first is
stricter and simpler. Once genesis exists, this is replayed state, so fix
it before genesis. With a revision already needed, now is cheapest.

A question for Sage, not Darkwing: `canonIssues` sorts, so "the row's first
issue" is always the lowest number. A row for #1508 that also lists #1495
would get its reviews on #1495. Is that the ruling's intent?

**R3. `--evidence` doesn't name the round.** 8.7 says in-review→done
"`--evidence` must name the current round and its candidate digest". The
format is `comment=<id>,candidate=<digest>`, and only the digest is
compared. If a row goes back for changes and is re-requested with the same
candidate, a comment from round 1 closes round 2. The review agent
reproduced it through the CLI (`in-review→done round 2`). Add `round=N` and
compare it with `cur.n`. The J5 test is titled "evidence naming the current
round", so it should assert this.

## 5. Nonblocking notes

Lock and write path:
- **N1. A refused op drops the release warning.** `withLock`
  (`store.mjs:401-413`) pushes the release message into `res`. When `fn`
  throws, that `res` is discarded, so "not the one this process took; left
  in place" is lost on the refusal path. 8.4 says release "reports it". The
  review agent reproduced this with a lock swapped at the `locked` hook.
  `unlock` (`lock.mjs:167`) drops the gate's release message too.
- **N2. An error after `link()` leaves the lock behind.** In `acquire`,
  `lstatOrNull(gate)` and `readOrNull(gate)` (`lock.mjs:125-126`) run
  before `release`. With the gate at mode 000, `acquire` throws EACCES and
  the lock stays. I reproduced it: afterwards both `mosaic-queue.lock` and
  the gate are present. The next op reports a dead owner, and `unlock`
  hits the same EACCES. That is fail-closed, but recovery is by hand. Wrap
  everything after the link in `try` with `release` in `finally`. The
  `io.stat(tmp)` after the link at line 102 has the same shape.
- **N3. Three durability steps have no test.** Removing any one of these
  passes 95/95:
  - the `queue.json` fsync and `docs/plans` fsync in `confirmTail`
    (`store.mjs:250-251`, 8.5 step 2);
  - the `.git` fsync after the witness rename (`store.mjs:245`, step 10);
  - the `docs/plans` fsync after the view rename (`store.mjs:329`, step
    11).

  The code is right. A fault-injecting `fsyncDir` on those paths would pin
  it.
- **N4. The classification-order test doesn't separate host from boot.**
  The foreign-host fixture carries the local boot id, so swapping lines 53
  and 54 of `lock.mjs` survives. A real foreign host has a different boot
  id, would then classify `mismatch`, and `unlock` would remove it. Add a
  foreign-host record with a different boot id.
- **N5. The filesystem allowlist is wider than 8.15.** tmpfs is accepted
  outside tests, and magic `0xef53` also matches ext2 and ext3. No effect
  on the canonical checkout (ext4).
- **N6.** When the `.git` fsync after the witness rename fails,
  the message says "witness not updated", but the rename already happened;
  only its durability is unconfirmed. Exit 3 and no receipt are right.
- **N7.** A killed writer's `queue.json.<op>.tmp` is removed only when
  that same op is retried. Lock and gate temp files from killed acquires
  stay in `.git/`.

Model and CLI:
- **N8. `cell()` escapes `|` but not `\`.** Piece text `a\| done | x` renders
  as `a\\| done \| x`, which `marked` splits into an extra cell, shifting
  later columns. Raw HTML passes through too. Newlines, control characters
  and the markers are refused, so no row can be forged in the Markdown.
  Escape or refuse `\` and `<`.
- **N9. `BRIEF-TEMPLATE.md:16` says the owner accepts the brief.** 8.7 makes
  queued→briefed privileged only. Please fix this in the revision.
- **N10. `set issues` resets `closes`** (`queue.mjs:508`) and quietly
  undoes a narrowing that was logged with `--reason` (J6).
- **N11. Replay is looser than the CLI on op ids.** It accepts up to 80
  characters for any entry, lets `accept-history` end in `.outcome`, and
  doesn't pattern-check the genesis op. A hand-built file, which recovery
  allows, could hold ops the CLI would refuse. Reported by the review
  agent; I confirmed the regexes by reading.
- **N12. `--by` silently overrides `MOSAIC_AGENT_NAME`** (`store.mjs:93`).
  J2 allows claimed actors, but logging a mismatch would give piece E
  something to find.

Commit path and suites:
- **N13.** `test-foundation.sh:76` and `test-discord.sh:142` should clear
  `NODE_TEST_CONTEXT` like the queue sites do. This is a follow-up for
  Sage, outside A1's paths.
- **N14.** The stale comment at `commit.test.mjs:170`; see question 2.
- **N15. `--install-hook --by` is self-asserted**, like every actor under
  J2. It is protocol, not a control.
- **N16. The hook runs `git diff --cached --quiet HEAD`,** which fails on
  an unborn HEAD, so an installed hook refuses a repository's first
  commit. Fail-closed, and irrelevant here.

## 6. Build-note choices

I accept them all, with three remarks:
- `set piece` and `set gate` aren't in 8.7's field table. Privileged only
  is a sound default; Sage should record it.
- "`unlock` works on a missing or invalid lock file" reads wrong. The code
  refuses an invalid lock, as 8.4 requires. The note means a missing or
  invalid `queue.json`.
- Messages naming `scripts/mosaic queue` are fine because genesis can't
  run before A2 adds the dispatcher.

## 7. For the re-review

Send a delta patch against `build.patch` and a new manifest. I'll rerun
the queue suite, R1's mutation plus the two agent survivors, and the R2
and R3 cases. The other suites can't reach `packages/queue`, so they need
no rerun unless the delta touches shared paths.

## Scratch

`/tmp/fb-qa1` (a `--shared` clone, not a worktree) and a second clone,
`/tmp/fb-qa1-m`, used for M1 and M4. Pins were re-verified 20/20 before
removal. Each other mutation ran in a private `mktemp -d` copy, deleted
afterwards. Two `/tmp/mosaic-queue-test-*` directories from 18:04 and 18:09
CDT are left over from test runs; I can't tell whose they are, so I left
them. No commit or push.