stack/agents/filbert/work/chat-02-backend-review-2026-09-26.md

CHAT-02 backend: Filbert's code review

Reviewer: Filbert, 2026-09-26. Requested by Dewey, assigned by Sage (D4).

Candidate: agents/dewey/work/chat-02/BACKEND.md, sha256 286c3ad5f147471f16ef88537c7680c5bd6130e6365ce1f0c4cb98cd74e75c2e, and the nine files its §1 pins. All nine verify. Measured against BRIEF.md R4 (636b0fac…, verified) and Sage's D1–D4 (2026-09-26_lead-decisions.md item 8). Jason's go on CHAT-02 is recorded in the same file (20:31Z).

Verdict: revise. The safe-open path, snapshots, cursor refusals, byte limits and the no-write discipline are sound, and the tests are strong. Three defects need fixing, and I reproduced each with a scratch script (§2). Three small items can go in the same revision (§3). No finding needs a lead ruling.

1. What I checked and reproduced

• Suites. The shared working tree now carries unpinned edits to packages/webui/src/public/app.js, index.html and packages/ledger/src/ledger.mjs, and four webui browser tests fail there. To test the candidate alone, I ran a detached worktree at HEAD 1c5f6bc3 with only the nine pinned files overlaid (pins re-verified in place). The result: conversation, control-board, webui and seat, 175/175. chat-00, chat-01 and chat-01c check.mjs all exit 0. Dewey: the shared-tree failures come from the unpinned webui files, not from this packet.

• Real data, read-only. Catalogue: 18 Pi rows in 71 ms. Following the three most recently active views took 1, 33 and 177 ms per poll, with the cache thrashing across three views. That is acceptable for §3 item 9.

• Pi semantics against the pinned 0.85.1 session-manager.js:

  • the default leaf is the last entry in file order (_buildIndex);
  • branch() moves the leaf in memory only;
  • resetLeaf() starts a new root entry, so a file can have several roots;
  • retainedTail is documented in docs/session-format.md 245.

  The parser matches all four.

• Safe open. It opens with O_NOFOLLOW, compares (dev, ino) after the fstat, re-checks the components after the open, and reads only from the descriptor. Registrations never add a root or name a file, and a spec must have the exact .pi/state/<seat>/sessions shape. The listing refuses symlinks and odd names without opening them.

• Accepted choices, packet §3:

  • item 1, the summary row;
  • item 2, sessionsDir matching, which is stricter than the brief's sessionFile;
  • item 3, the placeholder row;
  • item 4, the epoch comparable within one chain, which README 132–135 states;
  • item 7, apart from finding 3 below;
  • items 8, 9 and 10.

  Item 5 stands, apart from finding 1. Item 6 does not stand: see finding 2.

2. Required

1. A branch's id changes every time the conversation grows.
   • The branch id is the leaf entry's id (reader.mjs 270). Every append makes a new leaf, so one thread is named differently from one page to the next.
   • Scratch file: open a linear a→b, then append c→d.
     • The open page has branch: "b".
     • The follow, sent with branch: "b" on a cursor bound to b, returns a page with branch: "d", and its entries carry d.
     • open({branch: "b"}) then refuses unknown-branch (404, reconcile).
   • The F12 test asserts this behaviour (g.page.branch === "o5" after following o4).
   • Effects:
     • One view's entries carry different branch values.
     • A cursor bound to one branch serves a page labelled with another, although CHAT-01 line 66 binds a cursor to its branch.
     • Any branch id the Console keeps goes stale after one append. After a board restart (cursor-unknown), reopening the stored branch fails.
   • Fix: give each branch an id that growth cannot change. Recommended rule:
     • walk from each root in file order;
     • at a fork, the child earliest in file order continues the current branch, and each later child starts a branch named after its own entry id;
     • the first root names the first branch;
     • each later root (resetLeaf) starts its own branch.
   • Appending never changes which child came first, so ids are stable.
   • view.branches lists each branch's current leaf and last activity. page.branch always equals the cursor's branch.
   • Tests:
     • a follow keeps the branch id;
     • a pre-growth id still opens;
     • entries across follows share one branch value;
     • F12 with the new ids;
     • a two-root file.
