Repository navigation
fix(terminal): harden the rendering pipeline against dropped bytes, size drift, and flood lag - #221
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPer-stream PTY dedupe; per-agent integrations delivery with spawn-aware caching and cooldown; renderer adds EchoRouter, PTY size-sync, monotonic PTY replay baselines, runtime wiring, tests, and updated Playwright fidelity gating. ChangesBroker PTY Deduplication Refactoring
Integrations System-Message Delivery
Renderer Echo Routing, Size Sync, and Terminal Integration
Playwright fidelity tests & CI gating
Sequence DiagramsequenceDiagram
participant Broker as Broker (worker_stream)
participant BrokerManager as BrokerManager
participant Runtime as TerminalRuntime
participant EchoRouter as EchoRouter
participant Engine as PredictiveEchoEngine
participant Xterm as Xterm
participant User as User
Broker->>BrokerManager: PTY chunk (event_id/seq + bytes)
BrokerManager->>Runtime: emit PTY-chunk batch
Runtime->>EchoRouter: onServerOutput(coalesced bytes)
alt EchoRouter route == engine and Engine.hasPredictions
EchoRouter->>Engine: onServerOutput(bytes)
Engine->>Xterm: onEngineOutput(predicted/confirmed writes)
else
EchoRouter->>Xterm: direct write(bytes)
end
User->>Runtime: sendInput(data) -> noteUserInput
Runtime->>EchoRouter: onUserInput(data)
alt getInputSrtt() >= PREDICTION_ENGAGE_THRESHOLD_MS
EchoRouter->>Engine: reseed() / onUserInput -> engage predictions
else
EchoRouter->>Xterm: optimistic local echo (no engine)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (1)
57-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment references removed flag.
Line 58 mentions "traces from the continue-on-error fidelity step," but
continue-on-error: truewas removed from the fidelity step at line 53. Thealways()condition still makes sense (to capture traces from any failed step), but the comment should be updated to reflect the current behavior.📝 Suggested comment update
- name: Upload Playwright traces - # always() so traces from the continue-on-error fidelity step are captured too - # (a continue-on-error failure does not trip failure()). No-op when nothing failed. + # always() so traces are captured from any failed step. No-op when nothing failed. if: always()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 57 - 60, Update the inline comment above the "Upload Playwright traces" step so it no longer mentions "the continue-on-error fidelity step"; instead state that the if: always() condition ensures Playwright traces are uploaded for any failed step (or when failures occur), and that it is a no-op when there are no traces to upload; modify the comment text near the Upload Playwright traces step and the if: always() line to reflect this current behavior.src/main/broker.ts (1)
2625-2632:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSession teardown still leaves stale PTY dedupe entries.
Line 2626 cleans up per-agent, but session-level teardown removes agents without clearing
ptyStreamDedupefor that session key. This can retain stale dedupe state and grow memory over time.Suggested fix
+ private clearPtyDedupeForSession(sessionKey: string): void { + const prefix = `${sessionKey}:` + for (const key of Array.from(this.ptyStreamDedupe.keys())) { + if (key.startsWith(prefix)) this.ptyStreamDedupe.delete(key) + } + } + private dropSession(sessionKey: string, options: { disconnectOnly: boolean }): void { this.closeInputStreamsForSession(sessionKey) + this.clearPtyDedupeForSession(sessionKey) this.clearBrokerTimeoutCountsForProject(projectIdFromSessionKey(sessionKey))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/broker.ts` around lines 2625 - 2632, forgetAgentSession currently deletes only the single ptyStreamDedupe entry for `${sessionKey}:${name}` but when the last agent for a session is removed (sessionKeys.size === 0) it deletes the agent map without clearing other ptyStreamDedupe entries tied to that session, leaving stale entries. Update forgetAgentSession (or add a small helper called from it) so that when sessionKeys.size === 0 you also remove all ptyStreamDedupe entries whose keys startWith `${sessionKey}:` (or otherwise clear the session-scoped dedupe entries), using the existing ptyStreamDedupe and agentSessions symbols to locate and clean those entries before deleting the agentSessions entry.
🧹 Nitpick comments (1)
src/main/broker.test.ts (1)
1472-1564: ⚡ Quick winAdd one regression for
event_id/ididentity paths.The new matrix validates seq and no-identity behavior well, but dedupe precedence also uses
event_id/id. Add a case for sameevent_id+same bytes (suppressed) and sameevent_id+different bytes (delivered) to lock this branch.As per coding guidelines, "Prefer stable event identity over content matching. PTY and broker events should carry
event_id,id, orseq; dedupe by identity AND content hash together."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/broker.test.ts` around lines 1472 - 1564, Add a regression test in broker.test.ts that exercises the event identity paths: create a BrokerManager, mock window via createMockWindow and startLocalWithWindow, grab the localized listener (from local.onEvent.mock.calls.at(-1)?[0]) and send two pairs of worker_stream events using the same event_id (or id) — first pair with identical chunk bytes (expect only one 'broker:pty-chunk' delivery), second pair with different chunk bytes (expect both deliveries). Use the same PROJECT_ID and agent name(s) as other tests, filter win.webContents.send calls by 'broker:pty-chunk' and assert the expected deliveries to ensure dedupe honors event_id/id plus content-hash precedence.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/broker.ts`:
- Around line 2505-2508: The branch that returns false on missing identity
currently drops telemetry; modify the check around the identity variable (the
"if (!identity) return false" branch) to increment a low-volume telemetry
counter (e.g., pty_chunks.missing_identity) and emit a debug-gated or sampled
log message before returning false so missing-identity events are visible
without flooding; use the existing debug/logger and metrics utilities in the
module (or add a sampled logger helper) and ensure the telemetry is lightweight
and only triggers under the debug flag or with probabilistic sampling.
In `@src/renderer/src/lib/echo-router.ts`:
- Around line 135-137: In reseedPending, when the engine has changed (the check
deps.getEngine() !== engine) the current code discards heldChunks by setting
heldChunks = []; instead, preserve and immediately release them to avoid losing
live PTY output: detect the engine mismatch in reseedPending and call the
existing release mechanism (e.g., the function or method that normally
flushes/dispatches heldChunks) for each chunk or the batch before clearing;
reference the variables and functions heldChunks, reseedPending,
deps.getEngine(), engine and the release/dispatch routine so the code falls back
to direct release rather than dropping the buffered data.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 57-60: Update the inline comment above the "Upload Playwright
traces" step so it no longer mentions "the continue-on-error fidelity step";
instead state that the if: always() condition ensures Playwright traces are
uploaded for any failed step (or when failures occur), and that it is a no-op
when there are no traces to upload; modify the comment text near the Upload
Playwright traces step and the if: always() line to reflect this current
behavior.
In `@src/main/broker.ts`:
- Around line 2625-2632: forgetAgentSession currently deletes only the single
ptyStreamDedupe entry for `${sessionKey}:${name}` but when the last agent for a
session is removed (sessionKeys.size === 0) it deletes the agent map without
clearing other ptyStreamDedupe entries tied to that session, leaving stale
entries. Update forgetAgentSession (or add a small helper called from it) so
that when sessionKeys.size === 0 you also remove all ptyStreamDedupe entries
whose keys startWith `${sessionKey}:` (or otherwise clear the session-scoped
dedupe entries), using the existing ptyStreamDedupe and agentSessions symbols to
locate and clean those entries before deleting the agentSessions entry.
---
Nitpick comments:
In `@src/main/broker.test.ts`:
- Around line 1472-1564: Add a regression test in broker.test.ts that exercises
the event identity paths: create a BrokerManager, mock window via
createMockWindow and startLocalWithWindow, grab the localized listener (from
local.onEvent.mock.calls.at(-1)?[0]) and send two pairs of worker_stream events
using the same event_id (or id) — first pair with identical chunk bytes (expect
only one 'broker:pty-chunk' delivery), second pair with different chunk bytes
(expect both deliveries). Use the same PROJECT_ID and agent name(s) as other
tests, filter win.webContents.send calls by 'broker:pty-chunk' and assert the
expected deliveries to ensure dedupe honors event_id/id plus content-hash
precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d9f489f8-de78-4303-be88-081438cadc0d
📒 Files selected for processing (17)
.github/workflows/ci.ymlAGENTS.mdsrc/main/broker.test.tssrc/main/broker.tssrc/renderer/src/hooks/use-terminal.tssrc/renderer/src/lib/echo-router.test.tssrc/renderer/src/lib/echo-router.tssrc/renderer/src/lib/predictive-echo.tssrc/renderer/src/lib/pty-size-sync.test.tssrc/renderer/src/lib/pty-size-sync.tssrc/renderer/src/lib/terminal-runtime-registry.tssrc/renderer/src/stores/pty-buffer-store.test.tssrc/renderer/src/stores/pty-buffer-store.tssrc/renderer/src/stores/ui-store.tstests/playwright/REDRAW-DRAIN-RESULTS.mdtests/playwright/fidelity-no-duplication.spec.tstests/playwright/redraw-drain.spec.ts
Contributor
|
Implemented the fixes I validated in the current checkout. Changes
Addressed Comments
Advisory Notes Validation
Also ran |
khaliqgant
added a commit
that referenced
this pull request
Jun 10, 2026
…e swap, log identity-less PTY streams - echo-router: if the engine changes while a reseed capture is in flight, release the held chunks directly instead of dropping them (unreachable today outside dispose, but dropping live PTY bytes is the exact corruption class this PR kills — defend in depth). - broker: log once per stream (not per chunk) when PTY chunks arrive with no seq/event_id, per the AGENTS.md low-noise telemetry guideline — replay dedup is blind on such streams and that should be visible. Both behaviors pinned with tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Reviewed PR #221 against Addressed comments
VerificationPassed locally:
Notes:
|
Contributor
|
Implemented the still-valid fixes from the current checkout. Changes made:
Addressed comments
Advisory Notes
VerificationPassed:
|
Contributor
|
Implemented and verified fixes for the current checkout. Changes made:
Addressed comments
Advisory Notes
VerificationPassed:
|
…ize drift, and flood lag Four root-cause fixes for the recurring rendering corruption (stacked duplicate lines with stray escape fragments, content drifting mid-screen, typing lag under multi-agent output), each with a gating regression fence: - broker PTY dedup: drop a chunk ONLY on identity (seq/event_id) + content hash match within a bounded replay window. Content-only dedup dropped legitimate identical chunks (repeated keystroke echoes, byte-identical TUI repaint frames) and seq-only TTL dedup dropped fresh output for up to 60s after a daemon seq reset — dropped repaint bytes sever escape sequences and make every Ink repaint stack instead of overwrite. - PTY size sync: a resize only counts as delivered when the resizePty IPC resolves (desired/acked model with retrying backoff in pty-size-sync.ts). Failed resizes were recorded as sent, permanently desyncing PTY winsize from the grid — TUIs then repaint against the wrong row count. - echo routing (echo-router.ts): server output flows direct to xterm while the predictive-echo engine is dormant, killing the always-on headless model double-parse (the typing-lag hot path) and the stale-model cursor injection corruption class; the direct→engine transition reseeds the model from the live screen at a write-queue quiescent point, and >32KB flood frames with no predictions bypass the engine. - replay baselines: monotonic chunk totals (getPtyChunkTotal / getPtyChunksSinceTotal) replace buffer-length baselines that went stale across the 10k trim and replayed the whole buffer over a painted snapshot. Regression fence: broker dedup matrix (replay/seq-reset/no-identity), echo-router equivalence tests against the REAL engine + REAL xterm parser, pty-size-sync + buffer-baseline invariant tests, the fidelity playwright suite repaired (web-mock initial-tab override + agent selection; obsolete agent-graph flow replaced with an Issues↔Agents remount) and promoted to a hard CI gate, and redraw-drain now gating on drained/finalFrameRendered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e swap, log identity-less PTY streams - echo-router: if the engine changes while a reseed capture is in flight, release the held chunks directly instead of dropping them (unreachable today outside dispose, but dropping live PTY bytes is the exact corruption class this PR kills — defend in depth). - broker: log once per stream (not per chunk) when PTY chunks arrive with no seq/event_id, per the AGENTS.md low-noise telemetry guideline — replay dedup is blind on such streams and that should be visible. Both behaviors pinned with tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…+ stream-aware prediction gating, with a self-driving storm repro The post-fix corruption report (character-interleaved rows, stacked panel repaints, stray typed glyphs inside repainted rows) reproduced in a new Playwright storm harness and bisected to two renderer defects: - Write-order inversion: a flood-bypass route flip wrote directly to xterm while earlier chunks were still queued in the predictive-echo engine's async tail — bytes landed out of order, shearing escape sequences and overlaying row text. The router now has a single ordering domain: any write issued while engine work is queued chains behind it; the chain collapses back to synchronous direct writes once drained (zero overhead in the common local case). Reproduced by a unit test with a slowed model (failed before, passes after). - Predictions during active streaming: mosh-style optimistic glyphs are a quiet-prompt tool. While a TUI streams in-place repaints, the engine erases/re-renders its glyphs around EVERY frame; any model/live drift then erases real rows and strands glyphs inside repainted panels — the exact live symptoms. Streaming-sized chunks (>1KB) now arm a quiet window: outstanding predictions are rolled back at still-valid positions the moment streaming starts, and no new predictions render until the stream has been quiet for 300ms. Keystroke echo never trips the window. New rendering-corruption.spec.ts drives the real renderer through an Ink-style storm (integration-dump text, randomly split repaint frames, >32KB flood chunks, typing mid-flood with the engine engaged via new mock setInputSrtt/setInputEcho knobs, idle-prompt typing at the end) and gates on byte-faithfulness: the live viewport must equal a reference @xterm/headless render of the canonical chunk stream. Runs under the fidelity Playwright config in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…agent sessions
Agents were receiving the same <integrations-update> setup context several
times and acknowledging each copy — the duplicated-transcript reports
("Ready. I'll follow…" ×4, "Setup context received" ×2). Three compounding
vectors, all in the broadcast machinery:
- All-or-nothing retry: the delivered-signature was recorded only when
EVERY agent's send succeeded (Promise.all), so one flaky send re-broadcast
the identical update to every agent on the next reconcile. Delivery is
now recorded per agent (allSettled); retries reach only the agents that
missed it.
- Spawn double-delivery: a new agent received the snippet inside its spawn
task AND again from the next project broadcast (the guard was
project-level). The spawn handler now records the embedded snippet as
already-delivered for the new agent.
- Staged text churn: scope-label/channel-name hydration and subscription
readiness converge in stages, each slightly changing the rendered text
and re-broadcasting the full update mid-task. Broadcasts now honor a 60s
cooldown; later changes replace the pending timer so the trailing
broadcast carries the latest text once.
Per-agent records also mean a newly arrived agent gets the current update
without re-sending it to everyone else. Tests cover partial-failure retry,
new-agent-only delivery, spawn-skip, cooldown hold-and-release, and the
unchanged-update no-op (now hermetic against background cloud hydration).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ts; fix(avatar): allow pear-avatar: in img-src CSP Residual codex double-acknowledgment: a freshly spawned agent received the integrations snippet in its spawn task, then the next project broadcast rendered a slightly different text (spawn hardcoded subscriptionsReady=true; paths still hydrating) and delivered a near-identical second setup message. - Spawn records now carry fromSpawnAt; broadcasts whose text differs are skipped for that agent during a 60s grace window, with a follow-up scheduled after the grace so a real post-spawn change still lands once. - Spawn snippets render with the last reconciled subscription readiness instead of a hardcoded true, removing the main text-mismatch source. - Low-noise send telemetry: every integrations-update send logs one line (agent, project, size) so duplicate-transcript reports are diagnosable. Avatar: the renderer CSP (img-src 'self' data: https:) blocked the custom pear-avatar: protocol that serves cached avatars, so the cache built to survive expiring provider URLs was never usable — once the token-scoped Google avatar URL went stale the avatar broke. Allowlist pear-avatar:. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
khaliqgant
force-pushed
the
fix/terminal-rendering-hardening
branch
from
June 11, 2026 07:55
e46af01 to
6bd8394
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/broker.ts`:
- Around line 2501-2502: The class BrokerManager contains references to
this.ptyStreamDedupe and this.ptyStreamsMissingIdentity which are undefined and
break compilation; replace these stale references with the new deduper API
(this.ptyDeduper) used elsewhere: remove uses of this.ptyStreamDedupe and
this.ptyStreamsMissingIdentity and call the appropriate methods on
this.ptyDeduper (e.g., delete/remove by key or use its provided dedupe cleanup
method) using the same sessionKey:name composite key, ensuring the cleanup logic
matches existing ptyDeduper semantics so BrokerManager no longer references
non-existent properties.
In `@tests/playwright/fidelity-no-duplication.spec.ts`:
- Around line 337-340: Update the stale test comment so it accurately describes
the executed flow: instead of saying the test "Leave the Agents view entirely
(Issues tab) and come back", change it to state that the test toggles the Agents
view layout (split/tabs) mid-stream to ensure remounting within Agents does not
replay duplicate buffer content; reference the existing comment block near the
fidelity-no-duplication test that mentions "Agents view" and the split/tabs
toggles to locate and replace the wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb27411a-c3a9-462e-9e4b-cfbc45fce3ab
📒 Files selected for processing (24)
.github/workflows/ci.ymlAGENTS.mdplaywright.fidelity.config.tssrc/main/broker.test.tssrc/main/broker.tssrc/main/integrations.test.tssrc/main/integrations.tssrc/main/ipc-handlers.tssrc/renderer/index.htmlsrc/renderer/src/hooks/use-terminal.tssrc/renderer/src/lib/echo-router.test.tssrc/renderer/src/lib/echo-router.tssrc/renderer/src/lib/ipc-mock.tssrc/renderer/src/lib/predictive-echo.tssrc/renderer/src/lib/pty-size-sync.test.tssrc/renderer/src/lib/pty-size-sync.tssrc/renderer/src/lib/terminal-runtime-registry.tssrc/renderer/src/stores/pty-buffer-store.test.tssrc/renderer/src/stores/pty-buffer-store.tssrc/renderer/src/stores/ui-store.tstests/playwright/REDRAW-DRAIN-RESULTS.mdtests/playwright/fidelity-no-duplication.spec.tstests/playwright/redraw-drain.spec.tstests/playwright/rendering-corruption.spec.ts
✅ Files skipped from review due to trivial changes (3)
- src/renderer/index.html
- AGENTS.md
- tests/playwright/REDRAW-DRAIN-RESULTS.md
🚧 Files skipped from review as they are similar to previous changes (16)
- src/renderer/src/lib/predictive-echo.ts
- playwright.fidelity.config.ts
- src/main/ipc-handlers.ts
- src/renderer/src/hooks/use-terminal.ts
- src/renderer/src/stores/ui-store.ts
- src/renderer/src/lib/ipc-mock.ts
- tests/playwright/rendering-corruption.spec.ts
- src/renderer/src/lib/pty-size-sync.ts
- src/renderer/src/lib/pty-size-sync.test.ts
- src/renderer/src/stores/pty-buffer-store.ts
- tests/playwright/redraw-drain.spec.ts
- src/renderer/src/stores/pty-buffer-store.test.ts
- src/main/integrations.ts
- src/renderer/src/lib/echo-router.test.ts
- src/renderer/src/lib/echo-router.ts
- src/renderer/src/lib/terminal-runtime-registry.ts
Comment on lines
+337
to
+340
| // Leave the Agents view entirely (Issues tab) and come back mid-stream — | ||
| // the cross-view unmount/remount that used to replay duplicate buffer | ||
| // content on top of the parked runtime. (The old 'agent graph' layout | ||
| // this test used no longer exists.) |
There was a problem hiding this comment.
Update stale remount test comment to match executed flow.
Lines 337-340 describe leaving Agents for Issues, but Lines 342-346 only toggle split/tabs within Agents. The mismatch makes the regression intent harder to trust during future triage.
Suggested wording fix
- // Leave the Agents view entirely (Issues tab) and come back mid-stream —
- // the cross-view unmount/remount that used to replay duplicate buffer
- // content on top of the parked runtime. (The old 'agent graph' layout
- // this test used no longer exists.)
+ // Toggle terminal layout mid-stream (split -> tabs) to exercise the
+ // runtime remount path that previously replayed duplicate buffered content.
+ // (The old 'agent graph' layout this test used no longer exists.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/playwright/fidelity-no-duplication.spec.ts` around lines 337 - 340,
Update the stale test comment so it accurately describes the executed flow:
instead of saying the test "Leave the Agents view entirely (Issues tab) and come
back", change it to state that the test toggles the Agents view layout
(split/tabs) mid-stream to ensure remounting within Agents does not replay
duplicate buffer content; reference the existing comment block near the
fidelity-no-duplication test that mentions "Agents view" and the split/tabs
toggles to locate and replace the wording.
- broker.ts: drop PtyStreamDedupeState, ptyChunkHash, getBrokerEventName, getBrokerEventFrom, resolveBrokerConnectionPath (all superseded by PtyChunkDeduper/BrokerEventHistory extraction); revive promise to const - integrations.ts: let integrations → const (never reassigned) - ipc-handlers.ts: drop assertDirectory import (unused) - relay-workspace.ts: let pendingHandle → const (ref is post-await only) - pty-buffer-store.ts: drop stale no-console eslint-disable directive Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…roker.ts typecheck rebase artifacts The CI storm repro caught a real corruption window the slow runner hits: the predictive-echo engine places optimistic glyphs by reading its confirmed model's cursor synchronously, but while router ops are queued a server chunk has already reached the live terminal whose model parse is still backlogged — the model cursor is on a stale ROW. A prediction placed then renders on the wrong row, and the engine's cleanup (erasePredictionRegion) skips erasing glyphs whose row no longer matches its prediction row, stranding them on screen (the stacked-frame-remnant / displaced-echo class: stale FRAME headers above the panel, typed echo landing rows below it). Fix: the router only hands keystrokes to the engine when its op chain is empty (queuedOps === 0). Skipping costs only the optimistic glyph for that keystroke; the byte still reaches the PTY and echoes back. - echo-router.test.ts: writeDelayMs hook now models a PARSE backlog (delay before the model terminal consumes the bytes) instead of a delayed resolution, which is what a busy renderer actually looks like; new regression test strands a prediction across a row move and fails without the gate. - rendering-corruption.spec.ts: STORM_CPU=<n> bisect knob applies CDP CPU throttling to reproduce the slow-CI timing locally (1/4 fail at 4x pre-fix, 14/14 pass at 6x post-fix). - broker.ts: drop leftover references to the removed PTY content-dedup fields and silence the use-before-assign false positive in reviveSession (typecheck:node CI failures from the rebase). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rebase onto main resolved the dedup conflict by keeping main's extracted PtyChunkDeduper (content-alone 1s window + identity-alone 60s window) and dropping this branch's hardened implementation — exactly the semantics this branch exists to kill (content-alone drops lose real PTY bytes; identity-alone drops eat fresh output after a daemon seq reset). The branch's broker.test.ts invariants caught it. Port the hardened logic (recovered from pre-rebase fe4a19d) into the extracted class shape: - drop ONLY on identity (event_id/seq) AND content-hash match within a 512-chunk per-stream window - identity-less chunks always deliver, with a once-per-stream blind-spot log - same identity + different bytes, or seq at/below the watermark outside the window → daemon restart: reset the window and deliver - forgetStream() per-stream cleanup wired back into forgetAgentSession pty-dedup.test.ts rewritten to the hardened invariants (the old suite asserted the corrupting behavior). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Problem
Recurring terminal rendering corruption: the same line stacking down the screen with stray escape fragments (e.g.
Gpasteagain to expand), content drifting to the middle of the screen, and typing lag when many agent messages stream in.Root causes & fixes
1. The broker silently dropped real PTY bytes (
src/main/broker.ts) — the direct cause of the screenshot symptom. The chunk dedup dropped any chunk whose content matched the previous one within 1s, but identical consecutive chunks are normal terminal traffic (repeated keystroke echoes, byte-identical TUI repaint frames). It also deduped byseqalone with a 60s TTL, so a daemon restart (seq reset) could black-hole up to a minute of output. A dropped repaint chunk severs the cursor-positioning escape sequence spanning the gap — the visibleGis the tail of a severedESC[…G, and every Ink repaint then appends instead of overwriting. Dedup now drops only on identity (seq/event_id) + content hash match within a bounded replay window — provably a transport replay — and never otherwise. Suppressed replays get low-noise telemetry.2. A failed resize was recorded as delivered (
fitAndSync+ SIGWINCH bounce). FailedresizePtyIPCs (or a failed bounce-restore leg, stranding the PTY one row short) were latched by the change-gate, permanently desyncing PTY winsize from the rendered grid — TUIs then position repaints against the wrong row count (the mid-screen drift). Newpty-size-sync.ts: a desired/acked model that retries with backoff until the PTY confirms the current size.3. Every output byte was parsed twice, always. The predictive-echo engine's headless model re-parses all server output even on local sessions where it never engages (needs ≥30ms echo SRTT) — main-thread time stolen from keystroke echo under multi-agent floods. New
echo-router.ts: output flows direct to xterm while the engine is dormant; the direct→engine transition reseeds the model from the live screen at a write-queue quiescent point (chunks held during capture land exactly once); >32KB flood frames with no outstanding predictions bypass the engine. Also eliminates the stale-model cursor-injection corruption class.4. Snapshot replay could double-paint. The post-attach replay baseline was a buffer length, which goes stale across the 10k-chunk trim; the recovery path replayed the entire buffer over the already-painted snapshot. Baselines are now monotonic totals (
getPtyChunkTotal/getPtyChunksSinceTotal).Regression fence (all gating)
broker.test.ts: dedup matrix — identical no-identity chunks delivered, daemon seq-reset delivered, delayed replays dropped, per-agent watermarks.echo-router.test.ts: equivalence tests against the realPredictiveEchoEngine+ real xterm parser (@xterm/headless) — route transitions, held-chunk windows, flood bypass, reseed aborts must produce a byte-identical screen.pty-size-sync.test.ts/pty-buffer-store.test.ts: the resize and baseline invariants directly.continue-on-error). Now seedspear-web-initial-tab=agents, selects the spawned agent, and replaces the removed agent-graph flow with an Issues↔Agents cross-view remount mid-stream.drained,finalFrameRendered); timing metrics stay informational. Current run drains 1,920 repaint frames in 44ms.AGENTS.mdupdated: never drop a PTY chunk on content or seq alone is now written policy.Verification
npm test115/115 ·npx vitest run313 passed (+1 pre-existing expected fail)typecheck:webclean ·typecheck:nodeidentical error set to main (97 pre-existing) ·npm run buildOK🤖 Generated with Claude Code