Render gate: close the gate→write input race and the input-echo-lag residual - #1634
Open
mohidmakhdoomi wants to merge 30 commits into
Open
Render gate: close the gate→write input race and the input-echo-lag residual#1634mohidmakhdoomi wants to merge 30 commits into
mohidmakhdoomi wants to merge 30 commits into
Conversation
… 2 self-reversals
The render gate had no input-side signal at all: `bytesWritten` counts OUTPUT and `lastDataAt` tracks OUTPUT, so a human keystroke was invisible to it until the TUI happened to echo one back. Add the two signals that close that — a monotone `inputSeq` change counter and `lastInputAt` — recorded in `write()`, the single funnel every writer passes through, so a future input path counts by default rather than by remembering to. Recording the raw chunk would not work: xterm forwards terminal REPLIES (DA, DSR, CPR, XTWINOPS, DECRPM, DECRQSS, OSC colour, focus) through the same upstream path as keystrokes, and the web client strips only three of them and only during its initial phase. Counting those would hold mail with nobody at the keyboard — and self-trip, since our own delivery repaints the TUI, which queries the client, which answers. `stripTerminalReplies` is the server-side filter, so every client is covered rather than one cooperative one, and it is SIGNAL-ONLY: the PTY still receives each chunk verbatim, because applications block waiting on their DA/DSR answers. Mouse reports deliberately COUNT: they are built from a DOM event and are a human action that can change the composer (click, middle-click paste, drag), not a parser answer. And the CSI family's final-byte class is case-SENSITIVE on purpose — `ESC[C` is Right-arrow, so an `i` flag there would silently eat every arrow key out of the signal. Also adds an injectable clock to PtySessionConfig: `recordUserInput` used `Date.now()` while the gate reads `ports.now()`, so any test pairing a fake clock with a real PtySession compared two unrelated time bases.
… race Folds the session's monotone `inputSeq` into the gate's change token and adds an input-settle interval beside the existing output one, at both check points — the pre-lock one and the in-lock precheck. The counter is what the memo needed: a CachedVerdict survives across backstop ticks, so a CLEAN verdict was reusable ACROSS a keystroke — PTY input never advances the ring, so the output-only token was genuinely unchanged. It also covers the two unbounded awaits inside the gate→write gap (classify, and a watchEcho that can scan 1000 mirror lines), neither of which the settle bounds on a loaded box. The clock is what the counter cannot be: input that landed just BEFORE the sample and is not yet echoed moves nothing, both samples agree — correctly — and the classifier reads a genuinely empty composer. This BOUNDS that window; it does not close it, and the comments say so rather than claiming closure. A hold on input records `detail: 'recent-input'`, so `afx inbox` and the send response read `busy:recent-input` through the existing shared formatter. Plain `busy` nulls its detail and is excluded from `isClassifierStuck`, so without this a recurring input hold would be silent AND unescalatable. The detail stays out of `isUnverifiableVerdict` — it is the same self-clearing "human at the line" class as `user-text`, and escalating it would false-alarm on every typist. A token that moved is attributed: `recent-input` only when the INPUT half is what moved. Between the first byte and the trailing Enter the bytes are already out, so a race there is REPORTED, not held — re-writing a message that landed is the #1584 re-injection failure. `racedByInput` is omitted when false so exact `{status:'written'}` assertions keep their meaning, and `unverifiedCause` now reaches the sender: `tower-routes` surfaced only `verified` and the CLI warned only on `verified === false`, so the very case this exists for — a human's Enter submitting our half-written body, where the header landed and `verified` is true — reported an unqualified success to the one person standing there. The drainer arms one coalesced, generation-guarded re-drain per agent when a pass held solely on input. That is not speculative: 'submit' fires synchronously from stopComposing and its drain runs in a microtask, so that pass is ALWAYS held. A consecutive-input-hold counter warns at ~60 (a human types in bursts).
…detail 27 gate tests driving the real deliverAgentMail: R1 (a keystroke during the classify, during the in-lock window, and during watchEcho — the other unbounded await in the gap), the memo case the counter exists for, R2 with its >= boundary and its NaN/unknown-age hold, the attribution rule (an output-only token move must NOT be blamed on the human), the self-trip proof over a real paced write, all four reporting quadrants, and the re-drain's coalescing, generation guard and consecutive-hold diagnostic. Plus 15 on the session's own signals, including the two that would otherwise break silently: handleUserInput(DA reply) must leave inputSeq alone AND still write the reply to the PTY verbatim (applications block waiting on their DA/DSR answers, so a filter that swallowed them would hang every attached terminal), and attachShellper must not touch the input signals while it hydrates lastDataAt. The reply-filter file is the strict half: every literal the pinned bundle emits is stripped, and every arrow key, Home/End, shift-Tab and function key SURVIVES — an `i` flag on the lowercase final-byte class would eat all of them and silently re-open the race for ordinary keyboard navigation. A test pins the table to the xterm version it was read from, so a dependency bump cannot pass unnoticed. Also documents `busy:recent-input` in the operator docs (both trees): `busy` was described as a draft or menu, and `afx inbox` can now say something new.
Found by MEASURING a running Tower for the dev-approval gate, not by a test. `handleSend` calls deliverAgentMailSerialized DIRECTLY rather than through the drainer, so a pass that held on recent terminal input never reached armInputRetry and its retryAfterMs was dropped. Every unit test drives the drainer, so all 27 passed while the operator-facing path fell through to the quiescence trigger or the backstop — in exactly the case where a human is sitting there watching the send land. Measured at 237.8ms before (the quiescence debounce, not the re-drain), 61.9ms after. The evidence script's assertion now compares against the QUIESCENCE debounce rather than the backstop interval, because "faster than 1.5s" passed happily while quiescence did all the work — which is how this was missed the first time. MailboxDrainer.noteOutcome() is the seam: a no-op before start, and idempotent per agent since the timer already coalesces.
Follows the spec-1365 / pir-1475 precedent — isolated Towers on private ports with their own test DBs, real shellper-backed PTYs, and the real HTTP and WebSocket endpoints. Nothing on the path under test is stubbed: client → WebSocket → PtySession.write → stripTerminalReplies → inputSeq/lastInputAt → ringToken/inputSettled → paced write → PTY. 20/20 checks. Step 2 (the calibration that can invalidate the design): keystroke→echo measured on the real client path with one clock. p99 4.2ms (claude) / 3.3ms (codex), shellper-backed, against a 300ms budget. The rollback criterion is evaluated in the open and did not fire — but the fixture is a repaint shim, so the figures are a LOWER BOUND and the script says so where it applies the rule. Step 5 uses a Right-arrow over the terminal's own WebSocket: it counts as input but moves only the cursor, so the composer stays CLEAN and the input signal is the only thing holding the row. A printable character would hold `user-text` — the OLD guard — and prove nothing about this issue. Step 7 catches the plan's own prediction in the act: the delayed ^C fires unattended, is observed as input (held busy:recent-input at +1007ms), and the re-drain recovers it at +1381ms rather than the backstop. Honest gaps, recorded rather than papered over: the local (non-persistent) PTY combos fail with `nodePty.spawn is not a function`, reproduced identically at the merge-base and therefore pre-existing and out of scope — SKIPped with the reason attached. The baseline for the deltas is a merge-base worktree via PIR1473_BASELINE_DIST, not the shared main checkout, whose dist is from July and no longer starts; unset makes every delta SKIP rather than report one-sided numbers. Manual steps 1, 3, 4 and the real-harness half of step 2 need a human. They are listed in the transcript as outstanding and are NOT claimed by this run.
The dev-approval evidence script covers everything a machine can check. Manual steps 1 (reply traffic), 2's real-harness half (the 300ms calibration), 3 (mouse) and 4 (send while typing) need a real harness, a real browser and real hands. codev/evidence/1473-human-runbook.md is written for someone who has read neither the plan nor the diff: numbered actions, an expected result under each, and an explicit PASS/FAIL shape per step. scripts/pir-1473-human-harness.mts supplies everything around those hands — an isolated Tower on 14793 with its own DB, workspace and shellper socket dir, plus send/inbox/calibrate/down against it. The runbook cannot say `afx`: afx send and afx inbox construct TowerClient with no port, so they always drive the LIVE Tower on 4100 where two real builders are running. It also must not say `afx attach` — attach never touches PtySession, so step 1 would log zero chunks and read as a false pass (residual 3). AF_LOG_INPUT_SIGNAL=1 makes step 1 readable without editing code: per external write it prints the raw chunk, what the reply filter stripped, and what survived as input. `survived=<NOTHING>` is the hands-off PASS marker. escapeBytes renders ESC as \e so the diagnostic cannot repaint the terminal it is read in. Verified on a live isolated Tower: a DA reply moves nothing, a keystroke moves inputSeq, a mixed chunk splits correctly. Full suite unchanged at 286 files passing, 0 failures. Tower 4100 confirmed untouched after every run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o VS Code
Step 4 asserted a verdict its own procedure prevented. It told the human to type
printable characters and expect `busy:recent-input`, but mailbox-delivery.ts:797
returns `user-text` on a non-empty composer, before the input-settle check at
:840. The human's 10/10 `busy:user-text` was a correct reading of the old guard.
The evidence script already picks a Right-arrow for scripted step 5 for exactly
this reason, and says so in a comment. I wrote the reasoning there and then
contradicted it in the manual step.
Split into 4a (empty composer, cursor-only keys — Left/Right/Home/End — expecting
busy:recent-input, carrying the 10 varied repetitions) and 4b (the original typing
procedure, expected verdict busy:user-text, proving draft integrity and labelled
as evidence for the pre-existing guard). Up/Down are excluded from 4a: they recall
history into the composer and void the rep.
Both procedures validated against a real claude before being written down:
- 4a gives pending → busy:recent-input → DELIVERED at ~2 presses/sec.
- Faster than that sits in plain `busy` — echo makes the OUTPUT settle (:835) fire
first. The runbook now says press deliberately, and that bare `busy` is not a
failure.
- A rep against a mid-turn agent returns user-text and is void.
- 4b holds busy:user-text until the draft is cleared.
send --watch N prints a verdict timeline. Revision 1's "keep clicking, then run
inbox" was unrunnable: stopping to type in the other terminal ends the condition
under test, and one sample after the fact cannot distinguish "never held" from
"held and already cleared".
Step 1b: codev.towerPort was never the problem. The Agents view reads only
/api/overview, which left-joins the terminal registry onto a readdirSync of
<workspace>/.builders/ (overview.ts:866-869) keyed by worktreeNameToRoleId
(overview.ts:475-512) — which maps a directory `pir-1473-probe` to the roleId
`builder-pir-1473`. The harness created no directory and registered the literal
roleId, failing both halves. `up` now does both; verified against the endpoint,
which returns {roleId: "builder-pir-1473", id: "pir-1473-probe"}.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the click
Rev 2 fixed the sidebar listing and verified the listing. The click reads a
different source, and still failed.
list /api/overview, filesystem-derived — the row's `id` is the DIRECTORY
NAME verbatim
click views/builders.ts:439 hands that same b.id to openBuilderByRoleOrId →
resolveBuilderTerminal → resolveAgentName (agent-names.ts:39-60),
matched against /api/state's ids by EXACT or TAIL match
`pir-1473-probe` satisfies the first and fails the second: `builder-pir-1473`
neither equals it nor ends with `-pir-1473-probe`, so the click resolved
`missing` and produced the "terminal isn't available yet" toast while the row sat
right there. `pir-1473` satisfies both.
Adds `vscode-check`, which deliberately does not re-check the endpoint that was
changed. It reproduces the click: row id from /api/overview, /api/state fetched
as the client fetches it, then the extension's OWN resolveBuilderTerminal —
imported, not reimplemented, since terminal-resolve.ts is vscode-free for exactly
this. Against the live Tower it printed the human's failure verbatim before the
rename and PASS after.
No `builders` table row was added. /api/state's builders come from the terminal
registry, not that table — it returned a live terminalId with the table empty.
The table only affects the phase grouping, so the row shows under UNKNOWN; the
runbook now says that is expected rather than a fault.
The rename was applied to the running harness workspace, so 1b is retryable
without a restart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Routes the facts and lessons by tier. Both hot files sit at their 10-item cap, so
each gets an extended entry rather than an eleventh:
arch-critical.md the afx send mailbox-first fact now also states that the
gate samples INPUT (inputSeq + a 300ms settle) and that
terminal replies are stripped server-side, signal-only.
Anyone adding a message writer needs both halves of that
sentence; splitting them across tiers invites getting it
wrong while having read the hot file.
lessons-critical.md "'tests pass' is not 'it works'" now also says to derive
the check from the USER'S ACTION, not from the code you
changed. That is the failure this PR hit three times, and
the original wording prevented none of them — each check
did exercise a real path, just not the user's.
Cold tier carries the mechanism: the ordering classify -> token re-validation ->
output settle -> input settle (and why a non-empty composer can only ever yield
user-text), why the reply filter is signal-only and version-pinned, and the
/api/overview <-> /api/state naming coupling that nothing else documents.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Architect integration review — 3-way CMAPVerdicts: gemini APPROVE (HIGH) · codex COMMENT (HIGH) · claude REQUEST_CHANGES (HIGH). High-risk review by every axis (2,744 insertions, 27 files, core Tower/terminal/mailbox). All three agree the design is sound and the manual evidence is real. I verified every finding below against the branch myself rather than relaying it. Blocking1. The new hold class is "unrecognized" in 2. The shared contract still documents three values (claude). 3. 4. Non-blocking
Worth carrying beyond this PR
What I verified as accurateThe root cause holds against |
…d boundaries BLOCKING 1 — `afx inbox show` called the new hold class "unrecognized". describeDetail() had no `recent-input` case, so the verdict this issue exists to make diagnosable printed as 'unrecognized gate detail' in the view an operator opens BECAUSE they want the explanation, while the list view rendered it fine through the shared formatter. Added with user-text-style self-clearing wording; the test is table-driven over EVERY value the gate can persist so the next detail added cannot repeat this. BLOCKING 2 — packages/types still enumerated three details. api.ts and sse.ts documented the pre-#1473 vocabulary while the SSE payload genuinely carries recent-input (escalation is age-based, so a long-held row escalates whatever its detail says). Server and client disagreed on a shared contract. BLOCKING 3 — the starvation constant was sized against one cadence and documented against another. 60 checks x (300ms settle + 25ms margin) is ~19.5s, not the ~90s claimed. A COUNT also re-scales silently whenever the drain cadence changes. The constant exists to avoid libelling an ordinary typist as a machine, and ~20s of continuous input is ordinary — the manual verification for this very issue ran 15-20s of unbroken arrow presses per repetition, so it would have fired during the verification of the feature it belongs to. Now a wall-clock 90s measured from the start of the unbroken run. Three tests: 200 passes across 20s must NOT warn (fails against the old code), 20 passes across 95s must warn once, and two 60s runs split by a delivery must not add up. BLOCKING 4 — AF_LOG_INPUT_SIGNAL=1 logs keystrokes verbatim. survived="a" is literal typed input and the runbook has operators typing into live composers. No redaction leaves the diagnostic useful — printing the exact bytes IS the feature — so the control is the flag: prominent warnings at both sites in pty-session.ts and a callout at the top of the runbook. ALSO — two operator-facing boundaries had plumbing and no test. Neither /api/send's unverifiedCause nor commands/send.ts's cause-aware warning was pinned; removing that plumbing would have left the suite green. And the raw write route counts as input only because it passes no origin and the default is 'external' — an invisible coupling one word wide, where "tidying" it to 'delivery' reopens the race for every non-WebSocket client with every gate test still green. That one is tested against a REAL PtySession: a double could only assert what the double was told to do. arch.md gains the standing invariant: every write into a PtySession counts as human input unless it explicitly opts out, and 'delivery' is correct only for bytes the gate itself authorised. Not changed, with reasons in the review: the retryAfterMs asymmetry (those branches have no settle boundary to compute a deadline from), the xterm pin resolving from packages/codev rather than apps/web (real, but a cross-package ownership question that should not ship unreviewed inside this turn), and isUserIdle()'s now-vestigial API. afx attach is recorded as the largest remaining hole; deliberately not filed as an issue from here. Full suite: 286 files, 5836 tests, 0 failures (+17). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mailbox-delivery.ts still had {@link CONSECUTIVE_INPUT_HOLD_WARN_THRESHOLD}
pointing at the constant the previous commit replaced. Now names
CONSECUTIVE_INPUT_HOLD_WARN_MS, and says why the reset is what makes the run
UNBROKEN rather than cumulative.
The review now leads that finding with the concrete evidence rather than the
arithmetic: step 4a's ten repetitions each drove 15-20s of unbroken cursor-key
input, so the old count-based rule would have fired on the human confirming that
the feature respects ordinary typists. A guard whose own acceptance test is
indistinguishable from the abuse it catches is measuring the wrong thing, and
tuning the number does not fix it.
Also recorded: `consult -m claude` failed three times with "Prompt is too long" at
this diff size (41 files, +5834). Logged as a tooling limit rather than a missing
verdict — the architect's lane obtained a full claude review of the same branch
and its four findings are the four blocking fixes — but a PIR leaning on the porch
lane alone would silently lose a third of its consultation at this scale.
And noted in the disposition preamble that one reviewer claim did NOT survive
verification: gemini reported the governance updates as landing in codev-skeleton
too. They must not — those are <placeholder> starter templates for adopters, and
there is no skeleton arch.md/lessons-learned.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…osed Human-authorized (relayed via the architect) failure record written at the path porch expects a claude review. It is written to be unmistakable: no verdict line anywhere, so porch's own `grep ... || echo UNKNOWN` resolves it as UNKNOWN — the case the protocol already anticipates for an unavailable model. It records three attempts, the verbatim "Prompt is too long", the diff size (41 files, +5834/-88), that this is a hard input limit rather than a transient failure, where claude's actual opinion lives (the request-changes verdict in the architect's integration CMAP on PR #1634, whose four findings are the four fixed in d807c18), and that the authorization was to record a failure and NOT to substitute a review. The file itself is untracked: .gitignore:65 (codev/projects/*/*.txt) excludes every consultation output, so it sits alongside the gemini and codex verdicts exactly as they do. Not force-added. This review doc is the committed record of what it says. A first draft line-wrapped so a line BEGAN with the other lane's verdict token. porch's extraction is a line-anchored grep, so the file whose whole purpose is "this is not a review" would have reported one. Caught by running the protocol's own grep against the file rather than reading it. The review now carries the gap rather than just the incident: porch models consultation completeness as file presence per model (next.ts:598), so it cannot distinguish "impossible" from "not yet attempted" from "skipped". The phase offers two exits — make a file exist, or change the lane config repo-wide — and that shape pressures an agent toward manufacturing the missing file, which is the precise act that makes a consultation look like it happened when it did not. Not filed as an issue; the architect is carrying it with the xterm-pin and afx-attach follow-ups. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n" verdict Rebuttal at 1473-review-iter1-rebuttals.md (tracked — only codev/projects/*/*.txt is ignored, so it ships with the PR). All four codex findings accepted and fixed; none rebutted as a false positive. Correction to something the architect and I both assumed: the failure record does NOT land on UNKNOWN in porch. parseVerdict (commands/porch/verdict.ts:24-48) has no unknown value at all — a file with no VERDICT: line falls through to COMMENT, labelled in the source as a "non-blocking skip". So porch state now records the failed consultation as a claude review whose verdict was COMMENT, which is a reviewer position rather than what happened. The PIR review prompt's own extraction (line-anchored grep, || echo UNKNOWN) reads the same file as UNKNOWN. Two extractions, two answers, and the one that persists into status.yaml is the one that cannot express "no review". Not adding a VERDICT: line to fix it — any value would assert a position no reviewer held. The honest state is not expressible in porch's vocabulary, which is the gap itself, now recorded in both the failure record and the review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ped state status.yaml carries no per-model verdicts -- phases, gates, iteration and pr_history only -- so parseVerdict's COMMENT fallback surfaces solely in `porch next`'s transient output. The finding stands (verdict.ts:47 reads a consultation that could not run back as an actual reviewer position, under a comment that asserts "but the consult ran"), but it is a defect in porch's verdict vocabulary, not a false verdict written into project state. Both the review doc and the thread said the stronger thing; both now say the true one. Also inline what the gitignored failure record carries. codev/projects/*/*.txt does not ship with the PR, so the review's consultation subsection is the only durable account: it now states the three attempts, the verbatim "Prompt is too long", the 41 files / +5834 -88 diff size, and the link to claude's real opinion on PR #1634, rather than pointing at a file no outside reader can open.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PIR Review: Render gate — closing the gate→write input race
Fixes #1473
Summary
afx send's render gate proved a composer was empty using output signals only — the ring'scumulative byte count and the timestamp of the last output byte — so it could see an app
repainting but was blind to a human's keystrokes. Two races stayed open: a keystroke landing
after the gate sampled its change token (nothing the token counted moved, so both samples
agreed and the message was written onto a line someone had started typing on), and a keystroke
landing just before the sample and not yet echoed (no counter comparison can catch that — both
samples agree, correctly). This PR gives
PtySession.write()— the single funnel every writerpasses through — a monotone
inputSeqcounter and alastInputAttimestamp, foldsinputSeqinto the gate's change token, and adds a 300 ms input-settle beside the existing 250 ms output
settle. A race that happens during the paced write is reported (
racedByInput→unverifiedCause), never re-written, because re-writing a message that already landed is the#1584 re-injection failure.
The precondition was a server-side terminal-reply filter. xterm forwards DA/DSR/CPR/XTWINOPS/
DECRPM/DECRQSS/OSC-colour/focus replies upstream through the same path as keystrokes;
counting those as input would hold mail with nobody at the keyboard, and would self-trip —
delivery repaints the TUI, the TUI queries the client, the client answers, and the answer looks
like typing.
terminal-replies.tsstrips them from the signal only; the PTY still receivesevery byte verbatim, because applications block waiting on their own DA/DSR answers.
Files Changed
Against merge-base
03bc5213e— 41 files, +5834 / -88. Measured after thefinal review commit, so this list includes the review and governance files themselves.
codev-skeleton/resources/commands/agent-farm.md(+6 / -1)codev/evidence/1473-dev-approval-transcript.txt(+154 / -0)codev/evidence/1473-human-runbook.md(+416 / -0)codev/plans/1473-render-gate-fuller-close-of-th.md(+598 / -0)codev/projects/1473-render-gate-fuller-close-of-th/status.yaml(+27 / -0)codev/resources/arch-critical.md(+1 / -1)codev/resources/arch.md(+8 / -0)codev/resources/commands/agent-farm.md(+6 / -1)codev/resources/lessons-critical.md(+1 / -1)codev/resources/lessons-learned.md(+4 / -0)codev/reviews/1473-render-gate-fuller-close-of-th.md(+172 / -0)codev/state/pir-1473_thread.md(+559 / -0)packages/codev/scripts/pir-1473-dev-approval-evidence.mts(+917 / -0)packages/codev/scripts/pir-1473-human-harness.mts(+581 / -0)packages/codev/src/agent-farm/__tests__/bugfix-1573-delivery-verification.test.ts(+6 / -1)packages/codev/src/agent-farm/__tests__/bugfix-1584-no-rewrite-after-write.test.ts(+13 / -3)packages/codev/src/agent-farm/__tests__/cron-delivery.test.ts(+2 / -0)packages/codev/src/agent-farm/__tests__/inbox-cli.test.ts(+44 / -0)packages/codev/src/agent-farm/__tests__/pir-1473-input-race-gate.test.ts(+684 / -0)packages/codev/src/agent-farm/__tests__/send-delivery.test.ts(+18 / -0)packages/codev/src/agent-farm/__tests__/send-integration.e2e.test.ts(+7 / -0)packages/codev/src/agent-farm/__tests__/send-mailbox-repro.test.ts(+3 / -0)packages/codev/src/agent-farm/__tests__/send.test.ts(+72 / -0)packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts(+2 / -0)packages/codev/src/agent-farm/__tests__/spec-1470-reentry-delivery.test.ts(+2 / -0)packages/codev/src/agent-farm/__tests__/tower-routes.test.ts(+169 / -0)packages/codev/src/agent-farm/commands/inbox.ts(+11 / -3)packages/codev/src/agent-farm/commands/send.ts(+18 / -1)packages/codev/src/agent-farm/db/types.ts(+15 / -5)packages/codev/src/agent-farm/servers/mailbox-delivery.ts(+413 / -46)packages/codev/src/agent-farm/servers/mailbox-wiring.ts(+14 / -4)packages/codev/src/agent-farm/servers/message-write.ts(+60 / -4)packages/codev/src/agent-farm/servers/session-submit.ts(+7 / -4)packages/codev/src/agent-farm/servers/tower-routes.ts(+18 / -0)packages/codev/src/terminal/__tests__/pty-session-input-signal.test.ts(+232 / -0)packages/codev/src/terminal/__tests__/terminal-replies.test.ts(+208 / -0)packages/codev/src/terminal/pty-session.ts(+178 / -7)packages/codev/src/terminal/terminal-replies.ts(+159 / -0)packages/sdk/src/tower-client.ts(+14 / -0)packages/types/src/api.ts(+6 / -3)packages/types/src/sse.ts(+9 / -3)Commits
820c2072cchore(porch): 1473 init pira21856871[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Plan draft41b073245[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Plan revised — 3 blockers from 2-way consult + architect verificationc4b2e3ae8[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Plan revision 3 — 3-way CMAP: 6 blockers, 2 decide-items, 2 self-reversalsc0d367659[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Give PtySession an input observation for the delivery gate9c8ead811[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Gate consumes the input signal; report and retry an input race1b8a0de16[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Tests for both input residuals, plus the operator-facing detaila65338543[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Builder thread — implement phase noteseb093bac6[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Arm the input re-drain from the request path tood5f7bd4f8[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] dev-approval evidence: script + committed transcript739a52dab[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Human runbook for the four manual steps, plus its toolingf8e045634[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Runbook rev 2: split step 4, and make the probe visible to VS Codea4eeb1c4f[PIR Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473] Runbook rev 3: make the VS Code row clickable, and check the clickTest Results
pnpm --filter @cluesmith/codev build: ✓ passpnpm --filter @cluesmith/codev test: ✓ pass — 286 test files, 3 skipped, 5836 tests, 0failures. 133 tests are new — 116 in three new files, plus 17 added across four existing
files in response to the review findings below. A baseline run at merge-base
03bc5213efailed only
worktree-write-guard(environmental in a/tmpworktree; it passes here), sonothing red is being hidden.
codev/evidence/1473-dev-approval-transcript.txt— 20/20 checksagainst a real Tower on a private port with its own test DB, with the live Tower on 4100
verified untouched before and after.
Manual verification (the human, at the dev-approval gate)
[input-signal]lines across 60 s hands-off. Liveness confirmed by the built-in vacuity check: typing one character producedsurvived="a"inputSeq24→25 andsurvived="\x7f"25→26.claudecodexbusy:recent-input.busy:recent-input.busy:user-text; drafts never corrupted, fused, or submitted.The real-harness p99s run 4–8× above the scripted shim's 4.2 / 3.3 ms, exactly as the
evidence script's "this is a LOWER BOUND, the fixture is a shim not a real harness" caveat
predicted. That caveat is the reason the calibration was worth doing on real harnesses at all:
the 300 ms constant now rests on measured evidence from the applications it actually guards,
not on a proxy that happened to agree. Both real numbers still sit an order of magnitude inside
the budget, so the constant stands unchanged.
Architecture Updates
HOT —
codev/resources/arch-critical.md: extended the existingafx sendmailbox-firstfact rather than adding an eleventh (the file is at its 10-fact cap). The gate now samples
input as well as output, and terminal replies are filtered server-side, signal-only. Anyone
adding a message writer needs both halves of that sentence; splitting them across tiers would
let someone read the hot file and still get it wrong.
COLD —
codev/resources/arch.md: the mechanism (theinputSeq/lastInputAtpair onPtySession.write(), the ordering of the classify → token re-validation → output settle →input settle chain, and why the reply filter is signal-only), plus the
/api/overview↔/api/statenaming coupling documented under the VS Code Extension section.Lessons Learned Updates
HOT —
codev/resources/lessons-critical.md: sharpened the existing "'tests pass' is not'it works'" lesson rather than adding an eleventh. It said verify the real user path; it now
also says derive the check from the user's action, not from the code you changed. That is
the failure this PR hit three times, and the original wording did not prevent any of them —
each of those checks did exercise a real path, just not the one the user takes.
COLD —
codev/resources/lessons-learned.md: the three instances in full (Testing), and twonon-obvious mechanics that cost real time (Debugging).
Things to Look At During PR Review
1. The ordering in
mailbox-delivery.tsis load-bearing and easy to break.classify → token re-validation → output settle → input settle.!verdict.cleanreturns at:797; the input settle is at:840. So a non-empty composer can never yieldbusy:recent-input— it short-circuits touser-textfirst. Any reordering silently changeswhich guard fires, and both guards hold the message, so nothing goes red. This ordering is also
why manual step 4 had to be split (see below).
2. The reply filter is signal-only, deliberately.
stripTerminalRepliesremoves repliesfrom what counts as input;
PtySessionstill writes every byte to the PTY. Applicationsblock waiting on their own DA/DSR answers, so filtering the PTY write would hang them. The
filter is also pinned to the installed
@xterm/xtermversion by a test — a version bump thatadds a newly-answered query (kitty keyboard, XTVERSION) becomes an unrecognised reply, i.e. an
uncounted-reply hold, and must not pass silently.
3. Over-strip vs under-strip are both silent, in opposite directions. An under-strip holds
mail with nobody at the keyboard; an over-strip quietly stops counting a real keystroke and
re-opens the race this issue exists to close, while every gate test keeps passing. That is why
terminal-replies.test.tsis the densest file here, and why theAF_LOG_INPUT_SIGNALtraceexists: the filter's correctness is otherwise unobservable from outside the system.
4. A race during the write is reported, never retried.
racedByInput→unverifiedCause,and the row stays
delivered. Re-writing a message that already landed is #1584. The fourreporting quadrants have explicit tests;
cause: 'input-raced'wins precedence when theverdict is also unverified.
5.
noteOutcome()ontower-routes.ts.deliverAgentMailSerializedis called directly onthe
afx sendrequest path, outside the drainer, soarmInputRetrynever saw that pass'soutcome and
retryAfterMswas dropped — the operator-facing path fell through to quiescence.All 27 unit tests passed because every one of them drives the drainer. Worth a look as a class
of bug, not just an instance.
Review findings and their disposition
Two independent review lanes ran on this PR (the architect's integration CMAP and porch's
single-pass consultation). Verdicts: gemini APPROVE, codex COMMENT/REQUEST_CHANGES,
claude REQUEST_CHANGES. Every finding below was verified against the branch before acting;
none was taken on the summary alone — including one that did not survive the check (gemini
reported the governance updates as landing in
codev-skeleton/too; they must not, since thoseare
<placeholder>starter templates for adopters and there is no skeletonarch.md/lessons-learned.mdat all).The consultation lane degrades silently on a large PR
porch's
consult -m claude --protocol pir --type impl --project-id 1473was invoked threetimes on 2026-09-06 and produced no review. Attempts 1 and 3 died with the verbatim error
Prompt is too long; attempt 2 hit an unrelated usage limit that later reset. The two attemptsthat reached the model were hours apart, on unchanged input, and failed identically — a hard
model input limit at this diff size (41 files, +5834 / −88 against merge-base
03bc5213e), not a transient error. A fourth retry was considered and rejected for the samereason.
Coverage did not actually suffer. Claude did review this branch, in the architect's separate
integration CMAP lane, which ran on a different input and did not hit the limit; that review
asked for changes (HIGH confidence) and is published in full at
PR #1634 comment 5558099314.
Its four blocking findings are the four fixed below in
d807c1802. What this lane's failurecost was the second, independent opinion — not a claude review of this code.
The protocol gap is worth more than the incident. porch models consultation completeness as
file presence per model (
commands/porch/next.ts:598—reviews.length < effectiveModels.length). It has no representation of a model that could not run, so it cannotdistinguish "impossible" from "not yet attempted" from "deliberately skipped". The phase blocks
on a missing file and offers exactly two exits: make a file exist, or change the consultation
lane config repo-wide. The honest state — "this model cannot review a diff this size" — has
nowhere to live in porch's state at all.
That shape has a failure mode beyond inconvenience: the pressure it puts on an agent is to
manufacture the missing file, which is precisely the action that would make a consultation look
like it happened when it did not. Here the builder escalated instead and a human authorized a
failure record at that path
(
codev/projects/1473-*/1473-review-iter1-claude.txt) — a file carrying no verdict line, so thePIR review prompt's line-anchored
grep … || echo UNKNOWNextraction resolves it asUNKNOWN,the case the protocol already anticipates for an unavailable model. Anyone opening that file
finds an account of the three failed attempts and a pointer to where claude's real opinion
lives, not a review.
That file is gitignored (
.gitignore:65,codev/projects/*/*.txt), exactly as the geminiand codex outputs are, and it was deliberately not force-added. So it does not ship with this
PR, and this section is the only durable, shareable record of what happened — which is why
the attempts, the verbatim error, the diff size and the pointer are all stated above rather than
delegated to the
.txt.Writing that file surfaced a sharper instance of the same gap. porch's
parseVerdict(
commands/porch/verdict.ts:24-48) has no "unknown" or "did not run" value at all — a filewith no
VERDICT:line falls through toCOMMENTunder the comment "No valid VERDICT: linefound but the consult ran — treat as COMMENT (non-blocking skip)" (
verdict.ts:47). "But theconsult ran" is exactly the premise that is false here, so the fallback turns "this model could
not run" into
COMMENT, an actual reviewer position.Scope that claim carefully, because it is easy to overstate and we did at first: this is a
defect in porch's verdict vocabulary, not a false verdict written into shipped state.
status.yamlcarries phases, gate approvals and PR history — no per-model verdicts at all — sothe
COMMENTfallback surfaces only inporch next's transient output. No artifact thatreaches a reader claims claude reviewed this PR. The PIR review prompt's own extraction reads
the same file as
UNKNOWN. Two extractions, two answers, and neither vocabulary has a slot fora model that could not run. The remedy is not to add a
VERDICT:line: any value wouldassert a position no reviewer held. The honest state is simply not expressible.
Practical consequence for the next large PIR: expect the porch lane to lose a model somewhere
around this diff size, plan for a second lane or a split review rather than discovering it at
the gate, and do not read porch's
COMMENTfallback as proof a model actually ran.Fixed — the new hold class was "unrecognized" in
afx inbox show.describeDetail()(
commands/inbox.ts) had norecent-inputcase, so the one verdict this PR exists to makediagnosable printed as
unrecognized gate detail— in the view an operator opens becausethey want the explanation, while the list view rendered it correctly through the shared
formatter. Added with
user-text-style self-clearing wording, plus a table-driven test overevery value the gate can persist, so the next added detail cannot repeat this.
Fixed — the shared contract still enumerated three details.
packages/types/src/api.tsandsse.tsdocumented the pre-#1473 vocabulary. The SSE payload genuinely carriesrecent-input(escalation is age-based, so a long-held row escalates whatever its detail says), so server and
client disagreed on the contract. Both updated, with a note on the SSE type saying why the
fourth value reaches consumers.
Fixed — the starvation warning was sized against one cadence and documented against another.
CONSECUTIVE_INPUT_HOLD_WARN_THRESHOLD = 60claimed "~90s at the 300ms re-drain cadence", but60 × (300 + 25) ≈ 19.5s.
The arithmetic is the weaker half of the argument. The sharper evidence is that the manual
verification of this very feature would have tripped it: step 4a's ten repetitions each drove
15–20 seconds of unbroken cursor-key input, which is precisely the window the old rule called
machine-generated. A constant written to avoid libelling an ordinary typist as a machine would
have fired on the human confirming that the feature respects ordinary typists. When a guard's
own acceptance test is indistinguishable from the abuse it is meant to catch, the guard is
measuring the wrong thing — no amount of tuning the number fixes that.
So it is now a duration, not a count:
CONSECUTIVE_INPUT_HOLD_WARN_MS = 90_000, measured fromthe start of the unbroken run. That is what the comment always meant, and unlike a count it
cannot silently re-scale when the drain cadence changes — the backstop, quiescence and submit
triggers all drive passes too, so the pass rate was never a stable unit in the first place.
Three tests pin it: 200 passes across 20s must not warn (this one fails against the old
code), 20 passes across 95s must warn exactly once, and two 60s runs separated by a delivery
must not add up.
Fixed —
AF_LOG_INPUT_SIGNAL=1logs keystrokes verbatim.survived="a"is literal typedinput, and the runbook has operators typing into live composers. There is no redaction to add
without destroying the diagnostic — printing the exact bytes is the feature — so the control
is the flag, and it now carries a prominent sensitive-data warning at both sites in
pty-session.tsand a callout box at the top of the runbook telling the operator to typenothing real and not to paste raw trace output into an issue or chat.
Fixed — two operator-facing boundaries had plumbing and no test. Neither
/api/send'sunverifiedCausepropagation norcommands/send.ts's cause-aware warning was pinned; as codexput it, removing that plumbing would have left the suite green. Added route tests for both
causes plus the additive-absence case, and CLI tests for both wordings, the older-Tower
verified: falsefallback, and an explicit assertion that operator text never leaks theverifier's internals ("needle", "0 chars") — which the plan had called out by name.
Fixed — the raw write route's input coupling was untested.
POST /api/terminals/:id/writecounts as input only because it passes no
originand the default is'external'— aninvisible coupling one word wide, and "tidying" it to
'delivery'would reopen the race forevery non-WebSocket client while every gate test kept passing. Now tested against a real
PtySession(a double could only assert what the double was told to do): a keystroke advancesthe signal, a DA reply does not but still reaches the PTY, and a mixed chunk keeps only the
human residue.
Not changed —
retryAfterMsasymmetry (mailbox-delivery.ts:825,:897). Thetoken-moved-by-input branches omit it while the settle branches supply it. Deliberate: those
branches fire when the screen moved during the classify, so the input may still be arriving
and there is no settle boundary to compute a deadline from — the next pass re-samples and arms
the retry properly once the input is actually the only thing holding. Supplying a made-up
deadline there would arm a timer against a number that describes nothing.
Flagged, not fixed — the xterm pin test does not guard the emitting client.
terminal-replies.test.tsresolves@xterm/xtermfrompackages/codev, butapps/webdeclares its own and is what actually emits replies through
Terminal.tsx. Both are^5.5.0today, so the guard works now but would not trip on an
apps/web-only bump. Left alonedeliberately: pointing the test at the right package is a one-line change with a cross-package
dependency question behind it (which package should own the pin), and doing it inside a
REQUEST_CHANGES turn without a reviewer seeing it is how a small correct change becomes an
unreviewed one. Worth its own issue.
Flagged, not fixed —
isUserIdle()now has zero production consumers. The gate readslastInputAtdirectly, leaving Spec 403's typing-awareness API vestigial. A MAINTAIN candidate,not this PR's business.
afx attachis the largest remaining holeThe gate observes input at
PtySession.write().afx attachtalks to the shellper socketdirectly and never passes through it, so neither its keystrokes nor its terminal's replies are
observed at all — and it is the surface a human is most likely to be sitting at. The plan
scoped it out and this PR does not change it; the manual runbook is explicitly forbidden from
using it, because a step-1 trace run there would log zero chunks and read as a clean pass.
Two consequences for a reader: the
afxcommand documentation's claim that a held delivery"cannot fuse" with a draft is an absolute that
attachdoes not honour, and closing this holeis a separate piece of work. It deserves its own issue — deliberately not filed from here.
How to Test Locally
afx dev pir-1473node --experimental-strip-types packages/codev/scripts/pir-1473-dev-approval-evidence.mtscodev/evidence/1473-human-runbook.md, driven bypackages/codev/scripts/pir-1473-human-harness.mts(up/send/inbox/calibrate/vscode-check/down)busy:recent-inputappears when a human is at an empty composerand
busy:user-textwhen there is a draft; that a draft is never fused or submitted; that anidle terminal still delivers promptly (measured −1.9 ms against merge-base, i.e. no
regression on the common path).
Flaky Tests
None. No test was skipped or quarantined.