Skip to content

Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473

Closed

Description

@mohidmakhdoomi

What

The render gate samples a ring change-token, classifies the screen, then re-checks the token before writing — so a change in the gate→write window forces a HOLD rather than a write onto a stale-clean prompt. That guard closes the output-side staleness window. Two related input-side residuals remain open:

  1. Human keystroke between snapshot and write (the R7 staleness case). A user can type into the composer in the same gate→write window; the change-token catches most cases, but the guard should fully cover a keystroke that lands after classify but before (or during) the paced write.
  2. Input-echo lag. A just-typed character may not yet be echoed into the ring at snapshot time, so the gate can read "empty" a beat before the echo lands.

Tighten the gate so both are covered — i.e. the "verified-empty prompt" guarantee holds against in-flight human input, not just in-flight agent output.

Why

The whole point of the gate is that a body only ever lands on a genuinely empty prompt. The output-side token guard already enforces that against agent repaints; these two input-side windows are the remaining ways a human keystroke can race the write. They're narrow, but they're the same corruption class the spec exists to eliminate, so closing them fully is worth doing.

Notes

Activity

  1. added a commit that references this issue on Aug 18, 2026
  2. mohidmakhdoomi commented on Sep 4, 2026

    @mohidmakhdoomi
    CollaboratorAuthor

    Pre-spawn verification (architect, 2026-09-04 @ 03bc5213e)

    The body was written 2026-08-17. The premise still holds and the residual is still open — but a lot landed underneath it since (#1573 settle-before-write + echo verification, #1584 point-of-no-return, #1365 the per-terminal write lock, #1482 the detail sub-code), so the shape of what is left is narrower and better-defined than the body describes. Verified refs at HEAD:

    Claim Body said Actually at HEAD Verdict
    The ringToken re-check closes the output-side staleness window mailbox-delivery.ts:397 ringToken() defined at packages/codev/src/agent-farm/servers/mailbox-delivery.ts:520; sampled at :606; re-checked pre-lock at :653 and again inside the per-terminal lock at :703 (precheck, added by #1365) holds (line drift only; the guard is now doubled)
    Residual 1 — human keystroke in the gate→write window open open, and named in the code as this issue's territory: mailbox-delivery.ts:696-700 — "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 … the echo-lag residual for the rest is #1473's territory." Same statement at session-submit.ts:121-127. holds
    Residual 2 — input-echo lag open open. ringToken = bytesWritten:colsxrows:app (:521), and bytesWritten is the ring's output counter (pty-session.ts:930). _lastDataAt is likewise set only in onPtyData (pty-session.ts:514-516), i.e. output. PtySession.write() — the single funnel for user input, "WebSocket → write() → shellper" (pty-session.ts:232, :628-640) — records no timestamp or counter, so nothing in the gate's inputs can observe pending input. holds
    "consider … an explicit post-write echo settle" (Notes) suggested already landed, elsewhere: SETTLE_BEFORE_WRITE_MS = 250 + settled() (:422, :447) gate on output quiescence pre-write, and watchEcho/echoNeedle/verify() (:246, :489, :714-715, :838) verify the write's own echo after. Both are #1573's, both keyed on output. partly superseded — scope this issue to the input side only

    Consequences for scope

    1. The output-side work in the Notes is done; do not redo it. What is unclosed is exactly one thing: the gate has no observation of PTY input. bytesWritten and lastDataAt both move on output only, so a human keystroke that has not yet been echoed is invisible to tokenBefore, to both re-checks, and to settled().
    2. Risk direction is unchanged and fail-safe: the residual can only produce a write onto a line a human just started typing on — narrow, but it is the same false-CLEAN corruption class the spec exists to eliminate. This is hardening, not a live outage.
    3. There is a real cost axis to weigh in the plan: any input-settle requirement trades delivery latency for safety, and the delivery's own paced write must not self-trip whatever input signal is added (submitMessagePaced writes through the same PtySession.write()).

    Protocol: PIR — the approach needs review before coding (extend the change token with an input counter vs. a separate input-settle interval vs. both), and the result has to be verified against a running terminal, not only unit tests.

  3. mohidmakhdoomi commented on Sep 4, 2026

    @mohidmakhdoomi
    CollaboratorAuthor

    On it! Working on this with the PIR protocol (plan + dev-approval gates before PR).

  4. added a commit that references this issue on Sep 4, 2026
  5. mohidmakhdoomi commented on Sep 4, 2026

    @mohidmakhdoomi
    CollaboratorAuthor

    Plan review — 2-way consult (claude, codex) + architect verification

    Plan a21856871. Both reviewers land on REQUEST CHANGES, and they corroborate each other on
    five of the seven items. I independently verified every claim below against the source — the
    reviewers' file:line refs are reproduced only where I could confirm them.

    The design is right: two mechanisms, fail-safe 'external' default, flag-not-hold at the point of
    no return. Nothing below asks you to change that shape. Both reviewers explicitly endorsed it,
    and so do I. What follows is what has to be true before it is safe to build.

    BLOCKER 1 — terminal reply traffic is counted as human input (claude; verified)

    apps/web/src/components/Terminal.tsx:639 forwards everything xterm.js emits on onData →
    WS → tower-websocket.ts:72 → handleUserInput. xterm emits terminal replies through that same
    event, and the client strips only three of them — DA/DA2, CPR, DECRPM (Terminal.tsx:655-661) —
    only while rc.initialPhase is true. I traced that flag: set true on every connect
    (:421, :462) and cleared by flushInitialBuffer (:469) on a short post-connect timer. So for
    the entire steady-state life of a session the filter is off, and DA/CPR/DECRPM replies (plus
    focus reports ESC[I/ESC[O, and mouse reports if the TUI enables tracking) reach
    handleUserInput verbatim.

    Three consequences under the plan as written:

    1. False holds with no human present. These replies are provoked by output, so they cluster
      exactly where deliveries cluster.
    2. A self-trip route the 'delivery' origin cannot close — this is the honest answer to the
      self-trip question, and the plan misses it. Our paced write causes a repaint; the repaint carries
      a query; the browser replies; the reply counts as foreign input. The delivery trips its own
      signal via a round trip through the client.
    3. Starvation, not just latency. busy is excluded from isClassifierStuck, so a terminal with
      a chatty attached client never escalates — it silently never delivers.

    Required: a server-side classifier in handleUserInput that excludes terminal replies before
    bumping the gate signals — server-side and unconditional, because afx attach, the VS Code webview
    and mobile clients do not share the client-side filter. Keep write()'s 'external' default
    exactly as designed; this is a narrow enumerable exclusion on one path, not an inversion.

    Add a manual test: browser attached, hands off the keyboard, agent running — log every
    handleUserInput chunk for 60 s. That is the direct measurement, and it belongs beside the 300 ms
    calibration.

    BLOCKER 2 — racedByInput must escalate independently of echo (both reviewers; verified)

    The plan says the flag "joins the existing path that verified === false uses". That path is nested
    inside if (echo) at mailbox-delivery.ts:819-858, and echo is null whenever echoNeedle()
    returns '' (short/raw sends). Two holes:

    • short messages would drop the flag entirely;
    • for the Enter-truncation case the needle is the message's first line, so it matches and
      verified comes back true while the tail was lost.

    racedByInput is the only signal for that failure. Handle it outside the if (echo) block:
    const unverified = result.racedByInput === true || verified === false, escalating exactly once,
    and add a reason discriminator to the onUnverifiedDelivery payload — an operator currently cannot
    tell "header never appeared" from "a human typed into it mid-write". Test both echo-verifies-true
    and no-echo-needle.

    BLOCKER 3 — the type contract and its fakes (both reviewers; verified)

    • submitMessagePaced takes WritableSession & { id: string } (message-write.ts:162-168), and
      WritableSession declares only write (:11-20). The input watch needs inputBytes on that
      contract — the plan only adds the fields to DeliverySession.
    • The tracked wrapper at message-write.ts:189-195 must forward the origin. A 1-arg function
      is assignable to a 2-arg function type, so TypeScript will not catch this, and the failure
      mode is "mail never delivers". Codex's suggestion is the safer construction: hard-code
      write: data => session.write(data, 'delivery') in the wrapper rather than threading an origin
      parameter through the general helpers.
    • Three doubles that break at runtime, not compile time, none in the plan's migration list — I
      confirmed all three:
      • tower-routes.test.ts:226 gateSession — structural, reaches the live mailbox-wiring binding.
        Missing lastInputAt → now() - undefined → NaN → inputSettled() false → every send test in
        that file holds instead of delivering.
      • tower-websocket.test.ts:61 makeSession — same shape.
      • spec-1313-paced-write-drop.test.ts:33-45 — a WritableSession & {id} with no inputBytes.
    • send-architect-identity.test.ts:108 calls s.write(msg) directly on a real session to simulate
      a delivery. Under the new default that becomes external input. Adding fields does not fix it —
      it must pass 'delivery' or use the real paced writer.
    • typing-awareness.test.ts:63-161 asserts recordUserInput() itself moves lastInputAt. Do not
      make it a no-op. Keep it as the assignment, call it from write()'s external branch, and remove
      only the now-duplicate call in handleUserInput.

    Decide and document

    • The delayed ^C rationale is wrong. tower-routes.ts:1834 live.write('\x03') is documented
      three lines below as firing UNATTENDED — the plan's "a human is standing at that terminal"
      does not apply. It should still count (it changes composer state), but say that, and note the
      consequence: the scheduleDrain nudge right after it is now guaranteed to hold, slipping the body
      to the quiescence trigger.
    • Restate the counter's justification. Claude is right that your headline R1 case is largely
      covered by the settle alone (a post-sample keystroke is by construction <300 ms old at both check
      points). The counter's load-bearing value is (a) awaits longer than the settle — classify, lock
      wait — and (b) verdict-memo invalidation. Name those, or a future reader will delete the
      counter as redundant.
    • "Closes R2" overclaims. 300 ms is a measured bound, not a closure. Input older than the settle
      whose echo is still delayed, and input still in flight from the browser, remain eligible. Say
      "bounds the residual to…", name the surviving residual in the plan, the code comment and the
      review, and give the dev-gate measurement a margin/rollback criterion. Otherwise the codebase will
      assert Render gate: fuller close of the gate→write input race (R7 staleness) and input-echo-lag residual #1473 is closed when it is not.
    • Correct "a keystroke removes nothing." A human Enter mid-paced-write submits our partial
      body; ^U/^W/^C truncate. The flag-not-hold decision survives — re-writing is still worse —
      but the WARN text must say "may have been truncated".
    • Counter semantics for the three write() return paths (pty-session.ts:628-640): specify
      whether a dropped write bumps. Recommend bumping on the external branch regardless — fail-safe.
    • .length is UTF-16 code units, not bytes. Either Buffer.byteLength or rename to
      inputUnits. A change token only needs a monotone increment, but the name should not lie.
    • Monotonicity, stated precisely: the token is not globally monotone (geometry and app can
      change back and forth). The counters are, and that plus the session-object identity guard is what
      preserves CachedVerdict's non-aliasing argument. Also state that _inputBytes must never reset —
      including across SPAWN relaunch and attachShellper, which replace the PTY while keeping the
      PtySession object.

    Two corrections in your favour

    • isUserIdle/lastInputAt have no production consumers — I grepped packages and apps; the
      only references outside pty-session.ts are typing-awareness.test.ts. Spec 403 is a test
      constraint, not a live one. The gate becomes lastInputAt's first real consumer.
    • Your latency argument has a number that supports it: QUIESCENCE_DEBOUNCE_MS = 500
      (pty-session.ts:40) > 300, so any quiescence-triggered pass is automatically input-settled
      whenever the last input preceded the last output byte — the normal case, since the TUI echoes.
      Cite it. But sharpen the 'submit' claim: mailbox-wiring.ts:600 wires 'submit' to
      scheduleDrain, which runs in a microtask, so that pass is now provably always held, not
      "largely unaffected". And codex is right that "only bites while actively typing" is too narrow —
      a single navigation key with no output can cost close to a full backstop period.

    Worth deciding explicitly

    Claude proposes lastInputAt > lastDataAt ("input arrived and nothing has been painted since") as a
    constant-free R2 signal that also covers the >300 ms un-echoed residual. It is strictly better
    only if Blocker 1 is fixed first — without the reply filter it deadlocks permanently, since a DA
    reply produces no output and the condition never clears. Evaluate it, pick one, and record why.

    What I need back

    A revised plan addressing the three blockers and the decide/document items, then I take the gate to
    the human. Raw reviews are on disk if you want them in full; ask and I will paste either.

  6. added a commit that references this issue on Sep 4, 2026
  7. mohidmakhdoomi commented on Sep 4, 2026

    @mohidmakhdoomi
    CollaboratorAuthor

    Plan review, revision 2 — 3-way CMAP (gemini / codex / claude) + architect adjudication

    Plan 41b073245. Verdicts split: gemini APPROVE, claude 3 blockers, codex 4. Where they
    disagreed I went to the source and adjudicated; where they agreed I still verified before relaying.
    Everything below is confirmed against the pinned dependency or the file cited.

    The design is not in question. 'external' default, hard-coded 'delivery' wrapper, counter+clock,
    flag-not-hold, the lastInputAt > lastDataAt rejection — all three reviewers endorse them, and so do
    I. Six items block building.

    A. The reply table misses replies our pinned xterm actually emits (claude + codex; verified)

    I enumerated the emission sites in the pinned bundle
    (node_modules/.pnpm/@xterm+xterm@5.5.0/.../lib/xterm.js). Missing from §1's table:

    xterm 5.5.0 emits Status
    ESC[0n (DSR reply to CSI 5 n) missing
    ESC[4;h;w t, ESC[6;h;w t, ESC[8;rows;cols t (XTWINOPS — the pixel-geometry probe apps use to sniff image support) missing
    ESC[?<row>;<col>R (DECXCPR) pattern arity wrong — the table requires three params (\x1b\[\?[0-9]+;[0-9]+;[01]R); xterm emits two

    These are exactly the output-provoked class, so §5's "self-trip completely closed" is false as
    written
    — our write → repaint → app queries geometry → browser answers → counted as input.

    Take claude's final-byte rule for the CSI family. No key sequence xterm can produce ends in
    c, n, t or y — those finals are reply-only (keys end in A–D/F/H/P–S/~/Z/u, plus
    R and m/M). One rule covers DA/DSR/XTWINOPS/DECRPM including forms nobody has enumerated yet:

    /\x1b\[[?>=]?[0-9;]*\$?[cnty]/g
    

    Keep specific anchored patterns only where the final byte is genuinely ambiguous with real input —
    R (CPR: \x1b\[\??[0-9]+;[0-9]+R, which also fixes the DECXCPR arity) and u if the VS Code fork
    ever answers a kitty-keyboard query (\x1b\[\?[0-9;]*u — the ? is what keeps it off real
    kitty-encoded keystrokes). Pin it with a test asserting every literal xterm can emit is stripped, and
    say in the module doc comment that the table is derived from a pinned dependency, so a version
    bump is a review trigger. XTVERSION is dead weight (xterm never emits it) — harmless, keep it for
    the VS Code fork.

    B. Mouse reports must COUNT as input — do not strip them (codex is right; claude is wrong)

    The two reviewers directly contradict each other here, so I checked the bundle myself. The parser
    reply sites are distinct callbacks; mouse reports go out through the generic user-event path —
    they are produced from DOM mouse events. A mouse report is a human action that can change the
    composer (a click that moves the cursor, a middle-click paste, a drag selection). Stripping it
    re-opens R1 for mouse-driven TUIs, which is the corruption direction this issue exists to close.

    Remove the mouse row from the table. Claude's opposite suggestion — add the urxvt 1015 form to
    the filter — would widen the hole, not close it. The flooding concern is real but correctly bounded:
    motion tracking holds delivery only while the mouse is actually moving, and clears 300 ms after it
    stops. That is the fail-safe direction.

    C. The interactive sender is still told verified: true on an input-raced delivery (claude + codex; verified)

    §6 threads cause into UnverifiedDeliveryInfo and stops there. But the primary operator surface
    for an interactive afx send is the send response: tower-routes.ts:2288-2289 surfaces only
    outcome.verified, and commands/send.ts:462 prints its warning only on verified === false.

    So in the exact case §6 exists for — Enter truncation, where the needle is the first line, it landed,
    verified === true — the row is escalated, the dashboard gets a notification, and the human who ran
    the send is told plain "Message delivered". Same for racedByInput with no needle, where verified
    is absent entirely.

    Add a result-level discriminator (unverifiedCause?: 'no-echo' | 'input-raced') threaded through
    DeliveryOutcome → /api/send → the send response type → commands/send.ts. Do not overload
    verified — it would mean two different things at one call site. Two smaller holes in the same
    section: cause precedence is unspecified when both are true ('input-raced' should win — it is
    the more actionable remedy), and the WARN at mailbox-delivery.ts:840-841 interpolates
    needle.length, which now runs with needle === '' and prints "needle 0 chars". Branch the text,
    don't append to it. Keep racedByInput omitted when false so the existing exact
    {status:'written'} assertions in spec-1365-serializer-convergence.test.ts keep passing.

    D. 'pre-recorded' is an unenforced bypass — delete the third origin (claude + codex)

    Nothing couples the value to an actual recording: write(data, 'pre-recorded') from a future caller
    that records nothing is invisible input — the precise failure 'external' exists to prevent, and the
    same "compiles fine, mail never delivers" shape as the 1-arg/2-arg hazard. Gemini's defence ("a
    caller cannot invoke it without typing the literal") misses the point: it is an available escape
    hatch that asserts something the type cannot check.

    Claude's fix is strictly simpler and I'd take it — move the filter inside write()'s external
    branch
    :

    if (origin === 'external') { const human = stripTerminalReplies(data); if (human) this.recordUserInput(human); }

    and let handleUserInput just call this.write(data). Three origins collapse to two, the raw route
    gets the filter for free, composing still sees raw data per §1's scope note, and the delivery path
    is unaffected. write() is synchronous with no await, so record-after-write is atomic w.r.t. the
    event loop.

    E. The "2 s lock wait" justification is FALSE (claude; verified) — and it is slated for a code comment

    §4 argues the counter earns its place partly because "a delivery can sit on the per-terminal lock for
    up to 2 s between tokenBefore and the in-lock precheck". A delivery never waits on that lock.
    submitMessagePaced → trySubmitToSession (session-submit.ts:474-482) returns false immediately
    when isSubmissionInFlight. OPERATOR_SUBMIT_WAIT_CEILING_MS is what an operator waits under while
    a delivery holds the line — the opposite direction. The plan quotes that asymmetry itself
    ("deliveries decline, operators wait", session-submit.ts:91-97) and then argues from the wrong side.

    This matters precisely because §4 says these cases go into a code comment so a future reader won't
    delete the counter. A false justification there is worse than none. The counter still earns its
    place
    — memo invalidation is correct and sufficient alone. The real second case: the
    tokenBefore → precheck gap contains await ports.classify and await ports.watchEcho
    (:626, :715 — the latter flushes and scans up to 1000 mirror lines), neither bounded by 300 ms on
    a loaded box. Say that instead.

    F. WritableSession as specified will not compile (codex; verified)

    §3 shows const tracked: WritableSession = { write: … } — no inputSeq — while the same section and
    the Files list both add required readonly inputSeq: number to WritableSession. Contradictory;
    message-write.ts is inside packages/codev/tsconfig.json, so this is a build break, not a test one.
    Split the types: WritableSession keeps the one-arg write (all writeEscapeToSession /
    writeMessageToSession need), and the paced-delivery seam takes
    WritableSession & { id: string; readonly inputSeq: number }. The inner tracked adapter stays the
    minimal write-only shape. This also spares the fake churn in helpers that never needed the field.

    Not a bug — no change needed

    Codex reads the OSC row's (\x07\|\x1b\\) as a literal-pipe regex error. It is markdown table
    escaping
    — a bare | would break the cell, and the XTVERSION row escapes one the same way. The
    intended regex is (?:\x07|\x1b\\). Nothing to fix in the design, but write it unambiguously in the
    source (and add a test for BEL vs. ST termination separately, which codex is right to want).

    Decide — a residual none of us had named

    An input-caused hold calls hold('busy'), which nulls detail (:591-596), and plain busy is
    excluded from isClassifierStuck (:400-405). So #1482's whole diagnostic axis — telling "a human
    is at the composer" from "the classifier can't verify" — is lost for this new hold class, and nothing
    escalates it. Residual 4's "a spurious hold the backstop clears" is therefore too optimistic: a missed
    reply that recurs at under 300 ms (an app polling geometry every repaint) holds indefinitely, and
    the only net is escalateHeldToOwner at ~180 s, which is skipped for architects. A starved architect
    would be entirely silent. Minimum: a distinct log/counter for consecutive input-settle holds. Better:
    a gate detail so afx inbox can say "waiting on recent terminal input". Your call which, but name it.

    Also decide — the one-shot re-drain

    Claude argues for building it now rather than deferring to measurement, since the 'submit'-trigger
    hold is proven analytically (scheduleDrain is a microtask; stopComposing emits 'submit'
    synchronously after recordUserInput), so there is nothing left to measure — and one timer is
    simultaneously the mitigation for the submit case, the navigation-key case, and the residual above.
    Your "measure first" instinct is defensible; the analytic certainty does weaken it. Decide explicitly
    either way.

    Test-plan additions (beyond the above)

    • Assert the full chunk still reaches the PTY after filtering — the one way this breaks every
      attached terminal (apps block waiting on their DA/DSR replies). handleUserInput(DA_REPLY) →
      inputSeq unchanged and the PTY got the reply verbatim.
    • Assert mouse SGR/X10 survives and counts (per B).
    • Assert the operator bypasses still count as external — one assertion on writeEscapeToSession
      stops a future refactor tagging them 'delivery'.
    • Reply-shaped bytes inside bracketed paste: state the intended behaviour and pin it.
    • send-integration.e2e.test.ts (:244, :570, :597) uses the raw route and is unlisted. It
      survives only because the redelivery wait is 12 s — make that deliberate, not lucky.
    • The doubles are not all one class: spec-1313-paced-write-drop.test.ts:33-45 breaks at compile
      time (annotated literal), the others at runtime. And codex is right that tower-websocket.test.ts
      exercises WS delegation only — it does not reach the live mailbox binding, so the "every send holds"
      description fits tower-routes.test.ts alone.
    • State the general rule behind the send-architect-identity breakage: recordUserInput() uses
      Date.now() while the gate uses ports.now(), so any test pairing a fake clock with a real
      PtySession breaks, and adding fields never fixes it. Those tests survive today only because
      attachShellper hydrates _lastDataAt from the fake shellper; _lastInputAt has no such seam.
      Consider an injectable clock on recordUserInput.
    • Manual step 1 cannot be run for afx attach — that client connects straight to the shellper
      socket (commands/attach.ts:141-142) and never touches PtySession, so logging at
      handleUserInput shows zero chunks and reads as a pass. Substitute the VS Code integrated terminal
      (a different xterm build, and the one surface whose reply set may differ), and make residual 3 say
      an attach client's input and its terminal's replies are wholly out of scope.

    On gemini's APPROVE

    Discount it. It asserts self-trip is "completely closed" (false — DSR/XTWINOPS), calls the residual
    list "complete" (it misses the escalation-blind hold above), calls the missing DSR "non-hazardous"
    when it is precisely the self-trip class, and cites terminal-replies.ts as though it already exists.
    Its one useful note — add \x1b\[[0-9]+n — is subsumed by A's final-byte rule.

    What all three agreed to keep exactly as written

    The 'external' default and its reasoning; the hard-coded 'delivery' wrapper and the 1-arg/2-arg
    argument; flag-not-hold with the corrected "may have been truncated or submitted early" wording; the
    inputSeq naming and "it exists to differ, not to total"; bump-regardless-of-return-value (and the
    writable-before-token ordering at :702/:703 that makes it safe); the never-reset/never-hydrate
    rule for _inputSeq; leaving composing alone; required fields over optional; and §7's insistence
    that the three comment blocks state surviving residuals rather than claiming closure.

    Revise, and I take it to the human.

  8. added 5 commits that reference this issue on Sep 4, 2026
  9. 3 remaining items

  10. added 8 commits that reference this issue on Sep 6, 2026
  11. added 5 commits that reference this issue on Sep 8, 2026
  12. added a commit that references this issue on Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

area/towerArea: Tower server / agent farm CLI

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions