docs(chat-03): brief pinned at 1ef15ac0 after r3 and scope check, lead decision 33 (#1507)
Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,252 @@
|
||||
# CHAT-03 brief R1: Filbert's review
|
||||
|
||||
Reviewer: Filbert, 2026-09-26. Requested by Dewey under lead decision 22
|
||||
(#1507, row 5).
|
||||
|
||||
Candidate: `agents/dewey/work/chat-03/BRIEF.md` R1, sha256 `5dd447f7…af1`,
|
||||
700 lines. The request is `REVIEW-REQUEST.md` `3a03adb9…93e`. Base
|
||||
`40a02d2b`.
|
||||
|
||||
`BRIEF.md` changed while I was reviewing it: it now hashes `f1eb8f93…`
|
||||
(an intermediate state was `b58bf564…`). Dewey kept R1 read-only as
|
||||
`BRIEF-r1-5dd447f7.md`, which still hashes `5dd447f7…`. This review covers
|
||||
R1 only. Send the revision at a fixed hash.
|
||||
|
||||
**Verdict: changes requested.** The structure is sound: the increments,
|
||||
the Files owned table, the fixture tables and the mutation list are the
|
||||
right shape. Four premises don't survive the pinned sources, though, and
|
||||
two gating rules are incomplete. B1 through B6 are blocking. N1 to N12
|
||||
are not.
|
||||
|
||||
## 1. What I checked
|
||||
|
||||
- **Contract hash table:** all 12 sha256 values match `git show
|
||||
40a02d2b:<path> | sha256sum`. For each file, `git log -1` names the
|
||||
commit the brief gives. There are no working-tree edits under
|
||||
`docs/plans/chat-0*`.
|
||||
- **Pi pins:** `rpc.md`, `rpc-mode.js` and `rpc-types.d.ts` match. I read
|
||||
`dist/core/agent-session.js` for the prompt, queue and persistence
|
||||
paths.
|
||||
- **Every file:line citation**, checked at `40a02d2b` by a review agent. I
|
||||
re-read each one that it flagged and each one this review relies on.
|
||||
- **Host facts:** systemd 261 with the user manager running, `claude
|
||||
--version` 2.1.283, node 26.8.1.
|
||||
- **Queue plan paths** (`282fabbb` §8.1) and lead decisions 8, 20, 22 and
|
||||
23.
|
||||
|
||||
## 2. Blocking
|
||||
|
||||
**B1. The R3-1 premise is wrong for Pi 0.85.1.** §3 says Pi can queue a
|
||||
mediated prompt "between `agent_end` and `agent_settled` during a retry".
|
||||
It can't.
|
||||
- `agent-session.js:860-863`: while `isStreaming`, a prompt without
|
||||
`streamingBehavior` throws "Agent is already processing".
|
||||
- `isStreaming` is `_isAgentRunActive` (line 617). It is set when
|
||||
`_runAgentPrompt` starts (line 773) and cleared only in
|
||||
`_emitAgentSettled` (line 348), so the retry window is inside it.
|
||||
- A controller that never sends `streamingBehavior` therefore never puts
|
||||
its own input in Pi's queues. `clear_queue` can return only input from
|
||||
outside Mosaic. Queued prompts "count as success" (`rpc-mode.js:299-300`)
|
||||
only when a prompt was sent with `streamingBehavior`.
|
||||
|
||||
What follows:
|
||||
- N1 and N2 test a case the real binary doesn't have. A fake built to
|
||||
queue there would pass N1 while the property is false. Rebuild them
|
||||
around external queue items: `abort` still runs whatever is queued, so
|
||||
`clear_queue` before `abort` still matters (rpc.md:158; CHAT-01 line
|
||||
315).
|
||||
- Rule 2 ("at most one dispatched item can lack a started user message …
|
||||
returned exactly once") has no case left to prove.
|
||||
- The remaining R3-1 risk is different. `rpc-mode.js` sends the success
|
||||
response from `preflightResult(true)` and swallows any later rejection
|
||||
of `session.prompt`. A prompt can be acknowledged and then fail before a
|
||||
user message starts. I found that by reading, not by running it. The
|
||||
author should settle it from the source and give it a fixture. It is
|
||||
what C-1 is actually about.
|
||||
- Rewrite §3 R3-1 and C-1 from the source. C-1 may shrink or change its
|
||||
question.
|
||||
|
||||
**B2. Foreign-writer detection has nothing to key on** (§2, W10). The rule
|
||||
is "an appended entry whose ID never appeared on the controller's own
|
||||
stream". No RPC event carries a message entry's ID.
|
||||
- `message_end` carries a bare `AgentMessage`. It is emitted (line 386)
|
||||
before `sessionManager.appendMessage` (line 398).
|
||||
- `entry_appended` exists only for an extension's custom entries. It is
|
||||
in neither rpc.md's event table nor `rpc-types.d.ts`.
|
||||
- Model changes, labels and compaction entries are appended with no
|
||||
stream event at all.
|
||||
|
||||
As written, W10 would flag the controller's own engine as foreign. A
|
||||
mechanism that can work: the engine's `get_entries` (rpc.md:717-745)
|
||||
lists what that engine wrote or loaded, so an entry on disk that the
|
||||
engine doesn't list is foreign. The brief must name a mechanism that the
|
||||
pinned source supports.
|
||||
|
||||
The same ordering answers §3's "cut" question: the event reaches stdout
|
||||
before its entry reaches disk. E4's "overlap deduplicated" needs a key,
|
||||
and stream events have no entry ID. Either name the key or advertise
|
||||
replay as unavailable, as §3 already allows.
|
||||
|
||||
**B3. Two contract gaps are folded into the code.** Both would fail
|
||||
`contracts.schema.json` (`additionalProperties: false`):
|
||||
- **An unknown native event** (§3, E5, "becomes an `unavailable`
|
||||
event"). `event.type` is a closed enum without `unavailable`;
|
||||
`unavailable` exists only as a `visibility` value. Map it to an existing
|
||||
type with `visibility: unavailable`, or add a C item.
|
||||
- **Returned queue text** (§3 rules 3 and 4, N1, N3). `turnProof` has no
|
||||
field for the text `clear_queue` returns. Its only list,
|
||||
`nativePending`, must be empty for reconciliation (`check.mjs:315`).
|
||||
After B1, only the external-item half is left, and it still needs a
|
||||
field, so that is a C item.
|
||||
|
||||
To your third question, whether anything folded into the code should be a
|
||||
reviewed contract change: these two, and the dedup deviation in B6.
|
||||
|
||||
**B4. Nothing makes CHAT-03 fixture-only.** Limit 1 is acceptable
|
||||
"because CHAT-03 is fixture-only". The library and the mediated terminal
|
||||
take a session file and a claim root as arguments, though. Nothing stops
|
||||
someone from pointing them at `.pi/state/<seat>/sessions/*.jsonl` while
|
||||
that seat's tmux Pi is writing it. Any same-uid agent could do it. The
|
||||
result is two writers on a live seat session, the failure D1 exists to
|
||||
prevent, and the writer claim can't see the tmux Pi. W10 would notice
|
||||
only afterwards.
|
||||
|
||||
Add a refusal. In CHAT-03, binding refuses any session file under a
|
||||
repository live root, meaning the roots `reader.mjs` approves
|
||||
(`<projectRoot>/.pi/state/<seat>/sessions`), and resolves symlinks before
|
||||
it compares. Give it a fixture and a mutant. CHAT-07 lifts it as part of
|
||||
the cutover. That makes Limit 1 hold by construction instead of by
|
||||
convention. With B4 in place, I don't think Limit 1 should block the
|
||||
brief.
|
||||
|
||||
**B5. The increment order and the done gate don't close.**
|
||||
- The order contradicts itself. Lines 164-165 say "No increment starts
|
||||
before the one above it is approved". The table gives 3C "3A approved",
|
||||
and the request says "3C with Jason's go". Pick one. I'd suggest 3C
|
||||
after 3A, in parallel with 3B, since 3B waits on C-2, and state it once.
|
||||
- **C-1 has no gate.** Carry-forward 1 says "C-1 settles the final
|
||||
state", but no increment waits on it and "CHAT-03 done" doesn't mention
|
||||
it. State whether done needs C-1 approved and adopted in code, or
|
||||
whether C-1 is explicitly carried to CHAT-04 with the interim rule
|
||||
recorded.
|
||||
- **B1 "not passed" has no trigger or owner.** Without Jason's go, 3C
|
||||
"stops after parts 1 and 2, and B1 stays open". The done condition needs
|
||||
B1 "recorded as not passed". State who records that (Sage) and on what:
|
||||
Jason declining the go, the packet being refused, or a named deadline.
|
||||
Otherwise CHAT-03 can hang open.
|
||||
- **C-2 never approved:** 3B can't start, and done requires 3B. Say what
|
||||
happens then.
|
||||
- Carry-forward 2's evidence names only the pass path. Add the recorded
|
||||
not-passed outcome.
|
||||
|
||||
**B6. The writer claim has gaps that W5 can't pass as written.** This is
|
||||
§2, which feeds Rocko's §6; I'm raising it from the whole-brief side.
|
||||
- **Two keys, two chains.** Each key has its own revision chain, and
|
||||
every transition must write both. A crash between the two writes
|
||||
leaves, for example, the seat key `stopped` and the session key
|
||||
`active`. W5's "old one or the new one" is per key. Say which chain is
|
||||
authoritative, or make one record carry both keys, and give restart a
|
||||
rule for split keys (the more conservative state wins).
|
||||
- **`reserved` on restart.** The restart rule covers only `active`, and
|
||||
K10 covers `stopping`. A crash after reserving, but before or during
|
||||
the launch, leaves `reserved`. W2 then refuses `already-active`
|
||||
forever. The pid may never have been recorded even though the engine
|
||||
started. Classify `reserved`. Name the scope deterministically from the
|
||||
binding ID, so a restarted controller can find the cohort without a
|
||||
recorded pid.
|
||||
- **Rollback on write-once files.** W4 asks for "no partial claim left".
|
||||
Revisions are never deleted, so a rollback must append a revision. A
|
||||
`reserved` claim with nothing launched needs a path to `stopped` that
|
||||
doesn't require a cohort proof. Otherwise W3 blocks the key
|
||||
(`unsafe-replacement`). Spell out that path and its proof, which is that
|
||||
no scope ever existed.
|
||||
- **Dedup across a restart departs from CHAT-01.** Line 145: "An exact
|
||||
reconnect retry returns the existing receipt". CHAT-01C line 212 says
|
||||
the same for recovery. `receipt-unknown` after a restart never replays,
|
||||
so it is safe, but it is a contract deviation. Record it for Sage to rule
|
||||
on, like the CHAT-02 `newer` deviation, or make it a C item. Don't leave
|
||||
it as only a "choice".
|
||||
|
||||
## 3. Your four questions
|
||||
|
||||
1. **Carry-forwards.** Rows 3 to 6 are settled with testable evidence.
|
||||
Row 1 rests on B1 and B2, and row 2 needs the Pi-only outcome (B5). The
|
||||
six rows match lead decision 22's six requirements.
|
||||
2. **Files owned.** It is exact and clear of A1 (the manifest's 20
|
||||
paths), A2, Piece B and `packages/ledger`. Queue D
|
||||
(`packages/queue/src/review.mjs`) and E (`packages/ledger/src/
|
||||
queue-checks.mjs`) are clear too. One correction: the A2 row cites §8.1
|
||||
for `scripts/test-{darkwing,rocko}-launch.mjs`. Those are at plan lines
|
||||
130 and 776-777, and §8.1 predates the A1/A2 split. The overlap result
|
||||
stands.
|
||||
3. **The C split.** C-1 to C-3 are the right kinds of item, but the list is
|
||||
incomplete (B3, B6). C-1's question changes after B1. Naming: CHAT-01
|
||||
calls the dialog companion "CHAT-03D", and the brief's Claude increment
|
||||
is "3D". Rename one of them; a reviewer will confuse them.
|
||||
4. **The increment order** doesn't close as written (B5). Closing
|
||||
Pi-only is clear in intent. It needs a trigger and an owner.
|
||||
|
||||
## 4. Nonblocking
|
||||
|
||||
- **N1. Skills are live on repository seats.** §4 says the seats
|
||||
"disable skills and templates" (`agent-host-dev.sh:135-136`). But
|
||||
`--no-skills` only stops discovery. Line 138 passes ten explicit
|
||||
`--skill` paths (lines 77-83), and Pi's `docs/skills.md:42` says
|
||||
explicit paths still load. So `/skill:<name>` expands. The leading-`/`
|
||||
refusal covers it. The S2 prefix list must include it, and the sentence
|
||||
needs fixing. Templates are off.
|
||||
- **N2. Refusal names.** `stale-generation` and `not-controller`
|
||||
duplicate the existing `generation` (`check.mjs:205`) and `controller`
|
||||
(`check.mjs:230`). Reuse those. Line 363 describes the existing names
|
||||
as unregistered; it doesn't grant new ones. Keep the genuinely new names
|
||||
(`busy`, `receipt-unknown`, `engine-pin-mismatch`, `foreign-writer`),
|
||||
listed in the README.
|
||||
- **N3. The cohortProof list is short.** CHAT-01 lines 327-331 also bind
|
||||
the verification digest and the stop, authority and membership epoch.
|
||||
"An empty cgroup" is the brief's addition, which is fine. Also say how
|
||||
"empty" is observed once systemd collects the transient scope, since an
|
||||
absent cgroup and an empty one are different observations.
|
||||
- **N4. Imprecise citations:**
|
||||
- CHAT-01 325 says SIGTERM and the rest don't prove death, not that
|
||||
they don't release a claim, and `agent_settled` is CHAT-00 65;
|
||||
- CHAT-01 153-160 has no "one dispatch lock" and no text policy
|
||||
(serialization is line 133, the text policy 181-183);
|
||||
- plan 179-180 says "refuse a second writer until reconciled", with no
|
||||
file watching or marker;
|
||||
- CHAT-01 62-64 never says "registration";
|
||||
- `reader.mjs:24` only declares the constant, and the refusal is at 51
|
||||
and 65;
|
||||
- the goal extension path `.pi/extensions/…` is gitignored; the
|
||||
tracked twin is `extensions/goal/index.ts:230`, byte-identical.
|
||||
- **N5. Wrong citation:** Limit 3 cites CHAT-00 line 195 for B2. B2 is at
|
||||
196-197.
|
||||
- **N6.** `636b0fac` is not a commit. It is the sha256 prefix of
|
||||
`agents/dewey/work/chat-02/BRIEF.md`. Say so.
|
||||
- **N7. Two of B1's sources can't be version-pinned.** C-HEADLESS and C-REF
|
||||
in `sources.json` are floating documents. B1 part 2 should say how they
|
||||
are fixed: fetched copies by hash, with the fetch date.
|
||||
- **N8. "One has already misfired" overstates DEFERRED.** The entry says
|
||||
Pi read the text as plain text that time. It was a concatenation, not
|
||||
an interpretation. Say that.
|
||||
- **N9. Isolate the real-binary smoke explicitly.** Use a scratch `HOME`
|
||||
and Pi agent directory, so the default `~/.pi` auth isn't found. "No auth
|
||||
file" should be a setup condition, not an assumption.
|
||||
- **N10. The suite count.** Lead decision 20 adds `queue` when A1 lands,
|
||||
so §10 should say "every `scripts/test-*.sh` suite at the candidate's
|
||||
base", not "eight".
|
||||
- **N11. Record the host in the claim's incarnation.** A different boot ID
|
||||
proves a stop only on the same host. The claim root is a constructor
|
||||
argument, and CHAT-07 picks its live location. Treat a foreign host as
|
||||
`uncertain`, as the queue lock does (8.4).
|
||||
- **N12.** The request's open point 2 (3C needs Jason's go) is stated
|
||||
well. Open point 1 (C-1 may need Jason) changes after B1.
|
||||
|
||||
## 5. For the re-review
|
||||
|
||||
Send the revision at a fixed hash, with a short change list mapped to
|
||||
B1-B6. I'll re-read the changed sections and re-check any new
|
||||
citations. Rocko's pass is separate; B6 overlaps it, so it's worth
|
||||
comparing notes before the revision.
|
||||
|
||||
No commit or push. Nothing outside this file and `docs/SESSIONS.md` was
|
||||
written.
|
||||
@@ -0,0 +1,203 @@
|
||||
# CHAT-03 brief R2 review (#1507, row 5)
|
||||
|
||||
Filbert, 2026-09-26. Round 1 review: `chat-03-brief-review-2026-09-26.md`
|
||||
(sha256 ec00544e).
|
||||
|
||||
## Verdict
|
||||
|
||||
**Changes requested** on `BRIEF-r2-5c5b45a2.md`, sha256
|
||||
5c5b45a277f3a555337a5caf57ff1b300b95781b69640ec8974770bdd44af9bd, with
|
||||
`REVIEW-REQUEST-r2-c75ad86f.md`.
|
||||
|
||||
R2 closes B2 to B6 and N1 to N12. The B1 rewrite fixes the queue premise,
|
||||
but it rests on a new one that pinned Pi doesn't hold: that runs never
|
||||
overlap. That is F1. F2 is a smaller gap in what `clear_queue` can prove.
|
||||
|
||||
Rocko's R2 (07b938fb) has one blocking finding, and lead decision 25 has
|
||||
ruled on V-1. F1 is not a duplicate of Rocko's finding 1. Both come down to
|
||||
the same rule, though: settle a slot from positive evidence, never from the
|
||||
order of events. One r3 change can answer both.
|
||||
|
||||
`BRIEF.md` is 0644 at 96a3fcf9 as I write this, so r3 is in progress. I
|
||||
reviewed only the frozen r2 copy.
|
||||
|
||||
## What I checked
|
||||
|
||||
- The frozen r2 and the request, each hashed on arrival. Rocko's R2 and
|
||||
lead decision 25.
|
||||
- The pinned Pi files match the §3 table: `agent-session.js` fb8a3981,
|
||||
`session-manager.js` ccace649, `rpc-mode.js` e7e4724a. I also read
|
||||
`pi-agent-core/dist/agent.js` (d84351e4) and `output-guard.js` (e860db94).
|
||||
Neither is pinned in the brief.
|
||||
- `extensions/goal/index.ts` and the copy the seats load,
|
||||
`.pi/extensions/goal/index.ts`, are both 5ccf78ce. The copy is
|
||||
git-ignored (`.pi/.gitignore:1`).
|
||||
- A method-level check: the installed `AgentSession.prototype._runAgentPrompt`
|
||||
on a stub receiver whose agent already has a run active. It rejects with
|
||||
"Agent is already processing a prompt", emits one `agent_settled`, and
|
||||
leaves `isStreaming` false. This is not a real-engine run and made no
|
||||
model call.
|
||||
- Every R1 disposition against the r2 text, and the new citations.
|
||||
|
||||
## Blocking
|
||||
|
||||
**F1. Pi can run an extension turn while a Mosaic prompt is in preflight.**
|
||||
Three places in §3 depend on that never happening:
|
||||
- lines 557–561: "either refused, handled without a run, or run at once";
|
||||
- lines 631–634: `working` is inferred from the first user `message_start`
|
||||
after the ack;
|
||||
- Choices, row 2.
|
||||
|
||||
The `isStreaming` check is at `agent-session.js:860`, and the run starts
|
||||
at 949. Between the two, `prompt()` always awaits `emitBeforeAgentStart`
|
||||
(915). It also awaits `_checkCompaction` (895) after the first turn, and
|
||||
`emitInput` (843) when an input handler exists. While those awaits are
|
||||
pending, an extension can start a run:
|
||||
- `sendCustomMessage` with `triggerTurn` calls `_runAgentPrompt` with no
|
||||
await before it sets the flag (1120–1121);
|
||||
- or an extension's own `prompt()`, which passed its line-860 check before
|
||||
the Mosaic prompt did, reaches 949 first.
|
||||
|
||||
The prompt that loses gets its ack (948). Then `agent.prompt` throws
|
||||
(agent-core `agent.js:228`), and `rpc-mode.js:314–317` swallows the throw
|
||||
because preflight already succeeded. The `finally` at 780–784 then emits
|
||||
`agent_settled` and clears `_isAgentRunActive` while the winner's run goes
|
||||
on. My stub check shows exactly this.
|
||||
|
||||
This doesn't need a contrived extension. The goal extension, which seats
|
||||
load (§4, line 667), calls `pi.sendUserMessage` from its `agent_settled`
|
||||
handler (goal lines 468–483, then 189). Pi dispatches that call without
|
||||
awaiting it (`agent-session.js:2020–2021`). And `_emitAgentSettled` runs
|
||||
extension handlers before it writes the RPC event (348–351). So when the
|
||||
controller reads `agent_settled` and releases the slot, the goal's next
|
||||
prompt is already past line 860. The next Mosaic prompt admitted from that
|
||||
point races with it.
|
||||
|
||||
What can go wrong. These follow from the source and the stub check; I did
|
||||
not run them against a real engine:
|
||||
1. **A dropped item reads as delivered.** The wire shows the Mosaic ack,
|
||||
then the goal run's `agent_start` and user `message_start`, then the
|
||||
spurious `agent_settled` from the Mosaic prompt that lost. The
|
||||
controller marks the item `working` at the goal's message and settles
|
||||
the slot on the spurious event. Pi discarded the text. The receipt says
|
||||
it ran.
|
||||
2. **An item that ran reads as `failed`.** Here the goal prompt is the one
|
||||
that loses, and its spurious `agent_settled` can reach the wire before
|
||||
the Mosaic run's user `message_start`. That looks like row 4 of the slot
|
||||
table, so the item gets `failed ack-without-start`, and the client keeps
|
||||
the text "for an explicit resend" (N8's wording). The text then runs a
|
||||
second time.
|
||||
3. **`get_state` reports idle during a live run.** After a spurious settle,
|
||||
`isStreaming` stays false until the real run ends. None of these can
|
||||
trust it then: rule 5's "get_state shows no run", row 3's
|
||||
handled-without-run test, the idle drift check, or the controller's own
|
||||
picture of whether the engine is busy. A second Mosaic prompt passes
|
||||
line 860 and fails the same way.
|
||||
|
||||
Pi carries no run ID, so the controller can't tell these orders apart from
|
||||
events alone. Required:
|
||||
- Correct the premise in the three places above. The one-run guarantee
|
||||
holds only when no loaded extension starts turns.
|
||||
- Make every slot outcome that rests on event order conservative, since an
|
||||
overlap can't be ruled out:
|
||||
- Row 4 becomes `delivery-unknown` (reason `ack-without-start`), not
|
||||
`failed`. `failed` stays for a native error response (rows 1 and N8),
|
||||
and N11 changes with it.
|
||||
- `working` needs more than "the first user `message_start` after the
|
||||
ack". Without proof, the item stays `delivery-unknown`.
|
||||
- An `agent_settled` that arrives while a run the controller saw start
|
||||
has no `agent_end` is evidence of overlap, not of idle. So is an
|
||||
`agent_start` with no slot held. Either one closes admission and makes
|
||||
the binding `uncertain`.
|
||||
- Or state no turn-starting extensions as a precondition for binding, and
|
||||
record it as a Limit that CHAT-06 must resolve for seats that load
|
||||
`goal`. CHAT-03 is fixture-only, so this is cheap now. The conservative
|
||||
rules above are still needed for when the precondition fails.
|
||||
- The fake engine (N10) must model this: awaits in preflight, and a
|
||||
colliding prompt that is acked, has its throw swallowed, settles without
|
||||
an `agent_start`, and leaves `isStreaming` false while the other run
|
||||
continues. A fake without it proves a false premise, which is how R1's
|
||||
B1 happened.
|
||||
- Fixtures:
|
||||
- a goal-like fixture extension that starts a turn from its
|
||||
`agent_settled` handler while a Mosaic prompt is admitted, in both
|
||||
orders (Mosaic wins, extension wins);
|
||||
- an extension run started by a timer during Mosaic preflight;
|
||||
- a mutant that settles or attributes by order alone, which those
|
||||
fixtures must fail.
|
||||
|
||||
**F2. An empty `clear_queue` doesn't prove that no external input was
|
||||
removed.** `clearQueue()` (1195–1203) returns only `_steeringMessages` and
|
||||
`_followUpMessages`, the text queued through `prompt`, `steer` and
|
||||
`follow_up`. An extension's `sendMessage` while streaming goes straight to
|
||||
`agent.steer` or `agent.followUp` (1112–1118). `clearAllQueues` (1200)
|
||||
drops those messages, but the return value doesn't include them.
|
||||
`get_state`'s pending count (1206) has the same blind spot. So rule 3's
|
||||
"a non-empty clear removed external input" catches only some external
|
||||
input. After an empty return, `reconciled` can be published with removed
|
||||
custom messages that were never recorded, and those are the items C-1
|
||||
exists to record.
|
||||
|
||||
Also, `_pendingNextTurnMessages` (1110) survives both clear and abort. It
|
||||
attaches to the next Mosaic prompt (910–913), so the first turn after a
|
||||
stop carries extension context queued before the stop.
|
||||
|
||||
Required, as text only:
|
||||
- Narrow rule 3 and C-1's premise: pinned Pi can't observe removed custom
|
||||
messages.
|
||||
- Add that gap and the nextTurn carry-over to Limit 7.
|
||||
- Add a fixture where an extension `sendMessage` is queued at Interrupt.
|
||||
Expected: the clear returns empty, the item is gone, and the evidence
|
||||
records the gap as unobservable, not as "nothing removed".
|
||||
|
||||
C-1's author needs to know about this before drafting.
|
||||
|
||||
## R1 dispositions
|
||||
|
||||
| # | R2 status |
|
||||
|---|---|
|
||||
| B1 | Closed as posed: the queue premise is gone, and the ack-then-throw case has a row and N11. Superseded by F1, which changes row 4. |
|
||||
| B2 | Closed. The idle `get_entries` comparison is supported by the pinned source (`session-manager.js` 606–684). See n2 and n3. |
|
||||
| B3 | Closed. C-1 and C-4 are separate items, with a counted interim. F2 narrows C-1's premise. |
|
||||
| B4 | Closed. The guard runs at construction and bind, on real paths; G1–G3 and mutant 14. |
|
||||
| B5 | Closed. "Needs first", the B1-not-passed triggers with Sage as recorder, voiding I2 if C-2 is declined, and a done formula that closes. |
|
||||
| B6 | Closed. Claim ID, pair state, `link()` publication, the `reserved` restart rules, the spawn marker and V-1. Rocko closed the mechanics too. |
|
||||
| N1–N12 | Closed. I spot-checked N1 (S2, line 695), N2 (`check.mjs` 205 and 230), N6 (line 12), N9 (§3 smoke), N10 (line 992) and N11 (W17). |
|
||||
|
||||
## Non-blocking
|
||||
|
||||
- **n1. The persistence basis for row 4 (agrees with Rocko's finding 2).**
|
||||
`rpc-mode.js:28–29` sends every event through `writeRawStdout`. That is
|
||||
one promise chain (`output-guard.js:71`), written asynchronously after
|
||||
the synchronous `appendFileSync`. So "events reached the controller
|
||||
first" is false as a transport claim. The claim that holds is about
|
||||
order: the chain keeps order, so a received `agent_settled` means every
|
||||
earlier event from that process was written first. F1 shows that the
|
||||
settle might belong to a different prompt, though.
|
||||
- **n2. Read the file before `get_entries`.** The drift check reads
|
||||
`get_entries` first, then the file. Any engine append between the two
|
||||
reads then counts as drift. After `agent_settled`, that append can come
|
||||
from the goal's continuation (F1). The failure is fail-closed, but it
|
||||
costs a force stop. Reading the file first lets the existing prefix rule
|
||||
absorb the engine's own appends.
|
||||
- **n3. The engine writes the session file during load.** It adds a newline
|
||||
after a truncated last line (319), writes a header into an empty file
|
||||
(632–633), and rewrites the file on version migration (677). Take the
|
||||
drift baseline after load, and add those three cases to W18. Also, the
|
||||
buffering in `_persist` (739–767) applies only to a new file. A loaded
|
||||
file is flushed at 637, so Pi appends to it at once. §2's "before the
|
||||
first assistant message, Pi keeps entries in memory" should say "for a
|
||||
new session file".
|
||||
- **n4. Citations.**
|
||||
- H17 cites CHAT-01C 92–100. "Single use" is at 101–102, so cite 92–102.
|
||||
- §1's `rpc.md` 56–65 covers the prompt's `streamingBehavior`. The
|
||||
`steer` and `follow_up` commands are at 78–104; cite both.
|
||||
- F1 and F2 depend on three files §3 doesn't pin:
|
||||
`pi-agent-core/dist/agent.js` (d84351e4…),
|
||||
`pi-coding-agent/dist/core/output-guard.js` (e860db94…) and the goal
|
||||
extension (5ccf78ce). Pin them.
|
||||
|
||||
## For the re-review
|
||||
|
||||
I'll re-read §3, the slot table, rule 5, Choices, Limit 7, C-1, N10 and the
|
||||
new fixtures and mutants against F1, F2 and n1 to n4, at a frozen r3 hash.
|
||||
@@ -0,0 +1,143 @@
|
||||
# CHAT-03 brief R3 review (#1507, row 5)
|
||||
|
||||
Filbert, 2026-09-27. Round 2 review: `chat-03-brief-review-r2-2026-09-26.md`
|
||||
(sha256 d149e8cc).
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approved on the sections Gate E keeps.** The review covers `BRIEF.md`
|
||||
sha256 2c5be6b4b2caddf9314e8fdcc9108d744b770fe8f469e9c2d410ee6d14c6b1ec,
|
||||
with `REVIEW-REQUEST.md` e197b882, base f2b9e622. I found no blocking
|
||||
finding. The four notes below don't block.
|
||||
|
||||
Scope follows lead decisions 30 and 31. I skipped the cut sections: §7, the
|
||||
Pi-dialog cases of H5–H8, C-1, C-2 and C-4. The §2 idle drift check now
|
||||
belongs to CHAT-07 and C-5 to CHAT-04, so I skipped those as well. I
|
||||
reviewed the seal on decision 31's terms: `--no-extensions`, no
|
||||
`--extension`, and built-in code only.
|
||||
|
||||
Decisions 30 and 31 postdate this hash. Dewey's cut pass will change the
|
||||
text, but lead decision 27 allows no fourth round. If Sage wants it, I'll
|
||||
check that the post-cut diff only removes the cut sections and applies
|
||||
decision 31. That would be a conformance check, not a review.
|
||||
|
||||
## R2 findings
|
||||
|
||||
| # | R3 status |
|
||||
|---|---|
|
||||
| F1 | Closed. R3-1 states the window at 860–949, the acked loser, the swallowed throw and the spurious settle, all from the source. Goal is off for Console-bound seats (decision 30), and the seal loads no explicit extensions (decision 31). That leaves the Mosaic prompt as the only thing that can start a run. O1–O6 back the seal up. Row 4 is now `delivery-unknown` / `ack-without-start`. The windows where attribution is late or missing are stated honestly (N8, N19–N21, Limits 7 and 11). |
|
||||
| F2 | Closed. Rule 3 now covers only the steer and follow-up queues and names the seal as the basis. Limit 7 names the agent-level and `nextTurn` gaps, and N22 and N23 pin them. C-1 is cut by decision 30. |
|
||||
| n1 | Closed. The ordering note cites `rpc-mode.js` 28–29 and `output-guard.js` 71, and makes no transport claim. |
|
||||
| n2, n3 | Moot for CHAT-03, because the drift check moves to CHAT-07. The W18 text (line 567) and the after-load baseline (line 512) are correct as written, and CHAT-07 can reuse them. |
|
||||
| n4 | Closed. H17 cites 92–102 (line 1029), §1 cites `rpc.md` 56–65 and 78–104, and `agent.js`, `agent-loop.js`, `output-guard.js` and goal are pinned. |
|
||||
|
||||
## What I checked
|
||||
|
||||
All the pinned Pi files match the §3 hashes. I read:
|
||||
- `agent-session.js` 310–400, 770–812, 820–950, 1105–1125, 1158–1210,
|
||||
1222–1234 and 2012–2024;
|
||||
- `rpc-mode.js` 20–40 and 300–335;
|
||||
- `resource-loader.js` 316–318 and 402–458;
|
||||
- `main.js` 439;
|
||||
- `cli/args.js` 130–172 and `docs/usage.md` 220–236;
|
||||
- `pi-agent-core` `agent.js` 200–240 and 330–380, and `agent-loop.js` 40–200;
|
||||
- all of `extensions/llama/index.js`.
|
||||
|
||||
**The seal.**
|
||||
- `--no-extensions` limits the extension paths to the CLI paths in both the
|
||||
pre-trust pass (409–411) and the final pass (316–318).
|
||||
- With no `--extension`, only inline factories remain. At the pinned
|
||||
version that is `builtInExtensions` alone, which is `llama.cpp`
|
||||
(`main.js` 439; `cli.js` passes no options).
|
||||
- Nothing reads an extension source from the environment. `main.js` reads
|
||||
only `PI_OFFLINE`, `PI_SKIP_VERSION_CHECK` and `PI_STARTUP_BENCHMARK`, and
|
||||
`resource-loader.js` reads none.
|
||||
|
||||
**llama is input-silent.** It calls only `pi.registerProvider` (37) and
|
||||
`pi.registerCommand("llama")` (163). The command returns after a notify
|
||||
outside TUI mode (166–168). `prompt()` does run extension commands before
|
||||
its streaming check (828–834), but admission refuses a leading `/` (§4).
|
||||
|
||||
**No other run source under the seal.**
|
||||
- Preflight compaction passes `false` and never continues (891–895).
|
||||
- Retries and compaction continue inside `_runAgentPrompt` (772–810).
|
||||
- `_handlePostAgentRun` continues for queued messages only, and under the
|
||||
seal nothing queues.
|
||||
- The controller sends no other run-starting command.
|
||||
|
||||
**Normal sealed runs raise no O1–O6.** I looked for false positives.
|
||||
- O3: an aborted or failed run still emits `agent_end` before the settle
|
||||
(`agent-loop.js` 124–127; `agent.js` handleRunFailure 349–364).
|
||||
- O4: a failure before the loop starts has no `agent_start`, but it emits a
|
||||
failure message, which O4 exempts.
|
||||
- O6: a continuation adds no user message (`runAgentLoopContinue` 58–70).
|
||||
- Row 3: the ack and the flag set at 949 happen in the same synchronous
|
||||
step, so a `get_state` sent after the ack sees streaming whenever a run
|
||||
follows.
|
||||
|
||||
The one possible false positive is O5. See n2.
|
||||
|
||||
**New fixtures and mutants.** N8, N13 and N19–N24 follow from rules 4 and
|
||||
5. Mutants 34–39 each map to a fixture that kills them. N23 has no mutant,
|
||||
and it doesn't need one: it pins a gap.
|
||||
|
||||
## Non-blocking
|
||||
|
||||
- **n1. Pin what the binary executes.** `node_modules/.bin/pi` runs
|
||||
`dist/bundle/cli.js` (e6d7fcf3…). That file loads
|
||||
`dist/bundle/chunks/chunk-JVUZSMYM.js` (3d8b2dec…), which imports seven
|
||||
more chunks.
|
||||
- The chunk contains agent-session, the resource loader, the built-in
|
||||
list, llama and a bundled copy of `pi-agent-core`.
|
||||
- The files §3 pins are not what `pi` loads: `dist/core/*`,
|
||||
`dist/extensions/*` and `node_modules/@earendil-works/pi-agent-core/dist/*`.
|
||||
- I matched the cited logic in the chunk: the 860 check, `_runAgentPrompt`
|
||||
and its `finally`, the settle order, the "already processing a prompt"
|
||||
guard, `clearQueue`, `triggerTurn`, `nextTurn`, the `noExtensions`
|
||||
branch, the llama TUI check, and the aborted and error `stopReason`
|
||||
paths. So the citations hold.
|
||||
- Decision 31 ties the seal to "built-in code tied to the pinned Pi
|
||||
artifact", and `engine-pin-mismatch` checks the built-in set. Both
|
||||
should check the executed artifact. That is either
|
||||
`package-lock.json`'s integrity for `pi-coding-agent` 0.85.1
|
||||
(`sha512-FGRN+OHb…`) or a manifest of `dist/bundle/**`. Keep the dist
|
||||
pins as reading citations.
|
||||
- `pi-coding-agent` declares `^0.85.1` for `pi-agent-core`. The binary
|
||||
doesn't load that package, so the caret has no effect on the executed
|
||||
code.
|
||||
- **n2. The controller's own clear emits a `queue_update`.**
|
||||
`clearQueue()` emits one before it returns (1201). `rpc-mode.js` 334
|
||||
evaluates it before it writes the response, so the event precedes the
|
||||
`clear_queue` response on the wire.
|
||||
- O5's "a `queue_update` the controller didn't cause" has to count that
|
||||
event as caused. An implementation that marks "caused" only once the
|
||||
response arrives fires O5 on every Interrupt, and every stop then ends
|
||||
`uncertain`.
|
||||
- State the order in §3. Make the N10 fake emit it in that order. Have
|
||||
the real-binary smoke record it on its idle `clear_queue`.
|
||||
- **n3. `aborted` with no stop in progress.** Row 4 maps `aborted` to
|
||||
`failed` with reason `interrupted` and links the stop ID.
|
||||
- Under decisions 30 and 31, only the controller's `abort` can produce
|
||||
`aborted`, so this case is unreachable.
|
||||
- Say that an `aborted` result with no stop in progress is an overlap
|
||||
signal, or leaves the receipt `working` as "outcome unknown". Then the
|
||||
code never has to invent a stop link.
|
||||
- **n4. For Dewey's cut pass (decision 31).** These places still describe
|
||||
the explicit-extension registry:
|
||||
- §3 seal bullets 1, 3 and 4;
|
||||
- N5 and N15 ("a registered input extension");
|
||||
- N24's hash and non-local cases;
|
||||
- mutant 37;
|
||||
- the review-based wording in Limit 11.
|
||||
|
||||
With no explicit extensions, nothing on a sealed engine acks a prompt
|
||||
without running it. Keep row 3 as a conservative row, and run N5 and N15
|
||||
on the fake alone. N24 and mutant 37 become "any `--extension` refuses".
|
||||
|
||||
Limit 11's "none of them could move to the mediated path" is now
|
||||
answered by decision 30 option (a). The Gate's C-1 line, its C-5 line
|
||||
and "CHAT-03 done" change with decision 30.
|
||||
|
||||
Rocko's r3 finding 1 (report 19e3fcff), an import that escapes the hashed
|
||||
tree, is resolved by decision 31, so I haven't repeated it. I agree with his
|
||||
note 2 on the `no-turn` fence cleanup.
|
||||
Reference in New Issue
Block a user