2. The "assumed link" bridge merges branches.
   • branchPath (pi.mjs 94–104) joins a missing parent to the previous valid entry in the file whenever a malformed line sits between them.
   • Scratch file: a (root), b→a, then a malformed line that held X→a (a fork from a), then y→X.
     • view.branches lists b and y as separate branches.
     • The default page for y still shows root, then ON THE OTHER BRANCH (b's text), then the notice, then leaf. That is b's history rendered as y's.
   • F12 says "no merge", and the brief's F1 asks only that reading continues. A notice saying the link is assumed doesn't make the merged history correct.
   • Nothing is lost without the bridge. The entry before the gap has no children, so it is already a leaf, and the history before the gap reads as its own branch.
   • Fix: drop the bridge. Stop with the missing-parent notice and name the unreadable line(s) in it.
   • Tests: update the two F1 bridge tests, and keep a follow case in which the view stays put and reports the new default.
3. A directory component without search permission fails the whole catalogue.
   • lstatOrNull (safe-fs.mjs 38–45) rethrows EACCES, so checkRoot throws a plain error instead of a refusal. catalogue() rethrows it, and the route returns 500.
   • Scratch file: two seats, with .pi/state/bad at mode 000.
     • catalogue() throws EACCES on lstat of bad/sessions.
     • open() of an unknown id throws too, and so does any id resolved after the bad root.
   • Packet §3 item 7 and the test at line 639 cover the sessions directory at mode 0300 (search allowed), not a component above it.
   • Fix: map EACCES/EPERM in lstatOrNull through denied().
   • Test: a seat directory at mode 000 beside a readable one. The catalogue lists the readable root and one unreadable entry in refusedRoots. Opens in the readable root still work in either root order.

3. Small, same revision

1. Redacted thinking is marked visible. Pi stores Anthropic redacted_thinking as {type: "thinking", thinking: "[Reasoning redacted]", redacted: true} (pi-ai anthropic-messages.js 445–451). The parser gives it permitted-visible with that placeholder. CHAT-01 says unavailable reasoning has empty text. Map redacted === true to unavailable with "", and never read thinkingSignature. There are no live cases today.
2. A relative header cwd resolves against the board's working directory. cwdInProject calls resolve(cwd). With cwd: "." and the board started from the checkout, the file passes. Pi writes absolute paths. Refuse a non-absolute cwd as foreign-project, and add a test.
3. A file whose entries are all malformed shows no notices. With no leaf, build uses an empty path (reader.mjs 272), so view.unreadableLines is N and the page has no parts. Emit the malformed notices when there is no leaf.

4. Not findings

• The F8 fstat mismatch is covered by design only. That is acceptable, as packet §5 says.
• Cursors are reusable until expiry or eviction. A local process can evict another view's cursors by opening 1000 pages, which ends in a reconcile. That fits the one-actor loopback scope.
• Darkwing reviews the route code. I read it only as far as the reader calls it: the query validation, status map and 500 path look right.

To reach approve

Fix §2 items 1–3, with their tests. Take §3 items 1–3 or say why not. Re-pin, re-run the suites and the mutation scripts, and send the new packet hash. I'll review the delta.

Reproductions: agents/filbert/work/chat-02-backend-review-evidence/ branch.mjs (§2 item 1), bridge.mjs (item 2) and perm.mjs (item 3). Each builds its own temp project and imports the working-tree reader. Run them with node <file>.

Revision 2 delta review

Candidate: BACKEND.md sha256 0cf177b14cfdc52b7026d67333dc16eef345d80a7d908d50d69bb4301050ca85, and the nine files its §1 pins. All nine verify. Revision 1 is frozen as BACKEND-r1-286c3ad5.md (hash verified). I diffed safe-fs.mjs, pi.mjs, reader.mjs and serve.mjs against my revision-1 snapshot.

Checks:

• Suites. Detached worktree at 1c5f6bc3 with only the nine files overlaid: 181/181. chat-00, chat-01 and chat-01c exit 0.
• My reproductions, rerun on the candidate:
  • branch.mjs: open and follow both give main, and the pre-growth reopen works.
  • bridge.mjs: b.y shows the missing-parent notice (naming line 4) and leaf only. main stays separate.
  • perm.mjs: the catalogue returns the readable row, with bad in refusedRoots as unreadable. An unknown id refuses instead of throwing.
• A new probe (fork.mjs): a linear main, then a fork from its middle, then growth back on main, then a second root.
  • Every branch name held across all of it: main, b.x, b.r.
  • Follows stayed on their branch, and returned an empty page when the growth was elsewhere.
  • view.defaultBranch tracked Pi's last entry.
  • open by name still worked afterwards.
• Naming rule. At most one leaf can end each earliest-child walk. For that reason, two leaves cannot share a name. For a file that only grows, a name cannot change: children are sorted by file index, and the first root is fixed.
• Small items. Redacted thinking gives unavailable with ""; a relative cwd is foreign-project; an all-malformed file gives one notice per line. Each has a test.
• Malformed notices on every branch, after the leaf too. This keeps a branch's served parts a prefix of its later parts, because new lines only ever append at the end.

Verdict: approve 0cf177b1.

One nonblocking note: the new §5 limit, where a duplicate id with new content goes unnoticed, could be closed cheaply. idsDigest could hash the serialized parts, whose sizes are already computed, instead of their ids. Pi's generateId never reuses an id, so this can wait.

Darkwing owns the route delta. I read only the serve.mjs diff: the cursor without branch gives a 400, and there are three new REFUSAL_STATUS entries. It looks right.