From c1838c24ab938b9afeb669853f68e07785e5d0ed Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 21:59:12 -0700 Subject: [PATCH 1/5] =?UTF-8?q?[Spec=201273][Verify]=20fix:=20per-session?= =?UTF-8?q?=20submission=20lock=20=E2=80=94=20an=20awaited=20send=20now=20?= =?UTF-8?q?means=20submitted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify-phase e2e forensics: afx reset's /clear was NEVER EXECUTED. It arrived as literal text welded onto the front of the re-orientation, no separator, one merged user turn: /clear### [ARCHITECT INSTRUCTION | ...] ### No slash command ran; the probe's context was fully intact and it could still recite a secret word planted before the "reset". ROOT CAUSE (confirmed in source, not inferred). writeMessageToSession writes the text and schedules the Enter via setTimeout — SIMPLE_ENTER_DELAY_MS = 50, PACED_ENTER_DELAY_MS = 80 — then returns that offset as a NUMBER without awaiting it. /api/send responded off that. So an awaited send resolved ~50ms BEFORE its own message was submitted. Reset awaited sendRaw('/clear'), did a fast readOutput, then wrote the re-orientation — inside the window, into the same composer, ahead of the Enter. One Enter submitted both. The channel was right; the mistake was treating an awaited send as proof of submission when it only proved SCHEDULING. Third member of a family the ledger now tracks: an operation that reports success at a point earlier than the thing it claims. This also retro-explains phase 6's clear-confirmation finding — at the moment I sampled output the clear had not even been submitted. Two bugs, one seam. ORDERING IS NOT ATOMICITY. SendBuffer already serialises within a flush, and 1307's per-session FIFO fixes delivery ORDER. Neither would have saved this: the two writes were correctly ordered and still coalesced. Being second is not the same as being separate. THE PRIMITIVE (architect-assigned to me; 1307 adopts it unchanged and drops their narrower versions — deliverOrBuffer's writeCompletesInMs wait, SendBuffer.busyUntil, delayed-send.ts's chain): submitToSession(sessionId, write) — a promise chain per session. Each submission waits for the previous one to finish INCLUDING its Enter, so concurrent sends to one terminal cannot interleave in the composer. Wired to the escape path and the immediate message path only. The BUFFERED path is deliberately NOT awaited — a deferred message can sit up to 60s and awaiting it would hang the caller instead of returning deferred: true. That constraint came from 1307, who flagged it before I wired anything. CORRECTION TO MY OWN EARLIER CLAIM, verified at their prompting: I had said reset's writes bump _lastInputAt and trip their own buffering. FALSE. recordUserInput() is called only from pty-manager.ts:310,317 — the websocket handler, i.e. a human typing. Tower's own writes never touch it. DELIBERATE TEST CHANGE: "delivers message + Enter as a single atomic write" asserted writeCalls.length === 1 — i.e. that the route returned BEFORE the Enter was written. That was the bug, not the contract. It now asserts the property that actually mattered (Enter is a separate write, never appended) as properties rather than an exact count, since formatted messages are paced. Tests mutation-verified: removing the chaining fails the coalescing test and the ordering test, passes the rest. Suite 4057 -> 4058 (7 new), build clean. --- .../spec-1273-submission-lock.test.ts | 153 ++++++++++++++++++ .../agent-farm/__tests__/tower-routes.test.ts | 23 ++- .../src/agent-farm/servers/session-submit.ts | 117 ++++++++++++++ .../src/agent-farm/servers/tower-routes.ts | 20 ++- 4 files changed, 306 insertions(+), 7 deletions(-) create mode 100644 packages/codev/src/agent-farm/__tests__/spec-1273-submission-lock.test.ts create mode 100644 packages/codev/src/agent-farm/servers/session-submit.ts diff --git a/packages/codev/src/agent-farm/__tests__/spec-1273-submission-lock.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1273-submission-lock.test.ts new file mode 100644 index 000000000..d2c0c5be5 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/spec-1273-submission-lock.test.ts @@ -0,0 +1,153 @@ +/** + * Per-session submission lock (Spec 1273, verify phase). + * + * The regression under test is the one that reached production: two sends to + * one session coalescing into a single submission, because the first send's + * Enter was still pending when the second write landed in the composer. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { + submitToSession, + resetSubmissionChains, + pendingSubmissionSessions, + type SubmitClock, +} from '../servers/session-submit.js'; + +/** + * A composer that models the real failure. + * + * `write` appends to a pending buffer; `enter` submits whatever has + * accumulated. That is the PTY's actual behaviour, and it is why an unawaited + * Enter is dangerous: anything written before it joins the same submission. + */ +function makeComposer() { + let pending = ''; + const submitted: string[] = []; + return { + submitted, + write(text: string) { + pending += text; + }, + enter() { + submitted.push(pending); + pending = ''; + }, + }; +} + +/** + * Real timers with a short delay. + * + * A virtual clock proved more fragile than the thing it was testing — the + * property here is "the second write does not start until the first Enter has + * fired", which real timers demonstrate directly and cheaply. + */ +const clock: SubmitClock = { sleep: ms => new Promise(r => setTimeout(r, ms)) }; + +const ENTER_DELAY = 20; + +/** + * Yield past the chain's internal microtasks so the in-flight write has run, + * while staying well inside ENTER_DELAY so the NEXT one cannot have started. + * A bare `Promise.resolve()` is not enough: the chain adds ticks of its own. + */ +const afterCurrentWrite = () => new Promise(r => setTimeout(r, 0)); + +describe('Spec 1273 — submission lock', () => { + beforeEach(() => resetSubmissionChains()); + + it('keeps two sends to one session as two separate submissions', async () => { + // THE PRODUCTION BUG. Without the lock the second write joins the first's + // pending text and one Enter submits `/clear## CONTEXT RESET…` as a single + // message — exactly what reached the live probe builder. + const composer = makeComposer(); + + const write = (text: string) => () => { + composer.write(text); + setTimeout(() => composer.enter(), 0); + return ENTER_DELAY; + }; + + const first = submitToSession('term-1', write('/clear'), clock); + const second = submitToSession('term-1', write('## CONTEXT RESET'), clock); + + await first; + await second; + await new Promise(r => setTimeout(r, 0)); + + expect(composer.submitted).toEqual(['/clear', '## CONTEXT RESET']); + // The decisive assertion: never welded together. + expect(composer.submitted.some(m => m.startsWith('/clear#'))).toBe(false); + }); + + it('does not let the second write begin before the first has submitted', async () => { + const order: string[] = []; + + const first = submitToSession('term-1', () => { order.push('first'); return ENTER_DELAY; }, clock); + const second = submitToSession('term-1', () => { order.push('second'); return ENTER_DELAY; }, clock); + + // The first write has run; the second is held behind the pending Enter. + await afterCurrentWrite(); + expect(order).toEqual(['first']); + + await Promise.all([first, second]); + + expect(order).toEqual(['first', 'second']); + }); + + it('resolves only after the scheduled Enter, not when the write is scheduled', async () => { + // `await send(...)` must mean SUBMITTED. Responding on "scheduled" is the + // root cause: the HTTP 200 came back ~50ms before the message existed. + let resolved = false; + + const submission = submitToSession('term-1', () => ENTER_DELAY, clock).then(() => { + resolved = true; + }); + + await afterCurrentWrite(); + expect(resolved).toBe(false); + + await submission; + expect(resolved).toBe(true); + }); + + it('does not serialize across different sessions', async () => { + // The lock is per session; unrelated terminals must not queue behind a busy + // one, or one slow builder would stall messaging workspace-wide. + const order: string[] = []; + + const a = submitToSession('term-a', () => { order.push('a'); return ENTER_DELAY; }, clock); + const b = submitToSession('term-b', () => { order.push('b'); return ENTER_DELAY; }, clock); + + await afterCurrentWrite(); + expect([...order].sort()).toEqual(['a', 'b']); + + await Promise.all([a, b]); + }); + + it('returns immediately when there is no Enter to wait for', async () => { + // noEnter writes report 0; waiting on them would stall the chain forever. + await expect(submitToSession('term-1', () => 0)).resolves.toBeUndefined(); + }); + + it('a throwing submission does not poison the chain', async () => { + // The next message is a separate submission and is still entitled to run. + let ran = false; + + const bad = submitToSession('term-1', () => { + throw new Error('write failed'); + }, clock); + const good = submitToSession('term-1', () => { ran = true; return ENTER_DELAY; }, clock); + + await expect(bad).rejects.toThrow('write failed'); + await good; + expect(ran).toBe(true); + }); + + it('drains its bookkeeping so a long-lived Tower does not leak', async () => { + await submitToSession('term-1', () => 0); + await new Promise(r => setTimeout(r, 0)); + expect(pendingSubmissionSessions()).toBe(0); + }); +}); 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 530ec46a4..805953a91 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts @@ -1555,11 +1555,24 @@ describe('tower-routes', () => { const { res } = makeRes(); await handleRequest(req, res, ctx); - // Message is written first, then \r is sent separately after a 50ms delay - // so the PTY processes the multi-line paste before receiving Enter (Bugfix #492). - const writeCalls = mockWrite.mock.calls; - expect(writeCalls.length).toBe(1); // Initial write (message only) - expect(writeCalls[0][0]).not.toMatch(/\r$/); // No \r in initial write + // Message is written first, then \r is sent SEPARATELY after a delay, so + // the PTY processes the paste before receiving Enter (Bugfix #492/#481). + // That separation is the property this test exists to protect. + const writeCalls = mockWrite.mock.calls.map(c => c[0] as string); + expect(writeCalls[0]).toContain('hello'); + expect(writeCalls[0]).not.toMatch(/\r$/); // Enter is never appended + + // UPDATED (Spec 1273 verify): this previously asserted `length === 1` — + // i.e. that the route returned BEFORE the Enter was written. That was the + // bug, not the contract: an awaited send resolving before its own + // submission is how `afx reset` got `/clear` welded onto the front of the + // next message and never cleared anything. `/api/send` now awaits the + // submission, so by the time the request resolves the Enter HAS landed. + // + // Asserted as properties rather than an exact count, because the + // formatted message may be paced line-by-line (Bugfix #584). + expect(writeCalls.length).toBeGreaterThan(1); + expect(writeCalls.at(-1)).toBe('\r'); }); it('delivers message without Enter when noEnter is set (Bugfix #481)', async () => { diff --git a/packages/codev/src/agent-farm/servers/session-submit.ts b/packages/codev/src/agent-farm/servers/session-submit.ts new file mode 100644 index 000000000..46f10a8fa --- /dev/null +++ b/packages/codev/src/agent-farm/servers/session-submit.ts @@ -0,0 +1,117 @@ +/** + * Per-session submission lock (Spec 1273, verify phase). + * + * ## The bug this exists to make impossible + * + * `writeMessageToSession` writes text to the PTY and schedules the Enter that + * submits it **50–80ms later** (`message-write.ts`). The write functions return + * that completion offset but do not wait for it, and `/api/send` responded as + * soon as the write was *scheduled*. So an awaited send resolved before its own + * message had been submitted. + * + * In production (`afx reset`, 2026-07-31) that gap swallowed a `/clear` + * entirely. Reset awaited the raw write of `/clear`, then wrote the + * re-orientation — landing inside the 50ms window, into the same composer, + * ahead of the Enter. One Enter then submitted both as a single message + * beginning `/clear### [ARCHITECT INSTRUCTION...`. The slash command was never + * executed, the builder's context was never cleared, and every layer reported + * success. + * + * ## Ordering is not atomicity + * + * `SendBuffer` already serializes messages *within one flush* by threading a + * delay offset between them, and per-session FIFO (Spec 1307) fixes the *order* + * in which queued messages are delivered. Neither would have prevented this: the + * two writes were correctly ordered and still coalesced, because being second is + * not the same as being separate. What was missing is a guarantee that a + * submission completes — Enter included — before the next write to that session + * begins. + * + * ## What this provides + * + * A promise chain per session key. Each submission waits for the previous one to + * finish (including its Enter), so concurrent `/api/send` requests to the same + * terminal cannot interleave in the composer. `await submitToSession(...)` means + * *submitted*, not *scheduled* — which is what every caller already assumed it + * meant. + * + * Deliberately keyed by session id rather than holding a session object: Tower + * re-fetches sessions by id, and a lock that outlived its session would pin a + * dead reference. + */ + +/** + * 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. + */ +const chains = new Map>(); + +/** Injectable for tests; real timers otherwise. */ +export interface SubmitClock { + sleep(ms: number): Promise; +} + +const realClock: SubmitClock = { + sleep: (ms: number) => new Promise(resolve => setTimeout(resolve, ms)), +}; + +/** + * Run a write against a session so that it completes before any other + * submission to the same session begins. + * + * @param sessionId terminal/session id — the lock's granularity + * @param write performs the write; returns ms from now until the final + * keystroke (the Enter) has been written. This is exactly what + * `writeMessageToSession` / `writeEscapeToSession` already + * return, so callers pass them through unchanged. + * @returns resolves once the submission is complete + */ +export function submitToSession( + sessionId: string, + write: () => number, + clock: SubmitClock = realClock, +): Promise { + const previous = chains.get(sessionId) ?? Promise.resolve(); + + 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); + }); + + chains.set(sessionId, current); + + // 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); + }); + + return current; +} + +/** 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. */ +export function resetSubmissionChains(): void { + chains.clear(); +} diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index b11acd846..a5fad7a21 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -52,6 +52,7 @@ import { SendBuffer } from './send-buffer.js'; import type { BufferedMessage } from './send-buffer.js'; import type { PtySession } from '../../terminal/pty-session.js'; import { writeMessageToSession, writeEscapeToSession } from './message-write.js'; +import { submitToSession } from './session-submit.js'; import { getKnownWorkspacePaths, getInstances, @@ -1505,7 +1506,9 @@ async function handleSend( // trailing Enter is what lets them through, which is why it is the default // (matching the verified recovery `afx send --raw "$(printf '\x1b')"`). if (escape) { - writeEscapeToSession(session, noEnter); + // 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)); broadcastMessage({ type: 'message', from: { project: path.basename(fromWorkspace ?? workspace ?? 'unknown'), agent: from ?? 'unknown' }, @@ -1583,7 +1586,20 @@ async function handleSend( } else { // User is idle (or interrupt) — deliver immediately. // Bugfix #584: paces multi-line output to avoid paste detection. - writeMessageToSession(session, formattedMessage, noEnter); + // + // AWAITED (Spec 1273 verify). `writeMessageToSession` schedules the Enter + // 50–80ms out and returns immediately; responding on that meant a caller's + // `await send(...)` resolved BEFORE its message was submitted. Two sends in + // quick succession then landed in the same composer and were submitted as + // one message — which is how `afx reset` sent + // `/clear### [ARCHITECT INSTRUCTION...` and never cleared anything. + // + // Only the immediate path is awaited. The buffered path above must NOT be: + // a deferred message can sit up to 60s, and awaiting that would hang the + // caller instead of returning `deferred: true`. + await submitToSession(result.terminalId, () => + writeMessageToSession(session, formattedMessage, noEnter), + ); broadcastMessage(broadcastPayload); ctx.log('INFO', logMessage); } From e376aeb7340f48aea6bebe7ca5475bb5160f2913 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 22:10:04 -0700 Subject: [PATCH 2/5] [Spec 1273][Verify] test: pin batch reservation for the buffer-flush drain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raised by aspir-1307 before deleting SendBuffer.busyUntil: submitToSession is wired to the immediate path, not flush(), so deleting busyUntil could reopen the mid-flush interleave they closed in 17db2e9e — a message writing into a partially-delivered /clear, which is the shape of this spec's production failure one layer down. Checked rather than assumed: NO API extension is needed. `write` may perform many writes and return the FINAL completion offset, so one submission can reserve the session for an entire flush with its offset threading intact: submitToSession(sessionId, () => { let offset = 0; for (const msg of messages) offset = deliver(session, msg, offset); return offset; }); flush() is synchronous and does not need to await — `void submitToSession(...)` still serialises the batch against direct sends, because the chain orders by call, not by await. Test pins the property so it lives in a suite rather than in a message: a batch of paced writes and a concurrent direct send, asserting the direct send is never welded into the batch's pending text. Mutation-verified — removing the chaining fails it along with the other two ordering tests. Suite 4058 -> 4059. --- .../spec-1273-submission-lock.test.ts | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/packages/codev/src/agent-farm/__tests__/spec-1273-submission-lock.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1273-submission-lock.test.ts index d2c0c5be5..683ce10a1 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1273-submission-lock.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1273-submission-lock.test.ts @@ -145,6 +145,48 @@ describe('Spec 1273 — submission lock', () => { expect(ran).toBe(true); }); + it('reserves the session for a whole BATCH, so a direct send cannot interleave', async () => { + // The flush-drain case (raised by aspir-1307 for SendBuffer.busyUntil). + // + // No API extension is needed: `write` may perform MANY writes and return + // the final completion offset, so one submission can cover an entire + // buffer flush with its existing offset threading intact. A direct send + // arriving mid-drain queues behind the whole batch rather than writing into + // a partially-delivered message. + // + // Without that reservation the interleave is the same shape as the + // production failure one layer down: a message landing inside a + // half-delivered `/clear` yields `/clear` on one line. + const composer = makeComposer(); + + const batch = () => { + // Mirrors SendBuffer.flush: several paced writes, offsets threaded. + composer.write('buffered-1'); + setTimeout(() => composer.enter(), 0); + composer.write('buffered-2'); + setTimeout(() => composer.enter(), 1); + return ENTER_DELAY; + }; + + const direct = () => { + composer.write('/clear'); + setTimeout(() => composer.enter(), 0); + return ENTER_DELAY; + }; + + const flush = submitToSession('term-1', batch, clock); + const send = submitToSession('term-1', direct, clock); + + await Promise.all([flush, send]); + await new Promise(r => setTimeout(r, 5)); + + // The direct send is never welded into the batch's pending text. + expect(composer.submitted.some(m => m.includes('buffered') && m.includes('/clear'))).toBe( + false, + ); + expect(composer.submitted).toContain('/clear'); + }); + it('drains its bookkeeping so a long-lived Tower does not leak', async () => { await submitToSession('term-1', () => 0); await new Promise(r => setTimeout(r, 0)); From a349ce73e08260553f032c0d8ebfea3f55fdc237 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 22:19:28 -0700 Subject: [PATCH 3/5] [Spec 1273][Verify] docs: narrow the atomicity claim; record the delivery seam in the review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CMAP on #1320: Gemini APPROVE, Claude COMMENT, Codex REQUEST_CHANGES. Both non-approving findings accepted; neither is a code defect. 1. "Per-session atomicity does not cover every PTY writer" — correct, and my framing was overbroad. A lock only serialises writers that TAKE it, and only the escape + immediate /api/send paths do. Now enumerated in the docstring with the reason for each: - deliverBufferedMessage (flush) — not covered; 1307's to wire, batch form already supported and tested so no API change is needed - tower-cron delivery — not covered, same shape - POST /api/terminals/:id/write — not covered, raw passthrough - websocket keystrokes + shellper relay — DELIBERATELY never covered; a human is the composer's owner and queueing their typing behind an agent's message would make the UI feel stuck The honest guarantee is "two /api/send deliveries to one session cannot interleave", which is the failure that reached production. 2. "The review artifact is stale" — correct. The review documented the verify e2e's resolution findings but not the delivery seam that the forensics actually exposed. Added: root cause, why ordering is not atomicity, the retro-explanation of the phase-6 confirmation defect (same seam), and the scope note above. No behaviour change. --- .../1273-builder-context-reset-should-b.md | 27 +++++++++++++++++++ .../src/agent-farm/servers/session-submit.ts | 23 ++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/codev/reviews/1273-builder-context-reset-should-b.md b/codev/reviews/1273-builder-context-reset-should-b.md index a4233ab0b..5342022ef 100644 --- a/codev/reviews/1273-builder-context-reset-should-b.md +++ b/codev/reviews/1273-builder-context-reset-should-b.md @@ -201,6 +201,33 @@ The e2e was the first thing to run reset against reality, and it failed instantl the strongest possible restatement of the hot-tier lesson: *"it compiled" / "tests pass" is not "it works" — verify the real user path end-to-end.* +### The delivery seam — `/clear` was never executed (2026-08-01) + +The live e2e's forensics closed the loop on the headline path. `afx reset`'s `/clear` **never ran**: it +arrived as literal text welded onto the front of the re-orientation, one merged user turn beginning +`/clear### [ARCHITECT INSTRUCTION...`. The probe's context was fully intact — it could still recite a +secret word planted before the "reset" — while every layer reported success. + +**Root cause.** `writeMessageToSession` schedules the Enter that submits a message 50–80ms out and +returns that offset without awaiting it; `/api/send` responded off that. So an awaited send resolved +*before its own message was submitted*. Reset awaited the `/clear` write, sampled output, then wrote the +re-orientation — inside the window, into the same composer, ahead of the Enter. One Enter submitted both. + +This also retro-explains the phase-6 clear-confirmation defect: at the moment the check sampled output, +the clear had not been submitted. **Two bugs, one seam.** + +**Fix**: a per-session submission lock (`servers/session-submit.ts`), wired to the escape and +immediate-delivery paths. `await submitToSession(...)` means *submitted*, not *scheduled*. + +**Ordering is not atomicity.** `SendBuffer` already serialises within a flush, and Spec 1307's per-session +FIFO fixes delivery *order*. Neither would have prevented this — the two writes were correctly ordered +and still coalesced. Being second is not the same as being separate. Worth carrying: the two properties +look interchangeable in a design discussion and are not. + +**Scope, stated because the first framing was overbroad**: the lock covers what takes it — the two +`/api/send` paths. Buffer flush, cron delivery and the raw write endpoint still write directly; human +keystrokes deliberately always will. + ## Flaky Tests None encountered. No tests were skipped by this work. diff --git a/packages/codev/src/agent-farm/servers/session-submit.ts b/packages/codev/src/agent-farm/servers/session-submit.ts index 46f10a8fa..04979c29d 100644 --- a/packages/codev/src/agent-farm/servers/session-submit.ts +++ b/packages/codev/src/agent-farm/servers/session-submit.ts @@ -38,6 +38,29 @@ * Deliberately keyed by session id rather than holding a session object: Tower * re-fetches sessions by id, and a lock that outlived its session would pin a * dead reference. + * + * ## 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 immediate-delivery paths of `/api/send`. Every other PTY writer still + * writes directly, and it is worth being precise about why: + * + * - `tower-routes.ts` `deliverBufferedMessage` (buffer flush) — NOT covered. + * Adopting it is Spec 1307's work; the batch form + * (`write` performing the whole drain and returning the final offset) is + * supported and tested, so no API change is needed when they wire it. + * - `tower-cron.ts` cron delivery — NOT covered. Same shape as the buffered + * path; a scheduled message can land beside an in-flight submission. + * - `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 `/api/send` deliveries to one session cannot + * interleave**, which is the failure that reached production. Anything stronger + * requires the remaining writers to take the lock too. */ /** From 2bc85ea2bf2ba8fc6a7276949780d13c2ce15530 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 22:22:29 -0700 Subject: [PATCH 4/5] [Spec 1273][Verify] Thread: record the delivery seam and the sibling-builder coordination --- codev/state/aspir-1273_thread.md | 54 ++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/codev/state/aspir-1273_thread.md b/codev/state/aspir-1273_thread.md index 66cd102d8..e724f0847 100644 --- a/codev/state/aspir-1273_thread.md +++ b/codev/state/aspir-1273_thread.md @@ -393,6 +393,60 @@ reviewed 2-way. Reported to the architect. Remaining: the live reset against the planted-context probe, for the architect to re-run after install. +## The delivery seam — `/clear` was never executed (2026-08-01) + +The e2e's forensics closed the loop, and the answer was worse than "the fix was wrong": **the `/clear` +never ran at all.** It arrived as literal text welded onto the front of the re-orientation — one merged +user turn beginning `/clear### [ARCHITECT INSTRUCTION...`, no separator. The probe's context was fully +intact; it could still recite a secret word planted before the "reset". Every layer reported success. + +**Root cause.** `writeMessageToSession` schedules the Enter that submits a message 50–80ms out and +returns that offset *without awaiting it*; `/api/send` responded off that. So an awaited send resolved +**before its own message was submitted**. Reset awaited the `/clear` write, sampled output, then wrote +the re-orientation — inside the window, into the same composer, ahead of the Enter. One Enter submitted +both. + +The channel was right. The mistake was treating an awaited send as proof of submission when it only +proved *scheduling*. The architect's ledger has this as a family now — *an operation that reports success +at a point earlier than the thing it claims* — and this is its third member. + +It also retro-explains phase 6's clear-confirmation defect: at the moment the check sampled output, the +clear had not been submitted. **Two bugs, one seam.** I spent three CMAP rounds patching the confirmation +regex when the thing it was checking had not happened yet. + +**Ordering is not atomicity.** This is the piece worth carrying furthest. `SendBuffer` already serialises +within a flush, and 1307's per-session FIFO fixes delivery *order* — and neither would have prevented +this, because my two writes were correctly ordered and still coalesced. **Being second is not the same as +being separate.** In a design conversation the two properties sound interchangeable; they are not, and +the distinction is invisible until something concatenates. + +**Fix**: a per-session submission lock (`servers/session-submit.ts`) — a promise chain where each +submission waits out its own Enter before the next write to that session begins. `await +submitToSession(...)` means *submitted*. + +### Working with a sibling builder + +The architect assigned me the primitive and 1307 adopts it. Two things came out of that exchange that +neither of us would have got alone: + +- **They corrected me.** I had claimed reset's own writes bump `_lastInputAt` and so trip their own + buffering. False — `recordUserInput()` is called only from the websocket handler. It did not affect the + diagnosis, but it was load-bearing in how I was *describing* the failure, which is its own kind of + wrong. They told me to verify rather than take their word; I did. +- **I corrected the scope of my own fix.** They asked whether deleting their `busyUntil` would reopen a + mid-flush interleave. Rather than assert the primitive covered it, I checked: the existing signature + already supports a whole-batch reservation, and I pinned it with a mutation-verified test so their + deletion rests on a failing-on-mutation test rather than my assurance. Their reply — *a test adjacent to + the real path is not coverage of it* — is the right line, and they are re-verifying with their own + flush test regardless. + +Codex's CMAP then caught that my framing of the primitive was overbroad: a lock only serialises writers +that *take* it. The uncovered writers are now enumerated with reasons, including the one that should +never be covered — human keystrokes, because the human owns their own composer. + +**Still unproven**: that the clear now actually clears, and what a real clear emits. The confirmation +pattern remains an educated guess until the live re-run. + ## Status - [x] Explored afx/Tower internals From fa46e98d690e702bc5d635b204eada9bb235ed91 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Sat, 1 Aug 2026 17:22:57 -0700 Subject: [PATCH 5/5] [Spec 1273][Verify] merge main (#1143); re-verify the cron uncovered-writer claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge auto-resolved — no textual conflict with #1143's tower-cron rewrite, since my change is in tower-routes/session-submit and theirs is in tower-cron. RE-VERIFIED rather than ported. #1143 rewrote the cron delivery DECISION (exitCode resolves as data; a conditioned task now delivers whenever its condition is truthy, failures included) but NOT the delivery MECHANISM: deliverMessage still calls writeMessageToSession directly (tower-cron.ts:338), taking no lock. So the enumeration stands — cron remains an uncovered writer. What changed is the claim's WEIGHT, and the docstring now says so: delivery used to require a clean exit, so the uncovered-writer risk is now exercised on more occasions than when the list was written. The claim is unchanged; its exposure is not. ALSO FIXED — a real regression the merge surfaced, caught by this spec's own phase-7 parity tests. #1143 documented the new condition/exitCode cron semantics in .claude/skills/afx/SKILL.md but NOT in the .codex twin, in BOTH trees (workspace and skeleton). That is precisely the "updated one, forgot the twin" defect the phase-7 tests were written to catch, and they caught it on the first run after the merge: x keeps the two skill trees byte-identical (spec-1273 phase 7) x self-hosted root: provider skill sets and bytes (#1196 parity) x shipped skeleton: provider skill sets and bytes (#1196 parity) Ported the block verbatim to both .codex copies. Docs-only, no behaviour change. Codex-driven agents would otherwise have had no documentation of `condition` or `exitCode` at all. Suite 4106 passing, 0 failed, build clean. --- .codex/skills/afx/SKILL.md | 15 ++++++++++++++- codev-skeleton/.codex/skills/afx/SKILL.md | 15 ++++++++++++++- .../src/agent-farm/servers/session-submit.ts | 13 +++++++++++-- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/.codex/skills/afx/SKILL.md b/.codex/skills/afx/SKILL.md index c6c6386d0..a8935d99b 100644 --- a/.codex/skills/afx/SKILL.md +++ b/.codex/skills/afx/SKILL.md @@ -217,7 +217,20 @@ afx cron enable # Enable afx cron disable # Disable ``` -There is NO `afx cron add` — create YAML files in `.af-cron/` directly. +There is NO `afx cron add` — create YAML files in `.af-cron/` directly: + +```yaml +name: Service Health Check # required, unique per workspace +schedule: "*/15 * * * *" # required, cron expression (or @hourly/@daily/@startup) +command: ./health-check.sh # required, run via shell +message: "Health alert: ${output}" # required, ${output} = trimmed command output +condition: "exitCode != 0" # optional JS expression, see below +target: architect # optional, default architect +timeout: 30 # optional, seconds, default 30 +enabled: true # optional, default true +``` + +`condition` is a JavaScript expression with two variables in scope: `output` (string — the command's trimmed output) and `exitCode` (number — 0 on success, the command's exit code on non-zero exit, 124 on timeout, -1 on spawn failure). With a `condition`, the message is delivered exactly when it evaluates truthy — including on failed runs (e.g. `exitCode != 0` alerts when the command fails). Without a `condition`, the message is delivered only when the command exits 0. ## Other commands diff --git a/codev-skeleton/.codex/skills/afx/SKILL.md b/codev-skeleton/.codex/skills/afx/SKILL.md index 43f49ae4d..1122398d0 100644 --- a/codev-skeleton/.codex/skills/afx/SKILL.md +++ b/codev-skeleton/.codex/skills/afx/SKILL.md @@ -144,7 +144,20 @@ afx cron enable # Enable afx cron disable # Disable ``` -There is NO `afx cron add` — create YAML files in `.af-cron/` directly. +There is NO `afx cron add` — create YAML files in `.af-cron/` directly: + +```yaml +name: Service Health Check # required, unique per workspace +schedule: "*/15 * * * *" # required, cron expression (or @hourly/@daily/@startup) +command: ./health-check.sh # required, run via shell +message: "Health alert: ${output}" # required, ${output} = trimmed command output +condition: "exitCode != 0" # optional JS expression, see below +target: architect # optional, default architect +timeout: 30 # optional, seconds, default 30 +enabled: true # optional, default true +``` + +`condition` is a JavaScript expression with two variables in scope: `output` (string — the command's trimmed output) and `exitCode` (number — 0 on success, the command's exit code on non-zero exit, 124 on timeout, -1 on spawn failure). With a `condition`, the message is delivered exactly when it evaluates truthy — including on failed runs (e.g. `exitCode != 0` alerts when the command fails). Without a `condition`, the message is delivered only when the command exits 0. ## Other commands diff --git a/packages/codev/src/agent-farm/servers/session-submit.ts b/packages/codev/src/agent-farm/servers/session-submit.ts index 04979c29d..cb469f753 100644 --- a/packages/codev/src/agent-farm/servers/session-submit.ts +++ b/packages/codev/src/agent-farm/servers/session-submit.ts @@ -49,8 +49,17 @@ * Adopting it is Spec 1307's work; the batch form * (`write` performing the whole drain and returning the final offset) is * supported and tested, so no API change is needed when they wire it. - * - `tower-cron.ts` cron delivery — NOT covered. Same shape as the buffered - * path; a scheduled message can land beside an in-flight submission. + * - `tower-cron.ts` cron delivery — NOT covered, and RE-VERIFIED against + * #1143's rewrite of that region rather than assumed. `deliverMessage` + * still calls `writeMessageToSession` directly (`tower-cron.ts:338`), so a + * scheduled message can still land beside an in-flight submission. + * + * What #1143 changed is how OFTEN that happens. Delivery used to require a + * clean exit; a conditioned task now delivers whenever its condition is + * truthy, failures included, because a non-zero exit is data the condition + * inspects via `exitCode` rather than noise. So the uncovered-writer risk + * here is exercised on more occasions than when this list was first + * written — the claim is unchanged, its weight is not. * - `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