From c23fb4156bd348cadc9e99042df3d2ad0a4181af Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:25:51 -0400 Subject: [PATCH 01/26] chore(porch): 1365 init pir --- .../status.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 codev/projects/1365-serializer-convergence-route-m/status.yaml diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml new file mode 100644 index 000000000..61e983457 --- /dev/null +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -0,0 +1,18 @@ +id: '1365' +title: serializer-convergence-route-m +protocol: pir +phase: plan +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: pending + dev-approval: + status: pending + pr: + status: pending +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-17T23:25:51.772Z' +updated_at: '2026-08-17T23:25:51.773Z' From 2adbe0b9f6e090164e6b40a08efa594435601237 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:31:16 -0400 Subject: [PATCH 02/26] [PIR #1365] Plan draft: evaluation of the three write paths + convergence design Co-Authored-By: Claude Opus 5 (1M context) --- .../1365-serializer-convergence-route-m.md | 346 ++++++++++++++++++ codev/state/pir-1365_thread.md | 46 +++ 2 files changed, 392 insertions(+) create mode 100644 codev/plans/1365-serializer-convergence-route-m.md create mode 100644 codev/state/pir-1365_thread.md diff --git a/codev/plans/1365-serializer-convergence-route-m.md b/codev/plans/1365-serializer-convergence-route-m.md new file mode 100644 index 000000000..386143f9a --- /dev/null +++ b/codev/plans/1365-serializer-convergence-route-m.md @@ -0,0 +1,346 @@ +# PIR Plan: Serializer convergence — route the mailbox write edge through `submitToSession` + +Issue #1365. Refs #1313 / PR #1330 / #1480 (absorbed) / #1481 (interlock, sequenced after this). + +**This plan leads with the evaluation the issue's "Evaluate first" section demands.** The +four failure questions are answered end-to-end below, from the code, *before* any +convergence design. The evaluation's conclusion (converge) is what the `plan-approval` +gate is being asked to ratify; the implementation section is contingent on that ratification. + +--- + +## Part 1 — Evaluation: how the three write paths actually compose + +### The three paths, as they exist today + +| # | Path | Lock it takes | Where | +|---|---|---|---| +| A | Immediate `--interrupt` / `--escape` | **per-terminal** `submitToSession(terminalId, …)` | `tower-routes.ts:1963` (escape), `tower-routes.ts:2023` (interrupt) | +| B | Gated mailbox delivery (every normal send, cron, held-drain, owner notice) | **per-agent** `KeyedSerializer` keyed by `agentKey(workspacePath, toAgent)` | `mailbox-delivery.ts:490-500` → `mailbox-wiring.ts:215` → `writeMessagePaced` (`message-write.ts:133`) | +| C | Delayed `--interrupt` (Spec 1313 maintainer round) | **per-terminal** for the bare `^C` only; the body then rides path B | `tower-routes.ts:1724-1752` | + +A and C take the same lock. B takes a disjoint one. The two lock spaces never intersect, +so **A/C vs B is unserialized**, which is exactly what this issue names. + +Three facts bound the analysis and were verified in code, not assumed: + +1. `PtySession.write()` (`pty-session.ts:549`) is what *both* families call. It emits no + delivery signal — only `handleUserInput` (`pty-session.ts:802`, human keystrokes) emits + `'submit'`. So no write from inside a lock can synchronously re-enter the other lock. +2. `writeMessagePaced` (path B) reports `false` only when a **PTY write is dropped** + (#1198, dead shellper socket). It cannot detect *semantic* loss — bytes the PTY + accepted but the TUI then discarded. +3. The render gate's TOCTOU re-validation (`mailbox-delivery.ts:397`) closes the window + between *classify* and *write start*. It does **not** cover the window from write start + through the trailing Enter (50–130 ms+, longer for multi-line). + +### Q1 — Can a gated delivery interleave *between* a Ctrl+C and the post-interrupt clean prompt? + +**Yes, in both orderings, and the second is worse than the issue's framing suggests.** + +The immediate-interrupt critical section is `^C` → 100 ms settle → text → Enter, all inside +one `submitToSession` acquisition (`tower-routes.ts:2023-2026`). Path B holds none of that +lock, so: + +**Ordering 1 — mailbox write lands inside the interrupt's settle window.** The drainer +classifies clean at t₀, re-validates the ring token, starts `writeMessagePaced`. The +interrupt's `^C` fires at t₀+ε and its own text is scheduled for `^C`+100 ms. The mailbox +text and the interrupt text now occupy the same composer, and one Enter submits the fusion — +the exact `w1a` blob shape Spec 1313 exists to make impossible. Narrow (the `^C` must land +after the token re-check), but real. + +**Ordering 2 — the `^C` lands inside the mailbox delivery's own text→Enter window, and the +row is still marked `delivered`.** The mailbox path writes its text at t₀ and schedules +Enter at t₀+50 ms (short) or +80 ms after the last line (multi-line). A `^C` arriving in +that window clears the composer. At t₀+50 ms the mailbox Enter fires into an empty prompt. +`writeMessagePaced` returns `true` — every byte *was* accepted by the PTY — so +`markDelivered` transitions the row (`mailbox-delivery.ts:463`) and the delivery is +broadcast. + +That is **silent message loss with a false `delivered` audit record**. It is not a +cosmetic garble: the mailbox's whole contract is "a row reads `delivered` iff the message +reached the agent," and this path breaks it in the one direction the design refuses +elsewhere (the `#1198` dropped-write handling exists precisely to avoid marking a row +delivered when bytes did not land). The render gate cannot help — it proved the prompt +empty *before* the write, and nothing re-checks after. + +Note the asymmetry with the interrupt path's own claim-first tradeoff +(`tower-routes.ts:1997-2008`): there, a lost message is a *documented, deliberate* choice +by the operator taking an explicit bypass. Here it is an autonomous background delivery +losing a message it reports as delivered, with no operator present. Same symptom, +different — and unaccepted — provenance. + +### Q2 — Does the delayed-interrupt reshape leave a window where the body lands mid-turn? + +**No.** The reshape (`tower-routes.ts:1715-1753`) fires only the `^C` on the timer; the body +is a persisted `not_before` row that can only ever leave the mailbox through +`deliverAgentMail`, which requires a render-verified empty prompt. There is no code path +from the delayed-interrupt timer to a body write. A mid-turn screen classifies not-clean → +held. The reshape is sound and needs no change. + +Two true-but-benign residuals, worth writing down because #1481 will inherit them: + +- **`^C`→body is not atomic.** The `^C` callback returns `0`, so the terminal lock releases + immediately; `scheduleDrain` (line 1751) then runs the gate. Anything may occupy the + terminal in between. This is *correct by design* — the gate re-decides — but it means the + delayed interrupt guarantees "the turn was ended," never "this body is next." +- **The `^C` can be a no-op the caller cannot distinguish from success.** All liveness is + re-checked inside the lock (lines 1733-1735) and a dead/unwritable session simply logs and + drops the nudge. Fine today; a `--interrupt-after` that promises "interrupt *then* this + message" (#1481) will need a stronger statement than the current logs provide. + +Under Ordering 2 from Q1, though, the delayed path is *also* exposed: its `^C` is exactly +the writer that can clear another agent-bound delivery's half-written composer. The delayed +interrupt fires **unattended**, on a timer — so the "a human is standing at this terminal" +argument that makes the immediate path's race acceptable does not hold for it at all. + +### Q3 — Is escalation/held state consistent if an interrupt tears the session mid-write? + +**Bookkeeping stays internally consistent; its correspondence to reality does not.** + +- **Session torn down mid-write** (socket death, not interrupt): `writeMessagePaced` returns + `false` → `hold('no-live-pty')` (`mailbox-delivery.ts:458`), row stays held, escalation + clock keeps running, `onHeldStateChange` unaffected. **Correct.** +- **Interrupt clears the composer mid-write** (Q1 Ordering 2): row → `delivered`, leaves the + held set, `onHeldStateChange` fires, held count drops, any `escalated` flag becomes moot. + Every derived indicator agrees with the DB, and the DB is **wrong**. There is no + detector: nothing after the write re-examines the screen. **This is the inconsistency**, + and it is invisible rather than noisy. +- **Immediate interrupt's own row**: claimed `delivered` before the write, by design + (`tower-routes.ts:1997-2008`). Consistent with its documented tradeoff; unchanged here. +- **Escalation/liveness telemetry**: unaffected either way. `isClassifierStuck` + (`mailbox-delivery.ts:242`) only escalates `no-profile` / `no-region-end` / + `no-composer-marker`, so neither `busy` nor `no-live-pty` holds can false-alarm. + +### Q4 — Should the disjoint-lock boundary stay accepted, or converge? + +**Converge.** The three reasons, in order of weight: + +1. **The failure is silent loss with a false `delivered`, not a garbled composer.** The + issue's own "practical corruption surface is small" framing under-states it: the + accepted-boundary argument covers the *fusion* case (Ordering 1) but not the + *clear-then-Enter-into-nothing* case (Ordering 2), which was not separately reasoned + about when the boundary was accepted. A row that reads `delivered` while the agent never + saw the message is the one outcome Spec 1313's whole architecture is built to exclude. +2. **The "an operator is present" premise is false for the delayed path.** Path C fires on a + timer with nobody watching, and #1481 (`--interrupt-after`) turns that from a rarity into + a routine, *scheduled* co-occurrence of an interrupt and a pending gated delivery to the + same terminal. The boundary gets *more* load-bearing exactly where the workstream is + heading. +3. **The fix is cheap and cycle-free.** The per-terminal lock is acquired as a leaf inside + the per-agent serializer, so the order is always agent → terminal and never the reverse; + fact (1) above (writes emit no delivery signals) rules out re-entrancy. One mechanism, + not two. + +**What convergence must NOT do**, and this is the design's load-bearing constraint: taking +the per-terminal lock only moves the race unless the gate verdict is **re-validated inside +that lock**. A delivery that classifies clean, then waits ~150 ms behind an interrupt that +writes a whole message + Enter, and *then* writes, is strictly worse than today. The +in-lock precheck is not a refinement of the fix — it is the fix. + +Equally, the lock must stay a **leaf around the write only**, never widened to cover the +async classify: `--interrupt` is the human's escape hatch for a wedged agent, and making it +queue behind a gate classification would be a real UX regression for the one action that +must always get through fast. + +**Interlock note for #1481** (`--interrupt-after`): after this change, "interrupt, then +deliver this body" is expressible as *ordered acquisitions of one lock* rather than a race +between two. #1481 should build on that and should **not** re-introduce a body write outside +the gate. The residuals in Q2 (the `^C`→body gap is gate-mediated, not atomic; a no-op `^C` +is only logged) are the two things #1481 must design against explicitly. + +--- + +## Part 2 — Proposed change (contingent on the gate ratifying Part 1) + +Route path B's write edge through `submitToSession`, keyed by the terminal id, as a leaf +inside the existing per-agent serializer, with the gate verdict re-validated inside the lock. + +### Design + +**1. The delivery path learns the terminal id.** `DeliverySession` +(`mailbox-delivery.ts:52`) gains `readonly id: string`. `PtySession` already has it +(`pty-session.ts:118`), so the live binding is free; the four unit-test fakes gain one line +each. Preferred over casting `(session as PtySession).id` in the wiring — the delivery +module must *document* that its write takes a per-terminal lock, not hide it behind a cast. + +**2. The `writeMessage` port gains an in-lock precheck and a typed result.** Today it is +`(session, msg, noEnter) => boolean | Promise`, which cannot express "aborted +before writing anything." New shape: + +```ts +export type WriteResult = + | { status: 'written' } // every byte landed, Enter included + | { status: 'dropped' } // #1198 partial/dropped → hold no-live-pty + | { status: 'aborted'; reason: MailboxReason }; // precheck failed IN-lock → hold, nothing written + +writeMessage( + session: DeliverySession, + formattedMessage: string, + noEnter: boolean, + precheck: () => MailboxReason | null, // null = proceed +): WriteResult | Promise; +``` + +`deliverAgentMail` supplies the precheck, so reason authority stays in the delivery module: + +```ts +() => (!session.writable ? 'no-live-pty' + : ringToken(session, profile) !== tokenBefore ? 'busy' + : null) +``` + +This is the *same* pair of checks the code already runs at `mailbox-delivery.ts:397` and +`:424` — they are now re-run at the write instant, inside the lock, instead of only before +it. The pre-lock checks stay as cheap fast-paths (they avoid a pointless lock acquisition). + +**3. `submitMessagePaced` — the new leaf.** In `message-write.ts` (which then imports +`session-submit.ts`; `session-submit.ts` imports nothing, so no cycle): + +```ts +export async function submitMessagePaced( + session: WritableSession & { id: string }, message: string, noEnter: boolean, + precheck: () => MailboxReason | null, +): Promise +``` + +It acquires `submitToSession(session.id, …)`, runs `precheck()` first thing inside the +callback (bail → return offset `0`, nothing written), otherwise runs the existing tracked +paced write and returns the completion offset so the lock is held through the trailing +Enter. Ordering guarantee is preserved: `submitToSession` registers its `sleep` timer +*after* `write()` returns — i.e. after the Enter's `setTimeout` was registered at the same +offset — so the Enter still executes before the promise resolves, exactly as +`writeMessagePaced` documents (`message-write.ts:124-127`). + +`writeMessagePaced`'s only live caller is `mailbox-wiring.ts:215`; after this it is +test-only. It will be removed and its drop-semantics test (`spec-1313-paced-write-drop`) +re-pointed at `submitMessagePaced`, unless review prefers keeping the unlocked primitive +exported. + +**4. Wiring.** `mailbox-wiring.ts:215` becomes +`writeMessage: (session, msg, noEnter, precheck) => submitMessagePaced(session, msg, noEnter, precheck)`. + +**5. Outcome mapping in `deliverAgentMail`** (replacing the `if (!written)` at line 458): +`written` → `markDelivered` as today; `dropped` → `hold('no-live-pty')` as today; +`aborted` → `hold(reason)`. The `finally { memo?.delete(cacheKey) }` (line 447) stays +unconditional — an aborted write puts no bytes on the wire, but invalidating a verdict we +just proved stale is correct anyway, and keeping it unconditional preserves the +rejection-safety the comment argues for. + +### Lock-order / deadlock safety (must hold, and does) + +- Order is always **per-agent → per-terminal**; nothing acquires them in the reverse order. + Paths A and C take only the per-terminal lock and never enter the per-agent serializer. +- No synchronous re-entry: `PtySession.write()` emits no `'submit'` signal (only + `handleUserInput` does), and `'quiescence'` is emitted from an output timer + (`pty-session.ts:503-508`), so no `scheduleDrain` can be raised from inside a lock's + callback. Even if one were, it is `void`-ed onto a microtask and never awaited. +- Liveness: an interrupt now waits at most one in-flight paced write (~50–130 ms, longer for + a long multi-line body) — acceptable, and the point of the change. It is **not** made to + wait behind a gate classify. + +### Files to change + +- `packages/codev/src/agent-farm/servers/mailbox-delivery.ts` — `DeliverySession.id`; + `WriteResult`; `writeMessage` port signature + doc; precheck construction and outcome + mapping around `:397-:458`. +- `packages/codev/src/agent-farm/servers/message-write.ts` — add `submitMessagePaced`; + retire `writeMessagePaced`. +- `packages/codev/src/agent-farm/servers/mailbox-wiring.ts:215` — bind the new port shape. +- `packages/codev/src/agent-farm/servers/session-submit.ts:42-69` — **rewrite the "Exactly + what it covers" boundary comment**: it now covers gated mailbox deliveries; state the + per-agent → per-terminal order, why the lock is a leaf around the write and not the + classify, and what remains deliberately uncovered (`POST /api/terminals/:id/write`, WS + keystrokes). *Deliverable in either outcome.* +- `packages/codev/src/agent-farm/servers/tower-routes.ts:2017-2022` — replace the "flagged, + not done here" scope paragraph with the converged guarantee. +- `codev/resources/arch.md` §7 item 5 (line ~1801) — **replace the "disjoint lock … accepted, + documented boundary" sentence** with the converged model, plus a short statement of the + delayed-interrupt sequencing (`^C` on the timer, body through the gate, not atomic by + design). *Deliverable in either outcome.* +- Tests: new `packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts`; + fake-session updates in `send-delivery.test.ts`, `send-mailbox-repro.test.ts`, + `cron-delivery.test.ts`, `send-architect-identity.test.ts`; + `spec-1313-paced-write-drop.test.ts` re-pointed. + +No skeleton mirror: this is `packages/codev` source and our own `codev/resources/arch.md`, +not framework template content. + +### Phasing (git commits inside one PR) + +1. **Convergence + unit tests** — the code above, red-to-green on the new race tests. +2. **Boundary model documentation** — `session-submit.ts`, `tower-routes.ts`, `arch.md`, + written against what actually landed. +3. **CMAP consultation** (implementation + tests) and fixes; review doc. + +## Risks & Alternatives Considered + +- **Risk — the lock moves the race instead of closing it.** A delivery that classified clean + before queuing behind an interrupt would write onto a screen the interrupt just changed. + *Mitigation*: the in-lock precheck is mandatory, and is the primary thing the new tests + assert. Without it this change is a regression, and the plan should be rejected if the + precheck is dropped. +- **Risk — interrupt latency.** A `--interrupt` may now wait for an in-flight paced write. + Bounded by one message's pacing; the lock deliberately excludes the classify so the wait + can never be gate-length. +- **Risk — port signature churn breaks fakes.** Four test files. Compile-time, not runtime; + a fake that fails to update fails the build. +- **Risk — `aborted` re-holds a row that would previously have been written.** That is the + intent (it would have been written onto a screen that moved), but it makes `busy` holds + marginally more frequent under contention. The backstop re-delivers within 1.5 s. +- **Alternative — accept the boundary (wontfix).** Rejected on Q1 Ordering 2: a false + `delivered` on a lost message is not a robustness nicety, and #1481 removes the + "operator is present" premise. Had the analysis found only Ordering 1, wontfix would have + been defensible. +- **Alternative — widen `submitToSession` to hold across gate + write.** Rejected: it would + queue the human's escape hatch behind an async classification, and it buys nothing the + in-lock precheck does not. +- **Alternative — keep `boolean` and map an aborted write to `false`.** Simpler, no port + churn, but reports `no-live-pty` for a `busy` abort — a lie in `afx inbox` and in the send + response. Rejected for the typed result; noted as the fallback if review wants minimal + surface. +- **Alternative — post-write verification (re-classify after Enter, re-hold on mismatch).** + Rejected: detect-and-repair is the architecture Spec 1313 explicitly replaced, and a + re-hold risks double delivery. + +## Test Plan + +**Unit** (new `spec-1365-serializer-convergence.test.ts`, fake sessions + injected clock, +recording an ordered write log): + +- A gated delivery and a concurrent immediate-interrupt to the same terminal **never + interleave**: the `^C` appears either wholly before the delivery's text or wholly after + its Enter — never between. (Fails on `main`.) +- **Ordering 2 regression**: an interrupt racing an in-flight delivery no longer leaves the + row `delivered` — the delivery either completes intact or aborts and the row stays `held`. + (Fails on `main`: today the row reads `delivered` with nothing on the agent's screen.) +- **In-lock staleness**: gate returns clean, an interrupt writes while the delivery waits on + the terminal lock → delivery writes **zero** bytes and holds `busy`; `markDelivered` never + runs. +- **In-lock unwritable**: session becomes unwritable while queued → holds `no-live-pty`, + nothing written. +- **Preserved semantics**: dropped write (#1198) still holds `no-live-pty`; two deliveries to + one agent still cannot interleave; the promise still resolves *after* the Enter. +- **Lock hygiene**: after mixed delivery/interrupt/escape traffic settles, + `pendingSubmissionSessions() === 0` and the per-agent serializer is inactive — no leak, no + wedge, all promises settle (deadlock-freedom). +- **Escape** unchanged. + +**Regression suites to re-run**: `spec-1273-submission-lock`, `spec-1273-interrupt`, +`write-queue`, `mailbox`, `send-delivery`, `send-mailbox-repro`, `send.test`, +`spec-1313-paced-write-drop`, `spec-1307-send-delay`, `cron-delivery`, `tower-routes`, +plus a full `pnpm --filter @cluesmith/codev test` and `build`. + +**Manual (for the `dev-approval` gate)** — with a live Tower and two agents: + +1. `afx send ""` and, within the same second, + `afx interrupt ` from a second shell. Repeat ~10×. Expect: never a fused + composer, and **never** a row in `afx inbox`/`afx inbox show` reading `delivered` whose + text did not appear on the agent's screen. +2. `afx send --delay 5 --interrupt ""` against a mid-turn agent: `^C` at due + time, body lands only once the prompt is clean, exactly one copy. +3. `afx interrupt` against a busy agent still responds promptly (no gate-length stall). +4. `afx send --escape` unchanged; a not-writable target still 503s + `TERMINAL_NOT_WRITABLE`. + +**Cross-platform**: none — server-side Node only, no UI surface. diff --git a/codev/state/pir-1365_thread.md b/codev/state/pir-1365_thread.md new file mode 100644 index 000000000..5bf07655c --- /dev/null +++ b/codev/state/pir-1365_thread.md @@ -0,0 +1,46 @@ +# pir-1365 — Serializer convergence (issue #1365) + +## Plan phase (2026-08-17) + +Read the three write paths end to end before proposing anything, per the architect's +instruction that the issue's "Evaluate first" section governs. + +**What I found that changes the framing.** The issue (and the accepted-boundary comment in +`session-submit.ts`) reason about the *fusion* case: a gated delivery landing inside an +interrupt's `^C`→settle→text window. There is a second ordering nobody wrote down: the `^C` +landing inside the **delivery's own** text→Enter window (50–130 ms+). The `^C` clears the +composer, the delivery's Enter fires into nothing, `writeMessagePaced` returns `true` +(the PTY accepted every byte — it only detects #1198 socket drops, not semantic loss), and +`markDelivered` transitions the row. That is silent message loss with a false `delivered` +audit record — the one outcome Spec 1313's architecture exists to exclude. + +That, plus the fact that the delayed-interrupt path fires **unattended** (so the "an +operator is standing at this terminal" premise that makes the boundary acceptable does not +hold for it, and #1481 makes that co-occurrence routine), is why the evaluation lands on +**converge** rather than wontfix. + +**Design constraint I want the reviewer to weigh.** Taking the per-terminal lock *alone* +would make things worse, not better: a delivery that classified clean and then waits ~150 ms +behind an interrupt would write onto a screen the interrupt just changed. So the gate +verdict must be re-validated **inside** the lock (the same `writable` + `ringToken` pair the +code already checks pre-write, re-run at the write instant). The in-lock precheck is the +fix, not a refinement of it. Correspondingly the lock stays a leaf around the *write* only — +widening it to cover the async classify would queue `--interrupt`, the human's escape hatch, +behind a gate classification. + +Verified for deadlock-freedom: lock order is always per-agent → per-terminal (paths A/C +never enter the per-agent serializer), and `PtySession.write()` emits no `'submit'` signal +(only `handleUserInput` does; `'quiescence'` comes off an output timer), so nothing can +re-enter the other lock from inside a lock callback. + +Answers to the other two failure questions: the delayed-interrupt reshape leaves **no** +window for the body to land mid-turn (the body can only leave the mailbox through the gate); +escalation/held bookkeeping stays internally consistent but silently diverges from reality +in the Ordering-2 case, with no detector. + +Plan written to `codev/plans/1365-serializer-convergence-route-m.md`, committed, awaiting +`plan-approval`. Flagged for #1481: the `^C`→body gap is gate-mediated, not atomic, and a +no-op `^C` is only logged — both are things `--interrupt-after` must design against. + +**Standing constraint**: we are not cluesmith/codev maintainers. Never merge the PR; park it +after review and report protocol-complete. From c2192ab6332ba0456eebf76434429be3d5195012 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:31:21 -0400 Subject: [PATCH 03/26] chore(porch): 1365 plan-approval gate-requested --- codev/projects/1365-serializer-convergence-route-m/status.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 61e983457..707bbb83a 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -7,6 +7,7 @@ current_plan_phase: null gates: plan-approval: status: pending + requested_at: '2026-08-17T23:31:21.297Z' dev-approval: status: pending pr: @@ -15,4 +16,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-17T23:25:51.773Z' +updated_at: '2026-08-17T23:31:21.297Z' From a6cdbe272e9e1c9c52801eb4ccb2f6eca4da3ab1 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:44:42 -0400 Subject: [PATCH 04/26] [PIR #1365] Plan revised (rev 2): all 5 blocking review items + interrupt-latency ceiling Co-Authored-By: Claude Opus 5 (1M context) --- .../1365-serializer-convergence-route-m.md | 639 +++++++++++------- codev/state/pir-1365_thread.md | 42 ++ 2 files changed, 451 insertions(+), 230 deletions(-) diff --git a/codev/plans/1365-serializer-convergence-route-m.md b/codev/plans/1365-serializer-convergence-route-m.md index 386143f9a..dde7583b8 100644 --- a/codev/plans/1365-serializer-convergence-route-m.md +++ b/codev/plans/1365-serializer-convergence-route-m.md @@ -1,346 +1,525 @@ # PIR Plan: Serializer convergence — route the mailbox write edge through `submitToSession` -Issue #1365. Refs #1313 / PR #1330 / #1480 (absorbed) / #1481 (interlock, sequenced after this). +Issue #1365. Refs #1313 / PR #1330 / #1480 (absorbed) / #1481 (interlock, sequenced after this) / #1473 (echo-lag residual). + +**Revision 2** — incorporates the architect's 3-way plan review (gemini APPROVE, codex + +claude REQUEST_CHANGES; both REQUEST_CHANGES reviews ratify Part 1). All five blocking items +are addressed in Part 2; item 4 (`--escape`) also changed Part 1, which now treats escape as +a first-class instance of the bug. Changes from revision 1 are summarised at the end. **This plan leads with the evaluation the issue's "Evaluate first" section demands.** The -four failure questions are answered end-to-end below, from the code, *before* any -convergence design. The evaluation's conclusion (converge) is what the `plan-approval` -gate is being asked to ratify; the implementation section is contingent on that ratification. +four failure questions are answered end-to-end from the code, *before* any convergence +design. The evaluation's conclusion (converge) is what the `plan-approval` gate is being +asked to ratify; the implementation section is contingent on that ratification. --- -## Part 1 — Evaluation: how the three write paths actually compose +## Part 1 — Evaluation: how the write paths actually compose -### The three paths, as they exist today +### The paths, as they exist today | # | Path | Lock it takes | Where | |---|---|---|---| -| A | Immediate `--interrupt` / `--escape` | **per-terminal** `submitToSession(terminalId, …)` | `tower-routes.ts:1963` (escape), `tower-routes.ts:2023` (interrupt) | -| B | Gated mailbox delivery (every normal send, cron, held-drain, owner notice) | **per-agent** `KeyedSerializer` keyed by `agentKey(workspacePath, toAgent)` | `mailbox-delivery.ts:490-500` → `mailbox-wiring.ts:215` → `writeMessagePaced` (`message-write.ts:133`) | -| C | Delayed `--interrupt` (Spec 1313 maintainer round) | **per-terminal** for the bare `^C` only; the body then rides path B | `tower-routes.ts:1724-1752` | - -A and C take the same lock. B takes a disjoint one. The two lock spaces never intersect, -so **A/C vs B is unserialized**, which is exactly what this issue names. - -Three facts bound the analysis and were verified in code, not assumed: - -1. `PtySession.write()` (`pty-session.ts:549`) is what *both* families call. It emits no - delivery signal — only `handleUserInput` (`pty-session.ts:802`, human keystrokes) emits - `'submit'`. So no write from inside a lock can synchronously re-enter the other lock. -2. `writeMessagePaced` (path B) reports `false` only when a **PTY write is dropped** - (#1198, dead shellper socket). It cannot detect *semantic* loss — bytes the PTY - accepted but the TUI then discarded. +| A1 | Immediate `--interrupt` | **per-terminal** `submitToSession(terminalId, …)` | `servers/tower-routes.ts:2023` | +| A2 | Immediate `--escape` | **per-terminal** `submitToSession(terminalId, …)` | `servers/tower-routes.ts:1963` | +| B | Gated mailbox delivery (every normal send, cron, held-drain, owner notice) | **per-agent** `KeyedSerializer` keyed by `agentKey(workspacePath, toAgent)` | `servers/mailbox-delivery.ts:490-500` → `servers/mailbox-wiring.ts:215` → `writeMessagePaced` (`servers/message-write.ts:133`) | +| C | Delayed `--interrupt` | **per-terminal** for the bare `^C` only; the body then rides path B | `servers/tower-routes.ts:1724-1752` | + +A1/A2/C take the same lock. B takes a disjoint one. The two lock spaces never intersect, so +**A/C vs B is unserialized** — the boundary this issue names. + +Four facts bound the analysis, each verified in code rather than assumed: + +1. `PtySession.write()` (`terminal/pty-session.ts:549`) is what *both* families call, and it + emits no delivery signal — only `handleUserInput` (`terminal/pty-session.ts:802`, human + keystrokes) emits `'submit'`; `'quiescence'` comes off an output timer + (`terminal/pty-session.ts:503-508`). No write from inside a lock can synchronously + re-enter the other lock. +2. `writeMessagePaced` (path B) returns `false` only when a **PTY write is dropped** (#1198, + dead shellper socket). It cannot detect *semantic* loss — bytes the PTY accepted but the + TUI then discarded. 3. The render gate's TOCTOU re-validation (`mailbox-delivery.ts:397`) closes the window - between *classify* and *write start*. It does **not** cover the window from write start - through the trailing Enter (50–130 ms+, longer for multi-line). + between *classify* and *write start*. Nothing covers write-start through the trailing + Enter — 50 ms (short message) to `(lines−1)×10+80` ms (paced multi-line). +4. `ringToken` is built from `bytesWritten`, a cumulative **output** counter. Input that has + not yet been echoed is invisible to it. This is the echo-lag residual (#1473) and it + bounds what any pre-write check can promise. -### Q1 — Can a gated delivery interleave *between* a Ctrl+C and the post-interrupt clean prompt? +### Q1 — Can a gated delivery interleave between a Ctrl+C and the post-interrupt clean prompt? -**Yes, in both orderings, and the second is worse than the issue's framing suggests.** +**Yes, in both orderings — and `--escape` is a third instance of the same class.** The immediate-interrupt critical section is `^C` → 100 ms settle → text → Enter, all inside one `submitToSession` acquisition (`tower-routes.ts:2023-2026`). Path B holds none of that -lock, so: +lock. -**Ordering 1 — mailbox write lands inside the interrupt's settle window.** The drainer +**Ordering 1 — the mailbox write lands inside the interrupt's settle window.** The drainer classifies clean at t₀, re-validates the ring token, starts `writeMessagePaced`. The -interrupt's `^C` fires at t₀+ε and its own text is scheduled for `^C`+100 ms. The mailbox -text and the interrupt text now occupy the same composer, and one Enter submits the fusion — -the exact `w1a` blob shape Spec 1313 exists to make impossible. Narrow (the `^C` must land -after the token re-check), but real. - -**Ordering 2 — the `^C` lands inside the mailbox delivery's own text→Enter window, and the -row is still marked `delivered`.** The mailbox path writes its text at t₀ and schedules -Enter at t₀+50 ms (short) or +80 ms after the last line (multi-line). A `^C` arriving in -that window clears the composer. At t₀+50 ms the mailbox Enter fires into an empty prompt. -`writeMessagePaced` returns `true` — every byte *was* accepted by the PTY — so -`markDelivered` transitions the row (`mailbox-delivery.ts:463`) and the delivery is -broadcast. - -That is **silent message loss with a false `delivered` audit record**. It is not a -cosmetic garble: the mailbox's whole contract is "a row reads `delivered` iff the message -reached the agent," and this path breaks it in the one direction the design refuses -elsewhere (the `#1198` dropped-write handling exists precisely to avoid marking a row -delivered when bytes did not land). The render gate cannot help — it proved the prompt -empty *before* the write, and nothing re-checks after. +interrupt's `^C` fires at t₀+ε and its own text is scheduled for `^C`+100 ms. Both bodies now +occupy one composer and a single Enter submits the fusion — the `w1a` blob shape Spec 1313 +exists to make impossible. Narrow (the `^C` must land after the token re-check), but real. + +**Ordering 2 — the `^C` lands inside the delivery's own text→Enter window, and the row is +still marked `delivered`.** The delivery writes text at t₀ and schedules Enter at t₀+50 ms +(short) or +80 ms after the last line. A `^C` in that window clears the composer; at t₀+50 ms +the delivery's Enter fires into an empty prompt. `writeMessagePaced` returns `true` — every +byte *was* accepted by the PTY (fact 2) — so `markDelivered` transitions the row +(`mailbox-delivery.ts:463`) and the delivery is broadcast. + +That is **silent message loss with a false `delivered` audit record**, the one outcome Spec +1313's architecture exists to exclude. The gate cannot help: it proved the prompt empty +*before* the write, and nothing re-checks after. + +**Ordering 3 — `--escape` truncates an in-flight multi-line delivery** (raised by claude in +review; folded in here as first-class). `writeEscapeToSession` +(`servers/message-write.ts:53`) writes a bare ESC, then Enter 50 ms later. A multi-line +delivery writes its lines 10 ms apart, so an ESC arriving mid-sequence discards what has been +typed so far, the delivery's remaining lines land on the now-cleared composer, and either +Enter (escape's at +50 ms, or the delivery's own) submits a **truncated body**. The row is +marked `delivered`. Escape is not a milder cousin of interrupt here — for multi-line +deliveries it is the more likely trigger, because the delivery's exposed window is longest +exactly when the body is long. Note the asymmetry with the interrupt path's own claim-first tradeoff -(`tower-routes.ts:1997-2008`): there, a lost message is a *documented, deliberate* choice -by the operator taking an explicit bypass. Here it is an autonomous background delivery -losing a message it reports as delivered, with no operator present. Same symptom, -different — and unaccepted — provenance. +(`tower-routes.ts:1997-2008`): there, a lost message is a *documented, deliberate* choice by +an operator taking an explicit bypass. Here it is an autonomous background delivery losing a +message it reports as delivered, with no operator present. ### Q2 — Does the delayed-interrupt reshape leave a window where the body lands mid-turn? **No.** The reshape (`tower-routes.ts:1715-1753`) fires only the `^C` on the timer; the body -is a persisted `not_before` row that can only ever leave the mailbox through -`deliverAgentMail`, which requires a render-verified empty prompt. There is no code path -from the delayed-interrupt timer to a body write. A mid-turn screen classifies not-clean → -held. The reshape is sound and needs no change. +is a persisted `not_before` row that can leave the mailbox only through `deliverAgentMail`, +which requires a render-verified empty prompt. There is no code path from the timer to a body +write. A mid-turn screen classifies not-clean → held. The reshape is sound and needs no +change. -Two true-but-benign residuals, worth writing down because #1481 will inherit them: +Two true-but-benign residuals, recorded because #1481 inherits them: - **`^C`→body is not atomic.** The `^C` callback returns `0`, so the terminal lock releases immediately; `scheduleDrain` (line 1751) then runs the gate. Anything may occupy the - terminal in between. This is *correct by design* — the gate re-decides — but it means the - delayed interrupt guarantees "the turn was ended," never "this body is next." -- **The `^C` can be a no-op the caller cannot distinguish from success.** All liveness is - re-checked inside the lock (lines 1733-1735) and a dead/unwritable session simply logs and - drops the nudge. Fine today; a `--interrupt-after` that promises "interrupt *then* this - message" (#1481) will need a stronger statement than the current logs provide. - -Under Ordering 2 from Q1, though, the delayed path is *also* exposed: its `^C` is exactly -the writer that can clear another agent-bound delivery's half-written composer. The delayed -interrupt fires **unattended**, on a timer — so the "a human is standing at this terminal" -argument that makes the immediate path's race acceptable does not hold for it at all. + terminal in between. Correct by design — the gate re-decides — but the delayed interrupt + guarantees "the turn was ended," never "this body is next." +- **A no-op `^C` is indistinguishable from success to the caller.** Liveness is re-checked + inside the lock (lines 1733-1735); a dead/unwritable session logs and drops the nudge. + +Under Ordering 2, the delayed path is *also* an exposed writer: its `^C` can clear another +delivery's half-written composer. It fires **unattended**, on a timer — so the "a human is +standing at this terminal" premise that makes the immediate path's race acceptable does not +apply to it at all. ### Q3 — Is escalation/held state consistent if an interrupt tears the session mid-write? **Bookkeeping stays internally consistent; its correspondence to reality does not.** - **Session torn down mid-write** (socket death, not interrupt): `writeMessagePaced` returns - `false` → `hold('no-live-pty')` (`mailbox-delivery.ts:458`), row stays held, escalation - clock keeps running, `onHeldStateChange` unaffected. **Correct.** -- **Interrupt clears the composer mid-write** (Q1 Ordering 2): row → `delivered`, leaves the - held set, `onHeldStateChange` fires, held count drops, any `escalated` flag becomes moot. - Every derived indicator agrees with the DB, and the DB is **wrong**. There is no - detector: nothing after the write re-examines the screen. **This is the inconsistency**, - and it is invisible rather than noisy. -- **Immediate interrupt's own row**: claimed `delivered` before the write, by design - (`tower-routes.ts:1997-2008`). Consistent with its documented tradeoff; unchanged here. + `false` → `hold('no-live-pty')` (`mailbox-delivery.ts:458`); row stays held, escalation + clock keeps running. **Correct.** +- **Interrupt clears the composer mid-write** (Ordering 2) **or escape truncates it** + (Ordering 3): row → `delivered`, leaves the held set, `onHeldStateChange` fires, held count + drops, any `escalated` flag becomes moot. Every derived indicator agrees with the DB, and + the DB is **wrong**. Nothing after the write re-examines the screen, so there is no + detector. **This is the inconsistency** — invisible rather than noisy. Ordering 3 is the + worse variant: the agent receives a *partial* message, so the failure can propagate as + acted-upon-but-wrong rather than merely absent. +- **Immediate interrupt's own row**: claimed `delivered` before the write by design + (`tower-routes.ts:1997-2008`) — consistent with its documented tradeoff; unchanged here. - **Escalation/liveness telemetry**: unaffected either way. `isClassifierStuck` - (`mailbox-delivery.ts:242`) only escalates `no-profile` / `no-region-end` / + (`mailbox-delivery.ts:242`) escalates only `no-profile` / `no-region-end` / `no-composer-marker`, so neither `busy` nor `no-live-pty` holds can false-alarm. ### Q4 — Should the disjoint-lock boundary stay accepted, or converge? -**Converge.** The three reasons, in order of weight: +**Converge.** In order of weight: -1. **The failure is silent loss with a false `delivered`, not a garbled composer.** The - issue's own "practical corruption surface is small" framing under-states it: the - accepted-boundary argument covers the *fusion* case (Ordering 1) but not the - *clear-then-Enter-into-nothing* case (Ordering 2), which was not separately reasoned - about when the boundary was accepted. A row that reads `delivered` while the agent never - saw the message is the one outcome Spec 1313's whole architecture is built to exclude. +1. **The failure is silent loss (or silent truncation) with a false `delivered`, not a + garbled composer.** The accepted-boundary argument covers the fusion case (Ordering 1) but + not Orderings 2 and 3, which were not separately reasoned about when the boundary was + accepted. 2. **The "an operator is present" premise is false for the delayed path.** Path C fires on a timer with nobody watching, and #1481 (`--interrupt-after`) turns that from a rarity into a routine, *scheduled* co-occurrence of an interrupt and a pending gated delivery to the - same terminal. The boundary gets *more* load-bearing exactly where the workstream is - heading. -3. **The fix is cheap and cycle-free.** The per-terminal lock is acquired as a leaf inside - the per-agent serializer, so the order is always agent → terminal and never the reverse; - fact (1) above (writes emit no delivery signals) rules out re-entrancy. One mechanism, - not two. - -**What convergence must NOT do**, and this is the design's load-bearing constraint: taking -the per-terminal lock only moves the race unless the gate verdict is **re-validated inside -that lock**. A delivery that classifies clean, then waits ~150 ms behind an interrupt that -writes a whole message + Enter, and *then* writes, is strictly worse than today. The -in-lock precheck is not a refinement of the fix — it is the fix. - -Equally, the lock must stay a **leaf around the write only**, never widened to cover the -async classify: `--interrupt` is the human's escape hatch for a wedged agent, and making it -queue behind a gate classification would be a real UX regression for the one action that -must always get through fast. + same terminal. The boundary gets more load-bearing exactly where the workstream is heading. +3. **The fix is cheap and cycle-free.** The per-terminal lock is acquired as a leaf inside the + per-agent serializer, so the order is always agent → terminal, never the reverse; fact 1 + rules out re-entrancy. + +**What convergence buys, stated precisely** (revised per review item 3 — revision 1 overclaimed +here): + +> **Serialization is the structural guarantee.** After this change, no lock-taking writer — +> gated delivery, `--interrupt`, `--escape`, delayed `^C` — can put bytes on a terminal while +> another lock-taking writer's submission is in flight. That is what closes Orderings 1, 2 +> and 3, and it is a property of the lock, not of any check. +> +> **The in-lock precheck narrows, but does not close, the echo-lag residual.** Re-validating +> the gate verdict inside the lock is *necessary* — without it, a delivery that classified +> clean and then waited behind an interrupt would write onto a screen the interrupt just +> changed, which is worse than today. But `ringToken` tracks output (fact 4), so input from a +> writer that does **not** take the lock — the raw `POST /api/terminals/:id/write` +> passthrough, and human keystrokes over the WebSocket — can sit on the line un-echoed and +> defeat it. Those writers stay deliberately uncovered (a human owns their own composer), so +> that residual survives this change by design. It is **#1473's territory**, and the boundary +> comment must say so rather than implying the race is gone. + +The lock must also stay a **leaf around the write only**, never widened to cover the async +classify: `--interrupt` is the human's escape hatch for a wedged agent, and making it queue +behind a gate classification would be a real regression for the one action that must always +get through fast. **Interlock note for #1481** (`--interrupt-after`): after this change, "interrupt, then deliver this body" is expressible as *ordered acquisitions of one lock* rather than a race -between two. #1481 should build on that and should **not** re-introduce a body write outside -the gate. The residuals in Q2 (the `^C`→body gap is gate-mediated, not atomic; a no-op `^C` -is only logged) are the two things #1481 must design against explicitly. +between two. #1481 should build on that and must not re-introduce a body write outside the +gate. The Q2 residuals (the `^C`→body gap is gate-mediated, not atomic; a no-op `^C` is only +logged) are the two things it must design against explicitly. --- ## Part 2 — Proposed change (contingent on the gate ratifying Part 1) Route path B's write edge through `submitToSession`, keyed by the terminal id, as a leaf -inside the existing per-agent serializer, with the gate verdict re-validated inside the lock. +inside the existing per-agent serializer, with the delivery's preconditions re-validated +inside the lock — and with the delivery side using a **non-blocking** acquisition so the +drainer can never stall. -### Design +### D1. The delivery path learns the terminal id, and the id is guarded at runtime -**1. The delivery path learns the terminal id.** `DeliverySession` -(`mailbox-delivery.ts:52`) gains `readonly id: string`. `PtySession` already has it -(`pty-session.ts:118`), so the live binding is free; the four unit-test fakes gain one line -each. Preferred over casting `(session as PtySession).id` in the wiring — the delivery -module must *document* that its write takes a per-terminal lock, not hide it behind a cast. +`DeliverySession` (`mailbox-delivery.ts:52`) gains `readonly id: string`. `PtySession` +already has it (`terminal/pty-session.ts:118`), so the live binding is free. -**2. The `writeMessage` port gains an in-lock precheck and a typed result.** Today it is -`(session, msg, noEnter) => boolean | Promise`, which cannot express "aborted -before writing anything." New shape: +**Runtime guard (review item 5).** A missing id must not silently become a global lock. +`tower-routes.test.ts:221`'s `gateSession()` is an un-annotated object literal with **no +`id`** that reaches the *real* `mailbox-wiring` binding — verified. Structural typing means +such a fake can compile while keying every lock on `undefined`, collapsing per-terminal +serialization into one global lock without a single failing assertion. So `submitMessagePaced` +throws on a non-string/empty id. A throw there propagates out of `writeMessage`, past the +`finally` that invalidates the memo, through the per-agent serializer to the drainer's +per-agent `try/catch` (`mailbox-delivery.ts:661`) — logged, row stays **held**, never marked +delivered. Fail-loud and fail-safe. `gateSession()` gains a real `id` and is added to the +change list. + +### D2. Asymmetric acquisition — deliveries try, operators block (review item 2) + +The review is right that a symmetric blocking lock is a liveness regression: +`MailboxDrainer.tick` awaits agents **sequentially** (`mailbox-delivery.ts:644-664`), so one +agent blocked on a terminal lock stalls every other agent's delivery, plus that tick's +escalation, owner-notice and prune passes. + +`session-submit.ts` therefore gains a non-blocking sibling: ```ts +/** True while a submission is in flight for this session. */ +export function isSubmissionInFlight(sessionId: string): boolean; + +/** submitToSession, but abandons instead of queueing when the session is contended. + * Resolves `false` (nothing written) if another submission holds the session. */ +export function trySubmitToSession(sessionId, write, clock?): Promise; +``` + +The check-then-install is race-free without ceremony: JS is single-threaded and `chains.has` +→ `chains.set` has no await between them, so no second caller can interleave. + +- **Delivery (path B) uses `trySubmitToSession`.** Contended → write nothing, return + `aborted:'busy'`, row stays held, backstop retries in ≤1.5 s. This costs nothing real: a + contended terminal means another writer is mid-submission, so the in-lock precheck would + have aborted the delivery anyway. The drainer never blocks, so head-of-line blocking is + gone — a *stronger* liveness property than today, where the drainer awaits a full paced + write per agent. +- **`--interrupt` / `--escape` / delayed `^C` keep `submitToSession`** and block. They are + operator actions that must land. + +Starvation is not a concern in the other direction: interrupts are human-rate, deliveries +retry every 1.5 s. + +### D3. Interrupt latency has a new worst case, and needs a ceiling (new finding) + +Not in the review, and it changes the shape of D2's operator side. Today `--interrupt` never +waits. After convergence it waits for any in-flight delivery write, whose duration is +`(lines−1)×10+80` ms — and body size is bounded only by `parseJsonBody`'s **1 MiB** default +(`agent-farm/utils/server-utils.ts:47`). A 48 KB `--file` attachment of short lines is ~48k +lines ≈ **8 minutes**; a realistic 500-line paste is ~5 s. So "the escape hatch stalls behind +a long message" is an ordinary case, not a pathological one, and an unbounded block on +`afx interrupt` would be a worse regression than the bug being fixed. + +**Remedy: a bounded wait with explicit degradation.** `submitToSession` gains an optional +`waitCeilingMs` (default off; set for the interrupt/escape call sites, proposed **2000 ms**). +If the lock is not acquired within the ceiling, the operator write proceeds **unserialized** +and logs loudly at WARN with the session id and the wait. + +This is strictly better than today at every point: below the ceiling we get the full +guarantee; at or above it we degrade to exactly today's unserialized behaviour, which is the +status quo — never worse — and we now say so in the log instead of never knowing. The +alternative (unbounded block) trades a rare silent corruption for a routine visible hang on +the one action that exists to rescue a wedged agent. + +The ceiling value is a judgment call and is flagged for the reviewer. Consider it settled +only if the gate says so; the fallback is unbounded blocking plus a documented latency +tradeoff. + +### D4. The in-lock precheck (including the row-status re-check, review item 1) + +The `writeMessage` port cannot express "aborted before writing anything," so it changes shape: + +```ts +/** Why a gated write abandoned inside the lock. */ +export type WriteAbort = + | { kind: 'hold'; reason: MailboxReason } // re-hold: busy (screen moved / contended) or no-live-pty + | { kind: 'row-resolved' }; // dismissed/superseded under us — terminal state, no hold + export type WriteResult = - | { status: 'written' } // every byte landed, Enter included - | { status: 'dropped' } // #1198 partial/dropped → hold no-live-pty - | { status: 'aborted'; reason: MailboxReason }; // precheck failed IN-lock → hold, nothing written + | { status: 'written' } // every byte landed, Enter included + | { status: 'dropped' } // #1198 partial/dropped → hold no-live-pty + | { status: 'aborted'; abort: WriteAbort }; writeMessage( session: DeliverySession, formattedMessage: string, noEnter: boolean, - precheck: () => MailboxReason | null, // null = proceed + precheck: () => WriteAbort | null, // null = proceed; runs INSIDE the lock ): WriteResult | Promise; ``` -`deliverAgentMail` supplies the precheck, so reason authority stays in the delivery module: +`deliverAgentMail` supplies the precheck, so all reason authority stays in the delivery +module. It re-runs, at the write instant, the three checks the code already performs before +the lock: ```ts -() => (!session.writable ? 'no-live-pty' - : ringToken(session, profile) !== tokenBefore ? 'busy' - : null) +() => { + if (!session.writable) return { kind: 'hold', reason: 'no-live-pty' }; // mirrors :424 + if (ringToken(session, profile) !== tokenBefore) // mirrors :397 + return { kind: 'hold', reason: 'busy' }; + const now = getById(db, row.id); // mirrors :411 — review item 1 + if (!now || now.status !== 'held') return { kind: 'row-resolved' }; + return null; +} ``` -This is the *same* pair of checks the code already runs at `mailbox-delivery.ts:397` and -`:424` — they are now re-run at the write instant, inside the lock, instead of only before -it. The pre-lock checks stay as cheap fast-paths (they avoid a pointless lock acquisition). +The row-status re-check is load-bearing, not defensive tidiness: without it, this change +would **widen** the dismiss→bytes-on-wire window from ~zero to the whole lock wait. Dismiss +and supersede are independent synchronous DB writes not routed through the delivery +serializer (as `mailbox-delivery.ts:404-410` already explains), and `better-sqlite3` is +synchronous, so this re-read sees anything committed up to the write instant. The pre-lock +checks stay as cheap fast-paths that avoid a pointless acquisition. + +Outcome mapping in `deliverAgentMail`, replacing `if (!written)` at `:458`: + +| result | action | +|---|---| +| `written` | `markDelivered` + broadcast, as today | +| `dropped` | `hold('no-live-pty')`, as today | +| `aborted / hold` | `hold(reason)` | +| `aborted / row-resolved` | `ports.onHeldStateChange()` + `{ delivered: [], reason: null }` — exactly the existing `:411-416` branch | -**3. `submitMessagePaced` — the new leaf.** In `message-write.ts` (which then imports -`session-submit.ts`; `session-submit.ts` imports nothing, so no cycle): +The unconditional `finally { memo?.delete(cacheKey) }` (`:447`) stays. An aborted write puts +no bytes on the wire, but discarding a verdict we just proved stale is right anyway, and +keeping it unconditional preserves the rejection-safety its comment argues for. + +### D5. `submitMessagePaced` — the new leaf + +In `message-write.ts` (which then imports `session-submit.ts`; `session-submit.ts` imports +nothing, so no cycle): ```ts export async function submitMessagePaced( session: WritableSession & { id: string }, message: string, noEnter: boolean, - precheck: () => MailboxReason | null, + precheck: () => WriteAbort | null, ): Promise ``` -It acquires `submitToSession(session.id, …)`, runs `precheck()` first thing inside the -callback (bail → return offset `0`, nothing written), otherwise runs the existing tracked -paced write and returns the completion offset so the lock is held through the trailing -Enter. Ordering guarantee is preserved: `submitToSession` registers its `sleep` timer +It guards the id (D1), acquires via `trySubmitToSession` (D2) — contended → return +`aborted:{kind:'hold',reason:'busy'}` — runs `precheck()` first thing inside the callback +(abort → return offset `0`, nothing written), otherwise runs the existing tracked paced write +and returns its completion offset so the lock is held through the trailing Enter. + +Enter-before-resolve ordering is preserved: `submitToSession` registers its `sleep` timer *after* `write()` returns — i.e. after the Enter's `setTimeout` was registered at the same -offset — so the Enter still executes before the promise resolves, exactly as -`writeMessagePaced` documents (`message-write.ts:124-127`). +offset — so the Enter still executes first, exactly as `writeMessagePaced` documents +(`message-write.ts:124-127`). This is asserted with fake timers (see Test Plan). `writeMessagePaced`'s only live caller is `mailbox-wiring.ts:215`; after this it is -test-only. It will be removed and its drop-semantics test (`spec-1313-paced-write-drop`) -re-pointed at `submitMessagePaced`, unless review prefers keeping the unlocked primitive -exported. +test-only. It will be removed and `spec-1313-paced-write-drop.test.ts` re-pointed at +`submitMessagePaced`, unless review prefers keeping the unlocked primitive exported. -**4. Wiring.** `mailbox-wiring.ts:215` becomes -`writeMessage: (session, msg, noEnter, precheck) => submitMessagePaced(session, msg, noEnter, precheck)`. +### D6. Wiring -**5. Outcome mapping in `deliverAgentMail`** (replacing the `if (!written)` at line 458): -`written` → `markDelivered` as today; `dropped` → `hold('no-live-pty')` as today; -`aborted` → `hold(reason)`. The `finally { memo?.delete(cacheKey) }` (line 447) stays -unconditional — an aborted write puts no bytes on the wire, but invalidating a verdict we -just proved stale is correct anyway, and keeping it unconditional preserves the -rejection-safety the comment argues for. +`mailbox-wiring.ts:215` becomes +`writeMessage: (session, msg, noEnter, precheck) => submitMessagePaced(session, msg, noEnter, precheck)`. ### Lock-order / deadlock safety (must hold, and does) -- Order is always **per-agent → per-terminal**; nothing acquires them in the reverse order. - Paths A and C take only the per-terminal lock and never enter the per-agent serializer. -- No synchronous re-entry: `PtySession.write()` emits no `'submit'` signal (only - `handleUserInput` does), and `'quiescence'` is emitted from an output timer - (`pty-session.ts:503-508`), so no `scheduleDrain` can be raised from inside a lock's - callback. Even if one were, it is `void`-ed onto a microtask and never awaited. -- Liveness: an interrupt now waits at most one in-flight paced write (~50–130 ms, longer for - a long multi-line body) — acceptable, and the point of the change. It is **not** made to - wait behind a gate classify. +- Order is always **per-agent → per-terminal**; nothing acquires them in reverse. Paths + A1/A2/C take only the per-terminal lock and never enter the per-agent serializer. +- No synchronous re-entry (fact 1). Even if a `scheduleDrain` were raised from inside a lock + callback it is `void`-ed onto a microtask and never awaited. +- With D2 the delivery side never blocks at all, so the only wait in the system is an + operator waiting on a delivery — bounded by D3. ### Files to change - `packages/codev/src/agent-farm/servers/mailbox-delivery.ts` — `DeliverySession.id`; - `WriteResult`; `writeMessage` port signature + doc; precheck construction and outcome - mapping around `:397-:458`. -- `packages/codev/src/agent-farm/servers/message-write.ts` — add `submitMessagePaced`; - retire `writeMessagePaced`. + `WriteAbort`/`WriteResult`; `writeMessage` port signature + doc; precheck construction and + outcome mapping around `:397-:466`. +- `packages/codev/src/agent-farm/servers/session-submit.ts` — add `isSubmissionInFlight`, + `trySubmitToSession`, `waitCeilingMs`; **rewrite the "Exactly what it covers" boundary + comment** (`:42-69`): it now covers gated mailbox deliveries; state the per-agent → + per-terminal order, the asymmetric acquisition and why, the D3 ceiling and its degradation, + why the lock is a leaf around the write and not the classify, that **serialization is the + structural guarantee while the precheck only narrows the echo-lag residual (#1473)**, and + what stays deliberately uncovered (`POST /api/terminals/:id/write`, WS keystrokes). + *Deliverable in either outcome.* +- `packages/codev/src/agent-farm/servers/message-write.ts` — add `submitMessagePaced`; retire + `writeMessagePaced`. - `packages/codev/src/agent-farm/servers/mailbox-wiring.ts:215` — bind the new port shape. -- `packages/codev/src/agent-farm/servers/session-submit.ts:42-69` — **rewrite the "Exactly - what it covers" boundary comment**: it now covers gated mailbox deliveries; state the - per-agent → per-terminal order, why the lock is a leaf around the write and not the - classify, and what remains deliberately uncovered (`POST /api/terminals/:id/write`, WS - keystrokes). *Deliverable in either outcome.* -- `packages/codev/src/agent-farm/servers/tower-routes.ts:2017-2022` — replace the "flagged, - not done here" scope paragraph with the converged guarantee. -- `codev/resources/arch.md` §7 item 5 (line ~1801) — **replace the "disjoint lock … accepted, - documented boundary" sentence** with the converged model, plus a short statement of the - delayed-interrupt sequencing (`^C` on the timer, body through the gate, not atomic by - design). *Deliverable in either outcome.* +- `packages/codev/src/agent-farm/servers/tower-routes.ts` — replace the "flagged, not done + here" scope paragraph (`:2017-2022`) with the converged guarantee; pass `waitCeilingMs` at + the interrupt (`:2023`), escape (`:1963`) and delayed-`^C` (`:1728`) call sites. +- `codev/resources/arch.md` §7 item 5 (~line 1798) — replace the "disjoint lock … accepted, + documented boundary" sentence with the converged model: one lock at the write edge, the + agent→terminal order, the delivery-tries/operator-blocks asymmetry, the delayed-interrupt + sequencing (`^C` on the timer, body through the gate, not atomic by design), and the + #1473 residual stated honestly. *Deliverable in either outcome.* +- `codev/resources/arch-critical.md` — the review suggests a hot-tier fact for the + agent→terminal lock-order invariant. The hot tier is **at its 10-fact cap**, so this + requires demoting an existing fact. Proposed in the review phase as an explicit + displacement recommendation for the maintainer, not applied unilaterally. - Tests: new `packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts`; - fake-session updates in `send-delivery.test.ts`, `send-mailbox-repro.test.ts`, + fake/port updates in `tower-routes.test.ts` (`gateSession` at `:221` — add `id`), + `send-delivery.test.ts` (fake session **plus** the inline `ports.writeMessage` overrides at + `:422`, `:604`, `:618` — note `:604` currently returns `undefined` and relies on falsy⇒hold, + which the typed result makes explicit), `send-mailbox-repro.test.ts`, `cron-delivery.test.ts`, `send-architect-identity.test.ts`; `spec-1313-paced-write-drop.test.ts` re-pointed. -No skeleton mirror: this is `packages/codev` source and our own `codev/resources/arch.md`, -not framework template content. +No skeleton mirror: this is `packages/codev` source plus our own `codev/resources/`, not +framework template content. ### Phasing (git commits inside one PR) -1. **Convergence + unit tests** — the code above, red-to-green on the new race tests. -2. **Boundary model documentation** — `session-submit.ts`, `tower-routes.ts`, `arch.md`, +1. **Lock primitives** — `isSubmissionInFlight`, `trySubmitToSession`, `waitCeilingMs`, with + their own tests (including the D3 ceiling's degradation path). +2. **Convergence + delivery tests** — the port reshape, precheck, `submitMessagePaced`, + wiring, fake updates; red-to-green on the race tests. +3. **Boundary model documentation** — `session-submit.ts`, `tower-routes.ts`, `arch.md`, written against what actually landed. -3. **CMAP consultation** (implementation + tests) and fixes; review doc. +4. **CMAP consultation** (implementation + tests) and fixes; review doc, incl. the + `arch-critical.md` displacement proposal. ## Risks & Alternatives Considered - **Risk — the lock moves the race instead of closing it.** A delivery that classified clean - before queuing behind an interrupt would write onto a screen the interrupt just changed. - *Mitigation*: the in-lock precheck is mandatory, and is the primary thing the new tests - assert. Without it this change is a regression, and the plan should be rejected if the - precheck is dropped. -- **Risk — interrupt latency.** A `--interrupt` may now wait for an in-flight paced write. - Bounded by one message's pacing; the lock deliberately excludes the classify so the wait - can never be gate-length. -- **Risk — port signature churn breaks fakes.** Four test files. Compile-time, not runtime; - a fake that fails to update fails the build. -- **Risk — `aborted` re-holds a row that would previously have been written.** That is the - intent (it would have been written onto a screen that moved), but it makes `busy` holds - marginally more frequent under contention. The backstop re-delivers within 1.5 s. -- **Alternative — accept the boundary (wontfix).** Rejected on Q1 Ordering 2: a false - `delivered` on a lost message is not a robustness nicety, and #1481 removes the + before queuing would write onto a screen the writer ahead of it just changed. *Mitigation*: + the in-lock precheck (D4) is mandatory and is what the new tests assert; with D2 the + delivery does not queue at all. Dropping the precheck turns this change into a regression. +- **Risk — interrupt latency (D3).** Bounded by the ceiling, degrading to today's behaviour + with a loud WARN. Unbounded blocking is the rejected alternative. +- **Risk — head-of-line blocking in the sequential drainer.** Removed by D2; the drainer + never waits on a terminal lock. +- **Risk — port signature churn breaks fakes.** Six test files. Compile-time, except the + `undefined`-returning override at `send-delivery.test.ts:604`, which is called out + explicitly. +- **Risk — more `busy` holds under contention.** Intended (those writes would have landed on + a moved screen), and the backstop re-delivers within 1.5 s. +- **Risk — a silently global lock from a missing id.** Closed by D1's runtime guard plus the + different-terminals test. +- **Alternative — accept the boundary (wontfix).** Rejected on Orderings 2 and 3: a false + `delivered` on a lost or truncated message is not a robustness nicety, and #1481 removes the "operator is present" premise. Had the analysis found only Ordering 1, wontfix would have been defensible. -- **Alternative — widen `submitToSession` to hold across gate + write.** Rejected: it would - queue the human's escape hatch behind an async classification, and it buys nothing the - in-lock precheck does not. +- **Alternative — symmetric blocking lock.** Rejected per review item 2: stalls the + sequential drainer for every other agent. +- **Alternative — widen `submitToSession` to hold across gate + write.** Rejected: queues the + human's escape hatch behind an async classification and buys nothing the precheck does not. - **Alternative — keep `boolean` and map an aborted write to `false`.** Simpler, no port - churn, but reports `no-live-pty` for a `busy` abort — a lie in `afx inbox` and in the send - response. Rejected for the typed result; noted as the fallback if review wants minimal - surface. + churn, but reports `no-live-pty` for a `busy` abort and cannot express `row-resolved` at + all. Rejected; noted as the fallback if review wants minimal surface. - **Alternative — post-write verification (re-classify after Enter, re-hold on mismatch).** - Rejected: detect-and-repair is the architecture Spec 1313 explicitly replaced, and a - re-hold risks double delivery. + Rejected: detect-and-repair is the architecture Spec 1313 replaced, and a re-hold risks + double delivery. ## Test Plan -**Unit** (new `spec-1365-serializer-convergence.test.ts`, fake sessions + injected clock, -recording an ordered write log): - -- A gated delivery and a concurrent immediate-interrupt to the same terminal **never - interleave**: the `^C` appears either wholly before the delivery's text or wholly after - its Enter — never between. (Fails on `main`.) -- **Ordering 2 regression**: an interrupt racing an in-flight delivery no longer leaves the - row `delivered` — the delivery either completes intact or aborts and the row stays `held`. - (Fails on `main`: today the row reads `delivered` with nothing on the agent's screen.) -- **In-lock staleness**: gate returns clean, an interrupt writes while the delivery waits on - the terminal lock → delivery writes **zero** bytes and holds `busy`; `markDelivered` never - runs. -- **In-lock unwritable**: session becomes unwritable while queued → holds `no-live-pty`, - nothing written. -- **Preserved semantics**: dropped write (#1198) still holds `no-live-pty`; two deliveries to - one agent still cannot interleave; the promise still resolves *after* the Enter. -- **Lock hygiene**: after mixed delivery/interrupt/escape traffic settles, - `pendingSubmissionSessions() === 0` and the per-agent serializer is inactive — no leak, no - wedge, all promises settle (deadlock-freedom). -- **Escape** unchanged. - -**Regression suites to re-run**: `spec-1273-submission-lock`, `spec-1273-interrupt`, -`write-queue`, `mailbox`, `send-delivery`, `send-mailbox-repro`, `send.test`, -`spec-1313-paced-write-drop`, `spec-1307-send-delay`, `cron-delivery`, `tower-routes`, -plus a full `pnpm --filter @cluesmith/codev test` and `build`. - -**Manual (for the `dev-approval` gate)** — with a live Tower and two agents: +**Unit** — new `spec-1365-serializer-convergence.test.ts`, fake sessions with an ordered +write log, `vi.useFakeTimers()` (the paced writer uses raw `setTimeout`, so an injected clock +alone cannot assert ordering — review's non-blocking note): + +*Serialization (the structural guarantee)* +- A gated delivery and a concurrent immediate `--interrupt` to the same terminal **never + interleave**: the `^C` appears wholly before the delivery's text or wholly after its Enter. + (Fails on `main`.) +- Same for `--escape` (Ordering 3): a multi-line delivery is **never truncated** by an ESC + landing between its lines. (Fails on `main`.) +- **Ordering 2/3 regression**: an interrupt or escape racing an in-flight delivery no longer + leaves the row `delivered` — the delivery either completes intact or aborts with the row + still `held`. (Fails on `main`: today the row reads `delivered` with nothing, or a + fragment, on the agent's screen.) + +*In-lock precheck* +- Screen moved while queued → zero bytes written, hold `busy`, `markDelivered` never runs. +- Session became unwritable → zero bytes, hold `no-live-pty`. +- **Row dismissed/superseded during the lock wait → zero bytes, no hold, `onHeldStateChange` + fires, outcome `{delivered: [], reason: null}`** (review item 1). + +*Asymmetric acquisition (D2)* +- Contended terminal → delivery returns `aborted:'busy'` **immediately** (no await on the + holder) and the row stays held. +- **The drainer does not stall**: with agent A's terminal held by a long operator submission, + agent B's delivery in the same `tick()` still completes, and escalation/prune still run. +- Interrupt/escape still **block** and land after the in-flight write. + +*Ceiling (D3)* +- An operator submission waiting past `waitCeilingMs` proceeds unserialized and logs WARN; + below the ceiling it serializes normally. + +*Key hygiene (review item 5)* +- **Deliveries to two different terminals do not serialize** (proves the key is the real id). +- A session with a missing/empty `id` throws from `submitMessagePaced`, the row stays + **held**, and nothing is written. + +*Preserved semantics* +- Dropped write (#1198) still holds `no-live-pty`; two deliveries to one agent still cannot + interleave; the returned promise still resolves **after** the Enter; `noEnter` staging + unchanged. + +*Hygiene* +- After mixed delivery/interrupt/escape traffic settles, `pendingSubmissionSessions() === 0`, + the per-agent serializer is inactive, and every promise settles (deadlock-freedom). + +**Regression suites**: `spec-1273-submission-lock`, `spec-1273-interrupt`, `write-queue`, +`mailbox`, `send-delivery`, `send-mailbox-repro`, `send`, `spec-1313-paced-write-drop`, +`spec-1307-send-delay`, `cron-delivery`, `tower-routes`, plus a full +`pnpm --filter @cluesmith/codev test` and `build`. + +**Manual (for the `dev-approval` gate)** — live Tower, two agents: 1. `afx send ""` and, within the same second, - `afx interrupt ` from a second shell. Repeat ~10×. Expect: never a fused - composer, and **never** a row in `afx inbox`/`afx inbox show` reading `delivered` whose - text did not appear on the agent's screen. -2. `afx send --delay 5 --interrupt ""` against a mid-turn agent: `^C` at due + `afx interrupt ` from a second shell. Repeat ~10×. Expect: never a fused composer, + and **never** a row in `afx inbox show` reading `delivered` whose text did not appear + (whole and intact) on the agent's screen. +2. Same race with `afx send --escape` — expect no truncated body. +3. `afx send --delay 5 --interrupt ""` against a mid-turn agent: `^C` at due time, body lands only once the prompt is clean, exactly one copy. -3. `afx interrupt` against a busy agent still responds promptly (no gate-length stall). -4. `afx send --escape` unchanged; a not-writable target still 503s - `TERMINAL_NOT_WRITABLE`. +4. **Liveness**: with a large body mid-delivery to agent A, confirm agent B's held mail still + delivers promptly (D2), and that `afx interrupt ` returns within the ceiling (D3), + logging the WARN if it degrades. +5. A not-writable target still 503s `TERMINAL_NOT_WRITABLE`. **Cross-platform**: none — server-side Node only, no UI surface. + +--- + +## Changes from revision 1 (for the reviewer) + +| Review item | Where addressed | +|---|---| +| 1 — row-status re-check in the precheck | D4 (`getById` in the precheck; `row-resolved` in `WriteAbort`; outcome-mapping row; dedicated test) | +| 2 — head-of-line blocking / asymmetric try-lock | **Adopted** — D2 (`trySubmitToSession`, delivery fail-fasts, operators block) + drainer-does-not-stall test | +| 3 — echo-lag residual overclaimed | Q4's "What convergence buys, stated precisely"; fact 4 in Part 1; carried into the `session-submit.ts` and arch.md deliverables | +| 4 — `--escape` is a second instance | Part 1 Q1 Ordering 3 + Q3; first-class in the test matrix and manual steps | +| 5 — test-fake id hazard | D1 runtime guard; `gateSession` fix; different-terminals + missing-id tests | +| non-blocking — clock/fake timers | Test Plan preamble | +| non-blocking — file paths, `send-delivery.ts` overrides | Files to change (paths corrected; `:422`/`:604`/`:618` listed, `:604`'s `undefined` return called out) | +| non-blocking — `arch-critical.md` hot fact | Files to change — flagged as requiring displacement at the 10-fact cap; proposed in review, not applied unilaterally | + +**New finding not in the review**: D3 — interrupt latency. Body size is capped only by +`parseJsonBody`'s 1 MiB, so a blocking operator acquisition can wait minutes on a realistic +`--file`-sized body. Proposed remedy is a bounded wait that degrades to today's behaviour with +a loud WARN. The ceiling value (2000 ms) is a judgment call flagged for the gate. diff --git a/codev/state/pir-1365_thread.md b/codev/state/pir-1365_thread.md index 5bf07655c..5e4ee73a5 100644 --- a/codev/state/pir-1365_thread.md +++ b/codev/state/pir-1365_thread.md @@ -44,3 +44,45 @@ no-op `^C` is only logged — both are things `--interrupt-after` must design ag **Standing constraint**: we are not cluesmith/codev maintainers. Never merge the PR; park it after review and report protocol-complete. + +## Plan revision 2 (2026-08-17) — after the architect's 3-way review + +Verdicts gemini APPROVE / codex + claude REQUEST_CHANGES, with both REQUEST_CHANGES reviews +ratifying Part 1. Every blocking item was in Part 2's design, and every one of the five was +right. Verified each against the code before revising rather than taking the summaries as +ground truth: + +- **Item 5 confirmed on sight**: `tower-routes.test.ts:221` `gateSession()` is an + un-annotated literal with no `id`, and it reaches the *real* `mailbox-wiring` binding. That + would have keyed every lock on `undefined` — per-terminal serialization silently collapsing + into one global lock, with no failing assertion anywhere. Runtime guard in + `submitMessagePaced`, plus a different-terminals test. +- **Item 2 adopted as proposed** (asymmetric try-lock). The drainer awaits agents + sequentially, so a blocking acquisition would let one agent's terminal stall every other + agent's delivery *plus* that tick's escalation and prune. Deliveries now fail fast to + `busy` — which costs nothing, since a contended terminal means the precheck would have + aborted anyway — and the result is a *stronger* liveness property than today. +- **Item 1**: my revision-1 precheck re-validated writability + ringToken but not the row's + own status, which would have *widened* the dismiss→bytes-on-wire window from ~zero to the + whole lock wait. `getById` goes in the precheck; `WriteAbort` gains a `row-resolved` case + so "no hold, terminal state" stays expressible. +- **Item 3** was a fair hit. I wrote "the in-lock precheck is not a refinement — it is the + fix," which overclaims: `ringToken` tracks *output*, so un-echoed input from a writer that + doesn't take the lock still defeats it. Restated: serialization is the structural + guarantee, the precheck narrows the echo-lag residual, and that residual is #1473's. +- **Item 4**: escape is the *more* likely trigger for multi-line bodies, not a milder cousin — + the delivery's exposed window is longest exactly when the body is long, and the failure + mode is a truncated message marked `delivered` (worse than absent: it can be acted on). + +**New finding of my own (D3), flagged for the gate.** Nobody raised interrupt latency. Today +`--interrupt` never waits; after convergence it waits for any in-flight delivery write, and +body size is capped only by `parseJsonBody`'s 1 MiB default +(`agent-farm/utils/server-utils.ts:47`). A 48 KB `--file` of short lines is ~48k lines ≈ 8 +minutes of paced write. An unbounded block on the escape hatch would be a worse regression +than the bug. Proposed a bounded wait (2000 ms) that degrades to exactly today's unserialized +behaviour with a loud WARN — never worse than the status quo at any point, and no longer +silent. The ceiling value is a judgment call; flagged rather than assumed. + +Also noted: the suggested `arch-critical.md` hot-tier fact for the lock-order invariant needs +a *displacement* (the tier is at its 10-fact cap), so it is proposed in the review phase for +the maintainer rather than applied unilaterally. From fe231934a583f047a61a890b371a8502308a3340 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:46:57 -0400 Subject: [PATCH 05/26] chore(porch): 1365 plan-approval gate-approved --- .../projects/1365-serializer-convergence-route-m/status.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 707bbb83a..e93a9e6e6 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -6,8 +6,9 @@ plan_phases: [] current_plan_phase: null gates: plan-approval: - status: pending + status: approved requested_at: '2026-08-17T23:31:21.297Z' + approved_at: '2026-08-17T23:46:57.233Z' dev-approval: status: pending pr: @@ -16,4 +17,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-17T23:31:21.297Z' +updated_at: '2026-08-17T23:46:57.233Z' From 09ca45221f57a6fb5be0f6a440453945c089442c Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:46:58 -0400 Subject: [PATCH 06/26] chore(porch): 1365 implement phase-transition --- .../projects/1365-serializer-convergence-route-m/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index e93a9e6e6..4918cdc71 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -1,7 +1,7 @@ id: '1365' title: serializer-convergence-route-m protocol: pir -phase: plan +phase: implement plan_phases: [] current_plan_phase: null gates: @@ -17,4 +17,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-17T23:46:57.233Z' +updated_at: '2026-08-17T23:46:58.833Z' From 30af22b269f10cbff8fc8f4ad8a0c44eda35421d Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:59:31 -0400 Subject: [PATCH 07/26] [PIR #1365] Lock primitives: try-acquire, wait ceiling, degraded-write counter Adds the three pieces the convergence needs from the per-terminal lock: - trySubmitToSession / isSubmissionInFlight: a non-blocking acquisition for the gated delivery path, which must never queue. MailboxDrainer.tick walks agents sequentially, so one delivery parked on a terminal lock would stall every other agent's mail plus that tick's escalation and prune passes. - OPERATOR_SUBMIT_WAIT_CEILING_MS (2s) + SubmitOptions: operator submissions block, but boundedly. A paced write runs (lines-1)*10+80 ms and a body is capped only by parseJsonBody's 1 MiB, so an unbounded wait could stall afx interrupt -- the human's escape hatch -- for minutes. Past the ceiling it proceeds unserialized, which is exactly the pre-#1365 behaviour, so it is never worse than the old status quo. - unserializedWriteCount: degraded writes are counted per session so a concurrent delivery can detect that it was raced, rather than the ceiling opening a second silent-loss route. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/agent-farm/servers/session-submit.ts | 288 +++++++++++++++--- 1 file changed, 239 insertions(+), 49 deletions(-) diff --git a/packages/codev/src/agent-farm/servers/session-submit.ts b/packages/codev/src/agent-farm/servers/session-submit.ts index cf6bb5196..eb173dcba 100644 --- a/packages/codev/src/agent-farm/servers/session-submit.ts +++ b/packages/codev/src/agent-farm/servers/session-submit.ts @@ -41,38 +41,97 @@ * * ## Exactly what it covers — this is NOT blanket per-session atomicity * - * A lock only serialises writers that take it. Currently that is the `escape` - * and `interrupt` paths of `/api/send`. Every other PTY writer still writes - * directly, and it is worth being precise about why: - * - * - The mailbox delivery path (`deliverAgentMailSerialized`, Spec 1313) — every - * normal `/api/send` AND every cron notification (Phase 6 rerouted cron here; the - * old blind `writeMessageToSession` is gone). NOT covered by this lock, and does not - * need it: it runs its OWN per-agent write serializer that completion-chains each - * delivery on the prior one's paced write (text + Enter), so two mailbox deliveries - * to one agent cannot interleave. That is a disjoint lock from this per-session one, - * so a mailbox delivery is not serialised against a concurrent `escape`/`interrupt` - * here — but a normal mailbox delivery only ever writes onto a render-gate-verified - * empty prompt, and `interrupt` is the explicit gate-bypassing human action (see - * tower-routes.ts), so that residual cross-path race is an accepted, documented - * boundary, not a regression this lock must close. - * - `POST /api/terminals/:id/write` — NOT covered. It is a raw passthrough - * with no Enter semantics of its own. - * - `tower-websocket.ts` keystrokes and the shellper frame relay — DELIBERATELY - * not covered. That is a human typing into their own terminal; serialising - * it behind an agent's message would make the UI feel stuck, and the human - * is the composer's owner. - * - * So the guarantee is: **two lock-taking `/api/send` submissions (escape/interrupt) - * to one session cannot interleave**, which is the failure that reached production. - * Anything stronger requires the remaining writers to take the lock too. + * A lock only serialises writers that take it. As of Issue #1365 that is every writer + * that puts a *message* on a terminal: + * + * - `escape` and `interrupt` on `/api/send`, and the delayed `--interrupt` `^C` + * (`tower-routes.ts`) — the explicit human gate-bypasses. + * - The mailbox delivery path (`deliverAgentMailSerialized`, Spec 1313) — every normal + * `/api/send`, every cron notification, every backstop drain. It takes this lock as a + * LEAF inside its own per-agent serializer, via `submitMessagePaced`. + * + * These stay deliberately uncovered: + * + * - `POST /api/terminals/:id/write` — a raw passthrough with no Enter semantics of its own. + * - `tower-websocket.ts` keystrokes and the shellper frame relay — a human typing into + * their own terminal. Serialising that behind an agent's message would make the UI feel + * stuck, and the human is the composer's owner. + * + * ### Why the mailbox path was converged (Issue #1365) + * + * Until #1365 the delivery path held only its per-AGENT serializer — a disjoint lock — and + * the resulting cross-path race was accepted on the grounds that a gated delivery only ever + * writes onto a render-verified empty prompt and `interrupt` is an explicit human action. + * That reasoning covered one ordering (a delivery landing inside the interrupt's `^C`→settle + * →text window) and missed two: + * + * - a `^C` landing inside the DELIVERY's own text→Enter window (50–130 ms+) cleared the + * composer, so the delivery's Enter submitted nothing — yet every byte had reached the + * PTY, so the write reported success and the row was marked `delivered`. Silent loss with + * a false audit record, which is the one outcome Spec 1313 exists to exclude; + * - `--escape` (ESC, then Enter 50 ms later) produced the truncated variant, and is the + * MORE likely trigger for a multi-line body, because the delivery's exposed window is + * longest exactly when the body is long. + * + * The "a human is standing at this terminal" premise also does not hold for the DELAYED + * `^C`, which fires unattended on a timer. + * + * ### Lock order, and why there is no cycle + * + * Always per-agent → per-terminal. The delivery path takes this lock as a leaf inside + * `KeyedSerializer`; the operator paths take only this one and never enter the per-agent + * serializer. Re-entrancy is impossible in the other direction too: `PtySession.write()` + * emits no `'submit'` signal (only `handleUserInput` does, for human keystrokes), so no + * write from inside a lock can schedule a delivery synchronously. + * + * The lock is a leaf around the WRITE only, never the gate classify. `--interrupt` is the + * human's escape hatch for a wedged agent; making it queue behind a screen classification + * would be a real regression for the one action that must always get through. + * + * ### Asymmetric acquisition: deliveries decline, operators wait + * + * A delivery uses {@link trySubmitToSession} and abandons its turn on contention, because + * `MailboxDrainer.tick` walks agents SEQUENTIALLY — one delivery parked on a terminal lock + * would stall every other agent's mail, plus that tick's escalation and prune passes. It + * costs nothing: a contended terminal means the delivery's in-lock precheck would have + * aborted it anyway, and the row simply re-delivers on the next clean pass. + * + * Operators block — bounded by {@link OPERATOR_SUBMIT_WAIT_CEILING_MS}, because a paced + * write runs `(lines−1)×10+80` ms and a body is capped only by `parseJsonBody`'s 1 MiB. + * + * ### What is guaranteed, and what is not + * + * **Serialization is the structural guarantee**: no lock-taking writer can put bytes on a + * terminal while another lock-taking writer's submission is in flight. That is a property of + * the lock, not of any check. + * + * The delivery path ALSO re-validates its preconditions inside the lock (`writable`, the + * gate's `ringToken`, and the row's own status). That re-check narrows a window; it does not + * close one, and it should not be described as if it did: + * + * - `ringToken` counts OUTPUT bytes, so input written by an uncovered path (the raw + * passthrough, or a human's keystrokes) can sit un-echoed on the line and read as + * unchanged. That echo-lag residual survives this change by design — it is #1473's + * territory. + * - Its real structural value is that it makes the ACQUISITION POLICY a free choice: were + * the delivery ever switched from declining to waiting (e.g. to order an interrupt ahead + * of its own body, per #1481), the precheck is what would keep that safe. + * + * The one hole this lock opens is its own degraded path: an operator whose ceiling expires + * writes unserialized. Rather than leave that as a second silent-loss route, degraded writes + * are COUNTED per session ({@link unserializedWriteCount}); `submitMessagePaced` samples the + * counter around its write and reports `preempted`, so the delivery holds its row for + * redelivery instead of reporting a delivery that may have been clobbered. Deliberately no + * screen re-classification — the question is only "did anyone bypass the lock while I held + * it?", and a counter answers exactly that. */ /** * Tail of the in-flight submission chain per session. * * A session's entry is deleted once its chain drains, so this cannot grow - * without bound across a long-lived Tower. + * without bound across a long-lived Tower. Its presence is also the contention + * signal both {@link isSubmissionInFlight} and {@link trySubmitToSession} read. */ const chains = new Map>(); @@ -85,6 +144,68 @@ const realClock: SubmitClock = { sleep: (ms: number) => new Promise(resolve => setTimeout(resolve, ms)), }; +/** Options for an operator submission that must not wait unboundedly (Issue #1365). */ +export interface SubmitOptions { + /** + * Max ms to wait for an in-flight submission before proceeding UNSERIALIZED. + * Omitted (the default) means wait as long as it takes. + */ + waitCeilingMs?: number; + /** Called instead of the write's serialization when {@link waitCeilingMs} expires. */ + onCeilingExpired?: (waitedMs: number) => void; +} + +/** Marker resolved by the ceiling timer so the race can tell who won. */ +const CEILING_EXPIRED = Symbol('ceiling-expired'); + +/** + * How long an OPERATOR submission (`--interrupt`, `--escape`, the delayed `^C`) waits for an + * in-flight submission before proceeding unserialized (Issue #1365). + * + * Needed because a paced write's duration is `(lines−1)×10+80` ms and a request body is capped + * only by `parseJsonBody`'s 1 MiB, so a 48 KB `--file` of short lines is ~8 minutes on the wire. + * Blocking `--interrupt` — the human's escape hatch for a wedged agent — behind that would be a + * worse regression than the interleaving this lock closes. Two seconds comfortably covers every + * realistic message while keeping the escape hatch responsive. + */ +export const OPERATOR_SUBMIT_WAIT_CEILING_MS = 2000; + +/** + * Per-session count of submissions that gave up waiting and wrote UNSERIALIZED + * (Issue #1365 — the {@link OPERATOR_SUBMIT_WAIT_CEILING_MS} degraded path). + * + * A monotone counter, not a flag, so a concurrent writer can sample it before and after + * its own write and detect a race that started *and* finished in between. Entries are + * per session id and only ever created on the degraded path, which is rare by + * construction — it needs a write long enough to hold the line past the ceiling AND a + * concurrent operator action. + */ +const unserializedWrites = new Map(); + +/** + * How many unserialized (ceiling-expired) writes this session has seen. + * + * The point of comparison for a writer that wants to know whether its own submission was + * raced: sample before the first byte, compare after the last. Cheaper and far more + * direct than re-classifying the screen — it asks "did anyone bypass the lock while I + * held it?", which is exactly the question, and needs no terminal rendering at all. + */ +export function unserializedWriteCount(sessionId: string): number { + return unserializedWrites.get(sessionId) ?? 0; +} + +/** + * Whether a submission is queued or in flight for this session RIGHT NOW. + * + * Read by {@link trySubmitToSession} and exposed for telemetry. Safe to act on + * without a lock of its own: the runtime is single-threaded and every mutation of + * `chains` happens with no await in between, so a caller that observes `false` + * cannot be beaten to the install by another caller in the same tick. + */ +export function isSubmissionInFlight(sessionId: string): boolean { + return chains.has(sessionId); +} + /** * Run a write against a session so that it completes before any other * submission to the same session begins. @@ -94,52 +215,121 @@ const realClock: SubmitClock = { * keystroke (the Enter) has been written. This is exactly what * `writeMessageToSession` / `writeEscapeToSession` already * return, so callers pass them through unchanged. + * @param clock injectable sleeper; real timers by default + * @param options {@link SubmitOptions} — the operator paths pass a wait ceiling * @returns resolves once the submission is complete */ export function submitToSession( sessionId: string, write: () => number, clock: SubmitClock = realClock, + options: SubmitOptions = {}, ): Promise { const previous = chains.get(sessionId) ?? Promise.resolve(); + // A failed predecessor must not poison the chain — the next submission is a + // separate message and is still entitled to run. + const previousSettled = previous.then( + () => undefined, + () => undefined, + ); - const current = previous - // A failed predecessor must not poison the chain — the next submission is a - // separate message and is still entitled to run. - .catch(() => undefined) - .then(async () => { - const completesInMs = write(); - // Wait out the scheduled Enter. Zero means the write was fully synchronous - // (`noEnter`), so there is nothing pending to wait for. - if (completesInMs > 0) await clock.sleep(completesInMs); - }); + // Only a CONTENDED submission can wait, so an uncontended one must not arm a + // ceiling timer it would then leave dangling for its whole duration. + const contended = chains.has(sessionId); + const ceilingMs = options.waitCeilingMs; + const bounded = contended && ceilingMs !== undefined && ceilingMs >= 0; - chains.set(sessionId, current); + const current = (async () => { + if (bounded) { + const winner = await Promise.race([ + previousSettled, + clock.sleep(ceilingMs).then(() => CEILING_EXPIRED), + ]); + // Ceiling expired → proceed WITHOUT serialization. This is a deliberate, + // announced degradation to the pre-Issue-#1365 behaviour (where an operator + // write never waited at all), taken only when the alternative is stalling + // `--interrupt` — the human's escape hatch — behind a write that may run for + // minutes. See the boundary comment above for why it is never worse than the + // status quo. + if (winner === CEILING_EXPIRED) { + // Record it BEFORE the first byte so a delivery already holding the line sees the + // bump when it re-samples after its own write, and re-holds its row instead of + // reporting a delivery this write may have just clobbered. + unserializedWrites.set(sessionId, unserializedWriteCount(sessionId) + 1); + options.onCeilingExpired?.(ceilingMs); + } + } else { + await previousSettled; + } + const completesInMs = write(); + // Wait out the scheduled Enter. Zero means the write was fully synchronous + // (`noEnter`), so there is nothing pending to wait for. + if (completesInMs > 0) await clock.sleep(completesInMs); + })(); + + // The stored tail settles only once BOTH this submission and its predecessor are + // done. Identical to `current` on the normal path (current already awaited the + // predecessor); it matters on the degraded path, where the predecessor is still + // running — a third submission must not be released by our early finish. + const tail = bounded + ? Promise.all([previousSettled, current.then(() => undefined, () => undefined)]).then(() => undefined) + : current.then( + () => undefined, + () => undefined, + ); + + chains.set(sessionId, tail); // Drop the entry once this is the last submission in flight, so the map does // not accumulate one promise per session for the life of the process. // - // The rejection is swallowed HERE and re-surfaced only through the returned - // promise: without the catch, this bookkeeping branch would raise an - // unhandled rejection for any failed write, even when the caller handled it. - void current - .then( - () => undefined, - () => undefined, - ) - .then(() => { - if (chains.get(sessionId) === current) chains.delete(sessionId); - }); + // The tail already swallows both outcomes, so this bookkeeping branch can never + // raise an unhandled rejection for a failed write — the rejection surfaces only + // through the returned `current`. + void tail.then(() => { + if (chains.get(sessionId) === tail) chains.delete(sessionId); + }); return current; } +/** + * {@link submitToSession} for a writer that would rather skip its turn than wait + * for one — the gated mailbox delivery path (Issue #1365). + * + * Resolves `false` **without writing anything** when another submission already + * holds the session. That is the right trade for a gated delivery and the wrong one + * for an operator action, which is why the two differ: + * + * - A contended terminal means some other writer is mid-submission, so the + * delivery's in-lock precheck would have aborted it anyway (the screen has + * moved, or is about to). Nothing is lost by declining now: the row stays held + * and the backstop re-delivers within one tick. + * - Blocking instead would be a genuine liveness regression, because + * `MailboxDrainer.tick` walks agents SEQUENTIALLY — one agent parked on a + * terminal lock would stall every other agent's delivery, plus that tick's + * escalation, owner-notice and prune passes. + * + * @returns `true` when the write ran (and completed), `false` when the session was + * contended and nothing was written. + */ +export async function trySubmitToSession( + sessionId: string, + write: () => number, + clock: SubmitClock = realClock, +): Promise { + if (isSubmissionInFlight(sessionId)) return false; + await submitToSession(sessionId, write, clock); + return true; +} + /** Number of sessions with a submission in flight. Test/observability only. */ export function pendingSubmissionSessions(): number { return chains.size; } -/** Drop all chains. Test-only; a live Tower should let them drain. */ +/** Drop all chains and degraded-write counters. Test-only; a live Tower should let them drain. */ export function resetSubmissionChains(): void { chains.clear(); + unserializedWrites.clear(); } From e9fd2d425ea952b27dd368b9fdb5d6f9cd5e80dc Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:59:40 -0400 Subject: [PATCH 08/26] [PIR #1365] Route the mailbox write edge through the per-terminal submission lock The gated delivery path and the escape/interrupt paths held disjoint locks, so they could interleave on one terminal. The failure that mattered was not a garbled composer but a false 'delivered': a ^C landing inside a delivery's own text->Enter window cleared the composer, the delivery's Enter submitted nothing, every byte still reached the PTY so the write reported success, and the row was marked delivered for a message the agent never saw. --escape produced the truncated variant, and is the more likely trigger for a long body, whose exposed window is longest. submitMessagePaced replaces writeMessagePaced as the delivery's write edge: the same paced write, performed as one submission on the session's per-terminal lock, taken as a LEAF inside the per-agent serializer. Lock order is always per-agent -> per-terminal, and PtySession.write() emits no submit signal, so there is no cycle in either direction. The lock wraps the write only, never the gate classify -- --interrupt must not queue behind a screen classification. The port now reports written | dropped | contended | preempted | aborted instead of a bare boolean, so every not-delivered outcome holds the row rather than collapsing into 'no-live-pty'. Its precheck runs inside the lock and re-checks writability, the gate's ringToken, and the row's own status -- the last of which stops a dismiss landing during the write edge from putting bytes on the wire. Operator call sites (interrupt, escape, delayed ^C) pass the wait ceiling and log loudly when they degrade. Co-Authored-By: Claude Opus 5 (1M context) --- .../agent-farm/servers/mailbox-delivery.ts | 144 ++++++++++++++---- .../src/agent-farm/servers/mailbox-wiring.ts | 8 +- .../src/agent-farm/servers/message-write.ts | 110 ++++++++++--- .../src/agent-farm/servers/tower-routes.ts | 105 +++++++++---- 4 files changed, 291 insertions(+), 76 deletions(-) diff --git a/packages/codev/src/agent-farm/servers/mailbox-delivery.ts b/packages/codev/src/agent-farm/servers/mailbox-delivery.ts index 096bf4447..1b3cc54c7 100644 --- a/packages/codev/src/agent-farm/servers/mailbox-delivery.ts +++ b/packages/codev/src/agent-farm/servers/mailbox-delivery.ts @@ -12,9 +12,16 @@ * TWO deliberate exceptions write a body OUTSIDE this path, both explicit human * gate-bypasses documented at their `tower-routes.ts` call sites: immediate `--interrupt` * (Ctrl+C then the message) and `--escape` (a bare ESC). They are the operator's "I am at - * this terminal now" actions and take the separate per-terminal submission lock - * (`session-submit.ts`), not the per-agent serializer here — every autonomous/scheduled/ - * held send, by contrast, delivers through this gate. + * this terminal now" actions — every autonomous/scheduled/held send, by contrast, delivers + * through this gate. + * + * They are no longer on a DISJOINT lock, though (Issue #1365). This path's write edge takes + * the same per-terminal submission lock (`session-submit.ts`) those bypasses take, as a leaf + * inside the per-agent serializer below — so a `^C`/ESC can no longer land inside a + * delivery's own text→Enter window, clear or truncate the composer, and leave the row marked + * `delivered` for a message the agent never saw whole. Lock order is always per-agent → + * per-terminal; see `session-submit.ts` for the full boundary, including what the lock does + * NOT cover. * * This module replaces the in-memory `SendBuffer` (retired in this phase): held * messages now live in the durable `mailbox` table, so nothing is lost to a Tower @@ -41,6 +48,7 @@ import { } from '../db/mailbox.js'; import type { DbMailbox, MailboxReason } from '../db/types.js'; import type { GateProfile, GateVerdict } from './render-gate.js'; +import type { PacedSubmitResult } from './message-write.js'; import { KeyedSerializer } from './write-queue.js'; /** @@ -50,6 +58,13 @@ import { KeyedSerializer } from './write-queue.js'; * imports the terminal layer. */ export interface DeliverySession { + /** + * The live terminal/session id — the key of the per-terminal submission lock the write + * edge takes (Issue #1365). `PtySession` already carries it; a test double MUST supply a + * real, distinct one, and {@link submitMessagePaced} throws if it is missing rather than + * letting every lock collapse onto a single `undefined` key. + */ + readonly id: string; /** * The session's MONOTONE cumulative output-byte counter (Spec 1313 render-gate round 2) — * the gate's change token. It advances on ANY new output and NEVER decreases, so two samples @@ -79,6 +94,19 @@ export interface DeliverySession { write(data: string): boolean; } +/** + * Why a gated write abandoned inside the per-terminal lock (Issue #1365), decided by + * {@link deliverAgentMail}'s precheck at the write instant rather than before the lock. + */ +export type WriteAbort = + /** Re-hold the row for this reason and retry on a later clean pass. */ + | { kind: 'hold'; reason: MailboxReason } + /** The row was dismissed/superseded under us — a terminal state, so it must NOT be re-held. */ + | { kind: 'row-resolved' }; + +/** Outcome of the gated write edge. See {@link PacedSubmitResult}. */ +export type WriteResult = PacedSubmitResult; + /** Broadcast frame for a delivered message (the dashboard/inbox message event). */ export interface DeliveredBroadcast { type: 'message'; @@ -104,17 +132,29 @@ export interface DeliveryPorts { */ classify(session: DeliverySession, profile: GateProfile): Promise; /** - * Write a formatted message (text + Enter, unless `noEnter`) to the session and - * report whether every byte reached the terminal. Resolves `true` when the paced - * write — including the trailing Enter — has fully completed; `false` when any - * write was dropped (#1198: a shellper socket that died mid-pace). The delivery - * `await`s it for two reasons: (1) completion chaining — the per-agent serializer - * holds the line until the submit is entirely on the wire, so the next delivery - * never starts mid-write; (2) the boolean gates markDelivered — a dropped write - * holds the row (`no-live-pty`) instead of falsely reporting delivery (Spec 1313 - * integration review — the silent-loss finding). + * Write a formatted message (text + Enter, unless `noEnter`) to the session as ONE + * submission on the session's per-terminal lock, and report what actually happened. + * Resolves only once the paced write — trailing Enter included — has completed. + * + * The delivery `await`s it for two reasons: (1) completion chaining — the per-agent + * serializer holds the line until the submit is entirely on the wire, so the next + * delivery never starts mid-write; (2) the result gates markDelivered — anything but + * `written` must never be reported as delivered (Spec 1313 integration review — the + * silent-loss finding; Issue #1365 extended the same rule to in-lock refusals). + * + * `precheck` is invoked by the binding INSIDE the lock, immediately before the first + * byte, and returning non-null aborts with nothing written. It exists because taking + * the lock alone would only move the race: a delivery that classified a clean screen + * and then waited behind another submission would write onto the screen that + * submission just changed. All of the reason authority stays here in the delivery + * module — the binding only relays the verdict. */ - writeMessage(session: DeliverySession, formattedMessage: string, noEnter: boolean): boolean | Promise; + writeMessage( + session: DeliverySession, + formattedMessage: string, + noEnter: boolean, + precheck: () => WriteAbort | null, + ): WriteResult | Promise; /** Emit the delivered-message broadcast frame. */ broadcast(frame: DeliveredBroadcast): void; /** @@ -423,11 +463,32 @@ export async function deliverAgentMail( // delivered off the paced-write timer. if (!session.writable) return hold('no-live-pty'); - // Default false so an unobserved result is the SAFE failure mode (hold, never a false - // delivery); the try either assigns the real boolean or throws past this point. - let written = false; + // Re-validate EVERYTHING at the write instant, inside the per-terminal lock (Issue #1365). + // The three checks above (screen unchanged, session writable, row still held) were made + // before the lock; between them and the first byte another writer may hold the terminal, + // and this delivery may have waited. Without this, routing the write through the lock would + // merely relocate the race it exists to close: a delivery could write onto a screen that an + // `--interrupt`/`--escape` had just cleared. Cheap enough to repeat — a synchronous ring + // read and one indexed better-sqlite3 lookup. + // + // The residual it CANNOT close: `ringToken` counts OUTPUT bytes, so input written by a + // path that does not take this lock (the raw `/api/terminals/:id/write` passthrough, or a + // human's keystrokes over the WebSocket) can sit un-echoed on the line and read as + // unchanged. Serialization — not this precheck — is what makes the lock-taking writers + // safe; the echo-lag residual for the rest is #1473's territory. + const precheck = (): WriteAbort | null => { + if (!session.writable) return { kind: 'hold', reason: 'no-live-pty' }; + if (ringToken(session, profile) !== tokenBefore) return { kind: 'hold', reason: 'busy' }; + const stillHeld = getById(db, row.id); + if (!stillHeld || stillHeld.status !== 'held') return { kind: 'row-resolved' }; + return null; + }; + + // Default to a hold so an unobserved result is the SAFE failure mode (hold, never a false + // delivery); the try either assigns the real result or throws past this point. + let result: WriteResult = { status: 'aborted', abort: { kind: 'hold', reason: 'busy' } }; try { - written = await ports.writeMessage(session, current.formatted_message, current.no_enter === 1); + result = await ports.writeMessage(session, current.formatted_message, current.no_enter === 1, precheck); } finally { // Invalidate the memo on EVERY write outcome — a clean `true`, a dropped-write `false`, OR a // rejection — and BEFORE the markDelivered/held decisions below (CMAP round 3 moved it above the @@ -447,15 +508,46 @@ export async function deliverAgentMail( memo?.delete(cacheKey); } - // A dropped PTY write (#1198) means zero-or-partial bytes reached the terminal — the exact silent - // loss this spec exists to prevent (Spec 1313 integration review — Codex). The t=0 `writable` - // precheck above cannot catch a socket that dies mid-pace (the text/lines/Enter fire across - // setTimeout gaps), so writeMessage threads the per-write result: `false` → no complete submit - // landed. Hold the row (`no-live-pty`, retried on the next clean gate pass) instead of marking it - // delivered. Any bytes already on the wire only make the line dirty; the render gate then holds on - // that draft until the session recovers or is torn down — it can never be marked delivered on a - // dead PTY. - if (!written) return hold('no-live-pty'); + // Anything short of a complete submit holds the row — a delivery is marked delivered only when + // every byte, Enter included, reached the terminal. + // + // • `dropped` — a PTY write was dropped (#1198): zero-or-partial bytes reached the terminal, + // the exact silent loss this spec exists to prevent (Spec 1313 integration review — Codex). + // The t=0 `writable` precheck cannot catch a socket that dies mid-pace (the text/lines/Enter + // fire across setTimeout gaps). Any bytes already on the wire only make the line dirty; the + // render gate then holds on that draft until the session recovers or is torn down. + // • `contended` — another submission (an `--interrupt`/`--escape`, or a delivery to an agent + // sharing this terminal) held the lock, so nothing was written. `busy` is the honest reason: + // the line is occupied. Declining rather than queueing is deliberate — see + // {@link trySubmitToSession}: the drainer walks agents sequentially, so a blocking wait here + // would stall every OTHER agent's delivery behind this one terminal. + // • `aborted` — the in-lock precheck refused, with nothing written. A `hold` abort re-holds for + // the stated reason; `row-resolved` means the row was dismissed/superseded while we waited, + // which is a TERMINAL state and must not be re-held (same handling as the pre-lock check + // above, which this one backstops for the duration of the lock wait). + // • `preempted` — the bytes went out, but an operator submission whose wait ceiling expired + // wrote unserialized while they did, so the composer may have been cleared or truncated + // under them. Hold rather than mark delivered. This trades a possible DUPLICATE (if the + // message did land intact, the gate re-delivers it later) for never reporting a delivery + // that did not happen — the same call the `dropped` branch already makes, and the failure + // this whole issue exists to remove. It is the one hole the ceiling opens, and it is + // detected by counting lock bypasses, not by re-reading the screen. + if (result.status === 'dropped') return hold('no-live-pty'); + if (result.status === 'preempted') { + ports.log( + `[mailbox] write to ${toAgent} @ ${path.basename(workspacePath)} was raced by an unserialized ` + + `operator write — holding ${row.id.slice(0, 8)}… for redelivery rather than reporting it delivered`, + ); + return hold('busy'); + } + if (result.status === 'contended') return hold('busy'); + if (result.status === 'aborted') { + if (result.abort.kind === 'row-resolved') { + ports.onHeldStateChange(); // the held set changed under us → refresh the indicator + return { delivered: [], reason: null }; + } + return hold(result.abort.reason); + } // markDelivered is guarded (held→delivered only). If it did NOT transition, the row // was dismissed/superseded during the paced write — accept that terminal state and diff --git a/packages/codev/src/agent-farm/servers/mailbox-wiring.ts b/packages/codev/src/agent-farm/servers/mailbox-wiring.ts index f1b3ee817..650f3156c 100644 --- a/packages/codev/src/agent-farm/servers/mailbox-wiring.ts +++ b/packages/codev/src/agent-farm/servers/mailbox-wiring.ts @@ -18,7 +18,7 @@ import { loadConfig } from '../../lib/config.js'; import { terminalDeliverySignals, type PtySession } from '../../terminal/pty-session.js'; import { getWorkspaceTerminals, getTerminalManager } from './tower-terminals.js'; import { broadcastMessage, resolveAgentInRegistry, isResolveError } from './tower-messages.js'; -import { writeMessagePaced } from './message-write.js'; +import { submitMessagePaced } from './message-write.js'; import { classifyBuffer, type GateProfile, type GateVerdict } from './render-gate.js'; import { resolveProfile } from './gate-profiles.js'; import { harnessFromLaunchScript, type ContextFsPort } from '../commands/reset/context.js'; @@ -212,7 +212,11 @@ export function makeDeliveryPorts(log: LogFn): DeliveryPorts { getSessionForAgent: (ws, agent) => resolveLiveSessionForAgent(ws, agent), resolveProfile: (session) => resolveProfileForSession(session), classify: (session, profile) => classifyAgentScreen(session, profile), - writeMessage: (session, msg, noEnter) => writeMessagePaced(session, msg, noEnter), + // Issue #1365: the write edge takes the session's per-terminal submission lock as a + // LEAF inside the per-agent serializer, so a gated delivery and a concurrent + // `--interrupt`/`--escape` can no longer interleave. The precheck is the delivery + // module's, re-run inside that lock. + writeMessage: (session, msg, noEnter, precheck) => submitMessagePaced(session, msg, noEnter, precheck), broadcast: (frame) => broadcastDelivered(frame), onHeldStateChange: () => broadcastHeldStateChange(), onEscalation: (info) => broadcastEscalation(info), diff --git a/packages/codev/src/agent-farm/servers/message-write.ts b/packages/codev/src/agent-farm/servers/message-write.ts index e19f927fa..ac6f93856 100644 --- a/packages/codev/src/agent-farm/servers/message-write.ts +++ b/packages/codev/src/agent-farm/servers/message-write.ts @@ -5,6 +5,8 @@ * tower-routes.ts and tower-cron.ts. */ +import { trySubmitToSession, unserializedWriteCount, type SubmitClock } from './session-submit.js'; + /** Minimal writable session interface — avoids coupling to PtySession. */ export interface WritableSession { /** @@ -111,29 +113,78 @@ export function writeMessageToSession( } /** - * Paced write of a message (text + trailing Enter unless `noEnter`) that reports - * whether every byte reached the PTY. Resolves `true` when the whole submit landed, - * `false` when ANY scheduled write was dropped (#1198: a shellper socket that died - * mid-pace). This is the delivery layer's authoritative success signal — a mailbox - * delivery holds a row whose bytes never made it instead of marking it delivered - * (Spec 1313 integration review — the silent-loss finding). + * Outcome of a {@link submitMessagePaced} attempt. Generic in the caller's own abort + * vocabulary so this module stays free of mailbox concepts — the delivery layer + * instantiates `A` with its hold reasons. + */ +export type PacedSubmitResult = + /** The whole submit — text and, unless `noEnter`, the trailing Enter — reached the PTY. */ + | { status: 'written' } + /** #1198: a scheduled write was dropped mid-pace (the shellper socket died). */ + | { status: 'dropped' } + /** + * The bytes went out, but an operator submission bypassed the lock while they did + * (the ceiling-expired degraded path), so this submit cannot be trusted to have + * landed intact. + */ + | { status: 'preempted' } + /** Another submission held the terminal. NOTHING was written; the caller may retry later. */ + | { status: 'contended' } + /** The caller's in-lock precheck refused. NOTHING was written. */ + | { status: 'aborted'; abort: A }; + +/** + * Paced write of a message (text + trailing Enter unless `noEnter`) performed as ONE + * submission on the session's per-terminal lock (Issue #1365). + * + * This is the mailbox delivery path's write edge. Before #1365 it wrote directly, under + * the per-agent serializer only, so a gated delivery could interleave with a concurrent + * `--interrupt`/`--escape` on the same terminal: a `^C` or ESC landing between the text + * and its Enter cleared or truncated the composer while every byte still reported + * success, and the row was marked `delivered` for a message the agent never saw whole. + * Taking the same lock those paths take is what makes that impossible. * - * `writeMessageToSession` fires the text, any subsequent lines, and the trailing - * Enter across `setTimeout` gaps (10–130ms+), and a t=0 `writable` precheck cannot - * see a socket that dies *during* that sequence. So wrap the session and record - * whether any of those writes returned false. The returned promise resolves at the - * final scheduled offset (`doneMs`); `writeMessageToSession` registers the Enter's - * `setTimeout` at that same offset *before* this resolve is scheduled, so the Enter - * executes first and its result is observed by resolution time. + * Two properties are load-bearing and easy to lose in a refactor: * - * Awaiting the promise is also what makes the per-agent write serializer's - * completion-chaining real — the next delivery cannot begin until this submit - * (Enter included) is entirely on the wire. + * - **`precheck` runs INSIDE the lock**, immediately before the first byte. Acquiring + * the lock without it would merely move the race: a delivery that classified a clean + * screen, then waited behind another submission, would write onto the screen that + * submission just changed. Returning non-null aborts with nothing written. + * - **Contention is declined, not queued** (see {@link trySubmitToSession}). The gated + * delivery path must never block, because the drainer walks agents sequentially. + * + * The completion semantics callers depend on are unchanged from the pre-#1365 + * `writeMessagePaced`: the returned promise resolves only after the trailing Enter has + * been written. `writeMessageToSession` registers the Enter's `setTimeout` before + * `submitToSession` schedules its own equal-offset sleep, so the Enter still executes + * first — which is what makes the per-agent serializer's completion-chaining real. */ -export function writeMessagePaced( - session: WritableSession, message: string, noEnter: boolean, -): Promise { +export async function submitMessagePaced( + session: WritableSession & { id: string }, + message: string, + noEnter: boolean, + precheck: () => A | null, + clock?: SubmitClock, +): Promise> { + // Fail LOUD on a missing id rather than keying the lock on `undefined`. Sessions reach + // this through structurally-typed ports, so a double without an id compiles fine and + // would silently collapse every per-terminal lock into one global lock — serialization + // that looks present and is not. A throw here surfaces as a held row (the delivery path + // never marks a row delivered on a throw), which is the safe failure. + if (typeof session.id !== 'string' || session.id === '') { + throw new Error('submitMessagePaced: session.id must be a non-empty string (the per-terminal lock key)'); + } + + // The one thing the lock cannot stop is an operator submission whose wait ceiling expired + // and wrote anyway. Sample the session's degraded-write counter around our own submission: + // a bump means a `^C`/ESC bypassed us mid-write, so the composer may have been cleared or + // truncated under our bytes. Cheaper and more direct than re-classifying the screen — and it + // is the difference between re-holding the row and falsely reporting a delivery, which is + // the whole point of Issue #1365. + const bypassesBefore = unserializedWriteCount(session.id); + let delivered = true; + let abort: A | null = null; const tracked: WritableSession = { write: (data: string): boolean => { const ok = session.write(data); @@ -141,6 +192,23 @@ export function writeMessagePaced( return ok; }, }; - const doneMs = writeMessageToSession(tracked, message, noEnter); - return new Promise((resolve) => setTimeout(() => resolve(delivered), doneMs)); + + const ran = await trySubmitToSession( + session.id, + () => { + abort = precheck(); + if (abort !== null) return 0; // refused in-lock: not one byte goes out + return writeMessageToSession(tracked, message, noEnter); + }, + clock, + ); + + if (!ran) return { status: 'contended' }; + // Read through a cast: both flags are assigned inside the callback above, which + // TypeScript's flow analysis does not track back to this scope. + const refused = abort as A | null; + if (refused !== null) return { status: 'aborted', abort: refused }; + if (!(delivered as boolean)) return { status: 'dropped' }; + if (unserializedWriteCount(session.id) !== bypassesBefore) return { status: 'preempted' }; + return { status: 'written' }; } diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index 10ccf6116..aa6875220 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -63,10 +63,11 @@ import { type EnqueueInput, } from '../db/mailbox.js'; import type { MailboxReason } from '../db/types.js'; -// Spec 1273 per-terminal submission lock — preserved across the Spec 1313 merge for -// the two explicit human-bypass paths (escape + interrupt), which do NOT route -// through the mailbox's per-agent serializer and so need their own anti-fusion lock. -import { submitToSession } from './session-submit.js'; +// Spec 1273 per-terminal submission lock. Originally the anti-fusion lock for the two +// explicit human-bypass paths (escape + interrupt); since Issue #1365 the gated mailbox +// delivery path takes it too, so it now serializes the whole write edge of a terminal. +// The operator paths pass a wait ceiling — see OPERATOR_SUBMIT_WAIT_CEILING_MS. +import { submitToSession, OPERATOR_SUBMIT_WAIT_CEILING_MS } from './session-submit.js'; // Spec 1307 `--delay` — Tower-side deferred delivery, re-homed onto the Spec 1313 // mailbox (the merge that carried this feature was flattened by a later rebase, so it // is grafted here explicitly): the due-time callback enqueues to the mailbox and @@ -1614,6 +1615,30 @@ function holdAndRespond( }); } +/** + * Announce that an operator submission gave up waiting for the per-terminal lock and wrote + * unserialized (Issue #1365). + * + * This is the degraded path, and it is deliberately loud: below the ceiling an operator write + * cannot interleave with a gated delivery, and above it we fall back to exactly the behaviour + * that shipped before #1365 (an operator write that never waited). So the degradation is never + * worse than the old status quo — but it used to be invisible, and now it is not. + */ +function logCeilingExpired( + ctx: RouteContext, + action: string, + toAgent: string, + terminalId: string, + waitedMs: number, +): void { + ctx.log( + 'WARN', + `${action} → ${toAgent} (terminal ${terminalId.slice(0, 8)}...) waited ${waitedMs}ms for an in-flight ` + + `write and proceeded UNSERIALIZED — it may interleave with that write. A message long enough to ` + + `hold the line this long is the usual cause.`, + ); +} + /** Inputs for a delayed (`--delay`) send, captured from the parsed request. */ interface DelayedSendParams { to: string; @@ -1725,18 +1750,29 @@ function handleDelayedSend( if (!isStillLive()) return; // shutdown before/at due → drop the ^C nudge (body survives) if (terminalId) { let fired = false; - void submitToSession(terminalId, () => { - // Re-check EVERYTHING inside the lock: the queued submission can acquire the lock only - // AFTER a shutdown or a session teardown/respawn that landed while it waited behind an - // in-flight write. Re-fetch the session and re-check live + writable here; bail with no - // ^C otherwise (the body still delivers via the gate). - if (!isStillLive()) return 0; - const live = getTerminalManager().getSession(terminalId); - if (!live || !live.writable) return 0; - live.write('\x03'); // Ctrl+C only — end the turn; body follows via the gate - fired = true; - return 0; // no body, no Enter on this path - }) + void submitToSession( + terminalId, + () => { + // Re-check EVERYTHING inside the lock: the queued submission can acquire the lock only + // AFTER a shutdown or a session teardown/respawn that landed while it waited behind an + // in-flight write. Re-fetch the session and re-check live + writable here; bail with no + // ^C otherwise (the body still delivers via the gate). + if (!isStillLive()) return 0; + const live = getTerminalManager().getSession(terminalId); + if (!live || !live.writable) return 0; + live.write('\x03'); // Ctrl+C only — end the turn; body follows via the gate + fired = true; + return 0; // no body, no Enter on this path + }, + undefined, + // The same operator ceiling as the immediate paths (Issue #1365). This one fires + // UNATTENDED, so it is the writer most likely to meet a gated delivery mid-pace — + // which is precisely why the delayed `^C` must take this lock at all. + { + waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, + onCeilingExpired: (waitedMs) => logCeilingExpired(ctx, 'delayed interrupt ^C', toAgent, terminalId, waitedMs), + }, + ) .then(() => ctx.log('INFO', fired ? `Delayed interrupt ^C fired → ${toAgent} (terminal ${terminalId.slice(0, 8)}...); body delivers via the gate` @@ -1960,7 +1996,13 @@ async function handleSend( if (escape) { // Awaited: the response must not claim delivery before the ESC and its // Enter have actually been written (Spec 1273 verify). - await submitToSession(result.terminalId, () => writeEscapeToSession(session, noEnter)); + // Issue #1365: escape is an operator submission — it blocks behind an in-flight write + // (including a gated mailbox delivery, which now takes this same lock) so its ESC and + // Enter can no longer truncate a delivery mid-pace, but only up to the wait ceiling. + await submitToSession(result.terminalId, () => writeEscapeToSession(session, noEnter), undefined, { + waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, + onCeilingExpired: (waitedMs) => logCeilingExpired(ctx, 'ESC', toAgent, result.terminalId, waitedMs), + }); broadcastMessage({ type: 'message', from: { project: path.basename(senderWorkspace), agent: from ?? 'unknown' }, @@ -2014,16 +2056,25 @@ async function handleSend( // during the 100 ms gap. `writeMessageToSession(..., 100)` schedules the text 100 ms after // the ^C (the settle) and returns the completion offset, so the lock is held until the // whole interrupt is on the wire; uncontended, it runs at once. - // Scope of the guarantee: this serializes interrupt against interrupt/escape — the only - // /api/send writers that take this per-terminal lock. It does NOT serialize against a - // concurrent mailbox/backstop delivery (which writes through the per-AGENT serializer, a - // disjoint lock); interrupt is the explicit gate-bypassing human action, and closing that - // cross-path race would require the mailbox write edge to take this lock too (a separate, - // larger change — flagged, not done here). - await submitToSession(result.terminalId, () => { - session.write('\x03'); // Ctrl+C - return writeMessageToSession(session, formattedMessage, noEnter, 100); - }); + // Scope of the guarantee (Issue #1365 — this used to end at "interrupt vs interrupt/escape"): + // the gated mailbox delivery path now takes this SAME per-terminal lock, so an interrupt is + // serialized against a concurrent delivery too. That closes the case where a `^C` landed + // inside a delivery's own text→Enter window, clearing the composer while every byte still + // reported success — the row read `delivered` for a message the agent never saw. The lock + // order is per-agent → per-terminal (the delivery takes this one as a leaf), never the + // reverse, so there is no cycle. See session-submit.ts for the full boundary. + await submitToSession( + result.terminalId, + () => { + session.write('\x03'); // Ctrl+C + return writeMessageToSession(session, formattedMessage, noEnter, 100); + }, + undefined, + { + waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, + onCeilingExpired: (waitedMs) => logCeilingExpired(ctx, 'interrupt', toAgent, result.terminalId, waitedMs), + }, + ); broadcastMessage({ type: 'message', from: { project: path.basename(senderWorkspace), agent: from ?? 'unknown' }, From 194685e1d96e57e969febf489c4a423664114e01 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 19:59:54 -0400 Subject: [PATCH 09/26] [PIR #1365] Tests: interleaving, in-lock precheck, liveness, ceiling, key hygiene 23 new tests. The corruption cases are each paired with a bypass-the-lock CONTROL that reproduces the original bug (body lost to a ^C, body truncated by an ESC, both with the write still reporting success), so they assert the fix rather than merely exercising it. Also covered: the drainer keeps serving other agents while one terminal is held; a delivery declines contention immediately instead of waiting; the ceiling degrades and announces itself; a raced delivery reports preempted and HOLDS its row; deliveries to different terminals do not serialize; a session with no id throws. gateSession in tower-routes.test.ts gains a real id -- it is the hazard the review predicted: an un-annotated fake reaching the live wiring would have keyed every lock on undefined, collapsing per-terminal serialization into one global lock with nothing failing. The runtime guard turns that into a loud throw, and 13 tests duly failed until the fake was fixed. spec-1313-paced-write-drop is re-pointed from the retired writeMessagePaced onto submitMessagePaced, so the silent-loss fix stays guarded at the live write edge rather than at a function nothing calls. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/cron-delivery.test.ts | 8 +- .../__tests__/send-architect-identity.test.ts | 6 +- .../__tests__/send-delivery.test.ts | 19 +- .../__tests__/send-mailbox-repro.test.ts | 7 +- .../spec-1313-paced-write-drop.test.ts | 63 ++- .../spec-1365-serializer-convergence.test.ts | 516 ++++++++++++++++++ .../agent-farm/__tests__/tower-routes.test.ts | 8 +- 7 files changed, 594 insertions(+), 33 deletions(-) create mode 100644 packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts diff --git a/packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts b/packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts index 79fe5c467..3cd7298c3 100644 --- a/packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts +++ b/packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts @@ -32,6 +32,7 @@ const AGENT = 'main'; /** A minimal DeliverySession fake (records writes). */ function fakeSession(): DeliverySession { return { + id: 'term-1', bytesWritten: 0, info: { cols: 110, rows: 32 }, command: 'claude', @@ -81,9 +82,12 @@ function harness(): Harness { getSessionForAgent: () => session, resolveProfile: () => profile, classify: (_session: DeliverySession, _p: GateProfile): Promise => Promise.resolve(verdict), - writeMessage: (_s, formattedMessage, noEnter) => { + writeMessage: (_s, formattedMessage, noEnter, precheck) => { + // The precheck runs INSIDE the per-terminal lock in the live binding (Issue #1365). + const abort = precheck(); + if (abort) return { status: 'aborted', abort }; writes.push({ formattedMessage, noEnter }); - return true; // the write landed (Spec 1313: writeMessage reports delivery success) + return { status: 'written' }; // the write landed (Spec 1313: the port reports delivery success) }, broadcast: (f) => broadcasts.push(f), onHeldStateChange: () => { diff --git a/packages/codev/src/agent-farm/__tests__/send-architect-identity.test.ts b/packages/codev/src/agent-farm/__tests__/send-architect-identity.test.ts index b4af14bc8..063c1a068 100644 --- a/packages/codev/src/agent-farm/__tests__/send-architect-identity.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send-architect-identity.test.ts @@ -97,10 +97,12 @@ function realSeamPorts( // The REAL production classify seam (Spec 1313 round 2): read the session's persistent // mirror (seeded here via attachShellper's replay) and classify its viewport. classify: (s, prof) => classifyAgentScreen(s, prof), - writeMessage: (s, msg, noEnter) => { + writeMessage: (s, msg, noEnter, precheck) => { + const abort = precheck(); + if (abort) return { status: 'aborted' as const, abort }; writes.push({ msg, noEnter }); s.write(msg); // drive the real session's write path (fake shellper records it) - return true; // the write landed (Spec 1313: writeMessage reports delivery success) + return { status: 'written' as const }; // the write landed (Spec 1313: the port reports delivery success) }, broadcast: (f) => broadcasts.push(f), onHeldStateChange: () => {}, diff --git a/packages/codev/src/agent-farm/__tests__/send-delivery.test.ts b/packages/codev/src/agent-farm/__tests__/send-delivery.test.ts index a9a497b00..2043a1b4f 100644 --- a/packages/codev/src/agent-farm/__tests__/send-delivery.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send-delivery.test.ts @@ -43,6 +43,7 @@ const BUSY: GateVerdict = { clean: false, reason: 'busy', detail: 'user-text' }; function fakeSession(overrides: Partial = {}): DeliverySession & { writes: string[] } { const writes: string[] = []; return { + id: 'term-fake', bytesWritten: 0, info: { cols: 110, rows: 32 }, command: 'claude', @@ -120,9 +121,13 @@ function harness(): Harness { resolveProfile: () => profile, classify: (session: DeliverySession, p: GateProfile): Promise => classifyOverride ? classifyOverride(session, p) : Promise.resolve(verdict), - writeMessage: (_s, formattedMessage, noEnter) => { + writeMessage: (_s, formattedMessage, noEnter, precheck) => { + // Mirror the live binding's ordering: the precheck runs INSIDE the lock, before the + // first byte (Issue #1365), so an abort must record no write at all. + const abort = precheck(); + if (abort) return { status: 'aborted', abort }; writes.push({ formattedMessage, noEnter }); - return h.writeResult; + return h.writeResult ? { status: 'written' } : { status: 'dropped' }; }, broadcast: (f) => broadcasts.push(f), onHeldStateChange: () => { @@ -318,6 +323,7 @@ describe('deliverAgentMail (Spec 1313, Phase 4)', () => { // (the false-clean the gate exists to prevent). let bytes = 0; const session: DeliverySession = { + id: 'term-moving', get bytesWritten() { return bytes; }, @@ -422,7 +428,7 @@ describe('deliverAgentMailSerialized — concurrent-send serialization (Spec 131 h.ports.writeMessage = (_s, formattedMessage, noEnter) => Promise.resolve().then(() => { h.writes.push({ formattedMessage, noEnter }); - return true; // the write landed (Spec 1313: writeMessage reports delivery success) + return { status: 'written' as const }; // the write landed (Spec 1313: the port reports delivery success) }); h.setSession('spir-1', fakeSession()); mailbox.enqueue(db, { workspacePath: '/ws/a', toAgent: 'spir-1', body: '1', formattedMessage: 'F' }, 1000); @@ -546,6 +552,7 @@ describe('MailboxDrainer verdict memo (Spec 1313 render-gate follow-up)', () => let bytes = 7; // A moving token needs a live getter (a fakeSession spread would freeze bytesWritten to its value). h.setSession('spir-1', { + id: 'term-moving', get bytesWritten() { return bytes; }, info: { cols: 110, rows: 32 }, command: 'claude', @@ -604,6 +611,7 @@ describe('MailboxDrainer verdict memo (Spec 1313 render-gate follow-up)', () => h.ports.writeMessage = (_s, formattedMessage, noEnter) => { h.writes.push({ formattedMessage, noEnter }); if (formattedMessage === 'm1') mailbox.dismiss(db, m1.id, 1002); // operator dismisses during the paced write + return { status: 'written' as const }; }; const drainer = new MailboxDrainer({ intervalMs: 999999 }); drainer.start(h.ports, db); @@ -618,8 +626,8 @@ describe('MailboxDrainer verdict memo (Spec 1313 render-gate follow-up)', () => it('invalidates the memo even when writeMessage REJECTS after partial output (CMAP round 4 — Codex)', async () => { const h = harness(); // Round-4 completion of Fix 1: memo.delete must run on a write REJECTION too (via try/finally), - // not only a clean return. writeMessage's port contract is boolean|Promise, so a binding - // could reject after putting bytes on the wire; without the finally the stale CLEAN survives and a + // not only a clean return. writeMessage's port contract is WriteResult|Promise, so a + // binding could reject after putting bytes on the wire; without the finally the stale CLEAN survives and a // follow-up could memo-hit it. Here writeMessage records partial output then rejects → the row // stays held (deliverAgentMail throws, caught by the per-agent tick guard) → the NEXT tick must // re-classify fresh, not memo-hit. Static ring, so a re-classify can only come from invalidation. @@ -1050,6 +1058,7 @@ describe('MailboxDrainer owner starvation notice (Spec 1313 round 3, change 3)', // verdict memo re-classifies (a static token would serve the cached BUSY and never deliver). let bytes = 1; h.setSession('spir-1', { + id: 'term-moving', get bytesWritten() { return bytes; }, info: { cols: 110, rows: 32 }, command: 'claude', diff --git a/packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts b/packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts index 743ad8b3d..720b0841d 100644 --- a/packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts @@ -58,6 +58,7 @@ function flipSession(command = 'claude'): FlipSession { let bytes = 0; let screen = new SessionScreen(COLS, ROWS); return { + id: 'term-flip', get bytesWritten() { return bytes; }, @@ -99,9 +100,11 @@ function realGatePorts( const { term, cols, rows } = await session.screen.read(); return classifyBuffer(term, cols, rows, prof); }, - writeMessage: (_s, msg, noEnter) => { + writeMessage: (_s, msg, noEnter, precheck) => { + const abort = precheck(); + if (abort) return { status: 'aborted' as const, abort }; writes.push({ msg, noEnter }); - return true; // the write landed (Spec 1313: writeMessage reports delivery success) + return { status: 'written' as const }; // the write landed (Spec 1313: the port reports delivery success) }, broadcast: (f) => broadcasts.push(f), onHeldStateChange: () => {}, diff --git a/packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts index 45b7c1aa3..40cf6af75 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts @@ -7,17 +7,24 @@ * discarded the boolean and resolved on a pure timer — a message could be reported * `delivered` while zero bytes reached the terminal. * - * `writeMessagePaced` now threads the per-write result and resolves `false` when ANY - * scheduled write dropped. The load-bearing property the architect called out is that - * this must catch BOTH the first (synchronous) write AND the DELAYED writes — the - * trailing Enter and the per-line writes of a multi-line message — because a socket can - * die anywhere across the 10–130ms+ paced sequence, not only at t=0. These tests drive - * the real pacing under fake timers and assert the aggregate for each drop position. + * The write edge threads the per-write result and reports `dropped` when ANY scheduled + * write dropped. The load-bearing property the architect called out is that this must catch + * BOTH the first (synchronous) write AND the DELAYED writes — the trailing Enter and the + * per-line writes of a multi-line message — because a socket can die anywhere across the + * 10–130ms+ paced sequence, not only at t=0. These tests drive the real pacing under fake + * timers and assert the aggregate for each drop position. + * + * Issue #1365 re-pointed them from the retired `writeMessagePaced` onto `submitMessagePaced`, + * which is the same paced write performed under the per-terminal submission lock. The + * drop-threading contract is unchanged — only its vocabulary is (`written`/`dropped` instead + * of a bare boolean) — so these keep guarding the silent-loss fix at the live write edge + * rather than at a function nothing calls. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { writeMessagePaced } from '../servers/message-write.js'; +import { submitMessagePaced } from '../servers/message-write.js'; import type { WritableSession } from '../servers/message-write.js'; +import { resetSubmissionChains } from '../servers/session-submit.js'; /** * A WritableSession fake whose `write` returns false (a dropped write) whenever @@ -25,9 +32,10 @@ import type { WritableSession } from '../servers/message-write.js'; */ function makeSession( shouldDrop: (data: string, callIndex: number) => boolean = () => false, -): WritableSession & { writes: string[] } { +): WritableSession & { id: string; writes: string[] } { const writes: string[] = []; return { + id: 'term-drop', write: (data: string): boolean => { const idx = writes.length; writes.push(data); @@ -37,20 +45,33 @@ function makeSession( }; } -/** Run every scheduled paced write + the resolve timer, then await the promise. */ -async function settle(p: Promise): Promise { +/** + * Drive the write edge to completion and report whether the whole submit landed. + * + * `runAllTimersAsync` covers both the paced writes and the submission lock's own + * completion sleep, which is scheduled on the same (fake) clock. + */ +async function settle(p: Promise<{ status: string }>): Promise { await vi.runAllTimersAsync(); - return p; + return (await p).status === 'written'; } -describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss fix)', () => { - beforeEach(() => vi.useFakeTimers()); +/** The write edge as mailbox-wiring binds it: no in-lock refusal, so the write always runs. */ +function write(session: WritableSession & { id: string }, message: string, noEnter: boolean) { + return submitMessagePaced(session, message, noEnter, () => null); +} + +describe('submitMessagePaced — dropped-write threading (Spec 1313 silent-loss fix)', () => { + beforeEach(() => { + resetSubmissionChains(); + vi.useFakeTimers(); + }); afterEach(() => vi.useRealTimers()); describe('short message (single write + delayed Enter)', () => { it('all writes land → resolves true, text + Enter both on the wire', async () => { const session = makeSession(); - const result = await settle(writeMessagePaced(session, 'hello', false)); + const result = await settle(write(session, 'hello', false)); expect(result).toBe(true); expect(session.writes).toEqual(['hello', '\r']); @@ -59,7 +80,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('the FIRST (synchronous) write drops → resolves false', async () => { // The socket is already dead when the text write fires at t=0. const session = makeSession((_d, i) => i === 0); - const result = await settle(writeMessagePaced(session, 'hello', false)); + const result = await settle(write(session, 'hello', false)); expect(result).toBe(false); expect(session.writes[0]).toBe('hello'); // it WAS attempted @@ -69,7 +90,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f // The critical case the t=0 `writable` precheck cannot see: text writes fine, then // the socket dies before the Enter fires 50ms later, so the submit never completes. const session = makeSession((d) => d === '\r'); - const result = await settle(writeMessagePaced(session, 'hello', false)); + const result = await settle(write(session, 'hello', false)); expect(result).toBe(false); expect(session.writes).toContain('\r'); // the Enter was attempted (and dropped) @@ -77,7 +98,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('noEnter, text lands → resolves true, no Enter written', async () => { const session = makeSession(); - const result = await settle(writeMessagePaced(session, 'hi', true)); + const result = await settle(write(session, 'hi', true)); expect(result).toBe(true); expect(session.writes).toEqual(['hi']); @@ -85,7 +106,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('noEnter, text drops → resolves false', async () => { const session = makeSession((_d, i) => i === 0); - const result = await settle(writeMessagePaced(session, 'hi', true)); + const result = await settle(write(session, 'hi', true)); expect(result).toBe(false); }); @@ -96,7 +117,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('all lines + Enter land → resolves true, Enter last', async () => { const session = makeSession(); - const result = await settle(writeMessagePaced(session, MSG, false)); + const result = await settle(write(session, MSG, false)); expect(result).toBe(true); expect(session.writes.at(-1)).toBe('\r'); // Enter delivered after every line @@ -107,7 +128,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('a DELAYED middle line drops → resolves false', async () => { // Line 2 ("b\n") fires ~10ms in — a delayed write, not the synchronous first one. const session = makeSession((d) => d === 'b\n'); - const result = await settle(writeMessagePaced(session, MSG, false)); + const result = await settle(write(session, MSG, false)); expect(result).toBe(false); expect(session.writes).toContain('b\n'); // attempted mid-pace, dropped @@ -115,7 +136,7 @@ describe('writeMessagePaced — dropped-write threading (Spec 1313 silent-loss f it('the DELAYED trailing Enter drops (all lines landed) → resolves false', async () => { const session = makeSession((d) => d === '\r'); - const result = await settle(writeMessagePaced(session, MSG, false)); + const result = await settle(write(session, MSG, false)); expect(result).toBe(false); expect(session.writes).toContain('a\n'); // the lines themselves went out diff --git a/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts new file mode 100644 index 000000000..5a960c741 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts @@ -0,0 +1,516 @@ +/** + * Serializer convergence (Issue #1365) — the gated mailbox write edge takes the same + * per-terminal submission lock as `--interrupt` / `--escape`. + * + * Before this change the two paths held DISJOINT locks (per-agent for deliveries, + * per-terminal for operator actions), so they could interleave on one terminal. The + * failure that mattered was not a garbled composer but a **false `delivered`**: a `^C` + * landing inside a delivery's text→Enter window cleared the composer, the delivery's Enter + * submitted nothing, every byte still reported success, and the row was marked delivered + * for a message the agent never saw. `--escape` produces the truncated variant. + * + * The composer double below is what makes that visible: it models the one TUI behaviour the + * bug depends on — `^C`/ESC discard pending input, Enter submits whatever has accumulated. + * Each corruption test is paired with a bypass-the-lock control that reproduces the original + * bug, so these assert the fix rather than merely exercising it. + * + * Real timers throughout (the paced writer schedules on real `setTimeout`), with the + * production pacing constants: lines 10 ms apart, Enter 80 ms after the last line. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import Database from 'better-sqlite3'; +import { GLOBAL_SCHEMA } from '../db/schema.js'; +import * as mailbox from '../db/mailbox.js'; +import { + submitToSession, + trySubmitToSession, + isSubmissionInFlight, + pendingSubmissionSessions, + resetSubmissionChains, + unserializedWriteCount, + OPERATOR_SUBMIT_WAIT_CEILING_MS, +} from '../servers/session-submit.js'; +import { + submitMessagePaced, + writeMessageToSession, + writeEscapeToSession, +} from '../servers/message-write.js'; +import { + deliverAgentMail, + MailboxDrainer, + type DeliveryPorts, + type DeliverySession, + type WriteAbort, +} from '../servers/mailbox-delivery.js'; +import type { GateProfile, GateVerdict } from '../servers/render-gate.js'; + +const PROFILE: GateProfile = { app: 'claude', markerPattern: /^❯/, regionEndPatterns: [] }; +const CLEAN: GateVerdict = { clean: true, detail: 'empty' }; +const WS = '/ws/a'; +const AGENT = 'spir-1'; + +/** A body long enough to take the paced multi-line path (≥4 lines) and so hold the line ~110 ms. */ +const MULTILINE = 'L1\nL2\nL3\nL4'; +/** Time for a MULTILINE paced write to finish: last line at 30 ms + the 80 ms Enter delay. */ +const MULTILINE_DONE_MS = 3 * 10 + 80; + +const sleep = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)); + +/** + * A composer that models the failure. `^C` and ESC discard pending input the way a real + * TUI does; Enter submits whatever has accumulated. Everything else is text. + */ +function makeComposer(id = 'term-1') { + let pending = ''; + const submitted: string[] = []; + const bypasses: string[] = []; + const session = { + id, + write(data: string): boolean { + if (data === '\r') { + submitted.push(pending); + pending = ''; + } else if (data === '\x03') { + bypasses.push('interrupt'); + pending = ''; + } else if (data === '\x1b') { + bypasses.push('escape'); + pending = ''; + } else { + pending += data; + } + return true; + }, + }; + return { + session, + submitted, + bypasses, + get pending() { + return pending; + }, + }; +} + +/** The immediate `--interrupt` submission, exactly as tower-routes issues it. */ +function interruptSubmission(session: { id: string; write(d: string): boolean }, body: string) { + return () => { + session.write('\x03'); + return writeMessageToSession(session, body, false, 100); + }; +} + +/** A gated delivery's write edge, as mailbox-wiring binds it. */ +function deliveryWrite(session: { id: string; write(d: string): boolean }, body: string) { + return submitMessagePaced(session, body, false, () => null); +} + +describe('Issue #1365 — the gated write edge and the operator paths share one lock', () => { + beforeEach(() => resetSubmissionChains()); + + it('an interrupt cannot land inside a delivery text→Enter window (the false-delivered bug)', async () => { + const c = makeComposer(); + + const delivery = deliveryWrite(c.session, MULTILINE); + const interrupt = submitToSession(c.session.id, interruptSubmission(c.session, 'INT')); + const [result] = await Promise.all([delivery, interrupt]); + + // The delivery's body was submitted WHOLE, and the interrupt's separately. + expect(c.submitted).toEqual([MULTILINE, 'INT']); + // ...and only then may the row be marked delivered. + expect(result).toEqual({ status: 'written' }); + }); + + it('CONTROL: bypassing the lock reproduces the original bug — body lost, write still "succeeds"', async () => { + const c = makeComposer(); + + // Exactly the pre-#1365 delivery edge: a paced write under no per-terminal lock. + writeMessageToSession(c.session, MULTILINE, false); + await submitToSession(c.session.id, interruptSubmission(c.session, 'INT')); + await sleep(MULTILINE_DONE_MS + 20); + + // The ^C cleared the composer mid-write; the delivery's Enter submitted an empty line. + // Every byte reached the PTY, so the old boolean-returning port reported success and the + // row was marked delivered — a message the agent never saw. + expect(c.submitted).not.toContain(MULTILINE); + expect(c.submitted).toContain(''); + }); + + it('an escape cannot truncate an in-flight multi-line delivery', async () => { + const c = makeComposer(); + + const delivery = deliveryWrite(c.session, MULTILINE); + const escape = submitToSession(c.session.id, () => writeEscapeToSession(c.session, false)); + const [result] = await Promise.all([delivery, escape]); + + expect(c.submitted[0]).toBe(MULTILINE); // whole, not a tail of it + expect(result).toEqual({ status: 'written' }); + }); + + it('CONTROL: bypassing the lock lets an escape submit a TRUNCATED body', async () => { + const c = makeComposer(); + + writeMessageToSession(c.session, MULTILINE, false); + await submitToSession(c.session.id, () => writeEscapeToSession(c.session, false)); + await sleep(MULTILINE_DONE_MS + 20); + + // The ESC discarded the lines written so far; the escape's own Enter then submitted the + // remainder as if it were the whole message. + expect(c.submitted[0]).not.toBe(MULTILINE); + expect(MULTILINE.endsWith(c.submitted[0])).toBe(true); // a strict tail — i.e. truncated + expect(c.submitted[0].length).toBeLessThan(MULTILINE.length); + }); + + it('two deliveries to one terminal still cannot interleave', async () => { + const c = makeComposer(); + + const first = deliveryWrite(c.session, MULTILINE); + // A second delivery arriving mid-write is DECLINED, not queued — the caller re-holds. + const second = await deliveryWrite(c.session, 'SECOND'); + expect(second).toEqual({ status: 'contended' }); + expect(await first).toEqual({ status: 'written' }); + expect(c.submitted).toEqual([MULTILINE]); + }); + + it('deliveries to DIFFERENT terminals do not serialize (the lock key is the real session id)', async () => { + const a = makeComposer('term-a'); + const b = makeComposer('term-b'); + + const [ra, rb] = await Promise.all([deliveryWrite(a.session, MULTILINE), deliveryWrite(b.session, MULTILINE)]); + + // Both ran. Were the key `undefined` for every session — the hazard a structurally-typed + // fake without an `id` would create — the second would have been declined as contended. + expect(ra).toEqual({ status: 'written' }); + expect(rb).toEqual({ status: 'written' }); + expect(a.submitted).toEqual([MULTILINE]); + expect(b.submitted).toEqual([MULTILINE]); + }); + + it('a session with no id throws instead of keying every lock on undefined', async () => { + const c = makeComposer(); + const idless = { ...c.session, id: '' as string }; + + await expect(submitMessagePaced(idless, 'hi', false, () => null)).rejects.toThrow(/non-empty string/); + expect(c.submitted).toHaveLength(0); + }); + + it('the returned promise resolves only AFTER the trailing Enter', async () => { + const c = makeComposer(); + const result = await deliveryWrite(c.session, MULTILINE); + // Resolution implies submission — the property the per-agent serializer's completion + // chaining depends on. No extra wait: the Enter has already fired. + expect(c.submitted).toEqual([MULTILINE]); + expect(result).toEqual({ status: 'written' }); + }); + + it('reports a dropped write (#1198) rather than a delivery', async () => { + const session = { id: 'term-dead', write: () => false }; + expect(await submitMessagePaced(session, MULTILINE, false, () => null)).toEqual({ status: 'dropped' }); + }); + + it('an in-lock precheck refusal writes nothing', async () => { + const c = makeComposer(); + const abort: WriteAbort = { kind: 'hold', reason: 'busy' }; + + const result = await submitMessagePaced(c.session, MULTILINE, false, () => abort); + + expect(result).toEqual({ status: 'aborted', abort }); + expect(c.submitted).toHaveLength(0); + expect(c.pending).toBe(''); // not one byte, not even un-submitted text + }); + + it('leaves no chain behind once mixed traffic settles', async () => { + const c = makeComposer(); + await Promise.all([ + deliveryWrite(c.session, MULTILINE), + submitToSession(c.session.id, interruptSubmission(c.session, 'INT')), + submitToSession(c.session.id, () => writeEscapeToSession(c.session, false)), + ]); + await sleep(10); + expect(pendingSubmissionSessions()).toBe(0); + expect(isSubmissionInFlight(c.session.id)).toBe(false); + }); +}); + +describe('Issue #1365 — deliveries decline contention, operators wait (bounded)', () => { + beforeEach(() => resetSubmissionChains()); + + it('a delivery declines a contended terminal IMMEDIATELY instead of queueing', async () => { + const c = makeComposer(); + // An operator holds the line for well over a backstop tick. + const holder = submitToSession(c.session.id, () => { + c.session.write('slow'); + setTimeout(() => c.session.write('\r'), 300); + return 300; + }); + + const startedAt = Date.now(); + const result = await trySubmitToSession(c.session.id, () => 0); + const waited = Date.now() - startedAt; + + expect(result).toBe(false); + expect(waited).toBeLessThan(100); // did not wait out the 300 ms holder + await holder; + }); + + it('the drainer keeps delivering to OTHER agents while one terminal is held', async () => { + // The liveness property: MailboxDrainer.tick walks agents sequentially, so a delivery + // that blocked on a terminal lock would stall every other agent behind it. + const busy = makeComposer('term-busy'); + const free = makeComposer('term-free'); + const db = new Database(':memory:'); + db.exec(GLOBAL_SCHEMA); + try { + const sessions = new Map([ + ['agent-busy', sessionFor(busy.session)], + ['agent-free', sessionFor(free.session)], + ]); + const ports = livePorts(sessions); + mailbox.enqueue(db, { workspacePath: WS, toAgent: 'agent-busy', body: 'a', formattedMessage: 'A' }, 1000); + mailbox.enqueue(db, { workspacePath: WS, toAgent: 'agent-free', body: 'b', formattedMessage: 'B' }, 1000); + + const holder = submitToSession('term-busy', () => { + setTimeout(() => busy.session.write('held'), 250); + return 250; + }); + + const startedAt = Date.now(); + const drainer = new MailboxDrainer({ intervalMs: 999_999 }); + drainer.start(ports, db); + await drainer.tick(); + drainer.stop(); + const tickMs = Date.now() - startedAt; + + expect(free.submitted).toEqual(['B']); // the unblocked agent was served + expect(tickMs).toBeLessThan(250); // and the tick did not wait out the held terminal + await holder; + } finally { + db.close(); + } + }); + + it('an operator submission stops waiting at the ceiling and says so', async () => { + const c = makeComposer(); + const ceilingMs = 40; + const expired: number[] = []; + + const holder = submitToSession(c.session.id, () => { + setTimeout(() => c.session.write('\r'), 400); + return 400; + }); + const startedAt = Date.now(); + await submitToSession(c.session.id, () => { c.session.write('\x03'); return 0; }, undefined, { + waitCeilingMs: ceilingMs, + onCeilingExpired: (ms) => expired.push(ms), + }); + const waited = Date.now() - startedAt; + + expect(expired).toEqual([ceilingMs]); // degraded, and announced + expect(waited).toBeLessThan(400); // did not wait out the holder + expect(c.bypasses).toEqual(['interrupt']); // the ^C did go out — the escape hatch still works + expect(unserializedWriteCount(c.session.id)).toBe(1); + await holder; + }); + + it('an operator submission below the ceiling still serializes normally', async () => { + const c = makeComposer(); + const expired: number[] = []; + + const delivery = deliveryWrite(c.session, MULTILINE); // ~110 ms, well under the ceiling + await submitToSession(c.session.id, interruptSubmission(c.session, 'INT'), undefined, { + waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, + onCeilingExpired: (ms) => expired.push(ms), + }); + + expect(expired).toEqual([]); + expect(await delivery).toEqual({ status: 'written' }); + expect(c.submitted).toEqual([MULTILINE, 'INT']); + }); + + it('a delivery raced by a ceiling-expired write reports preempted, never written', async () => { + // The one hole the ceiling opens: an operator that gave up waiting writes into a + // delivery already on the wire. Detected by counting lock bypasses — no re-classify. + const c = makeComposer(); + + const delivery = deliveryWrite(c.session, MULTILINE); + await sleep(5); // let the delivery take the lock + await submitToSession(c.session.id, () => { c.session.write('\x03'); return 0; }, undefined, { + waitCeilingMs: 0, // give up at once — the degraded path + }); + + expect(await delivery).toEqual({ status: 'preempted' }); + }); +}); + +/** Wrap a composer as the DeliverySession the delivery path expects. */ +function sessionFor(session: { id: string; write(d: string): boolean }): DeliverySession { + return { + id: session.id, + bytesWritten: 0, + info: { cols: 110, rows: 32 }, + command: 'claude', + launchArgs: [], + cwd: WS, + writable: true, + write: (d: string) => session.write(d), + }; +} + +/** Delivery ports whose write edge is the REAL locked one, so the lock is under test. */ +function livePorts(sessions: Map, log: string[] = []): DeliveryPorts { + return { + getSessionForAgent: (_ws, agent) => sessions.get(agent) ?? null, + resolveProfile: () => PROFILE, + classify: () => Promise.resolve(CLEAN), + writeMessage: (session, msg, noEnter, precheck) => submitMessagePaced(session, msg, noEnter, precheck), + broadcast: () => {}, + onHeldStateChange: () => {}, + onEscalation: () => {}, + onLiveness: () => {}, + log: (m) => log.push(m), + now: () => 1000, + }; +} + +describe('Issue #1365 — a raced delivery is held, never marked delivered', () => { + let db: Database.Database; + beforeEach(() => { + resetSubmissionChains(); + db = new Database(':memory:'); + db.exec(GLOBAL_SCHEMA); + }); + afterEach(() => db.close()); + + const enqueue = (formattedMessage = MULTILINE) => + mailbox.enqueue(db, { workspacePath: WS, toAgent: AGENT, body: 'hi', formattedMessage }, 1000); + + it('an uncontended delivery still delivers and marks the row', async () => { + const c = makeComposer(); + const ports = livePorts(new Map([[AGENT, sessionFor(c.session)]])); + const row = enqueue(); + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out.delivered).toEqual([row.id]); + expect(mailbox.getById(db, row.id)?.status).toBe('delivered'); + expect(c.submitted).toEqual([MULTILINE]); + }); + + it('a delivery racing an interrupt HOLDS the row instead of reporting it delivered', async () => { + // The regression this issue exists to close. Pre-#1365 this row read `delivered` while + // the agent's composer had been cleared — silent loss with a false audit record. + const c = makeComposer(); + const ports = livePorts(new Map([[AGENT, sessionFor(c.session)]])); + const row = enqueue(); + + // The interrupt takes the terminal first; the delivery must decline, not write into it. + const interrupt = submitToSession(c.session.id, interruptSubmission(c.session, 'INT')); + const out = await deliverAgentMail(ports, db, WS, AGENT); + await interrupt; + + expect(out).toEqual({ delivered: [], reason: 'busy' }); + const stored = mailbox.getById(db, row.id); + expect(stored?.status).toBe('held'); // still deliverable — nothing was lost + expect(stored?.reason).toBe('busy'); + expect(c.submitted).toEqual(['INT']); // only the operator's message went out + }); + + it('the held row delivers on the next pass, once the terminal is free', async () => { + const c = makeComposer(); + const ports = livePorts(new Map([[AGENT, sessionFor(c.session)]])); + const row = enqueue(); + + const interrupt = submitToSession(c.session.id, interruptSubmission(c.session, 'INT')); + await deliverAgentMail(ports, db, WS, AGENT); // declined + await interrupt; + const out = await deliverAgentMail(ports, db, WS, AGENT); // retried + + expect(out.delivered).toEqual([row.id]); + expect(c.submitted).toEqual(['INT', MULTILINE]); + }); + + it('a row dismissed while the write edge runs is NOT re-held (row-resolved)', async () => { + const c = makeComposer(); + const sessions = new Map([[AGENT, sessionFor(c.session)]]); + const ports = livePorts(sessions); + let heldChanges = 0; + ports.onHeldStateChange = () => { heldChanges++; }; + const row = enqueue(); + + // Dismiss inside the lock, at the write instant — the window the in-lock row-status + // re-check exists to cover. The port relays the delivery module's own precheck. + ports.writeMessage = (session, msg, noEnter, precheck) => + submitMessagePaced(session, msg, noEnter, () => { + mailbox.dismiss(db, row.id, 1001); + return precheck(); + }); + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out).toEqual({ delivered: [], reason: null }); // terminal state — not re-held + expect(mailbox.getById(db, row.id)?.status).toBe('dismissed'); + expect(heldChanges).toBeGreaterThan(0); // the indicator was refreshed + expect(c.submitted).toHaveLength(0); // and nothing went on the wire + }); + + it('a session that became unwritable inside the lock holds no-live-pty', async () => { + const c = makeComposer(); + let writable = true; + const session: DeliverySession = { ...sessionFor(c.session), get writable() { return writable; } }; + const ports = livePorts(new Map([[AGENT, session]])); + const row = enqueue(); + + ports.writeMessage = (s, msg, noEnter, precheck) => + submitMessagePaced(s, msg, noEnter, () => { + writable = false; // the shellper socket dies while we hold the lock + return precheck(); + }); + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out.reason).toBe('no-live-pty'); + expect(mailbox.getById(db, row.id)?.status).toBe('held'); + expect(c.submitted).toHaveLength(0); + }); + + it('a screen that moved inside the lock holds busy', async () => { + const c = makeComposer(); + let bytes = 10; + const session: DeliverySession = { ...sessionFor(c.session), get bytesWritten() { return bytes; } }; + const ports = livePorts(new Map([[AGENT, session]])); + const row = enqueue(); + + ports.writeMessage = (s, msg, noEnter, precheck) => + submitMessagePaced(s, msg, noEnter, () => { + bytes += 1; // new output landed between the gate and the first byte + return precheck(); + }); + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out.reason).toBe('busy'); + expect(mailbox.getById(db, row.id)?.status).toBe('held'); + expect(c.submitted).toHaveLength(0); + }); + + it('a preempted write holds the row and logs why', async () => { + const c = makeComposer(); + const logs: string[] = []; + const ports = livePorts(new Map([[AGENT, sessionFor(c.session)]]), logs); + const row = enqueue(); + + ports.writeMessage = async (s, msg, noEnter, precheck) => { + const write = submitMessagePaced(s, msg, noEnter, precheck); + await sleep(5); + await submitToSession(s.id, () => { c.session.write('\x03'); return 0; }, undefined, { waitCeilingMs: 0 }); + return write; + }; + + const out = await deliverAgentMail(ports, db, WS, AGENT); + + expect(out.reason).toBe('busy'); + expect(mailbox.getById(db, row.id)?.status).toBe('held'); // redelivered later, never falsely delivered + expect(logs.some((l) => l.includes('unserialized'))).toBe(true); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts index 5e0f11aae..17a69b6c8 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts @@ -218,11 +218,17 @@ function makeRes(): { res: http.ServerResponse; body: () => string; statusCode: * requires that proven lower bound, else a bare marker is an indeterminate partial and is held). * `bytesWritten` is the monotone change token the delivery path samples. */ -function gateSession(mockWrite: (data: string) => void, ring: string, writable = true) { +function gateSession(mockWrite: (data: string) => void, ring: string, writable = true, id = 'term-001') { const raw = `${ring}\r\n${'─'.repeat(20)}\r\n`; const gateScreen = new SessionScreen(80, 24); gateScreen.feed(raw); return { + // The per-terminal submission lock's key (Issue #1365). It MUST be a real, distinct id: + // this double reaches the live mailbox-wiring binding, and because the port is + // structurally typed an omitted `id` would compile and silently key every lock on the + // same `undefined` — serialization that looks present and is not. `submitMessagePaced` + // throws on a missing id so that mistake fails loudly instead of quietly. + id, // Model a live PTY: every write lands. The delivery path now threads the write's // boolean (Spec 1313 silent-loss fix), so a double whose write returned undefined // would read as a DROPPED write and be held. Wrap mockWrite so call-assertions still From dcf22a5f29b1feb35f0a07cbcba6fb375276ff5e Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:00:02 -0400 Subject: [PATCH 10/26] [PIR #1365] Document the converged write-edge model in one place Rewrites session-submit.ts's 'Exactly what it covers' boundary comment (in the code commits) and arch.md section 7 item 5 so the model lives in one place rather than as a patchwork of separately-reasoned decisions: which writers take the lock and which stay deliberately uncovered, the per-agent -> per-terminal order, the deliveries-decline / operators-block asymmetry and why each side differs, the ceiling and its degradation, and the delayed-interrupt sequencing (^C on the timer, body through the gate, not atomic by design). States the guarantee honestly, per the plan review: serialization is the structural guarantee; the in-lock precheck NARROWS the echo-lag residual but cannot close it, since ringToken counts output and un-echoed input from an uncovered writer still reads as unchanged. That residual is #1473. The precheck's structural value is that it keeps the acquisition policy a free choice -- switching the delivery from declining to waiting (e.g. #1481 ordering an interrupt ahead of its body) stays safe. Co-Authored-By: Claude Opus 5 (1M context) --- codev/resources/arch.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/codev/resources/arch.md b/codev/resources/arch.md index 9079f4fc9..3a364e393 100644 --- a/codev/resources/arch.md +++ b/codev/resources/arch.md @@ -1795,7 +1795,15 @@ Spec 1313 replaced Spec 403's in-memory, timer-based, force-flushing `SendBuffer 2. **Gate before every write.** The render-gate (`render-gate.ts`) classifies the session's **persistent bounded headless-screen mirror** (`SessionScreen`, fed the session's output byte-for-byte from birth on the live path — its current viewport IS the live screen) and applies a per-app classifier profile (`gate-profiles.ts`: **claude** & **codex** use a dim-placeholder rule; **agy** uses a color-keyed `placeholderFgPalette` rule — see below). Clean (marker present AND a *positively-bounded* composer region — a rule/status line below the marker, never a scan to the screen bottom — with zero normal-intensity cells) → deliver; else → keep holding. One measured exemption (Spec 1313, 2026-08-06): claude paints a **suggested-command ghost** into an *idle* composer when its own last reply mentioned a runnable command, and the ghost's first char doubles as the software block cursor — SGR-7 **inverse at normal intensity** over a SGR-2-dim tail. The classifier exempts exactly that one cell (inverse + non-dim, at the headless cursor, with a **non-empty** dim tail as positive ghost evidence — `isGhostCursorCell`), so an idle ghost no longer classifies `busy` forever and strands mail to an unattended agent; it is deliberately not a blanket inverse skip (an inverse selection over a real draft keeps every other cell counted), and a real draft never trips it because claude never inverse-renders typed text (the block cursor rests on trailing whitespace) and a lone inverse cell with an empty tail — a 1-char draft with the cursor on its only char — stays `busy` (fail-toward-hold). codex renders its own ghost wholly dim, so the dim rule already covers it. **Why a persistent mirror, not a whole-ring re-render** (Spec 1313 round 2): the gate originally rebuilt the screen each check by replaying the whole output ring through a throwaway terminal. But #1205 caps the ring's newline-free `partial` at 2 MiB (`trimPartial` halves to ~1 MiB), and a claude/codex alt-screen frame is one giant partial — so once a busy long-lived agent's frame crossed the cap the ring handed the gate a **torn** front (dropped composer marker/rule → permanent `busy`), resurrecting the over-ceiling outage for exactly the busiest agents. A bounded terminal mirror needs only the live byte stream, so the cap is irrelevant, the live-ring tear is gone, each classify is O(viewport) not O(ring size), and the whole-render era's unbounded-`partial` OOM risk (#1047) is closed. The mirror is fed at `PtySession`'s single output chokepoint (`onPtyData` + the `attachShellper` replay seed), in lockstep with `RingBuffer.bytesWritten` — the **monotone** gate change-token (cumulative output bytes; the old `currentSeq:partialBytes` pair fell on a trim and could alias a stale verdict). The drainer still memoizes each verdict against the live session + that token so a static held screen skips re-classify; the cost-aware backstop backoff the whole-render era needed is retired (a viewport classify is cheap regardless of history). The throwaway-terminal `classifyScreen(snapshot)` path survives only for the fixture suite. (Caveat: on adopt/reconnect after a Tower restart the mirror is seeded from the FULL shellper replay — ≤8 MB wire cap; only the *ring* seed is `capRingSeed`-trimmed to 1 MiB (PIR #1354) — so a long-lived alt-screen frame is born torn only when its coherent start predates the shellper's whole retention; that residual case classifies not-clean → the gate HOLDS, fail-safe, and self-heals on the next repaint. Startup cost is bounded by draining each >1 MiB seed's parse before adopting the next session, measured ~58 ms/session at 8 MB. Residual tracked as #1361.) 3. **Honest response vocabulary.** `delivered` (gate passed, write completed) or `held` + row id + **why-held reason** ∈ {`busy` (draft/menu/mode), `no-profile` (unknown app), `no-live-pty` (no live terminal)}. Additive over the old shape (`ok`/`terminalId`/`deferred` retained for old binaries; a `held` outcome is still `ok:true`). Surfaced to senders through `packages/core/src/tower-client.ts` and `commands/send.ts` (both single-send and `--all` aggregation). 4. **Delivery moments** (each runs the gate; the gate decides): enqueue-time, **user-submit** trigger, **output-quiescence** trigger (Spec 467 `lastDataAt`), and a **poll backstop** (`DEFAULT_BACKSTOP_INTERVAL_MS = 1500`). Submit/quiescence come from `PtySession`'s single `handleUserInput` chokepoint. A missed trigger only delays to the next backstop — it can't corrupt anything (triggers *schedule*; they never authorize). -5. **Per-agent write serialization.** `write-queue.ts` chains writes for one agent on completion (keyed by `agentKey` — *per-agent*, not per-PTY; a message's text and its Enter are one unit), so concurrent gated deliveries to one agent can't interleave/blob. This is a **disjoint** lock from the per-terminal submission lock that `escape`/`interrupt` take (`session-submit.ts`): a gated delivery is *not* serialized against a concurrent interrupt/escape — an accepted, documented boundary (a gated delivery only ever writes onto a render-verified empty prompt, and `interrupt` is the explicit gate-bypassing human action), not full per-session atomicity. Held rows drain **oldest-eligible-first**: a row is eligible when `not_before IS NULL OR not_before <= now`, so a pre-due delayed row (Spec 1313 round 3) is excluded from the scan and never blocks a later row that is already due. +5. **Write serialization — two locks, one order (Issue #1365).** `write-queue.ts` chains writes for one *agent* on completion (keyed by `agentKey` — per-agent, not per-PTY; a message's text and its Enter are one unit), so concurrent gated deliveries to one agent can't interleave/blob. The delivery's write edge then takes the per-*terminal* submission lock (`session-submit.ts`) as a **leaf** inside that per-agent serializer, via `submitMessagePaced` — the same lock `escape`/`interrupt`/the delayed `^C` take. **Lock order is always per-agent → per-terminal, never the reverse**, so there is no cycle; re-entrancy is impossible too, since `PtySession.write()` emits no `'submit'` signal (only `handleUserInput` does). The lock wraps the *write* only, never the gate classify — `--interrupt` is the human's escape hatch and must not queue behind a screen classification. Held rows drain **oldest-eligible-first**: a row is eligible when `not_before IS NULL OR not_before <= now`, so a pre-due delayed row (Spec 1313 round 3) is excluded from the scan and never blocks a later row that is already due. + + *Why converged.* Until #1365 the two locks were **disjoint** and the resulting cross-path race was an accepted boundary ("a gated delivery only ever writes onto a render-verified empty prompt; `interrupt` is an explicit human action"). That reasoning covered one ordering and missed two: a `^C` landing inside the *delivery's own* text→Enter window (50–130 ms+) cleared the composer, so the delivery's Enter submitted nothing — yet every byte reached the PTY, the write reported success, and the row was marked **`delivered` for a message the agent never saw**; `--escape` produced the truncated variant, and is the *more* likely trigger for a long body, whose exposed window is longest. The "a human is at this terminal" premise also fails for the **delayed** `^C`, which fires unattended. + + *Asymmetric acquisition.* A delivery **declines** a contended terminal (`trySubmitToSession` → hold `busy`, retry next pass) rather than queueing, because `MailboxDrainer.tick` walks agents sequentially and one blocked delivery would stall every other agent's mail plus that tick's escalation/prune. It costs nothing: a contended terminal means the in-lock precheck would have aborted anyway. Operators **block**, bounded by `OPERATOR_SUBMIT_WAIT_CEILING_MS` (2 s) — a paced write runs `(lines−1)×10+80` ms and a body is capped only by `parseJsonBody`'s 1 MiB, so an unbounded wait could stall `afx interrupt` for minutes. Past the ceiling the operator write proceeds **unserialized** with a loud WARN: that is exactly the pre-#1365 behaviour, so it is never worse than the old status quo — only no longer silent. Degraded writes are counted per session, and a delivery that was raced reports `preempted` → **holds its row for redelivery** rather than reporting a delivery that may have been clobbered (trading a possible duplicate for never falsely reporting delivery, the same call the dropped-write branch already makes). + + *What is and isn't guaranteed.* **Serialization is the structural guarantee**: no lock-taking writer can put bytes on a terminal while another's submission is in flight. The delivery additionally re-validates `writable`, the gate's `ringToken`, and the row's own status **inside** the lock — that narrows a window but does not close one, because `ringToken` counts *output*, so un-echoed input from an uncovered writer (the raw `/api/terminals/:id/write` passthrough, or a human's keystrokes — both deliberately uncovered) can still read as unchanged. That echo-lag residual is **#1473**. The in-lock precheck's structural value is that it keeps the acquisition policy a free choice: switching the delivery from declining to waiting (e.g. #1481 ordering an interrupt ahead of its body) stays safe. + + *Delayed `--interrupt` sequencing.* The timer fires **only** the `^C` (inside the lock, re-checking liveness there); the body is an ordinary `not_before` row that delivers through the gate afterwards. So the body can never land mid-turn — but `^C`→body is **not atomic** by design: the gate re-decides, which means the delayed interrupt guarantees "the turn was ended", never "this body is next". 6. **Rows address agents, not PTYs.** A respawned terminal for the same agent drains its predecessor's held mail on the first clean gate pass. Dead-session sends persist as `no-live-pty` and deliver on respawn (no drop-with-WARN). 7. **`--interrupt`** is the sole bypass — an explicit, deliberate sender action (interrupts the agent, writes without a gate check). It is a per-message command, not a timeout/valve, so it does not weaken the no-force-path invariant. `noEnter` sends are gate-checked staging (write text, no Enter) → report `delivered`. From 54d5700891d5f6dabc13defad66d4c62254b5b96 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:00:28 -0400 Subject: [PATCH 11/26] [PIR #1365] Thread log: implement phase Co-Authored-By: Claude Opus 5 (1M context) --- codev/state/pir-1365_thread.md | 35 ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/codev/state/pir-1365_thread.md b/codev/state/pir-1365_thread.md index 5e4ee73a5..9b477680e 100644 --- a/codev/state/pir-1365_thread.md +++ b/codev/state/pir-1365_thread.md @@ -86,3 +86,38 @@ silent. The ceiling value is a judgment call; flagged rather than assumed. Also noted: the suggested `arch-critical.md` hot-tier fact for the lock-order invariant needs a *displacement* (the tier is at its 10-fact cap), so it is proposed in the review phase for the maintainer rather than applied unilaterally. + +## Implement phase (2026-08-17) + +Human approved the gate including the D3 ceiling at 2000 ms. Four commits: lock primitives → +convergence → tests → docs. Full suite green (4879 passed, 0 failed), build clean. + +**Took the architect's optional degraded-path flag.** It earns its complexity for a specific +reason: my own D3 ceiling is what opens that hole. Without it I'd have traded one route to a +false `delivered` (the ^C mid-write) for a rarer one (an operator that gave up waiting and +wrote into a delivery already on the wire) — which is not a fix, it's a relocation. +Implemented as a per-session monotone counter of unserialized writes, sampled around the +delivery's own submission; a bump yields `preempted` → hold for redelivery. No screen +re-classification: the question is only "did anyone bypass the lock while I held it?", and a +counter answers exactly that. It does trade a possible **duplicate** for never falsely +reporting delivery — the same call the existing dropped-write branch already makes, and +documented as such. + +**The review's item-5 hazard was real and it bit immediately.** Adding the runtime id guard +turned 13 `tower-routes.test.ts` tests red — `gateSession` had no `id`, reached the live +wiring, and would have keyed every lock on `undefined`. Silent global lock, no failing +assertion, exactly as claude predicted. Worth recording as the general lesson: a +structurally-typed port makes an omitted field compile *and* pass, so a new lock key needs a +runtime guard, not just a type. + +**Honest note on the in-lock precheck.** With try-lock semantics the delivery never waits, so +the precheck cannot fire from a lock wait in production today — no macrotask can interleave +between the pre-lock checks and the in-lock ones. I kept it (the review asked for it, the +human ratified it) but documented its *actual* value rather than implying it closes a live +race: it backstops the injected port boundary, and it is what keeps the acquisition policy a +free choice if #1481 later wants the delivery to wait behind an interrupt. The tests exercise +it at its own level rather than pretending a production path reaches it. + +Also re-pointed `spec-1313-paced-write-drop` from the retired `writeMessagePaced` onto +`submitMessagePaced`, so the silent-loss guard stays on the live write edge instead of on a +function nothing calls. From a5cc7d57f0c69e52cc3a1b60c89ec7933dba814f Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:01:11 -0400 Subject: [PATCH 12/26] chore(porch): 1365 dev-approval gate-requested --- codev/projects/1365-serializer-convergence-route-m/status.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 4918cdc71..1c0d8d88d 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -11,10 +11,11 @@ gates: approved_at: '2026-08-17T23:46:57.233Z' dev-approval: status: pending + requested_at: '2026-08-18T00:01:11.748Z' pr: status: pending iteration: 1 build_complete: false history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-17T23:46:58.833Z' +updated_at: '2026-08-18T00:01:11.749Z' From ee3a17dfb36ed5137300016e795b8ef0337d232b Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:09:41 -0400 Subject: [PATCH 13/26] [PIR #1365] dev-approval evidence: 4 scenarios against an isolated live Tower afx dev from this worktree would bind the live Tower's port (4100 is shared by design) and restarting the live Tower kills every builder session, so the running-worktree evidence is produced the way send-integration.e2e.test.ts does: this worktree's built Tower spawned on port 14650, real shellper-backed PTY sessions, real HTTP endpoints. Nothing stubbed -- routes -> mailbox -> render gate -> locks -> PTY is the wire path. The oracle is the existing echo-terminal fixture (stty raw -echo; exec cat): the PTY re-emits every byte written to it, in order, so GET /api/terminals/:id/output is a faithful ordered transcript of what every writer actually put on the terminal. The profile resolves through the real wrapped-launch fallback (.builder-start.sh), which is how a live builder's profile resolves -- without it the gate holds everything no-profile and there is nothing to assert about. 66/66 checks. Scenario 3 is the sharpest: the interrupt returned in 2156ms rather than waiting out a ~4.1s paced write, Tower logged the degradation at WARN, and the raced delivery reported preempted and HELD its row -- the degraded path declining to claim a delivery it could not youch for, end to end. Two honest limits, stated in the transcript rather than approximated: the 503 TERMINAL_NOT_WRITABLE branch needs a shellper socket that died while the session still reports running, which cannot be produced from the public API without staging it (covered by tower-routes.test.ts:1560, and untouched by this change); and this fixture's agent exists only as a live terminal, never as a registry-known builder, so a send after its death correctly 404s rather than exercising the hold-instead-of-404 seam. Co-Authored-By: Claude Opus 5 (1M context) --- .../evidence/1365-dev-approval-transcript.txt | 80 ++++ .../codev/scripts/spec-1365-e2e-evidence.mts | 420 ++++++++++++++++++ 2 files changed, 500 insertions(+) create mode 100644 codev/evidence/1365-dev-approval-transcript.txt create mode 100644 packages/codev/scripts/spec-1365-e2e-evidence.mts diff --git a/codev/evidence/1365-dev-approval-transcript.txt b/codev/evidence/1365-dev-approval-transcript.txt new file mode 100644 index 000000000..7cfbbf07f --- /dev/null +++ b/codev/evidence/1365-dev-approval-transcript.txt @@ -0,0 +1,80 @@ +Issue #1365 — dev-approval evidence +isolated Tower on port 14650 (NOT 4100 — the live Tower is untouched) +tower build: /home/user/code/codev_root/codev/.builders/pir-1365/packages/codev/dist/agent-farm/servers/tower-server.js +started: 2026-08-18T00:08:35.776Z + +workspace: /home/user/.agent-farm/test-workspaces/pir1365-rTSkWJ +terminal: 7dbbf04f-cc9d-4444-a11a-b863edf2bb7a (real shellper-backed PTY, echo oracle) + +============================================================================== +SCENARIO 1 — 10x long multi-line send + concurrent --interrupt +============================================================================== + PASS run 0: body never fragmented on the wire (whole=1 starts=1) + PASS run 0: the body was not duplicated — saw 1 + PASS run 0: reported delivered → the body IS on the wire, whole + PASS run 0: the interrupt itself landed + PASS run 0: the interrupt's ^C landed + PASS run 1: body never fragmented on the wire (whole=1 starts=1) + PASS run 1: the body was not duplicated — saw 1 + PASS run 1: reported delivered → the body IS on the wire, whole + PASS run 1: the interrupt itself landed + PASS run 1: the interrupt's ^C landed + … [runs 2-8 identical — 35 further checks, all PASS; trimmed for length] + PASS run 9: body never fragmented on the wire (whole=1 starts=1) + PASS run 9: the body was not duplicated — saw 1 + PASS run 9: reported delivered → the body IS on the wire, whole + PASS run 9: the interrupt itself landed + PASS run 9: the interrupt's ^C landed + + at request time: delivered=10 held=0 + bodies that reached the wire: 10/10 — every one of them WHOLE + (holding is a correct outcome; fragmenting or lying about delivery is not) + rows still held after scenario 1: 0 + +============================================================================== +SCENARIO 2 — --delay 5 --interrupt against a MID-TURN agent +============================================================================== + PASS the send is SCHEDULED, not written at request time + PASS no body on the wire at request time + PASS the ^C fired at due time + PASS the body did NOT land mid-turn (the screen is still a draft) + PASS the body landed EXACTLY once on the clean prompt — saw 1 + PASS the ^C preceded the body + +============================================================================== +SCENARIO 3 — --interrupt against a busy line returns within the 2000ms ceiling +============================================================================== + interrupt round-trip: 2156ms (ceiling 2000ms) + PASS the interrupt returned near the ceiling, not after the whole write — 2156ms + PASS the interrupt still succeeded (the escape hatch works) + delivery outcome: delivered=false held=true reason=busy + tower: [2026-08-18T00:09:14.349Z] [WARN] interrupt → pir-1365-probe (terminal 7dbbf04f...) waited 2000ms for an in-flight write and proceeded UNSERIALIZED — it may interleave with that write. A message long enough to hold the line this long is the usual cause. + tower: [2026-08-18T00:09:16.139Z] [INFO] [mailbox] write to pir-1365-probe @ pir1365-rTSkWJ was raced by an unserialized operator write — holding 1ca72147… for redelivery rather than reporting it delivered + PASS Tower logged the ceiling degradation loudly (WARN) + PASS the raced delivery reported PREEMPTED and held its row for redelivery + PASS the raced delivery did NOT claim delivered — the false-delivered failure cannot recur here + +============================================================================== +SCENARIO 4 — --escape unchanged; a non-writable terminal is refused +============================================================================== + PASS escape returns 200 ok + PASS the ESC byte reached the PTY + PASS its trailing Enter reached the PTY + escape to a killed terminal → 404 NOT_FOUND + PASS an operator action to a dead terminal is REFUSED, never silently dropped + normal send to the same dead agent → 404 held=undefined reason=undefined + PASS an unknown-after-death agent 404s (registry seam needs a real builder — see note) + + NOT SCRIPTED HERE, and why: + 503 TERMINAL_NOT_WRITABLE needs a shellper socket that has DIED while the + session still reports status=running (#1198). That state cannot be forced + from the public API without killing the shellper out of band, which would + be staging the result rather than observing it. It is covered by + tower-routes.test.ts:1560 against the real route, and this change does not + touch that branch — only the lock the branch returns before reaching. + +============================================================================== +RESULT +============================================================================== + 66/66 checks passed + ALL SCENARIOS PASSED diff --git a/packages/codev/scripts/spec-1365-e2e-evidence.mts b/packages/codev/scripts/spec-1365-e2e-evidence.mts new file mode 100644 index 000000000..9ea9f1d33 --- /dev/null +++ b/packages/codev/scripts/spec-1365-e2e-evidence.mts @@ -0,0 +1,420 @@ +/** + * Issue #1365 — dev-approval evidence against a real, ISOLATED Tower. + * + * Running `afx dev` from this worktree would bind the live Tower's port (4100 is shared by + * design) and restarting the live Tower kills every builder session, so the running-worktree + * evidence is produced the way `send-integration.e2e.test.ts` does instead: spawn this + * worktree's built Tower on a private port, register REAL shellper-backed PTY sessions, and + * drive the REAL HTTP endpoints. Nothing is stubbed — routes → mailbox → render gate → the + * locks → PTY is the full wire path under test. + * + * The oracle is an echo terminal (`stty raw -echo; exec cat`), the same fixture the existing + * e2e uses: `cat` re-emits every byte written to the PTY input, in order, into the output + * ring. So `GET /api/terminals/:id/output` is a faithful, ordered transcript of every byte + * every writer put on that terminal — which is exactly what "did these two writers + * interleave?" needs. Raw mode also means the `^C` is echoed as a byte rather than raising + * SIGINT, so the session survives and stays registered across scenarios. + * + * Usage: pnpm --filter @cluesmith/codev build && node --experimental-strip-types \ + * scripts/spec-1365-e2e-evidence.mts + */ + +import { spawn, type ChildProcess } from 'node:child_process'; +import { resolve } from 'node:path'; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync } from 'node:fs'; +import { homedir } from 'node:os'; +import net from 'node:net'; + +const PORT = 14650; // private to this script (14500 = cli-tower-mode, 14600 = send-integration) +const BASE = `http://localhost:${PORT}`; +const TOWER = resolve(import.meta.dirname, '../dist/agent-farm/servers/tower-server.js'); + +const ESC = '\x1b'; +const CTRL_C = '\x03'; +const COMPOSER_RULE = '─'.repeat(22); +const CLEAR = `${ESC}[2J${ESC}[H`; +/** A CLEAN claude composer: marker + dim placeholder only → the render gate delivers. */ +const CLEAN_COMPOSER = `${CLEAR}❯ ${ESC}[2mTry "fix the flaky test"${ESC}[0m\r\n${COMPOSER_RULE}\r\n`; +/** An OCCUPIED composer: a draft at normal intensity → the render gate holds (mid-turn). */ +const DRAFT_COMPOSER = `${CLEAR}❯ ${ESC}[0mdeploy the hotfix to prod\r\n${COMPOSER_RULE}\r\n`; + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +let failures = 0; +let checks = 0; + +function check(ok: boolean, label: string, detail = ''): void { + checks++; + if (!ok) failures++; + console.log(` ${ok ? 'PASS' : 'FAIL'} ${label}${detail ? ` — ${detail}` : ''}`); +} + +function section(title: string): void { + console.log(`\n${'='.repeat(78)}\n${title}\n${'='.repeat(78)}`); +} + +// ---------------------------------------------------------------- Tower lifecycle + +async function portListening(port: number): Promise { + return new Promise((r) => { + const s = new net.Socket(); + s.setTimeout(1000); + s.on('connect', () => { s.destroy(); r(true); }); + s.on('timeout', () => { s.destroy(); r(false); }); + s.on('error', () => r(false)); + s.connect(port, '127.0.0.1'); + }); +} + +/** Everything Tower itself logged — the server-side corroboration for the wire assertions. */ +const towerLog: string[] = []; + +/** Tower log lines matching a pattern, since a marker index (for per-scenario slicing). */ +function towerLogSince(since: number, pattern: RegExp): string[] { + return towerLog.slice(since).filter((l) => pattern.test(l)); +} + +async function startTower(): Promise { + const proc = spawn('node', [TOWER, String(PORT)], { + stdio: ['ignore', 'pipe', 'pipe'], + env: { ...process.env, NODE_ENV: 'test', AF_TEST_DB: `test-1365-${PORT}.db` }, + }); + let stderr = ''; + const collect = (d: Buffer): void => { + for (const line of d.toString().split('\n')) if (line.trim()) towerLog.push(line); + }; + proc.stdout?.on('data', collect); + proc.stderr?.on('data', collect); + proc.stderr?.on('data', (d) => (stderr += d.toString())); + for (let i = 0; i < 75; i++) { + if (await portListening(PORT)) return proc; + await sleep(200); + } + proc.kill(); + throw new Error(`Tower did not start on ${PORT}. stderr:\n${stderr}`); +} + +// ---------------------------------------------------------------- workspace + terminals + +function makeWorkspace(): string { + const base = resolve(homedir(), '.agent-farm', 'test-workspaces'); + mkdirSync(base, { recursive: true }); + const ws = mkdtempSync(resolve(base, 'pir1365-')); + for (const d of ['codev', '.agent-farm', '.codev']) mkdirSync(resolve(ws, d), { recursive: true }); + writeFileSync( + resolve(ws, '.codev', 'config.json'), + JSON.stringify({ shell: { architect: 'sh -c "sleep 3600"', builder: 'bash', shell: 'bash' } }), + ); + // A REAL builder launches through this wrapper, so its PtySession.command is the shell, + // not the harness — and `resolveProfileForSession` recovers the harness by reading this + // file (the wrapped-launch fallback, same code path `afx reset` uses). Without it the gate + // holds every send `no-profile` and nothing would ever be delivered to assert about. This + // is fidelity, not a shortcut: it is exactly how a live builder's profile resolves. + writeFileSync( + resolve(ws, '.builder-start.sh'), + '#!/usr/bin/env bash\nexec claude --dangerously-skip-permissions\n', + ); + return ws; +} + +async function activate(ws: string): Promise { + const encoded = Buffer.from(ws).toString('base64url'); + for (let i = 0; i < 30; i++) { + const res = await fetch(`${BASE}/api/workspaces/${encoded}/activate`, { method: 'POST' }); + if (res.ok) break; + await sleep(500); + } + for (let i = 0; i < 60; i++) { + const list = await (await fetch(`${BASE}/api/workspaces`)).json(); + if (list.workspaces.some((w: { path: string }) => w.path === ws)) return; + await sleep(500); + } + throw new Error('workspace never activated'); +} + +/** A real shellper-backed PTY that echoes its input — see the header for why. */ +async function registerEchoTerminal(ws: string, roleId: string): Promise { + const res = await fetch(`${BASE}/api/terminals`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + command: 'sh', + args: ['-c', 'stty raw -echo 2>/dev/null; exec cat'], + cwd: ws, + cols: 110, + rows: 32, + workspacePath: ws, + type: 'builder', + roleId, + persistent: true, + }), + }); + if (res.status !== 201) throw new Error(`terminal register failed: ${res.status}`); + return (await res.json()).id; +} + +async function paint(terminalId: string, screen: string): Promise { + await fetch(`${BASE}/api/terminals/${terminalId}/write`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ data: screen }), + }); + await sleep(250); // let the PTY echo land and the gate mirror catch up +} + +/** + * Every byte the echo PTY has emitted, in order — the interleaving oracle. + * + * `GET /api/terminals/:id/output` projects the ring as `{lines: string[]}`, so the lines are + * rejoined here. Reading the ring rather than a mock is the point: this is the same output + * stream the render gate classifies and the operator sees. + */ +async function transcript(terminalId: string): Promise { + const res = await fetch(`${BASE}/api/terminals/${terminalId}/output?lines=1000000`); + const data = await res.json(); + return Array.isArray(data.lines) ? data.lines.join('\n') : JSON.stringify(data); +} + +interface SendOptions { interrupt?: boolean; escape?: boolean; deliverAfter?: number } +interface SendResult { status: number; body: Record; elapsedMs: number } + +async function send(ws: string, to: string, message: string, options: SendOptions = {}): Promise { + const startedAt = Date.now(); + const res = await fetch(`${BASE}/api/send`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ to, workspace: ws, from: 'architect', message, options }), + }); + const body = await res.json().catch(() => ({})); + return { status: res.status, body, elapsedMs: Date.now() - startedAt }; +} + +/** The held rows `afx inbox` would show — metadata only, as the route projects them. */ +async function inbox(ws: string): Promise>> { + const res = await fetch(`${BASE}/api/inbox?workspace=${encodeURIComponent(ws)}`); + if (!res.ok) return []; + return res.json(); +} + +// ---------------------------------------------------------------- scenarios + +/** + * A body long enough to take the paced multi-line path and hold the line while the + * interrupt races it. 12 lines ≈ 11×10 + 80 = 190 ms of exposed text→Enter window. + */ +function longBody(i: number): string { + return Array.from({ length: 12 }, (_, n) => `S1-${i}-line${n}`).join('\n'); +} + +/** Occurrences of `needle` in `haystack`. */ +function countOf(haystack: string, needle: string): number { + return haystack.split(needle).length - 1; +} + +/** + * Scenario 1 — a long multi-line send raced by a concurrent interrupt, ×10. + * + * The property under test is NOT "the delivery always wins" — a gated delivery is entitled to + * hold when the line is busy, and a row reported `held` at request time may still be + * delivered moments later by the fast trigger or the backstop. Both are correct. + * + * The invariant is about FRAGMENTATION, which is what interleaving actually looks like on the + * wire: whenever any part of the body reaches the terminal, the WHOLE body reaches it as one + * contiguous run. So count the body's first line and count the whole body — if a racing `^C` + * ever split a delivery, there would be a first line with no whole body behind it, and the + * two counts would diverge. That is a sharper detector than "is the body present", and it is + * exactly the failure this issue exists to remove. + */ +async function scenario1(ws: string, agent: string, terminalId: string): Promise { + section('SCENARIO 1 — 10x long multi-line send + concurrent --interrupt'); + let deliveredAtRequest = 0; + let heldAtRequest = 0; + let landedWhole = 0; + + for (let i = 0; i < 10; i++) { + await paint(terminalId, CLEAN_COMPOSER); + const before = (await transcript(terminalId)).length; + + const body = longBody(i); + const [normal] = await Promise.all([ + send(ws, agent, body), + send(ws, agent, `S1-${i}-INTERRUPT`, { interrupt: true }), + ]); + // Long enough for the paced writes AND for a fast-trigger/backstop redelivery of a row + // that was held at request time, so the observed end state is stable. + await sleep(2200); + + const after = (await transcript(terminalId)).slice(before); + const wholeBodies = countOf(after, body); + const firstLines = countOf(after, `S1-${i}-line0`); + + // THE assertion: no fragment of the body exists that is not part of a whole body. + check(firstLines === wholeBodies, + `run ${i}: body never fragmented on the wire (whole=${wholeBodies} starts=${firstLines})`); + check(wholeBodies <= 1, `run ${i}: the body was not duplicated`, `saw ${wholeBodies}`); + + if (normal.body.delivered === true) { + deliveredAtRequest++; + check(wholeBodies === 1, `run ${i}: reported delivered → the body IS on the wire, whole`); + } else { + heldAtRequest++; + // Held at request time is fine; it may still deliver via the fast trigger/backstop. + console.log(` note run ${i}: held (${String(normal.body.reason)}) at request time, ` + + `${wholeBodies === 1 ? 'delivered whole by a later gated pass' : 'still pending'}`); + } + if (wholeBodies === 1) landedWhole++; + + check(after.includes(`S1-${i}-INTERRUPT`), `run ${i}: the interrupt itself landed`); + check(after.includes(CTRL_C), `run ${i}: the interrupt's ^C landed`); + } + + console.log(`\n at request time: delivered=${deliveredAtRequest} held=${heldAtRequest}`); + console.log(` bodies that reached the wire: ${landedWhole}/10 — every one of them WHOLE`); + console.log(' (holding is a correct outcome; fragmenting or lying about delivery is not)'); + + const held = await inbox(ws); + console.log(` rows still held after scenario 1: ${held.length}` + + (held.length ? ` (${held.map((r) => String(r.reason)).join(', ')}) — still deliverable, nothing lost` : '')); +} + +/** Scenario 2 — `--delay 5 --interrupt` against a mid-turn agent. */ +async function scenario2(ws: string, agent: string, terminalId: string): Promise { + section('SCENARIO 2 — --delay 5 --interrupt against a MID-TURN agent'); + await paint(terminalId, DRAFT_COMPOSER); // mid-turn: the gate must hold the body + const before = (await transcript(terminalId)).length; + + const BODY = 'S2-DELAYED-BODY'; + const scheduled = await send(ws, agent, BODY, { deliverAfter: 5, interrupt: true }); + check(scheduled.body.scheduled === true, 'the send is SCHEDULED, not written at request time'); + check(!(await transcript(terminalId)).slice(before).includes(BODY), 'no body on the wire at request time'); + + await sleep(6500); // past the due time + const afterDue = (await transcript(terminalId)).slice(before); + check(afterDue.includes(CTRL_C), 'the ^C fired at due time'); + check(!afterDue.includes(BODY), 'the body did NOT land mid-turn (the screen is still a draft)'); + + // The turn ends: a clean prompt appears, and the gate lets the body through. + await paint(terminalId, CLEAN_COMPOSER); + for (let i = 0; i < 20 && !(await transcript(terminalId)).slice(before).includes(BODY); i++) await sleep(300); + + const final = (await transcript(terminalId)).slice(before); + const occurrences = final.split(BODY).length - 1; + check(occurrences === 1, 'the body landed EXACTLY once on the clean prompt', `saw ${occurrences}`); + check(final.indexOf(CTRL_C) < final.indexOf(BODY), 'the ^C preceded the body'); +} + +/** Scenario 3 — an interrupt must not stall behind a long delivery (the D3 ceiling). */ +async function scenario3(ws: string, agent: string, terminalId: string): Promise { + section('SCENARIO 3 — --interrupt against a busy line returns within the 2000ms ceiling'); + await paint(terminalId, CLEAN_COMPOSER); + + // ~400 lines ⇒ 399×10 + 80 ≈ 4.07 s of paced write: comfortably longer than the ceiling, + // so an UNBOUNDED wait would show up as a ~4 s interrupt. This is not a pathological body — + // it is the size of a modest --file attachment. + const hugeBody = Array.from({ length: 400 }, (_, n) => `S3-line${n}`).join('\n'); + const logMark = towerLog.length; + const delivery = send(ws, agent, hugeBody); + await sleep(300); // let the delivery take the terminal lock + + const interrupt = await send(ws, agent, 'S3-INTERRUPT', { interrupt: true }); + console.log(` interrupt round-trip: ${interrupt.elapsedMs}ms (ceiling 2000ms)`); + check(interrupt.elapsedMs < 3200, 'the interrupt returned near the ceiling, not after the whole write', + `${interrupt.elapsedMs}ms`); + check(interrupt.status === 200, 'the interrupt still succeeded (the escape hatch works)'); + + const deliveryResult = await delivery; + console.log(` delivery outcome: delivered=${String(deliveryResult.body.delivered)} ` + + `held=${String(deliveryResult.body.held)} reason=${String(deliveryResult.body.reason)}`); + await sleep(500); + + // Server-side corroboration. Two lines matter, and together they are the whole D3 story: + // the operator announced its degradation, and the raced delivery refused to claim success. + const degraded = towerLogSince(logMark, /UNSERIALIZED/); + const preempted = towerLogSince(logMark, /raced by an unserialized/); + for (const l of [...degraded, ...preempted]) console.log(` tower: ${l.trim()}`); + check(degraded.length === 1, 'Tower logged the ceiling degradation loudly (WARN)'); + check(preempted.length === 1, 'the raced delivery reported PREEMPTED and held its row for redelivery'); + check(deliveryResult.body.delivered !== true, + 'the raced delivery did NOT claim delivered — the false-delivered failure cannot recur here'); +} + +/** Scenario 4 — `--escape` is unchanged, and a dead terminal is refused loudly. */ +async function scenario4(ws: string, agent: string, terminalId: string): Promise { + section('SCENARIO 4 — --escape unchanged; a non-writable terminal is refused'); + await paint(terminalId, CLEAN_COMPOSER); + const before = (await transcript(terminalId)).length; + + const esc = await send(ws, agent, '', { escape: true }); + await sleep(300); + const after = (await transcript(terminalId)).slice(before); + check(esc.status === 200 && esc.body.ok === true, 'escape returns 200 ok'); + check(after.includes(ESC), 'the ESC byte reached the PTY'); + check(after.includes('\r'), 'its trailing Enter reached the PTY'); + + // Refusal path: kill the terminal, then retry. Documented honestly below — this exercises + // the no-live-session refusal, NOT the shellper-socket-down 503. + await fetch(`${BASE}/api/terminals/${terminalId}`, { method: 'DELETE' }); + await sleep(1200); + const dead = await send(ws, agent, '', { escape: true }); + console.log(` escape to a killed terminal → ${dead.status} ${String(dead.body.error)}`); + check(dead.status >= 400, 'an operator action to a dead terminal is REFUSED, never silently dropped'); + + const normal = await send(ws, agent, 'S4-NORMAL-AFTER-DEATH'); + console.log(` normal send to the same dead agent → ${normal.status} ` + + `held=${String(normal.body.held)} reason=${String(normal.body.reason)}`); + // This fixture's agent exists ONLY as a live terminal (registered via POST /api/terminals), + // never as a spawned builder in global.db, so once its terminal is killed the agent is + // unknown to the registry and 404 is the correct answer. The Spec 1313 "hold instead of + // 404" seam needs a registry-known agent, which this harness deliberately does not fake — + // it is covered by send-delivery/tower-routes unit tests against the real registry. + check(normal.status === 404, + 'an unknown-after-death agent 404s (registry seam needs a real builder — see note)'); + + console.log('\n NOT SCRIPTED HERE, and why:'); + console.log(' 503 TERMINAL_NOT_WRITABLE needs a shellper socket that has DIED while the'); + console.log(' session still reports status=running (#1198). That state cannot be forced'); + console.log(' from the public API without killing the shellper out of band, which would'); + console.log(' be staging the result rather than observing it. It is covered by'); + console.log(' tower-routes.test.ts:1560 against the real route, and this change does not'); + console.log(' touch that branch — only the lock the branch returns before reaching.'); +} + +// ---------------------------------------------------------------- main + +async function main(): Promise { + console.log(`Issue #1365 — dev-approval evidence`); + console.log(`isolated Tower on port ${PORT} (NOT 4100 — the live Tower is untouched)`); + console.log(`tower build: ${TOWER}`); + console.log(`started: ${new Date().toISOString()}`); + + let tower: ChildProcess | null = null; + let ws = ''; + try { + tower = await startTower(); + ws = makeWorkspace(); + await activate(ws); + + const agent = 'pir-1365-probe'; + const terminalId = await registerEchoTerminal(ws, agent); + console.log(`\nworkspace: ${ws}\nterminal: ${terminalId} (real shellper-backed PTY, echo oracle)`); + + await scenario1(ws, agent, terminalId); + await scenario2(ws, agent, terminalId); + await scenario3(ws, agent, terminalId); + await scenario4(ws, agent, terminalId); + + section('RESULT'); + console.log(` ${checks - failures}/${checks} checks passed`); + console.log(failures === 0 ? ' ALL SCENARIOS PASSED' : ` ${failures} FAILED`); + } finally { + if (tower) { + tower.kill('SIGTERM'); + await sleep(1500); + tower.kill('SIGKILL'); + } + if (ws) rmSync(ws, { recursive: true, force: true }); + } + process.exit(failures === 0 ? 0 : 1); +} + +void main(); From 1483d65c2df83513bf576e93500c6b3240c72922 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:10:10 -0400 Subject: [PATCH 14/26] [PIR #1365] Thread log: dev-approval evidence Co-Authored-By: Claude Opus 5 (1M context) --- codev/state/pir-1365_thread.md | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/codev/state/pir-1365_thread.md b/codev/state/pir-1365_thread.md index 9b477680e..6b8154e52 100644 --- a/codev/state/pir-1365_thread.md +++ b/codev/state/pir-1365_thread.md @@ -121,3 +121,43 @@ it at its own level rather than pretending a production path reaches it. Also re-pointed `spec-1313-paced-write-drop` from the retired `writeMessagePaced` onto `submitMessagePaced`, so the silent-loss guard stays on the live write edge instead of on a function nothing calls. + +## dev-approval evidence (2026-08-18) + +`afx dev` was off the table (4100 is shared; restarting the live Tower kills every builder), +so I built the evidence the way `send-integration.e2e.test.ts` does: this worktree's Tower on +port 14650, real shellper-backed PTYs, real HTTP endpoints, nothing stubbed. Script + +transcript committed (`packages/codev/scripts/spec-1365-e2e-evidence.mts`, +`codev/evidence/1365-dev-approval-transcript.txt`). 66/66. Live Tower on 4100 verified +untouched afterwards; no orphan processes. + +**The oracle is the interesting part.** The echo terminal (`stty raw -echo; exec cat`) re-emits +every byte written to it in order, so `GET /api/terminals/:id/output` is a faithful ordered +record of what each writer actually put on the terminal. That turns "did these two writers +interleave?" into a string question instead of an inference. + +**Two wrong turns worth recording, both mine, both fixed rather than papered over:** + +1. First run: every send held `no-profile`, so nothing ever delivered and the whole scenario + asserted nothing. Cause: a shellper-backed session reports `command: ''`. Fix was to write + a real `.builder-start.sh` so the profile resolves through the *wrapped-launch fallback* — + which is how a genuine builder's profile resolves, so this is fidelity, not a workaround. +2. A run reported `held` at request time and then had bytes on the wire 400 ms later, which my + assertion called a lie. It wasn't: the fast trigger/backstop had legitimately delivered the + row after the response. My assertion was wrong. Rather than loosen it I made it *sharper* — + count whole bodies vs. count first-lines, so any fragment without a whole body behind it + fails. That is a better interleaving detector than "is the body present", because + fragmentation is precisely what interleaving looks like on the wire. + +**Scenario 3 is the strongest single piece of evidence** and it exercises the degraded path I +added: the interrupt returned in 2156 ms instead of waiting out a ~4.1 s paced write; Tower +logged the degradation at WARN; and the raced delivery reported `preempted` and *held its row* +— `delivered=false held=true reason=busy`. The ceiling and its compensating flag both firing +end to end, on the real wire. + +**Two limits stated rather than approximated:** the 503 `TERMINAL_NOT_WRITABLE` branch needs a +shellper socket that died while the session still reports `running`, which can't be produced +from the public API without staging it (covered by `tower-routes.test.ts:1560`, untouched by +this change); and this fixture's agent is only ever a live terminal, never a registry-known +builder, so a send after its death correctly 404s instead of exercising the +hold-instead-of-404 seam. From 2e70d300c9b94ef8b8e1e4b361247e4f7bc392b6 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:12:08 -0400 Subject: [PATCH 15/26] chore(porch): 1365 dev-approval gate-approved --- .../projects/1365-serializer-convergence-route-m/status.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 1c0d8d88d..1a37acdde 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -10,12 +10,13 @@ gates: requested_at: '2026-08-17T23:31:21.297Z' approved_at: '2026-08-17T23:46:57.233Z' dev-approval: - status: pending + status: approved requested_at: '2026-08-18T00:01:11.748Z' + approved_at: '2026-08-18T00:12:08.059Z' pr: status: pending iteration: 1 build_complete: false history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-18T00:01:11.749Z' +updated_at: '2026-08-18T00:12:08.060Z' From d2ba06dfa40a1d7b306ed96663e661d3905a57af Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:12:09 -0400 Subject: [PATCH 16/26] chore(porch): 1365 review phase-transition --- .../projects/1365-serializer-convergence-route-m/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 1a37acdde..9f7056167 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -1,7 +1,7 @@ id: '1365' title: serializer-convergence-route-m protocol: pir -phase: implement +phase: review plan_phases: [] current_plan_phase: null gates: @@ -19,4 +19,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-18T00:12:08.060Z' +updated_at: '2026-08-18T00:12:09.510Z' From ad824bf2208bd876a258741d2be99566cfe6e508 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:14:26 -0400 Subject: [PATCH 17/26] [PIR #1365] Review + retrospective Routes the governance updates by tier: the arch.md section 7 rewrite landed with the code; the hot-tier lock-order invariant is APPENDED to the existing mailbox-first fact rather than added as an eleventh, so the 10-fact cap holds with no displacement. Three cold lessons. Both hot lessons candidates were considered and deliberately not promoted -- promoting either needs a displacement, which is the maintainer's call, not a builder's. Co-Authored-By: Claude Opus 5 (1M context) --- codev/resources/arch-critical.md | 2 +- codev/resources/lessons-learned.md | 3 + .../1365-serializer-convergence-route-m.md | 180 ++++++++++++++++++ 3 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 codev/reviews/1365-serializer-convergence-route-m.md diff --git a/codev/resources/arch-critical.md b/codev/resources/arch-critical.md index 17bfac744..97d79af83 100644 --- a/codev/resources/arch-critical.md +++ b/codev/resources/arch-critical.md @@ -14,7 +14,7 @@ and keeps the map in sync with arch.md's top-level sections. See codev/resources - State lives in a single user-global ~/.agent-farm/global.db (Issue #1118 retired the per-workspace state.db; architect/builders keyed by workspace_path); one Tower on port 4100. Never modify state by hand. - Worktrees in .builders/ are Agent-Farm-managed — never delete manually (use afx cleanup); run afx from the main workspace root only. - Server/client isolation (#1189): codev-core (server) and codev-sdk (client) never import each other; both import only codev-types. The sdk is environment-agnostic (no node:*/vscode/direct fetch outside its /node adapter; zero runtime deps) — boundary tests on both sides enforce this in CI. -- `afx send` is mailbox-first (Spec 1313): persist to global.db first, then deliver only onto a render-gate-verified empty prompt. Any new message writer routes through the mailbox+gate — never write a PTY directly, never force-inject. Response: `delivered` | `held`+reason. +- `afx send` is mailbox-first (Spec 1313): persist to global.db first, then deliver only onto a render-gate-verified empty prompt. Any new message writer routes through the mailbox+gate — never write a PTY directly, never force-inject. Response: `delivered` | `held`+reason. Every message writer also takes the per-terminal `submitToSession` lock (#1365); lock order is per-agent → per-terminal, never the reverse. - Two human gates (spec-approval, plan-approval) plus the pr gate; only humans transition conceived→specified and committed→integrated. ## Map of arch.md (consult when…) diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index a6d353a77..9189cec61 100644 --- a/codev/resources/lessons-learned.md +++ b/codev/resources/lessons-learned.md @@ -192,6 +192,9 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated - [From 818] An acceptance criterion of "rule structurally identical to X" is a written-rule trap when the rule lives as duplicated prose in two views. Two copies drift even with diligence; the only durable enforcement is one shared function both views import. Extract when the second consumer lands — not before (no abstraction without users) and not later (drift starts on day one). - [From 1107] To place an *interactive* React widget (text input, buttons) inside an `innerHTML`-managed body, don't hand-build DOM there — inject an empty placeholder node in an effect and `createPortal` the React component into it. React owns the widget's state/focus/keyboard, while it still sits in normal document flow. Make the placeholder-injection effect idempotent (reuse a correctly-placed node; bail when `previousElementSibling` already matches the anchor) or the `setState`-on-inject loops; an `html` rebuild disconnects the node, which the same guard detects and re-creates. This is the read-while-write composer (#1107) but applies to any overlay/widget over imperatively-rendered content. - [From #1338] Retiring an entry from a shared resolver/registry must **fail closed at every resolution path**, not just delete the entry: a pure delete makes the explicit-name path throw a generic "unknown" error (no migration guidance) and the auto-detect path *silently* fall back to the default provider (here, the claude harness) — a dangerous mis-injection, not a visible failure. Keep the retired name in the detector and add a retirement sentinel checked BEFORE both exits so every path yields the same specific message; then grep every caller — spawn preflight, launch, and especially the reconnect/clean-exit relaunch paths that mint fresh sessions — because those are exactly the ones an "it's unreachable" analysis misses (three of them surfaced only under adversarial review here). +- [From #1365] "Every byte reached the PTY" is not "the message landed." A write-success boolean sees transport acceptance, not semantic loss: a `^C` or ESC that clears a TUI composer mid-write leaves every `write()` returning `true`, so the row was marked `delivered` for a message the agent never saw. Any success signal derived from "did the transport accept the bytes" needs a second question — "could anything have discarded them?" — answered from a source the transport can't lie about. Here that is a per-terminal counter of lock bypasses sampled around the write, deliberately NOT a re-read of the screen (detect-and-repair is the architecture Spec 1313 replaced). +- [From #1365] A structurally-typed port makes an omitted field compile AND pass, so a new lock/identity key needs a **runtime** guard, not just a type. A test double reaching the live wiring without an `id` keyed every per-terminal lock on `undefined` — one silently global lock, serialization that looks present and is not, with no failing assertion anywhere. Adding a throw on the missing key turned it into 13 loud failures immediately. The general shape: when a value becomes a *key* (lock, cache, registry), assert its presence at the boundary — type-checking the shape does not check the key. +- [From #1365] Converging two locks is not just "take the same lock": the acquisition POLICY has to match each caller's liveness needs. A blocking acquisition would have been a regression in both directions — a gated delivery blocking would stall the drainer's sequential walk over every OTHER agent, and an operator action blocking behind a paced write (bounded only by the 1 MiB body cap ≈ minutes) would stall the human's escape hatch. The shape that works is asymmetric: the background writer declines contention and retries on its existing schedule; the operator waits, but boundedly, degrading to the documented pre-existing behaviour with a loud log rather than to a hang. ## Process diff --git a/codev/reviews/1365-serializer-convergence-route-m.md b/codev/reviews/1365-serializer-convergence-route-m.md new file mode 100644 index 000000000..d282e7fbb --- /dev/null +++ b/codev/reviews/1365-serializer-convergence-route-m.md @@ -0,0 +1,180 @@ +# PIR Review: Serializer convergence — one lock at the terminal write edge + +Fixes #1365 + +## Summary + +The gated mailbox delivery path and the `--interrupt` / `--escape` paths held **disjoint** +locks (per-agent vs per-terminal), so they could interleave on one terminal. The failure that +mattered was not a garbled composer but a **false `delivered`**: a `^C` landing inside a +delivery's own text→Enter window cleared the composer, the delivery's Enter submitted nothing, +every byte still reached the PTY so the write reported success, and the row was marked +delivered for a message the agent never saw. This PR routes the delivery's write edge through +the same per-terminal submission lock, taken as a leaf inside the per-agent serializer +(order: per-agent → per-terminal, no cycle), and lands the resulting model as one documented +boundary instead of three separately-reasoned decisions. + +The issue asked for an evaluation *before* a remedy. That evaluation is in +`codev/plans/1365-serializer-convergence-route-m.md` Part 1; it ratified convergence, and +this is its implementation. + +## Files Changed + +Implementation: + +- `packages/codev/src/agent-farm/servers/session-submit.ts` (+288 / −33) — `trySubmitToSession`, + `isSubmissionInFlight`, `OPERATOR_SUBMIT_WAIT_CEILING_MS` + `SubmitOptions`, + `unserializedWriteCount`, and the rewritten boundary comment +- `packages/codev/src/agent-farm/servers/mailbox-delivery.ts` (+144 / −20) — `DeliverySession.id`, + `WriteAbort` / `WriteResult`, the in-lock precheck, the outcome mapping +- `packages/codev/src/agent-farm/servers/message-write.ts` (+110 / −24) — `submitMessagePaced` + (replaces `writeMessagePaced`), `PacedSubmitResult` +- `packages/codev/src/agent-farm/servers/tower-routes.ts` (+105 / −38) — wait ceiling at the three + operator call sites, `logCeilingExpired`, updated scope comments +- `packages/codev/src/agent-farm/servers/mailbox-wiring.ts` (+8 / −2) — binds the new write edge + +Tests: + +- `packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts` (+516 / −0) — new +- `packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts` (+63 / −18) — re-pointed +- `tower-routes.test.ts` (+8 / −2), `send-delivery.test.ts` (+19 / −8), + `send-mailbox-repro.test.ts` (+7 / −2), `cron-delivery.test.ts` (+8 / −3), + `send-architect-identity.test.ts` (+6 / −2) — fakes updated + +Evidence + docs: + +- `packages/codev/scripts/spec-1365-e2e-evidence.mts` (+420 / −0) — dev-approval evidence script +- `codev/evidence/1365-dev-approval-transcript.txt` (+80 / −0) — its transcript +- `codev/resources/arch.md` (+10 / −2), `codev/resources/arch-critical.md`, + `codev/resources/lessons-learned.md` +- `codev/plans/1365-...md`, `codev/state/pir-1365_thread.md` + +## Commits + +- `2adbe0b9` [PIR #1365] Plan draft: evaluation of the three write paths + convergence design +- `a6cdbe27` [PIR #1365] Plan revised (rev 2): all 5 blocking review items + interrupt-latency ceiling +- `30af22b2` [PIR #1365] Lock primitives: try-acquire, wait ceiling, degraded-write counter +- `e9fd2d42` [PIR #1365] Route the mailbox write edge through the per-terminal submission lock +- `194685e1` [PIR #1365] Tests: interleaving, in-lock precheck, liveness, ceiling, key hygiene +- `dcf22a5f` [PIR #1365] Document the converged write-edge model in one place +- `54d57008` [PIR #1365] Thread log: implement phase +- `ee3a17df` [PIR #1365] dev-approval evidence: 4 scenarios against an isolated live Tower +- `1483d65c` [PIR #1365] Thread log: dev-approval evidence + +## Test Results + +- `pnpm --filter @cluesmith/codev build`: ✓ pass +- `pnpm --filter @cluesmith/codev test`: ✓ pass — **4879 passed / 0 failed / 48 skipped**, + 246 files. 23 new tests in `spec-1365-serializer-convergence.test.ts`. +- **Manual verification** (dev-approval gate, human-approved): `afx dev` was not usable — + 4100 is shared by design and restarting the live Tower kills every builder session — so the + running-worktree evidence was scripted against an **isolated Tower on port 14650** with real + shellper-backed PTYs and real HTTP endpoints (routes → mailbox → render gate → locks → PTY, + nothing stubbed). **66/66 checks.** Full transcript: + `codev/evidence/1365-dev-approval-transcript.txt`; script: + `packages/codev/scripts/spec-1365-e2e-evidence.mts`. + - S1 — 10× long multi-line send raced by `--interrupt`: 10/10 bodies reached the wire + **whole**, zero fragmentation, zero duplication. + - S2 — `--delay 5 --interrupt` mid-turn: scheduled; `^C` at due time; body did **not** land + mid-turn; landed **exactly once** after the prompt cleared; `^C` before body. + - S3 — interrupt vs a busy line: returned in **2156 ms** rather than waiting out a ~4.1 s + paced write; Tower logged the degradation at WARN; the raced delivery reported + `preempted` and **held its row** (`delivered=false held=true reason=busy`). + - S4 — `--escape` unchanged (ESC + Enter on the wire); a dead terminal is refused, never + silently dropped. + - Live Tower on 4100 verified healthy and untouched afterwards; no orphan processes. + +## Architecture Updates + +**COLD — `codev/resources/arch.md` §7 item 5** (rewritten). The old text described the two +locks as disjoint with the cross-path race as an accepted boundary; that is now false. The +replacement carries the whole model in one place: which writers take the lock and which stay +deliberately uncovered, the per-agent → per-terminal order and why there is no cycle, the +deliveries-decline / operators-block asymmetry and the reason each side differs, the wait +ceiling and its degradation, the delayed-interrupt sequencing, and — stated honestly — that +serialization is the structural guarantee while the in-lock precheck only *narrows* the +echo-lag residual (#1473). + +**HOT — `codev/resources/arch-critical.md`**: the existing mailbox-first fact already governs +"any new message writer", so the lock-order invariant was **appended to that fact** rather +than added as an eleventh. This keeps the tier at its 10-fact cap with **no displacement** — +the hot tier gains the one clause a future author actually needs at decision time ("take +`submitToSession`; order is per-agent → per-terminal"), not a second entry on the same +subject. + +## Lessons Learned Updates + +**COLD — `codev/resources/lessons-learned.md` → Architecture**, three entries: + +1. *"Every byte reached the PTY" is not "the message landed."* A write-success boolean sees + transport acceptance, not semantic loss — a `^C` that clears the composer leaves every + write returning `true`. Any success signal derived from "did the transport accept the + bytes" needs a second question, answered from a source the transport can't lie about. +2. *A structurally-typed port makes an omitted field compile **and** pass.* When a value + becomes a **key** (lock, cache, registry), assert its presence at the boundary — type-checking + the shape does not check the key. Here a double without an `id` keyed every per-terminal + lock on `undefined`: a silently global lock, no failing assertion anywhere. +3. *Converging two locks is not just "take the same lock" — the acquisition **policy** has to + match each caller's liveness needs.* Blocking would have regressed both sides (the + sequential drainer, and the human's escape hatch). The shape that works is asymmetric: + the background writer declines contention and retries on its existing schedule; the + operator waits, but boundedly, degrading to documented prior behaviour rather than a hang. + +**Considered for HOT and deliberately not promoted**: both are real but narrower than the +current ten hot lessons, and promoting either would require *displacing* an existing one. +Displacement at the cap is the maintainer's call, not a builder's — flagged here rather than +taken unilaterally. + +## Things to Look At During PR Review + +1. **The in-lock precheck's honest status.** With try-lock semantics the delivery never waits, + so in production today the precheck cannot observe a state change a pre-lock check missed — + no macrotask can interleave between them. I kept it (both plan reviewers asked for it, the + human ratified it) but documented its *actual* value rather than implying it closes a live + race: it backstops the injected port boundary, and it is what keeps the acquisition policy + a free choice if #1481 later wants the delivery to wait behind an interrupt. If a reviewer + would rather not carry code whose value is conditional on a future change, this is the + place to say so. +2. **The wait ceiling is a judgment call** (`OPERATOR_SUBMIT_WAIT_CEILING_MS = 2000`, human- + ratified at the plan gate). It exists because `--interrupt` previously never waited, and a + paced write runs `(lines−1)×10+80` ms against a body capped only by `parseJsonBody`'s + 1 MiB — a 48 KB `--file` of short lines is ~8 minutes. Past the ceiling the operator write + proceeds unserialized, which is exactly the pre-#1365 behaviour, so it is never worse than + the old status quo — only no longer silent. +3. **`preempted` trades a possible duplicate for never falsely reporting delivery.** A + delivery raced by a ceiling-expired write holds its row instead of marking it delivered, so + if the message *did* land intact the gate may deliver it again later. That is the same call + the existing dropped-write branch already makes, and the opposite of the interrupt path's + claim-first tradeoff — the asymmetry is deliberate (an operator's own message vs an + autonomous background delivery), but it is worth a second opinion. +4. **Port signature churn.** `writeMessage` gained a 4th parameter and a typed result across + six test files. One override (`send-delivery.test.ts:604`) previously returned `undefined` + and relied on falsy ⇒ hold; it is now explicit. +5. **`writeMessagePaced` was removed**, not deprecated — its only live caller was the mailbox + wiring. Its drop-semantics test is re-pointed at `submitMessagePaced` so the #1198 + silent-loss guard stays on the live write edge rather than on a function nothing calls. + +**Interlock for #1481 (`--interrupt-after`)**: "interrupt, then deliver this body" is now +expressible as ordered acquisitions of *one* lock rather than a race between two. Two +residuals it must design against, both documented: the `^C`→body gap is gate-mediated and +deliberately **not** atomic (the delayed interrupt guarantees "the turn was ended", never +"this body is next"), and a no-op `^C` is only logged. + +## How to Test Locally + +- **View diff**: VSCode sidebar → right-click builder `pir-1365` → **Review Diff** +- **Run dev**: `afx dev pir-1365` — but note it will contend for the live Tower's port; the + isolated-Tower script below is why the dev-approval evidence took that route instead +- **Re-run the evidence**: `pnpm --filter @cluesmith/codev build && node + --experimental-strip-types packages/codev/scripts/spec-1365-e2e-evidence.mts` + (isolated Tower on 14650, ~90 s, exits non-zero on any failed check) +- **Unit**: `pnpm --filter @cluesmith/codev test spec-1365-serializer-convergence` +- **What to verify**: a long multi-line send raced by `afx interrupt` never fuses and never + leaves an `afx inbox show` row reading `delivered` whose text is absent or partial; another + agent's mail keeps flowing while a large body is mid-delivery; `afx interrupt` stays + responsive against a busy line; `--escape` behaviour is unchanged. + +## Flaky Tests + +None. No tests were skipped or quarantined, and no pre-existing unrelated failures were +touched. From 4f7d856892e5a5895496c614a9164166efdfc296 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:14:35 -0400 Subject: [PATCH 18/26] chore(porch): 1365 record PR #1492 --- .../1365-serializer-convergence-route-m/status.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 9f7056167..46a600530 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -19,4 +19,9 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-18T00:12:09.510Z' +updated_at: '2026-08-18T00:14:35.870Z' +pr_history: + - phase: review + pr_number: 1492 + branch: builder/pir-1365 + created_at: '2026-08-18T00:14:35.869Z' From 9fea5c80ce0e04d7388eafc26ffe1b9cde05c8b9 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 20:14:43 -0400 Subject: [PATCH 19/26] chore(porch): 1365 review build-complete --- .../projects/1365-serializer-convergence-route-m/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 46a600530..9f2829ba4 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -16,10 +16,10 @@ gates: pr: status: pending iteration: 1 -build_complete: false +build_complete: true history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-18T00:14:35.870Z' +updated_at: '2026-08-18T00:14:43.293Z' pr_history: - phase: review pr_number: 1492 From ece06a5e9b5b16d4262e3baee49bcbe2b4fbbe51 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 22:51:44 -0400 Subject: [PATCH 20/26] [PIR #1365] Fix codex/claude finding: kind-aware ceiling + report degraded writes Two real defects found by the PR #1492 consultation (codex and claude converged on the same core finding independently), both verified against the code before fixing. 1. OPERATOR-VS-OPERATOR REGRESSION. Before #1365 submitToSession had no ceiling, so two operator submissions to one terminal were ALWAYS fully serialized (Spec 1273). My ceiling let a second operator degrade past a long body-bearing first operator, making that one pair strictly WORSE than the status quo -- so the 'never worse' claim was false in that corner. Chain entries now carry a SubmissionKind and the ceiling arms only when nothing ahead is an operator (queued counts, not just in-flight). Restores exact pre-#1365 op-op serialization while keeping the escape hatch responsive against a long delivery, which was the ceiling's actual motivation. 'Never worse' is now true per pair, and the four claim-sites that overstated it are corrected. 2. UNREPORTED DEGRADED INTERRUPT BODY. A ceiling-expired interrupt writes its own body unserialized into a still-pacing predecessor, yet the row was claimed delivered up front and the response said delivered:true with no qualification. Claim-first is kept (un-claiming risks double delivery) but the truth is now surfaced: /api/send returns degraded:true + degradedReason, threaded through the SDK client and warned about by afx send. An indicator nobody surfaces is half a fix. Tests: op-op never degrades; a third operator does not bypass a QUEUED one; a body-bearing interrupt crossing the ceiling behind a delivery reports degraded. Note the pre-existing ceiling test needed its holder changed from an operator to a delivery -- that fixture change IS the behaviour change, not a workaround. 4882 passed / 0 failed. Refs #1492. Co-Authored-By: Claude Opus 5 (1M context) --- codev/resources/arch.md | 2 +- .../spec-1365-serializer-convergence.test.ts | 49 +++++++++++- .../agent-farm/__tests__/tower-routes.test.ts | 37 +++++++++- .../codev/src/agent-farm/commands/send.ts | 12 +++ .../src/agent-farm/servers/session-submit.ts | 74 ++++++++++++++++++- .../src/agent-farm/servers/tower-routes.ts | 31 +++++++- packages/sdk/src/tower-client.ts | 15 ++++ 7 files changed, 212 insertions(+), 8 deletions(-) diff --git a/codev/resources/arch.md b/codev/resources/arch.md index 3a364e393..725f0aa3e 100644 --- a/codev/resources/arch.md +++ b/codev/resources/arch.md @@ -1799,7 +1799,7 @@ Spec 1313 replaced Spec 403's in-memory, timer-based, force-flushing `SendBuffer *Why converged.* Until #1365 the two locks were **disjoint** and the resulting cross-path race was an accepted boundary ("a gated delivery only ever writes onto a render-verified empty prompt; `interrupt` is an explicit human action"). That reasoning covered one ordering and missed two: a `^C` landing inside the *delivery's own* text→Enter window (50–130 ms+) cleared the composer, so the delivery's Enter submitted nothing — yet every byte reached the PTY, the write reported success, and the row was marked **`delivered` for a message the agent never saw**; `--escape` produced the truncated variant, and is the *more* likely trigger for a long body, whose exposed window is longest. The "a human is at this terminal" premise also fails for the **delayed** `^C`, which fires unattended. - *Asymmetric acquisition.* A delivery **declines** a contended terminal (`trySubmitToSession` → hold `busy`, retry next pass) rather than queueing, because `MailboxDrainer.tick` walks agents sequentially and one blocked delivery would stall every other agent's mail plus that tick's escalation/prune. It costs nothing: a contended terminal means the in-lock precheck would have aborted anyway. Operators **block**, bounded by `OPERATOR_SUBMIT_WAIT_CEILING_MS` (2 s) — a paced write runs `(lines−1)×10+80` ms and a body is capped only by `parseJsonBody`'s 1 MiB, so an unbounded wait could stall `afx interrupt` for minutes. Past the ceiling the operator write proceeds **unserialized** with a loud WARN: that is exactly the pre-#1365 behaviour, so it is never worse than the old status quo — only no longer silent. Degraded writes are counted per session, and a delivery that was raced reports `preempted` → **holds its row for redelivery** rather than reporting a delivery that may have been clobbered (trading a possible duplicate for never falsely reporting delivery, the same call the dropped-write branch already makes). + *Asymmetric acquisition.* A delivery **declines** a contended terminal (`trySubmitToSession` → hold `busy`, retry next pass) rather than queueing, because `MailboxDrainer.tick` walks agents sequentially and one blocked delivery would stall every other agent's mail plus that tick's escalation/prune. It costs nothing: a contended terminal means the in-lock precheck would have aborted anyway. Operators **block**, bounded by `OPERATOR_SUBMIT_WAIT_CEILING_MS` (2 s) — a paced write runs `(lines−1)×10+80` ms and a body is capped only by `parseJsonBody`'s 1 MiB, so an unbounded wait could stall `afx interrupt` for minutes. **The ceiling may only bypass a DELIVERY write, never another operator** (`SubmissionKind` + a pending-operator count): operator-vs-operator was always fully serialized before #1365, so a ceiling that could skip a queued or in-flight operator would make that pair strictly *worse* than the old behaviour. With that restriction the "never worse than the old status quo" guarantee is true per pair — op↔op unchanged and unbounded; op↔delivery serialized under the ceiling and degraded to the old disjoint-lock behaviour above it; delivery↔delivery unchanged. Past the ceiling the operator write proceeds **unserialized** with a loud WARN **and** an explicit `degraded: true` in the `/api/send` response (surfaced by `afx send`), because the interrupt path has already claimed its row `delivered` and must not report an unqualified success for a body that may have interleaved. Degraded writes are counted per session, and a delivery that was raced reports `preempted` → **holds its row for redelivery** rather than reporting a delivery that may have been clobbered (trading a possible duplicate for never falsely reporting delivery, the same call the dropped-write branch already makes). *What is and isn't guaranteed.* **Serialization is the structural guarantee**: no lock-taking writer can put bytes on a terminal while another's submission is in flight. The delivery additionally re-validates `writable`, the gate's `ringToken`, and the row's own status **inside** the lock — that narrows a window but does not close one, because `ringToken` counts *output*, so un-echoed input from an uncovered writer (the raw `/api/terminals/:id/write` passthrough, or a human's keystrokes — both deliberately uncovered) can still read as unchanged. That echo-lag residual is **#1473**. The in-lock precheck's structural value is that it keeps the acquisition policy a free choice: switching the delivery from declining to waiting (e.g. #1481 ordering an interrupt ahead of its body) stays safe. diff --git a/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts index 5a960c741..944536b6e 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts @@ -295,7 +295,10 @@ describe('Issue #1365 — deliveries decline contention, operators wait (bounded const ceilingMs = 40; const expired: number[] = []; - const holder = submitToSession(c.session.id, () => { + // The holder must be a DELIVERY write: since the codex review of PR #1492 the ceiling + // deliberately refuses to bypass another OPERATOR, so an operator holder would (correctly) + // never expire this ceiling at all. + const holder = trySubmitToSession(c.session.id, () => { setTimeout(() => c.session.write('\r'), 400); return 400; }); @@ -328,6 +331,50 @@ describe('Issue #1365 — deliveries decline contention, operators wait (bounded expect(c.submitted).toEqual([MULTILINE, 'INT']); }); + it('operator vs operator NEVER degrades — the wait stays unbounded, as before #1365', async () => { + // The regression codex caught in PR #1492 review. Before this change `submitToSession` had + // no ceiling at all, so two operator submissions to one terminal were ALWAYS fully + // serialized. A ceiling that could skip a body-bearing operator would make this one pair + // strictly WORSE than the old behaviour — the opposite of the point. + const c = makeComposer(); + const expired: number[] = []; + + // A body-bearing interrupt that holds the line far longer than the ceiling. + const first = submitToSession(c.session.id, interruptSubmission(c.session, 'OP-ONE'), undefined, { + waitCeilingMs: 30, + onCeilingExpired: (ms) => expired.push(ms), + }); + await sleep(5); + const second = submitToSession(c.session.id, interruptSubmission(c.session, 'OP-TWO'), undefined, { + waitCeilingMs: 30, // would fire long before the first operator finishes, if it were armed + onCeilingExpired: (ms) => expired.push(ms), + }); + await Promise.all([first, second]); + + expect(expired).toEqual([]); // neither operator gave up on the other + expect(c.submitted).toEqual(['OP-ONE', 'OP-TWO']); // strictly ordered, neither clobbered + }); + + it('a THIRD operator does not bypass a QUEUED one (a waiting operator counts, not just a writing one)', async () => { + const c = makeComposer(); + const expired: number[] = []; + const opts = { waitCeilingMs: 30, onCeilingExpired: (ms: number) => expired.push(ms) }; + + // Head is a DELIVERY (bypassable); B queues behind it; C must not skip B. + const delivery = deliveryWrite(c.session, MULTILINE); + await sleep(5); + const b = submitToSession(c.session.id, interruptSubmission(c.session, 'OP-B'), undefined, opts); + await sleep(5); + const c3 = submitToSession(c.session.id, interruptSubmission(c.session, 'OP-C'), undefined, opts); + await Promise.all([delivery, b, c3]); + + // B may degrade past the delivery (that is the ceiling's whole purpose); C may not + // degrade past B, so at most one degradation is recorded and the operators stay ordered. + expect(expired.length).toBeLessThanOrEqual(1); + const ops = c.submitted.filter((s) => s.startsWith('OP-')); + expect(ops).toEqual(['OP-B', 'OP-C']); + }); + it('a delivery raced by a ceiling-expired write reports preempted, never written', async () => { // The one hole the ceiling opens: an operator that gave up waiting writes into a // delivery already on the wire. Detected by counting lock bypasses — no re-classify. diff --git a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts index 17a69b6c8..6cc2647e9 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts @@ -21,7 +21,7 @@ import { SessionScreen } from '../../terminal/session-screen.js'; // generation); submitToSession lets a test pre-occupy a session's lock to drive the // shutdown-during-lock-wait window deterministically. import { shutdownDelayedSends } from '../servers/delayed-send.js'; -import { submitToSession, resetSubmissionChains } from '../servers/session-submit.js'; +import { submitToSession, trySubmitToSession, resetSubmissionChains } from '../servers/session-submit.js'; // ============================================================================ // Mocks @@ -1718,6 +1718,41 @@ describe('tower-routes', () => { // Message SHOULD be written — user is idle (Bugfix #492) expect(mockWrite).toHaveBeenCalled(); }); + + it('a body-bearing interrupt that crosses the wait ceiling reports degraded (Issue #1365)', async () => { + // codex review of PR #1492: the interrupt claims its mailbox row `delivered` BEFORE the + // write (un-claiming would risk a double delivery), so if the ceiling expires and the + // write goes out unserialized — possibly interleaving with the delivery it skipped — an + // unqualified `delivered: true` would be a lie of omission. The response must say so. + mockParseJsonBody.mockResolvedValue({ + to: 'architect', message: 'urgent', workspace: '/tmp/ws', options: { interrupt: true }, + }); + mockResolveTarget.mockReturnValue({ + terminalId: 'term-ceiling', workspacePath: '/tmp/ws', agent: 'architect', + }); + const mockWrite = vi.fn(); + mockGetTerminalManager.mockReturnValue({ + getSession: () => gateSession(mockWrite, '❯ ', true, 'term-ceiling'), + listSessions: () => [], + }); + + // Occupy the terminal with a DELIVERY write long enough to outlast the ceiling. A + // delivery is the only thing the ceiling is allowed to bypass. + const holder = trySubmitToSession('term-ceiling', () => 4000); + + const req = makeReq('POST', '/api/send'); + const ctx = makeCtx(); + const { res, statusCode, body } = makeRes(); + await handleRequest(req, res, ctx); + + expect(statusCode()).toBe(200); + const parsed = JSON.parse(body()); + expect(parsed.delivered).toBe(true); // claim-first is preserved... + expect(parsed.degraded).toBe(true); // ...but the sender is told it was not serialized + expect(parsed.degradedReason).toBe('submit-wait-ceiling-expired'); + expect(mockWrite).toHaveBeenCalled(); // the escape hatch still landed + await holder; + }, 10_000); }); // ========================================================================== diff --git a/packages/codev/src/agent-farm/commands/send.ts b/packages/codev/src/agent-farm/commands/send.ts index 6be2cbd6f..a58cc9625 100644 --- a/packages/codev/src/agent-farm/commands/send.ts +++ b/packages/codev/src/agent-farm/commands/send.ts @@ -385,6 +385,18 @@ export async function send(options: SendOptions): Promise { ); } else { logger.success(`Message delivered to ${result.resolvedTo ?? target}`); + // Issue #1365: an interrupt/escape that gave up waiting for the terminal's submission + // lock wrote unserialized, so its bytes may have interleaved with the delivery it + // skipped. The row is claimed `delivered` before the write (un-claiming would risk a + // double delivery), so without this the sender would read an unqualified success for a + // possibly-mangled body. Warn rather than fail: the write did happen. + if (result.degraded) { + logger.warn( + `...but it was NOT serialized against a write already in flight on that terminal ` + + `(${result.degradedReason ?? 'wait ceiling expired'}), so it may have interleaved. ` + + `Check the agent's prompt before assuming it read cleanly.`, + ); + } } } catch (error) { fatal(error instanceof Error ? error.message : String(error)); diff --git a/packages/codev/src/agent-farm/servers/session-submit.ts b/packages/codev/src/agent-farm/servers/session-submit.ts index eb173dcba..d8c598651 100644 --- a/packages/codev/src/agent-farm/servers/session-submit.ts +++ b/packages/codev/src/agent-farm/servers/session-submit.ts @@ -99,6 +99,18 @@ * Operators block — bounded by {@link OPERATOR_SUBMIT_WAIT_CEILING_MS}, because a paced * write runs `(lines−1)×10+80` ms and a body is capped only by `parseJsonBody`'s 1 MiB. * + * The ceiling may only ever bypass a DELIVERY write, never another operator (codex review of + * PR #1492). Operator-vs-operator was ALWAYS fully serialized before #1365 — `submitToSession` + * had no ceiling — so a ceiling that could skip a queued or in-flight operator would make that + * one pair strictly WORSE than the old behaviour, which is the opposite of the point. The + * {@link SubmissionKind} tag plus the pending-operator count is what keeps the guarantee true + * per pair: + * + * - operator vs operator — fully serialized, unbounded wait, exactly as before #1365; + * - operator vs delivery — serialized under the ceiling, and above it degraded to the + * pre-#1365 behaviour (two disjoint locks, i.e. no serialization at all), so never worse; + * - delivery vs delivery — the per-agent serializer, unchanged, plus a declined contention. + * * ### What is guaranteed, and what is not * * **Serialization is the structural guarantee**: no lock-taking writer can put bytes on a @@ -144,11 +156,32 @@ const realClock: SubmitClock = { sleep: (ms: number) => new Promise(resolve => setTimeout(resolve, ms)), }; +/** + * What kind of writer a submission is (Issue #1365, codex review). + * + * Load-bearing for the wait ceiling: the ceiling may only ever let a writer bypass a + * DELIVERY write, never another operator. See {@link SubmitOptions.waitCeilingMs}. + */ +export type SubmissionKind = 'operator' | 'delivery-write'; + /** Options for an operator submission that must not wait unboundedly (Issue #1365). */ export interface SubmitOptions { + /** + * What this submission is. Defaults to `operator` — every pre-#1365 caller of this + * function is one, and defaulting to the kind that is never bypassable is the safe way + * round. + */ + kind?: SubmissionKind; /** * Max ms to wait for an in-flight submission before proceeding UNSERIALIZED. * Omitted (the default) means wait as long as it takes. + * + * **Only armed when nothing ahead of us is an operator submission.** Two operator + * submissions to one terminal were ALWAYS fully serialized before #1365, and a ceiling + * that could bypass an operator would make this pair strictly worse than that — the one + * corner where "never worse than the old status quo" would otherwise be false. The + * ceiling exists to keep the escape hatch responsive against a long DELIVERY, which is + * the only thing it may skip. */ waitCeilingMs?: number; /** Called instead of the write's serialization when {@link waitCeilingMs} expires. */ @@ -167,6 +200,11 @@ const CEILING_EXPIRED = Symbol('ceiling-expired'); * Blocking `--interrupt` — the human's escape hatch for a wedged agent — behind that would be a * worse regression than the interleaving this lock closes. Two seconds comfortably covers every * realistic message while keeping the escape hatch responsive. + * + * It applies ONLY against a delivery write; behind another operator the wait stays unbounded + * (see {@link SubmitOptions.waitCeilingMs}). And when it does expire, the caller must SAY so — + * `/api/send` reports `degraded: true` — because a degraded operator write may interleave with + * the write it skipped, and the interrupt path has already claimed its row `delivered`. */ export const OPERATOR_SUBMIT_WAIT_CEILING_MS = 2000; @@ -182,6 +220,16 @@ export const OPERATOR_SUBMIT_WAIT_CEILING_MS = 2000; */ const unserializedWrites = new Map(); +/** + * Per-session count of OPERATOR submissions queued or in flight (Issue #1365, codex review). + * + * The ceiling consults this before arming: while any operator is ahead of us — running OR + * merely queued — we wait as long as it takes, exactly as every submission did before the + * ceiling existed. A queued operator counts because bypassing one that has not started yet + * is the same violation as bypassing one mid-write. + */ +const pendingOperators = new Map(); + /** * How many unserialized (ceiling-expired) writes this session has seen. * @@ -237,7 +285,15 @@ export function submitToSession( // ceiling timer it would then leave dangling for its whole duration. const contended = chains.has(sessionId); const ceilingMs = options.waitCeilingMs; - const bounded = contended && ceilingMs !== undefined && ceilingMs >= 0; + // An operator ahead of us — in flight or merely queued — makes this an operator-vs-operator + // wait, which was UNBOUNDED before #1365 and must stay unbounded (see waitCeilingMs). + const behindOperator = (pendingOperators.get(sessionId) ?? 0) > 0; + const bounded = contended && ceilingMs !== undefined && ceilingMs >= 0 && !behindOperator; + + // Count ourselves only AFTER the check above, so we do not read our own presence as a + // reason to block, and BEFORE any await, so a later operator sees us while we are queued. + const kind = options.kind ?? 'operator'; + if (kind === 'operator') pendingOperators.set(sessionId, (pendingOperators.get(sessionId) ?? 0) + 1); const current = (async () => { if (bounded) { @@ -280,6 +336,19 @@ export function submitToSession( chains.set(sessionId, tail); + // Release our operator claim as soon as OUR write is done — a later operator may then be + // bypass-eligible again if only deliveries remain ahead of it. + if (kind === 'operator') { + void current.then( + () => undefined, + () => undefined, + ).then(() => { + const remaining = (pendingOperators.get(sessionId) ?? 1) - 1; + if (remaining > 0) pendingOperators.set(sessionId, remaining); + else pendingOperators.delete(sessionId); + }); + } + // Drop the entry once this is the last submission in flight, so the map does // not accumulate one promise per session for the life of the process. // @@ -319,7 +388,7 @@ export async function trySubmitToSession( clock: SubmitClock = realClock, ): Promise { if (isSubmissionInFlight(sessionId)) return false; - await submitToSession(sessionId, write, clock); + await submitToSession(sessionId, write, clock, { kind: 'delivery-write' }); return true; } @@ -332,4 +401,5 @@ export function pendingSubmissionSessions(): number { export function resetSubmissionChains(): void { chains.clear(); unserializedWrites.clear(); + pendingOperators.clear(); } diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index aa6875220..a1beed466 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -1624,6 +1624,9 @@ function holdAndRespond( * that shipped before #1365 (an operator write that never waited). So the degradation is never * worse than the old status quo — but it used to be invisible, and now it is not. */ +/** Machine-readable reason paired with `degraded: true` on an operator send response. */ +const DEGRADED_SUBMIT_REASON = 'submit-wait-ceiling-expired'; + function logCeilingExpired( ctx: RouteContext, action: string, @@ -1999,9 +2002,13 @@ async function handleSend( // Issue #1365: escape is an operator submission — it blocks behind an in-flight write // (including a gated mailbox delivery, which now takes this same lock) so its ESC and // Enter can no longer truncate a delivery mid-pace, but only up to the wait ceiling. + let escapeDegraded = false; await submitToSession(result.terminalId, () => writeEscapeToSession(session, noEnter), undefined, { waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, - onCeilingExpired: (waitedMs) => logCeilingExpired(ctx, 'ESC', toAgent, result.terminalId, waitedMs), + onCeilingExpired: (waitedMs) => { + escapeDegraded = true; + logCeilingExpired(ctx, 'ESC', toAgent, result.terminalId, waitedMs); + }, }); broadcastMessage({ type: 'message', @@ -2012,7 +2019,13 @@ async function handleSend( timestamp: new Date().toISOString(), }); ctx.log('INFO', `Interrupt (ESC) sent: ${from ?? 'unknown'} → ${toAgent} (terminal ${result.terminalId.slice(0, 8)}...)`); - sendJson(res, 200, { ok: true, terminalId: result.terminalId, resolvedTo: toAgent, deferred: false }); + sendJson(res, 200, { + ok: true, + terminalId: result.terminalId, + resolvedTo: toAgent, + deferred: false, + ...(escapeDegraded ? { degraded: true, degradedReason: DEGRADED_SUBMIT_REASON } : {}), + }); return; } @@ -2063,6 +2076,13 @@ async function handleSend( // reported success — the row read `delivered` for a message the agent never saw. The lock // order is per-agent → per-terminal (the delivery takes this one as a leaf), never the // reverse, so there is no cycle. See session-submit.ts for the full boundary. + // Whether this interrupt gave up waiting and wrote unserialized. It must reach the SENDER, + // not just the Tower log (codex review of PR #1492): the row above was claimed `delivered` + // before the write, so a degraded interrupt would otherwise report an unqualified success + // for a body that may have interleaved with the delivery it skipped. We keep the + // claim-first tradeoff (un-claiming risks a double delivery — see above) and tell the + // truth about it instead. + let degraded = false; await submitToSession( result.terminalId, () => { @@ -2072,7 +2092,10 @@ async function handleSend( undefined, { waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, - onCeilingExpired: (waitedMs) => logCeilingExpired(ctx, 'interrupt', toAgent, result.terminalId, waitedMs), + onCeilingExpired: (waitedMs) => { + degraded = true; + logCeilingExpired(ctx, 'interrupt', toAgent, result.terminalId, waitedMs); + }, }, ); broadcastMessage({ @@ -2093,6 +2116,8 @@ async function handleSend( held: false, mailboxId: row.id, reason: null, + // Additive and present only when it happened, so older clients are unaffected. + ...(degraded ? { degraded: true, degradedReason: DEGRADED_SUBMIT_REASON } : {}), }); return; } diff --git a/packages/sdk/src/tower-client.ts b/packages/sdk/src/tower-client.ts index d31114918..c9ce0c4a9 100644 --- a/packages/sdk/src/tower-client.ts +++ b/packages/sdk/src/tower-client.ts @@ -821,6 +821,17 @@ export class TowerClient { held?: boolean; reason?: string; mailboxId?: string; + /** + * Issue #1365: an `interrupt`/`escape` submission that gave up waiting for the terminal's + * submission lock and wrote UNSERIALIZED, so its bytes may have interleaved with the + * delivery write it skipped. Present only when it happened; absent means normally + * serialized. It matters most for `interrupt`, whose mailbox row is claimed `delivered` + * BEFORE the write (un-claiming would risk a double delivery), so `delivered: true` alone + * would otherwise be an unqualified success for a possibly-mangled body. + */ + degraded?: boolean; + /** Machine-readable companion to {@link degraded} (`submit-wait-ceiling-expired`). */ + degradedReason?: string; /** * Spec 1313 round 3: due time (epoch ms) of a scheduled (`deliverAfter`) send. Present * only when `scheduled` — the row is persisted at request time and delivers not before @@ -837,6 +848,8 @@ export class TowerClient { held?: boolean; reason?: string | null; mailboxId?: string; + degraded?: boolean; + degradedReason?: string; notBefore?: number; }>( '/api/send', @@ -872,6 +885,8 @@ export class TowerClient { held: result.data!.held, reason: result.data!.reason ?? undefined, mailboxId: result.data!.mailboxId, + degraded: result.data!.degraded, + degradedReason: result.data!.degradedReason, notBefore: result.data!.notBefore, }; } From 0dc75d8eedc2c607907c63c4ade62876479a183c Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 23:05:09 -0400 Subject: [PATCH 21/26] [PIR #1365] Review round 2: byte-accurate bypass count, counter eviction, claim sweep The two non-blocking correctness notes from the claude lane, plus the residual doc claims. Both blocking findings were already fixed in ece06a5e. 1. THE BYPASS COUNTER NOW COUNTS BYTES, NOT INTENT. It was bumped on ceiling expiry unconditionally, but the delayed `^C` re-checks isStillLive()/writable INSIDE the lock and can return having written nothing. That no-op was still counted, forcing a concurrent delivery into a spurious `preempted` re-delivery -- a duplicate charged for a race that never happened. SubmitOptions.wroteBytes is consulted straight after the write callback, with NO await in between, so the ordering guarantee the old placement provided (a delivery cannot observe our bytes without also observing the bump) is unchanged. 2. unserializedWrites NOW SELF-EVICTS. It was retained for the life of the Tower -- one entry per session that ever degraded, the leak class #1472 fixed. It cannot self-delete on drain the way chains and pendingOperators do, because it must OUTLIVE the submission whose watcher is about to compare against it: a reset landing between a watcher's two reads reads as "nobody raced me", the exact false `delivered` this issue exists to eliminate. So eviction is interlocked with an explicit watchBypasses() window -- refused while a watch is open, attempted from BOTH the chain's drain cleanup and the last watch's release, so whichever runs second evicts and no ordering leaks. Needs no session-teardown hook and therefore no terminal/ -> agent-farm/ layer crossing. Also: the residual "never worse than the status quo" claim-sites now state the guarantee per pair (tower-routes' logCeilingExpired doc + the degraded-path inline comment; arch.md and the session-submit boundary comment were done in ece06a5e). Stale {@link writeMessagePaced} repointed. DEGRADED_SUBMIT_REASON moved out from between logCeilingExpired's JSDoc and its function. Two nits refused with reasons rather than silently skipped (ceiling-timer cancellation needs abort semantics on the injected SubmitClock; the timing assertions are load-bearing) -- see the rebuttal file. Review doc carries the full REQUEST_CHANGES disposition, the two files outside the stated PR scope, and the codev/evidence/ placement the maintainer may veto. Tests: a degraded write that writes NOTHING is not counted; the counter is evicted once the session goes idle; eviction cannot land inside a watcher window and mask a race. Build clean (codev + sdk). 4885 passed / 0 failed / 48 skipped, 246 files. Refs #1492. Co-Authored-By: Claude Opus 5 (1M context) --- .../1365-review-iter1-rebuttals.md | 124 +++++++++++++++ .../1365-serializer-convergence-route-m.md | 149 ++++++++++++++++-- codev/state/pir-1365_thread.md | 54 +++++++ .../spec-1365-serializer-convergence.test.ts | 69 ++++++++ .../src/agent-farm/servers/message-write.ts | 72 +++++---- .../src/agent-farm/servers/session-submit.ts | 127 +++++++++++++-- .../src/agent-farm/servers/tower-routes.ts | 19 ++- 7 files changed, 542 insertions(+), 72 deletions(-) create mode 100644 codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md diff --git a/codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md b/codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md new file mode 100644 index 000000000..312b2f690 --- /dev/null +++ b/codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md @@ -0,0 +1,124 @@ +# Review iteration 1 — dispositions + +| Lane | Verdict | Action | +|---|---|---| +| gemini | APPROVE, no key issues | none required | +| codex | APPROVE, no key issues | none required | +| claude | **REQUEST_CHANGES** | all six points addressed below | + +A second, independent CMAP was run by the architect against PR #1492. Its **codex** lane +returned REQUEST_CHANGES and converged on the *same* first finding as this lane's claude — +which is why an APPROVE here was not treated as settling the matter. Every finding was +verified against the code before being accepted or refused; none was dismissed because some +other lane approved. + +--- + +## 1. Operator-vs-operator ceiling regression (blocking) — ACCEPTED, fixed + +**Finding.** `bounded` keyed only off "is anything in flight" and never asked what *kind* of +writer was ahead, so `afx send --interrupt <48 KB --file>` followed by a second `--interrupt` +let the second bypass the first after 2 s. Operator-vs-operator was **always** fully serialized +before #1365 — `submitToSession` had no ceiling at all; that pair is Spec 1273's `/clear` +fusion bug. My ceiling therefore made one pair strictly *worse* than the status quo, and +falsified the review doc's "never worse" claim. + +**Verified.** Real. Reproduced by reading the `bounded` expression at HEAD: nothing in it +distinguished a delivery holder from an operator holder. + +**Fix** (`ece06a5e`). Chain entries carry a `SubmissionKind`; a `pendingOperators` per-session +count tracks operators **queued as well as in flight**; `bounded` gains `&& !behindOperator`. +Self-counted *after* the check (so a submission does not read its own presence as a reason to +block) and *before* any await (so a later operator sees it while merely queued). Queued has to +count: bypassing an operator that has not started is the same violation as bypassing one +mid-write. + +**Pinned by** `operator vs operator NEVER degrades — the wait stays unbounded, as before #1365` +and `a THIRD operator does not bypass a QUEUED one`. The pre-existing ceiling test needed its +holder changed from an operator to a delivery — that fixture change **is** the behaviour +change, not a workaround. + +## 2. A ceiling-degraded `--interrupt` reported `delivered: true` (blocking) — ACCEPTED, fixed + +**Finding.** The row is claimed `delivered` before the write, so a degraded operator write +returned unqualified success with only a Tower-side WARN — the same false-success class this +issue exists to remove, relocated from the delivery path to the operator path. + +**Verified.** Real, and squarely against this PR's own thesis: a success signal must not lie. + +**Fix** (`ece06a5e`). Claim-first is **kept** — un-claiming reopens the double-delivery hole +reasoned through at implement-phase CMAP round 3 — and the truth is surfaced instead: +`/api/send` returns `degraded: true` + `degradedReason: 'submit-wait-ceiling-expired'`, +threaded through `packages/sdk/src/tower-client.ts` and warned about by `afx send`. An +indicator nobody surfaces is half a fix. + +**Pinned by** `a body-bearing interrupt that crosses the wait ceiling reports degraded` +(`tower-routes.test.ts`), whose holder is a delivery — the only thing the ceiling may bypass. + +## 3. Review doc stale relative to the worktree — ACCEPTED, fixed + +Files Changed, the commit list and the test figure are refreshed (**4885 passed / 0 failed / +48 skipped**, 246 files), and the "never worse than the old status quo" wording in *Things to +Look At* item 2 is rewritten to state the guarantee **per pair**. A full `never worse` / +`status quo` / `never waited` grep across `packages/codev/src/agent-farm/` and +`codev/resources/` confirmed no residual claim-site still overstates it. + +## 4. Two files outside the stated 21-file scope — ACCEPTED, disclosed + +`packages/codev/src/agent-farm/commands/send.ts` and `packages/sdk/src/tower-client.ts`. They +are the minimum needed to make a degraded write visible to the *sender* rather than only to +the Tower log, which requires crossing the server→client boundary. Now called out explicitly +in the review doc's Files Changed section so the human meets no surprise at the diff. The +architectural boundary itself is respected: `codev-sdk` still imports only `codev-types`. + +## 5. Nits + +| Nit | Disposition | +|---|---| +| Stale `{@link writeMessagePaced}` in `message-write.ts` | **Fixed** — repointed at `submitMessagePaced` | +| `unserializedWrites` entries never pruned | **Fixed** — see below | +| `DEGRADED_SUBMIT_REASON` orphaning `logCeilingExpired`'s JSDoc | **Fixed** — moved above it | +| Ceiling timer not cancelled when the predecessor wins | **Not taken — flagged** | +| `waited < 100` / `tickMs < 250` timing-sensitive | **Not taken — flagged** | + +**The counter-eviction fix, and why it is not a two-line delete.** `unserializedWrites` cannot +self-delete on drain the way `chains` and `pendingOperators` do: it must **outlive** the +submission whose watcher is about to compare against it, and a reset landing between a +watcher's two reads would read as "nobody raced me" — the exact false `delivered` this issue +exists to eliminate. So eviction is interlocked with an explicit `watchBypasses(sessionId)` +window (held by `submitMessagePaced` across its write, released in a `finally`): refused while +any watch is open, and attempted from **both** the chain's drain cleanup and the last watch's +release, so whichever runs second evicts and no ordering leaks. This needs no session-teardown +hook, and therefore no `terminal/` → `agent-farm/` layer crossing. Pinned by `the +degraded-write counter is evicted once the session goes idle` and `eviction cannot land inside +a watcher window and mask a race`. + +**Also fixed, from the architect's relay of the same lane:** the bypass counter was bumped on +ceiling expiry *regardless of whether bytes went out*. The delayed `^C` re-checks liveness +inside the lock and can write nothing; that no-op forced a concurrent delivery into a spurious +`preempted` re-delivery. `SubmitOptions.wroteBytes` is now consulted straight after the write +callback — no `await` in between, so the ordering guarantee the old placement provided is +unchanged. Pinned by `a degraded write that writes NOTHING is not counted as a bypass`. + +**Why the two refusals.** Cancelling the ceiling timer means adding abort semantics to the +injected `SubmitClock` interface that every test double implements — a broader change than the +cost of one short-lived timer per *contended* operator submission, late in a review round; a +reviewer who disagrees should say so, it is small, just not free. The timing assertions are +deliberate: they are what make "the drainer does not stall" and "the escape hatch stays +responsive" testable claims rather than prose, and both carry ≥2.5× headroom over the +behaviour they exclude. + +## 6. Process note: hot-tier `arch-critical.md` — DISCLOSED, human's call + +The plan said the hot-tier change would be *proposed*; I appended the clause directly. The +reviewer's own read is that it is defensible (it extends the existing mailbox-first fact rather +than adding an eleventh, so the 10-fact cap holds with no displacement) but that ratifying it +is the human's call. Agreed on both counts — it is disclosed in the review doc, and reverting +it is a one-line edit. + +--- + +**Result:** both blocking findings accepted and fixed, three of five nits taken, two refused +with reasons, one process note escalated to the human. `pnpm --filter @cluesmith/codev build` +and `pnpm --filter @cluesmith/codev-sdk build` clean; full suite **4885 passed / 0 failed / +48 skipped**. diff --git a/codev/reviews/1365-serializer-convergence-route-m.md b/codev/reviews/1365-serializer-convergence-route-m.md index d282e7fbb..e6b9a6a04 100644 --- a/codev/reviews/1365-serializer-convergence-route-m.md +++ b/codev/reviews/1365-serializer-convergence-route-m.md @@ -22,24 +22,32 @@ this is its implementation. Implementation: -- `packages/codev/src/agent-farm/servers/session-submit.ts` (+288 / −33) — `trySubmitToSession`, - `isSubmissionInFlight`, `OPERATOR_SUBMIT_WAIT_CEILING_MS` + `SubmitOptions`, - `unserializedWriteCount`, and the rewritten boundary comment +- `packages/codev/src/agent-farm/servers/session-submit.ts` — `trySubmitToSession`, + `isSubmissionInFlight`, `OPERATOR_SUBMIT_WAIT_CEILING_MS` + `SubmitOptions`, `SubmissionKind` + + `pendingOperators`, `unserializedWriteCount` / `watchBypasses`, and the rewritten boundary + comment - `packages/codev/src/agent-farm/servers/mailbox-delivery.ts` (+144 / −20) — `DeliverySession.id`, `WriteAbort` / `WriteResult`, the in-lock precheck, the outcome mapping -- `packages/codev/src/agent-farm/servers/message-write.ts` (+110 / −24) — `submitMessagePaced` +- `packages/codev/src/agent-farm/servers/message-write.ts` — `submitMessagePaced` (replaces `writeMessagePaced`), `PacedSubmitResult` -- `packages/codev/src/agent-farm/servers/tower-routes.ts` (+105 / −38) — wait ceiling at the three - operator call sites, `logCeilingExpired`, updated scope comments -- `packages/codev/src/agent-farm/servers/mailbox-wiring.ts` (+8 / −2) — binds the new write edge +- `packages/codev/src/agent-farm/servers/tower-routes.ts` — wait ceiling at the three + operator call sites, `logCeilingExpired`, `degraded` on the send response, updated scope comments +- `packages/codev/src/agent-farm/servers/mailbox-wiring.ts` — binds the new write edge +- `packages/codev/src/agent-farm/commands/send.ts` — warns the sender on a degraded write +- `packages/sdk/src/tower-client.ts` — `degraded` / `degradedReason` on the sendMessage result + +**Two of those are outside the 21-file scope this PR originally stated** (`commands/send.ts` +and the SDK client), added by the review round below. They are the minimum needed to make a +degraded operator write visible to the *sender* rather than only to the Tower log, which +required crossing the server→client boundary. Flagged so the diff holds no surprises; the +boundary rule itself is respected (`codev-sdk` still imports only `codev-types`). Tests: -- `packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts` (+516 / −0) — new -- `packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts` (+63 / −18) — re-pointed -- `tower-routes.test.ts` (+8 / −2), `send-delivery.test.ts` (+19 / −8), - `send-mailbox-repro.test.ts` (+7 / −2), `cron-delivery.test.ts` (+8 / −3), - `send-architect-identity.test.ts` (+6 / −2) — fakes updated +- `packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts` — new +- `packages/codev/src/agent-farm/__tests__/spec-1313-paced-write-drop.test.ts` — re-pointed +- `tower-routes.test.ts`, `send-delivery.test.ts`, `send-mailbox-repro.test.ts`, + `cron-delivery.test.ts`, `send-architect-identity.test.ts` — fakes updated Evidence + docs: @@ -60,12 +68,18 @@ Evidence + docs: - `54d57008` [PIR #1365] Thread log: implement phase - `ee3a17df` [PIR #1365] dev-approval evidence: 4 scenarios against an isolated live Tower - `1483d65c` [PIR #1365] Thread log: dev-approval evidence +- `ad824bf2` [PIR #1365] Review + retrospective +- `ece06a5e` [PIR #1365] Fix codex/claude finding: kind-aware ceiling + report degraded writes +- *(this commit)* [PIR #1365] Review round 2: byte-accurate bypass count, counter eviction, + claim-site sweep ## Test Results - `pnpm --filter @cluesmith/codev build`: ✓ pass -- `pnpm --filter @cluesmith/codev test`: ✓ pass — **4879 passed / 0 failed / 48 skipped**, - 246 files. 23 new tests in `spec-1365-serializer-convergence.test.ts`. +- `pnpm --filter @cluesmith/codev-sdk build`: ✓ pass +- `pnpm --filter @cluesmith/codev test`: ✓ pass — **4885 passed / 0 failed / 48 skipped**, + 246 files. 28 new tests in `spec-1365-serializer-convergence.test.ts`, plus the degraded- + interrupt response test in `tower-routes.test.ts`. - **Manual verification** (dev-approval gate, human-approved): `afx dev` was not usable — 4100 is shared by design and restarting the live Tower kills every builder session — so the running-worktree evidence was scripted against an **isolated Tower on port 14650** with real @@ -125,6 +139,95 @@ current ten hot lessons, and promoting either would require *displacing* an exis Displacement at the cap is the maintainer's call, not a builder's — flagged here rather than taken unilaterally. +## Review Round: two REQUEST_CHANGES, and what happened to each + +PIR's consultation is **single-pass** — there is no second automated round — so the human at +the `pr` gate is the only remaining reviewer of these dispositions. They are written out in +full rather than summarised. + +Two independent review sets ran, and they did not agree: + +| Reviewer | Protocol CMAP (`codev/projects/1365-.../`) | Architect's CMAP on PR #1492 | +|---|---|---| +| gemini | APPROVE | APPROVE | +| codex | APPROVE | **REQUEST_CHANGES** | +| claude | **REQUEST_CHANGES** | **REQUEST_CHANGES** | + +My own codex lane approved; the architect's codex lane found a real bug. I verified every +finding against the code before acting on it, and **none was dismissed on the strength of +another lane's APPROVE**. Both REQUEST_CHANGES lanes converged independently on the same two +blocking findings. + +**Blocking 1 — the ceiling could bypass another *operator's* submission.** ACCEPTED, real, +fixed in `ece06a5e`. `bounded` keyed only off "is anything in flight" without asking *what +kind* of writer was ahead, so a second `--interrupt` could skip a first one carrying a long +body after 2 s. Operator-vs-operator was **always** fully serialized before #1365 +(`submitToSession` had no ceiling at all — it is Spec 1273's `/clear` fusion bug), so my +ceiling made that one pair strictly *worse* than the status quo. That also falsified this +document's own "never worse" claim. Fix: chain entries carry a `SubmissionKind`, a +`pendingOperators` count tracks operators **queued as well as in flight**, and the ceiling arms +only when nothing ahead is an operator. Queued has to count — bypassing an operator that has +not started yet is the same violation as bypassing one mid-write. Pinned by *"operator vs +operator NEVER degrades"* and *"a THIRD operator does not bypass a QUEUED one"*. Note the +pre-existing ceiling test needed its holder changed from an operator to a delivery: **that +fixture change is the behaviour change**, not a workaround for it. + +**Blocking 2 — a ceiling-degraded `--interrupt` still reported unqualified success.** +ACCEPTED, real, fixed in `ece06a5e`. The row is claimed `delivered` before the write, so a +degraded interrupt returned `delivered: true` with only a Tower-side WARN — the same +lying-success-signal class this whole issue exists to remove, relocated from the delivery path +to the operator path. Claim-first is *kept* (un-claiming risks a double delivery, reasoned +through at CMAP round 3 of the implement phase); what changed is that the truth is now +surfaced: `/api/send` returns `degraded: true` + `degradedReason`, threaded through the SDK +client and warned about by `afx send`. An indicator nobody surfaces is half a fix. Pinned by +*"a body-bearing interrupt that crosses the wait ceiling reports degraded"* in +`tower-routes.test.ts`. + +**Non-blocking, taken anyway (this commit):** + +- *The bypass counter was bumped on ceiling expiry regardless of whether bytes went out.* The + delayed `^C` re-checks `isStillLive()` / `writable` **inside** the lock and can return having + written nothing; that no-op was still counted, forcing a concurrent delivery into a spurious + `preempted` re-delivery. The counter answers "did bytes bypass the lock while I held it?", so + only bytes may bump it: `SubmitOptions.wroteBytes` is consulted straight after the write + callback, with no `await` in between, so the ordering guarantee the old placement provided is + unchanged. Pinned by *"a degraded write that writes NOTHING is not counted as a bypass"*. +- *`unserializedWrites` was never pruned* — one entry per session that ever degraded, retained + for the life of the Tower. The leak class #1472 just fixed. It cannot self-delete on drain + the way `chains` and `pendingOperators` do, because it must **outlive** the submission whose + watcher is about to compare against it: a reset landing between a watcher's two reads would + read as "nobody raced me" — the exact false `delivered` this issue exists to eliminate. So + eviction is interlocked with an explicit `watchBypasses` window: refused while a watch is + open, attempted from *both* the chain's drain cleanup and the last watch's release, so + whichever runs second is the one that evicts and no ordering leaks. This needs **no + session-teardown hook** and therefore no `terminal/` → `agent-farm/` layer crossing. Pinned by + *"the degraded-write counter is evicted once the session goes idle"* and *"eviction cannot + land inside a watcher window and mask a race"*. +- *Stale `{@link writeMessagePaced}`* in `message-write.ts` — repointed at `submitMessagePaced`. +- *`DEGRADED_SUBMIT_REASON` was inserted between `logCeilingExpired`'s JSDoc and its function*, + orphaning the comment — moved above it. +- *The residual "never worse than the status quo" claim-sites* — swept and rewritten to state + the guarantee **per pair** (op↔op unchanged and unbounded; op↔delivery serialized under the + ceiling and degraded to the old disjoint-lock behaviour above it; delivery↔delivery + unchanged). `arch.md` §7 item 5 and the `session-submit.ts` boundary comment were corrected in + `ece06a5e`; `tower-routes.ts`'s `logCeilingExpired` doc comment and the degraded-path inline + comment in this one. + +**Non-blocking, NOT taken — flagged instead:** + +- *The ceiling timer is not cancelled when the predecessor wins the race.* `Promise.race` + leaves a ≤2 s `setTimeout` pending whose resolution is then discarded. Cancelling it means + adding abort semantics to the injected `SubmitClock` interface, which every test double + implements. The cost of leaving it is one short-lived timer per *contended* operator + submission; the cost of fixing it is a broader interface change late in a review round. A + reviewer who disagrees should say so — it is a small change, just not a free one. +- *`waited < 100` / `tickMs < 250` are timing-sensitive under CI load.* Real, and deliberate: + these are the assertions that make "the drainer does not stall" and "the escape hatch stays + responsive" *testable* claims rather than prose. Both have ≥2.5× headroom over the behaviour + they exclude. If they flake in CI, raising the bounds preserves the property. +- *Hot-tier `arch-critical.md` was appended to directly, where the plan said it would be + proposed.* Disclosed below; it is the human's call, and reverting it is a one-line edit. + ## Things to Look At During PR Review 1. **The in-lock precheck's honest status.** With try-lock semantics the delivery never waits, @@ -138,9 +241,13 @@ taken unilaterally. 2. **The wait ceiling is a judgment call** (`OPERATOR_SUBMIT_WAIT_CEILING_MS = 2000`, human- ratified at the plan gate). It exists because `--interrupt` previously never waited, and a paced write runs `(lines−1)×10+80` ms against a body capped only by `parseJsonBody`'s - 1 MiB — a 48 KB `--file` of short lines is ~8 minutes. Past the ceiling the operator write - proceeds unserialized, which is exactly the pre-#1365 behaviour, so it is never worse than - the old status quo — only no longer silent. + 1 MiB — a 48 KB `--file` of short lines is ~8 minutes. It arms **only against a delivery + write**: behind another operator the wait stays unbounded, exactly as before #1365 (see the + review round above — a ceiling that could skip an operator made that pair strictly worse, + and that was a real blocking finding, not a hypothetical). So past the ceiling the operator + write falls back to precisely the pre-#1365 operator-vs-delivery behaviour — two disjoint + locks, no serialization — which is no worse for that pair, only no longer silent. The + guarantee is **per pair**, and the 2 s value itself remains a judgment call. 3. **`preempted` trades a possible duplicate for never falsely reporting delivery.** A delivery raced by a ceiling-expired write holds its row instead of marking it delivered, so if the message *did* land intact the gate may deliver it again later. That is the same call @@ -153,6 +260,14 @@ taken unilaterally. 5. **`writeMessagePaced` was removed**, not deprecated — its only live caller was the mailbox wiring. Its drop-semantics test is re-pointed at `submitMessagePaced` so the #1198 silent-loss guard stays on the live write edge rather than on a function nothing calls. +6. **The dev-approval transcript is committed under `codev/evidence/`, a new directory.** That + placement is a deliberate choice, not an accident: the evidence is part of the PIR record + for this project, the way `codev/specs/`, `codev/plans/` and `codev/reviews/` are, and a + gate approved on evidence that then vanishes leaves the approval unauditable. It is + nonetheless a new top-level convention in the repo, and **the maintainer may veto it** — + moving or dropping the file changes nothing else in the PR (the generating script, + `packages/codev/scripts/spec-1365-e2e-evidence.mts`, is re-runnable and is the durable + artifact). **Interlock for #1481 (`--interrupt-after`)**: "interrupt, then deliver this body" is now expressible as ordered acquisitions of *one* lock rather than a race between two. Two diff --git a/codev/state/pir-1365_thread.md b/codev/state/pir-1365_thread.md index 6b8154e52..3351c5dd6 100644 --- a/codev/state/pir-1365_thread.md +++ b/codev/state/pir-1365_thread.md @@ -161,3 +161,57 @@ from the public API without staging it (covered by `tower-routes.test.ts:1560`, this change); and this fixture's agent is only ever a live terminal, never a registry-known builder, so a send after its death correctly 404s instead of exercising the hold-instead-of-404 seam. + +--- + +## Review phase — two CMAP rounds, and the one that mattered was not mine + +**2026-08-18.** The protocol's own consultation lane came back gemini APPROVE / codex APPROVE / +claude REQUEST_CHANGES. The architect independently ran a *second* CMAP against PR #1492 and got +gemini APPROVE / **codex REQUEST_CHANGES** / claude REQUEST_CHANGES. Same model, opposite +verdicts, on the same branch. + +The lesson I want a future builder to take from this: **my codex lane approved and it was +wrong.** Two of the three lanes on the other CMAP converged, independently, on a real bug I had +introduced — and the temptation, when one lane says APPROVE, is to treat the outlier as noise. I +verified every finding against the code myself before acting on it. Both blocking ones were +real. An APPROVE is not evidence that a finding is false; it is evidence that one reviewer did +not find it. + +**The bug was my own D3 ceiling, and it was the classic shape: a fix that relocates its defect.** +`OPERATOR_SUBMIT_WAIT_CEILING_MS` existed so `--interrupt` could not be stalled for minutes +behind a long delivery. But `bounded` only asked "is anything in flight", never "*what kind* of +writer is ahead" — so a second `--interrupt` could bypass a first one carrying a 48 KB body. +Operator-vs-operator had been *fully* serialized since Spec 1273 (it is the `/clear` fusion bug), +so my ceiling made exactly one pair strictly worse than the status quo, in a PR whose review doc +claimed "never worse". Fixed in `ece06a5e` by tagging chain entries with a `SubmissionKind` and +counting operators that are **queued** as well as in-flight — bypassing an operator that has not +started yet is the same violation as bypassing one mid-write. The guarantee is now stated **per +pair**, which is the only way it is true. + +The second blocking finding was the same class one layer out: a degraded interrupt still +returned `delivered: true`. In a PR whose entire thesis is *a success signal must not lie*. That +one stung. Fixed by surfacing `degraded` through `/api/send` → SDK → `afx send` rather than by +un-claiming the row (un-claiming reopens double-delivery, which round 3 of the implement-phase +CMAP had already settled). + +**Round 2 (this commit)** took the two non-blocking correctness notes and swept the doc claims. +The counter-eviction one was the only part with real design content: `unserializedWrites` cannot +self-delete on drain the way `chains` does, because it has to *outlive* the submission whose +watcher is about to compare against it — a reset landing between a watcher's two reads reads as +"nobody raced me", which is the precise false `delivered` this whole issue exists to kill. So +eviction is interlocked with an explicit `watchBypasses` window and attempted from both ends +(drain cleanup and last release), whichever runs second winning. That also avoids needing a +session-teardown hook, which would have meant `terminal/` importing `agent-farm/` — a layer +crossing not worth a memory nit. + +I refused two nits and said why in the rebuttal rather than quietly skipping them: cancelling the +ceiling timer needs abort semantics on the injected `SubmitClock` that every test double +implements, and the timing assertions are load-bearing (they are what makes "the escape hatch +stays responsive" a testable claim instead of prose). + +Dispositions in `codev/projects/1365-serializer-convergence-route-m/1365-review-iter1-rebuttals.md`; +the human-facing version, including the two files that fell outside the stated PR scope and the +`codev/evidence/` placement the maintainer may veto, is in the review doc. + +Build clean (codev + sdk); full suite **4885 passed / 0 failed / 48 skipped**, 246 files. diff --git a/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts index 944536b6e..30d1f7746 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts @@ -29,6 +29,7 @@ import { pendingSubmissionSessions, resetSubmissionChains, unserializedWriteCount, + bypassCountedSessions, OPERATOR_SUBMIT_WAIT_CEILING_MS, } from '../servers/session-submit.js'; import { @@ -388,6 +389,74 @@ describe('Issue #1365 — deliveries decline contention, operators wait (bounded expect(await delivery).toEqual({ status: 'preempted' }); }); + + it('a degraded write that writes NOTHING is not counted as a bypass', async () => { + // The delayed `^C` re-checks liveness INSIDE the lock and can legitimately return without + // writing a byte. Counting that as a bypass would hold and re-deliver a message that + // nothing actually raced — a duplicate charged for a race that never happened (claude + // review of PR #1492). + const c = makeComposer(); + const expired: number[] = []; + + const delivery = deliveryWrite(c.session, MULTILINE); + await sleep(5); // let the delivery take the lock + await submitToSession( + c.session.id, + () => 0, // the liveness re-check failed: no ^C, no bytes + undefined, + { + waitCeilingMs: 0, // give up at once — the degraded path + onCeilingExpired: (ms) => expired.push(ms), + wroteBytes: () => false, + }, + ); + + expect(expired).toEqual([0]); // it DID give up waiting, and still says so + expect(unserializedWriteCount(c.session.id)).toBe(0); // but nothing bypassed the line + expect(await delivery).toEqual({ status: 'written' }); // so the delivery is not re-held + expect(c.submitted).toEqual([MULTILINE]); + }); + + it('the degraded-write counter is evicted once the session goes idle', async () => { + // `chains` and `pendingOperators` self-delete when they drain; this counter cannot, because + // it must outlive the submission whose watcher is about to compare against it. Left + // unevicted it would retain one entry per session that ever degraded, for the life of the + // Tower — the leak class #1472 fixed (claude review of PR #1492). + const c = makeComposer(); + + const delivery = deliveryWrite(c.session, MULTILINE); + await sleep(5); + await submitToSession(c.session.id, () => { c.session.write('\x03'); return 0; }, undefined, { + waitCeilingMs: 0, + }); + + expect(unserializedWriteCount(c.session.id)).toBe(1); // counted while it still matters + expect(await delivery).toEqual({ status: 'preempted' }); // and the watcher saw it + await sleep(0); // let the chain's drain cleanup run + + expect(bypassCountedSessions()).toBe(0); // then it is gone, with no teardown hook + expect(pendingSubmissionSessions()).toBe(0); + }); + + it('eviction cannot land inside a watcher window and mask a race', async () => { + // The interlock that makes eviction safe: a watch pins the count. If the drain cleanup + // could reset it to 0 between the watcher's two reads, a raced delivery would read + // "nobody bypassed me" and report `written` — the exact false `delivered` this issue + // exists to eliminate. Two deliveries, both raced, both must report preempted. + const a = makeComposer('term-a'); + const b = makeComposer('term-b'); + + const first = deliveryWrite(a.session, MULTILINE); + const second = deliveryWrite(b.session, MULTILINE); + await sleep(5); + await submitToSession(a.session.id, () => { a.session.write('\x03'); return 0; }, undefined, { waitCeilingMs: 0 }); + await submitToSession(b.session.id, () => { b.session.write('\x03'); return 0; }, undefined, { waitCeilingMs: 0 }); + + expect(await first).toEqual({ status: 'preempted' }); + expect(await second).toEqual({ status: 'preempted' }); + await sleep(0); + expect(bypassCountedSessions()).toBe(0); + }); }); /** Wrap a composer as the DeliverySession the delivery path expects. */ diff --git a/packages/codev/src/agent-farm/servers/message-write.ts b/packages/codev/src/agent-farm/servers/message-write.ts index ac6f93856..57ca118de 100644 --- a/packages/codev/src/agent-farm/servers/message-write.ts +++ b/packages/codev/src/agent-farm/servers/message-write.ts @@ -5,14 +5,14 @@ * tower-routes.ts and tower-cron.ts. */ -import { trySubmitToSession, unserializedWriteCount, type SubmitClock } from './session-submit.js'; +import { trySubmitToSession, watchBypasses, type SubmitClock } from './session-submit.js'; /** Minimal writable session interface — avoids coupling to PtySession. */ export interface WritableSession { /** * Write input to the underlying PTY. Returns `false` when the write was dropped * (#1198: a shellper-backed session whose socket has died still reports status - * 'running', yet its writes silently no-op). {@link writeMessagePaced} threads this + * 'running', yet its writes silently no-op). {@link submitMessagePaced} threads this * boolean so a mailbox delivery whose bytes never reached the terminal is held, not * marked delivered (Spec 1313 integration review — the silent-loss finding). */ @@ -176,39 +176,43 @@ export async function submitMessagePaced( } // The one thing the lock cannot stop is an operator submission whose wait ceiling expired - // and wrote anyway. Sample the session's degraded-write counter around our own submission: + // and wrote anyway. Watch the session's degraded-write counter across our own submission: // a bump means a `^C`/ESC bypassed us mid-write, so the composer may have been cleared or // truncated under our bytes. Cheaper and more direct than re-classifying the screen — and it // is the difference between re-holding the row and falsely reporting a delivery, which is - // the whole point of Issue #1365. - const bypassesBefore = unserializedWriteCount(session.id); - - let delivered = true; - let abort: A | null = null; - const tracked: WritableSession = { - write: (data: string): boolean => { - const ok = session.write(data); - if (!ok) delivered = false; - return ok; - }, - }; - - const ran = await trySubmitToSession( - session.id, - () => { - abort = precheck(); - if (abort !== null) return 0; // refused in-lock: not one byte goes out - return writeMessageToSession(tracked, message, noEnter); - }, - clock, - ); - - if (!ran) return { status: 'contended' }; - // Read through a cast: both flags are assigned inside the callback above, which - // TypeScript's flow analysis does not track back to this scope. - const refused = abort as A | null; - if (refused !== null) return { status: 'aborted', abort: refused }; - if (!(delivered as boolean)) return { status: 'dropped' }; - if (unserializedWriteCount(session.id) !== bypassesBefore) return { status: 'preempted' }; - return { status: 'written' }; + // the whole point of Issue #1365. The watch also pins the counter against eviction for + // exactly as long as we need to compare it; `finally` is what keeps that pin from leaking. + const bypasses = watchBypasses(session.id); + try { + let delivered = true; + let abort: A | null = null; + const tracked: WritableSession = { + write: (data: string): boolean => { + const ok = session.write(data); + if (!ok) delivered = false; + return ok; + }, + }; + + const ran = await trySubmitToSession( + session.id, + () => { + abort = precheck(); + if (abort !== null) return 0; // refused in-lock: not one byte goes out + return writeMessageToSession(tracked, message, noEnter); + }, + clock, + ); + + if (!ran) return { status: 'contended' }; + // Read through a cast: both flags are assigned inside the callback above, which + // TypeScript's flow analysis does not track back to this scope. + const refused = abort as A | null; + if (refused !== null) return { status: 'aborted', abort: refused }; + if (!(delivered as boolean)) return { status: 'dropped' }; + if (bypasses.raced()) return { status: 'preempted' }; + return { status: 'written' }; + } finally { + bypasses.release(); + } } diff --git a/packages/codev/src/agent-farm/servers/session-submit.ts b/packages/codev/src/agent-farm/servers/session-submit.ts index d8c598651..3da89670f 100644 --- a/packages/codev/src/agent-farm/servers/session-submit.ts +++ b/packages/codev/src/agent-farm/servers/session-submit.ts @@ -131,11 +131,14 @@ * * The one hole this lock opens is its own degraded path: an operator whose ceiling expires * writes unserialized. Rather than leave that as a second silent-loss route, degraded writes - * are COUNTED per session ({@link unserializedWriteCount}); `submitMessagePaced` samples the - * counter around its write and reports `preempted`, so the delivery holds its row for - * redelivery instead of reporting a delivery that may have been clobbered. Deliberately no - * screen re-classification — the question is only "did anyone bypass the lock while I held - * it?", and a counter answers exactly that. + * are COUNTED per session ({@link unserializedWriteCount}); `submitMessagePaced` holds a + * {@link watchBypasses} watch across its write and reports `preempted`, so the delivery holds + * its row for redelivery instead of reporting a delivery that may have been clobbered. + * Deliberately no screen re-classification — the question is only "did anyone bypass the lock + * while I held it?", and a counter answers exactly that. The count is BYTES, not intent: a + * degraded write whose callback declines to write anything (the delayed `^C` re-checks + * liveness inside the lock) bumps nothing, because there is nothing for a delivery to have + * been raced by. */ /** @@ -186,6 +189,17 @@ export interface SubmitOptions { waitCeilingMs?: number; /** Called instead of the write's serialization when {@link waitCeilingMs} expires. */ onCeilingExpired?: (waitedMs: number) => void; + /** + * Consulted immediately after the write callback returns, on the DEGRADED path only: + * did the write actually put bytes on the terminal? Defaults to yes. + * + * The delayed `^C` re-checks `isStillLive()` and `writable` INSIDE the lock and can + * legitimately write nothing. Counting that as a bypass would make a concurrent delivery + * report `preempted` and re-deliver — a duplicate charged for a race that never happened. + * {@link unserializedWriteCount} answers "did bytes bypass the lock while I held it?", so + * only bytes may bump it. + */ + wroteBytes?: () => boolean; } /** Marker resolved by the ceiling timer so the race can tell who won. */ @@ -217,9 +231,75 @@ export const OPERATOR_SUBMIT_WAIT_CEILING_MS = 2000; * per session id and only ever created on the degraded path, which is rare by * construction — it needs a write long enough to hold the line past the ceiling AND a * concurrent operator action. + * + * Evicted by {@link evictBypassCountIfIdle} once the session has no submission in flight and + * no {@link watchBypasses} watch outstanding, so a long-lived Tower does not retain one entry + * per session that ever degraded (claude review of PR #1492 — the leak class #1472 fixed). + * Unlike {@link chains} it cannot simply self-delete on drain: it must outlive the submission + * whose watcher is about to compare against it. */ const unserializedWrites = new Map(); +/** + * Open {@link watchBypasses} watches per session — the interlock that makes eviction safe. + * + * A watcher compares the counter before and after its own write. Resetting the counter to 0 + * between those two reads would read as "nobody raced me", which is exactly the false + * `delivered` this whole issue exists to eliminate. So eviction is refused while a watch is + * open, and re-attempted when the last one closes. + */ +const bypassWatchers = new Map(); + +/** A live comparison window over a session's degraded-write count. See {@link watchBypasses}. */ +export interface BypassWatch { + /** Did a degraded write put bytes on this terminal since the watch opened? */ + raced(): boolean; + /** Close the watch. Idempotent; call it from a `finally` so no path leaks a watcher. */ + release(): void; +} + +/** + * Watch a session for degraded (ceiling-bypassing) writes across your own write. + * + * Open before the first byte, {@link BypassWatch.raced} after the last, and + * {@link BypassWatch.release} in a `finally`. Holding the watch is what pins the underlying + * count in place: without it the entry may be evicted the moment the session goes idle, and + * a reset between the two reads would look like "no race". + */ +export function watchBypasses(sessionId: string): BypassWatch { + const before = unserializedWrites.get(sessionId) ?? 0; + bypassWatchers.set(sessionId, (bypassWatchers.get(sessionId) ?? 0) + 1); + let released = false; + return { + raced: () => (unserializedWrites.get(sessionId) ?? 0) !== before, + release: () => { + if (released) return; // idempotent: a `finally` may run after an explicit release + released = true; + const remaining = (bypassWatchers.get(sessionId) ?? 1) - 1; + if (remaining > 0) { + bypassWatchers.set(sessionId, remaining); + } else { + bypassWatchers.delete(sessionId); + evictBypassCountIfIdle(sessionId); + } + }, + }; +} + +/** + * Drop a session's degraded-write count once nothing can still be comparing against it. + * + * Called from BOTH ends of the interlock — the chain's drain cleanup and the last watch's + * release — so whichever happens second is the one that evicts, and neither ordering leaks. + * Requires no session-teardown hook: a session with no chain and no watcher is quiescent by + * definition, and the next degraded write simply re-creates the entry at 0. + */ +function evictBypassCountIfIdle(sessionId: string): void { + if (bypassWatchers.has(sessionId)) return; // a comparison window is open — resetting would lie + if (chains.has(sessionId)) return; // a submission is live and could still degrade + unserializedWrites.delete(sessionId); +} + /** * Per-session count of OPERATOR submissions queued or in flight (Issue #1365, codex review). * @@ -296,28 +376,34 @@ export function submitToSession( if (kind === 'operator') pendingOperators.set(sessionId, (pendingOperators.get(sessionId) ?? 0) + 1); const current = (async () => { + let bypassed = false; if (bounded) { const winner = await Promise.race([ previousSettled, clock.sleep(ceilingMs).then(() => CEILING_EXPIRED), ]); // Ceiling expired → proceed WITHOUT serialization. This is a deliberate, - // announced degradation to the pre-Issue-#1365 behaviour (where an operator - // write never waited at all), taken only when the alternative is stalling - // `--interrupt` — the human's escape hatch — behind a write that may run for - // minutes. See the boundary comment above for why it is never worse than the - // status quo. + // announced degradation to exactly the pre-Issue-#1365 behaviour for the ONE pair it + // can affect: an operator write against a delivery, which held a disjoint lock and so + // was never serialized against an operator at all. (Operator-vs-operator never arms + // the ceiling — see `bounded` above — so that pair keeps its unbounded wait.) Taken + // only when the alternative is stalling `--interrupt`, the human's escape hatch, + // behind a write that may run for minutes. See the boundary comment above. if (winner === CEILING_EXPIRED) { - // Record it BEFORE the first byte so a delivery already holding the line sees the - // bump when it re-samples after its own write, and re-holds its row instead of - // reporting a delivery this write may have just clobbered. - unserializedWrites.set(sessionId, unserializedWriteCount(sessionId) + 1); + bypassed = true; options.onCeilingExpired?.(ceilingMs); } } else { await previousSettled; } const completesInMs = write(); + // Count the bypass only once bytes actually went out: a degraded write that declined to + // write anything raced nobody. Placed straight after `write()` with NO await in between, + // so a delivery holding the line still cannot observe our bytes without also observing + // the bump when it re-samples after its own write. + if (bypassed && (options.wroteBytes?.() ?? true)) { + unserializedWrites.set(sessionId, unserializedWriteCount(sessionId) + 1); + } // Wait out the scheduled Enter. Zero means the write was fully synchronous // (`noEnter`), so there is nothing pending to wait for. if (completesInMs > 0) await clock.sleep(completesInMs); @@ -357,6 +443,9 @@ export function submitToSession( // through the returned `current`. void tail.then(() => { if (chains.get(sessionId) === tail) chains.delete(sessionId); + // The session may now be quiescent — try to drop its degraded-write count too. Refused + // while a watch is open; that watch's release re-tries, so the second one wins. + evictBypassCountIfIdle(sessionId); }); return current; @@ -402,4 +491,14 @@ export function resetSubmissionChains(): void { chains.clear(); unserializedWrites.clear(); pendingOperators.clear(); + bypassWatchers.clear(); +} + +/** + * How many sessions still carry a degraded-write count. Test/observability only — the + * assertion that {@link unserializedWrites} self-evicts rather than growing for the life of + * a Tower. + */ +export function bypassCountedSessions(): number { + return unserializedWrites.size; } diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index a1beed466..87d7745c4 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -1615,18 +1615,19 @@ function holdAndRespond( }); } +/** Machine-readable reason paired with `degraded: true` on an operator send response. */ +const DEGRADED_SUBMIT_REASON = 'submit-wait-ceiling-expired'; + /** * Announce that an operator submission gave up waiting for the per-terminal lock and wrote * unserialized (Issue #1365). * - * This is the degraded path, and it is deliberately loud: below the ceiling an operator write - * cannot interleave with a gated delivery, and above it we fall back to exactly the behaviour - * that shipped before #1365 (an operator write that never waited). So the degradation is never - * worse than the old status quo — but it used to be invisible, and now it is not. + * This is the degraded path, and it is deliberately loud. The ceiling arms only against a + * DELIVERY write (behind another operator the wait stays unbounded, as before #1365), so what + * expiry falls back to is exactly the pre-#1365 operator-vs-delivery behaviour: two disjoint + * locks, no serialization. That one pair is therefore no worse than the old status quo — but + * it used to be invisible, and now it is not, here and in the `degraded` flag on the response. */ -/** Machine-readable reason paired with `degraded: true` on an operator send response. */ -const DEGRADED_SUBMIT_REASON = 'submit-wait-ceiling-expired'; - function logCeilingExpired( ctx: RouteContext, action: string, @@ -1774,6 +1775,10 @@ function handleDelayedSend( { waitCeilingMs: OPERATOR_SUBMIT_WAIT_CEILING_MS, onCeilingExpired: (waitedMs) => logCeilingExpired(ctx, 'delayed interrupt ^C', toAgent, terminalId, waitedMs), + // This is the one operator path whose write can be a NO-OP (the liveness re-check + // above). A degraded no-op raced nobody, so it must not bump the bypass counter — + // that would make a concurrent delivery hold and re-deliver for nothing. + wroteBytes: () => fired, }, ) .then(() => From 0ac85d88828cedfe961b8c85ce958f3b3fef9e88 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 23:05:32 -0400 Subject: [PATCH 22/26] [PIR #1365] Review doc: fill in the round-2 commit hash Co-Authored-By: Claude Opus 5 (1M context) --- codev/reviews/1365-serializer-convergence-route-m.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/codev/reviews/1365-serializer-convergence-route-m.md b/codev/reviews/1365-serializer-convergence-route-m.md index e6b9a6a04..b0685b136 100644 --- a/codev/reviews/1365-serializer-convergence-route-m.md +++ b/codev/reviews/1365-serializer-convergence-route-m.md @@ -70,8 +70,7 @@ Evidence + docs: - `1483d65c` [PIR #1365] Thread log: dev-approval evidence - `ad824bf2` [PIR #1365] Review + retrospective - `ece06a5e` [PIR #1365] Fix codex/claude finding: kind-aware ceiling + report degraded writes -- *(this commit)* [PIR #1365] Review round 2: byte-accurate bypass count, counter eviction, - claim-site sweep +- `0dc75d8e` [PIR #1365] Review round 2: byte-accurate bypass count, counter eviction, claim sweep ## Test Results From 618925f1d51fa491f9db68d4b6ed23693ce24874 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 23:05:41 -0400 Subject: [PATCH 23/26] chore(porch): 1365 pr gate-requested --- .../projects/1365-serializer-convergence-route-m/status.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 9f2829ba4..212226edb 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -15,13 +15,15 @@ gates: approved_at: '2026-08-18T00:12:08.059Z' pr: status: pending + requested_at: '2026-08-18T03:05:41.820Z' iteration: 1 build_complete: true history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-18T00:14:43.293Z' +updated_at: '2026-08-18T03:05:41.820Z' pr_history: - phase: review pr_number: 1492 branch: builder/pir-1365 created_at: '2026-08-18T00:14:35.869Z' +pr_ready_for_human: true From 81d02ad03531046fe08e677308744eff133ac769 Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 23:12:26 -0400 Subject: [PATCH 24/26] chore(porch): 1365 pr gate-approved --- .../1365-serializer-convergence-route-m/status.yaml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 212226edb..661f77939 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -14,16 +14,17 @@ gates: requested_at: '2026-08-18T00:01:11.748Z' approved_at: '2026-08-18T00:12:08.059Z' pr: - status: pending + status: approved requested_at: '2026-08-18T03:05:41.820Z' + approved_at: '2026-08-18T03:12:26.436Z' iteration: 1 build_complete: true history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-18T03:05:41.820Z' +updated_at: '2026-08-18T03:12:26.436Z' pr_history: - phase: review pr_number: 1492 branch: builder/pir-1365 created_at: '2026-08-18T00:14:35.869Z' -pr_ready_for_human: true +pr_ready_for_human: false From df96c2e174695b0b31e13bf51aa410bdccd04f7b Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 23:12:30 -0400 Subject: [PATCH 25/26] chore(porch): 1365 protocol complete --- .../projects/1365-serializer-convergence-route-m/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1365-serializer-convergence-route-m/status.yaml b/codev/projects/1365-serializer-convergence-route-m/status.yaml index 661f77939..70eba4f45 100644 --- a/codev/projects/1365-serializer-convergence-route-m/status.yaml +++ b/codev/projects/1365-serializer-convergence-route-m/status.yaml @@ -1,7 +1,7 @@ id: '1365' title: serializer-convergence-route-m protocol: pir -phase: review +phase: verified plan_phases: [] current_plan_phase: null gates: @@ -21,7 +21,7 @@ iteration: 1 build_complete: true history: [] started_at: '2026-08-17T23:25:51.772Z' -updated_at: '2026-08-18T03:12:26.436Z' +updated_at: '2026-08-18T03:12:30.821Z' pr_history: - phase: review pr_number: 1492 From 6c9fa818a0763daf4cf5ab4546f27fcea65511aa Mon Sep 17 00:00:00 2001 From: Mohid Makhdoomi Date: Mon, 17 Aug 2026 23:16:43 -0400 Subject: [PATCH 26/26] [PIR #1365] Review doc + thread log: record the origin/main merge Post-merge figures: build clean (codev + sdk), full suite 4934 passed / 0 failed / 48 skipped, 248 files (was 4885 / 246 pre-merge; the delta is main's own tests). Merge ebbc495dc, keep-both resolution of the lessons-learned append collision, and the verification that every #1365 hunk survived byte-identical. Co-Authored-By: Claude Opus 5 (1M context) --- .../1365-serializer-convergence-route-m.md | 16 ++++++-- codev/state/pir-1365_thread.md | 37 +++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/codev/reviews/1365-serializer-convergence-route-m.md b/codev/reviews/1365-serializer-convergence-route-m.md index b0685b136..f0999e334 100644 --- a/codev/reviews/1365-serializer-convergence-route-m.md +++ b/codev/reviews/1365-serializer-convergence-route-m.md @@ -76,9 +76,19 @@ Evidence + docs: - `pnpm --filter @cluesmith/codev build`: ✓ pass - `pnpm --filter @cluesmith/codev-sdk build`: ✓ pass -- `pnpm --filter @cluesmith/codev test`: ✓ pass — **4885 passed / 0 failed / 48 skipped**, - 246 files. 28 new tests in `spec-1365-serializer-convergence.test.ts`, plus the degraded- - interrupt response test in `tower-routes.test.ts`. +- `pnpm --filter @cluesmith/codev test`: ✓ pass — **4934 passed / 0 failed / 48 skipped**, + 248 files, after merging `origin/main`. 28 new tests in + `spec-1365-serializer-convergence.test.ts`, plus the degraded-interrupt response test in + `tower-routes.test.ts`. (Before the merge, on the #1365 work alone: 4885 / 0 / 48, 246 files + — the delta is main's own tests arriving, not tests changing behaviour here.) +- **Merged `origin/main` after the `pr` gate was approved** (merge `ebbc495dc`, not a rebase — + the reviewed history is preserved). `main` had advanced 25 commits during the review round + (AIR #1489's `afx reset` → `afx refresh`, secfix-1's Tower auth hardening, PIR #1495). One + conflict, in `codev/resources/lessons-learned.md` § Architecture: an append-only collision + (three #1365 entries vs one secfix-1 entry at the same point), resolved **keep-both**, all + four entries present and unmodified. Every code file auto-merged; every #1365 hunk survives + byte-identical, and `message-write.ts`, `mailbox-delivery.ts`, `commands/send.ts` and + `spec-1365-serializer-convergence.test.ts` were untouched by the merge. - **Manual verification** (dev-approval gate, human-approved): `afx dev` was not usable — 4100 is shared by design and restarting the live Tower kills every builder session — so the running-worktree evidence was scripted against an **isolated Tower on port 14650** with real diff --git a/codev/state/pir-1365_thread.md b/codev/state/pir-1365_thread.md index 3351c5dd6..8b1c86f61 100644 --- a/codev/state/pir-1365_thread.md +++ b/codev/state/pir-1365_thread.md @@ -215,3 +215,40 @@ the human-facing version, including the two files that fell outside the stated P `codev/evidence/` placement the maintainer may veto, is in the review doc. Build clean (codev + sdk); full suite **4885 passed / 0 failed / 48 skipped**, 246 files. + +--- + +## Post-gate: merging main back in + +**2026-08-18.** The human approved the `pr` gate and porch reported the protocol complete +(`phase: verified`). Porch's final task says "merge the PR" — not taken; PR #1492 and issue +#1365 are parked for the maintainer, per standing order. + +Then GitHub flagged the PR `mergeable=CONFLICTING`: `main` had moved 25 commits during the +review round. I reported it rather than fixing it unasked, because merging would have changed +the tree the architect had just verified with a 129/129 re-run — and got the go-ahead. + +**The conflict was a nothing, and that is worth recording precisely because it looked +alarming.** Exactly one file: `codev/resources/lessons-learned.md`, § Architecture. Both sides +had *appended* — three #1365 lessons here, one secfix-1 lesson on main, at the same insertion +point. Not a competing edit; git simply cannot know that two appends at one anchor are +independent. Keep-both, all four entries intact. + +Every code file auto-merged. What the merge *did* change in this PR's files came entirely from +main: AIR #1489's `afx reset` → `afx refresh` rename landed in two of my comments +(`session-submit.ts`, `mailbox-wiring.ts`), and secfix-1's auth hardening rewrote parts of +`tower-routes.ts`, `tower-client.ts` (`codev-web-key` → `codev-tower-key`) and +`tower-routes.test.ts`. I verified this rather than assuming it: blob-hashed the nine files +before the merge and diffed each afterwards, then grepped every #1365 marker +(`wroteBytes`, `watchBypasses`, `pendingOperators`, `SubmissionKind`, `logCeilingExpired`, +`DEGRADED_SUBMIT_REASON`, the degraded-interrupt test) to confirm each survived. Four files +were byte-identical: `message-write.ts`, `mailbox-delivery.ts`, `commands/send.ts`, +`spec-1365-serializer-convergence.test.ts`. + +`pnpm install --frozen-lockfile` was necessary before rebuilding — main moved `pnpm-lock.yaml`, +and secfix-1's own lesson in that very file is about a dep that only fails in a *packaged* +install. Build clean (codev + sdk). Full suite **4934 passed / 0 failed / 48 skipped, 248 +files** — up from 4885/246, the delta being main's own new tests, not behaviour change here. + +Merge commit `ebbc495dc`. Merge, not rebase: rebasing would have rewritten 83 pushed commits +and destroyed the history the human just approved.