feat(conversation): CHAT-02 read-only Pi history reader and two board routes (#1507)
packages/conversation is a library with no server: safe-fs, the Pi session parser, CHAT-01 pages, pinned snapshots, cursors and follow. The control board adds GET /api/conversations and /api/conversation behind the Host and Origin guard. Both are read-only, their queries are validated, and each refusal code maps to a status. Dewey authored it (packet 0cf177b1, revision 2). Filbert reviewed the code: R1 revise (branch ids moving on append, the assumed-link bridge merging branches, one unreadable seat directory turning the catalogue into a 500), then R2 approve (3b14d66c). Darkwing reviewed the routes: R1 approve (07b10ad1), R2 approve (b9d92003). The package lands with the routes, because serve.mjs imports the reader at load. On an index export: the eight suites 24/90/43/17/14/15/63/18, conversation and control-board 153/153, webui 9/9. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,206 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user