fix(terminal): keep reconciler live through cursor queries (#421)#422
Conversation
khaliqgant
added
the
no-agent-relay-review
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThe PR excludes query-only cursor-position responses from reconciler visual-activity clocks, adds focused runtime tests, separates raw and visual terminal-fidelity metrics, records byte-level checkpoint evidence, and adds a turn-boundary follow-up workload. ChangesTerminal activity and fidelity
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PTY
participant TerminalRuntimeRegistry
participant xterm
participant TerminalReconciler
participant FidelityOracle
PTY->>TerminalRuntimeRegistry: deliver query or screen-mutating output
TerminalRuntimeRegistry->>xterm: write combined PTY output
TerminalRuntimeRegistry->>TerminalReconciler: advance activity for visual output only
TerminalRuntimeRegistry->>FidelityOracle: expose raw and visual activity
FidelityOracle->>TerminalReconciler: evaluate quiet checkpoint
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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! |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4773b6479c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hasFreshPrompt = harness.cli === 'codex' | ||
| // Codex renders a rotating placeholder after the prompt glyph; the | ||
| // literal text changes between launches (for example "Implement | ||
| // {feature}" or "Find and fix a bug in @filename"). | ||
| ? /^\s*›\s+\S.*$/mu.test(afterMarker) | ||
| : /^\s*❯[\s\u00a0]*$/mu.test(afterMarker) | ||
| return hasFreshPrompt && (harness.cli !== 'claude' || hasDuration) |
There was a problem hiding this comment.
For OpenCode and Grok matrix runs, this condition falls into the non-Codex branch and waits only for Claude’s exact ❯ prompt line. SUPPORTED_CLIS includes both of those CLIs (tests/term-fidelity/harness.ts:8), and runCanonicalWorkloads invokes this new workload unconditionally, so their canonical runs will spend the full five-minute timeout here and fail before submitting the follow-up. Add CLI-specific completion detection for those renderers, or limit this workload to the CLIs whose prompt protocol it recognizes.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/term-fidelity/README.md (1)
34-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale workload count: "all six" should be "all seven".
This PR brings the matrix to 7 workloads (see the renumbered list below and the 7-entry array in
workloads.ts), but this line still says six.📝 Suggested fix
-`TERM_FIDELITY_WORKLOAD=<workload-directory-name>` to execute one canonical -workload. Normal matrix and CI runs must leave it unset so all six execute. +`TERM_FIDELITY_WORKLOAD=<workload-directory-name>` to execute one canonical +workload. Normal matrix and CI runs must leave it unset so all seven execute.🤖 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/term-fidelity/README.md` at line 34, Update the workload-count wording in the README from “all six” to “all seven” to match the seven workloads defined by the matrix and the workloads array.
🧹 Nitpick comments (1)
tests/term-fidelity/oracle.ts (1)
592-618: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate raw-stream replay/diff work on failing checkpoints.
This new diagnostic block recomputes
getRawStream→replayRawToGrid→brokerCellGrid→countCellDiffs(×2), and the existing divergence branch below (!quiet.reached || differingCells > 0) recomputes the exact same values independently. For any failing checkpoint this now replays the full raw byte stream through a fresh headless terminal and diffs the grid twice.♻️ Suggested refactor: compute once, reuse in the divergence branch
- try { - const raw = await getRawStream(harness.page, agentName) - const rawReplay = await replayRawToGrid(raw, broker.rows, broker.cols) - const brokerCells = await brokerCellGrid(broker) - const rawVsBroker = countCellDiffs(rawReplay.cells, brokerCells, broker.rows, broker.cols) - const rawVsRenderer = countCellDiffs(rawReplay.cells, renderer.cells, broker.rows, broker.cols) - const cursorMatch = ... - console.log(...) - } catch (error) { - console.warn(...) - } + let rawEvidence: { raw: string; rawReplay: Awaited<ReturnType<typeof replayRawToGrid>>; brokerCells: Cell[][] } | null = null + try { + const raw = await getRawStream(harness.page, agentName) + const rawReplay = await replayRawToGrid(raw, broker.rows, broker.cols) + const brokerCells = await brokerCellGrid(broker) + rawEvidence = { raw, rawReplay, brokerCells } + const rawVsBroker = countCellDiffs(rawReplay.cells, brokerCells, broker.rows, broker.cols) + const rawVsRenderer = countCellDiffs(rawReplay.cells, renderer.cells, broker.rows, broker.cols) + // ...cursorMatch/console.log unchanged + } catch (error) { + console.warn(...) + }Then thread
rawEvidenceinto the divergence branch below instead of re-fetching/re-replaying.🤖 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/term-fidelity/oracle.ts` around lines 592 - 618, Refactor the checkpoint diagnostics so the raw-stream evidence is computed once and reused. Store the results of getRawStream, replayRawToGrid, brokerCellGrid, both countCellDiffs comparisons, and cursor matching in a rawEvidence value, then pass or reuse it in the existing !quiet.reached || differingCells > 0 divergence branch instead of recomputing them there. Preserve the current clean-checkpoint logging and divergence artifact behavior.
🤖 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/renderer/src/lib/terminal-runtime-registry.dom.test.ts`:
- Around line 247-340: Add a regression test covering replay or duplicate
delivery through the terminal runtime catch-up path, such as re-attachment or
post-snapshot replay, rather than only the live subscribePtyBuffer path. Verify
that replayed query-only batches preserve the reconciler activity serial and
quiet state, while replayed mixed query-and-paint batches increment the serial
and close the quiet gate. Anchor the test to writeChunks and the existing
runtime/reconciler harness symbols.
---
Outside diff comments:
In `@tests/term-fidelity/README.md`:
- Line 34: Update the workload-count wording in the README from “all six” to
“all seven” to match the seven workloads defined by the matrix and the workloads
array.
---
Nitpick comments:
In `@tests/term-fidelity/oracle.ts`:
- Around line 592-618: Refactor the checkpoint diagnostics so the raw-stream
evidence is computed once and reused. Store the results of getRawStream,
replayRawToGrid, brokerCellGrid, both countCellDiffs comparisons, and cursor
matching in a rawEvidence value, then pass or reuse it in the existing
!quiet.reached || differingCells > 0 divergence branch instead of recomputing
them there. Preserve the current clean-checkpoint logging and divergence
artifact behavior.
🪄 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: df96f5c6-fcde-4db5-88cb-8eb58234f0e6
📒 Files selected for processing (7)
src/renderer/src/lib/terminal-reconciler.tssrc/renderer/src/lib/terminal-runtime-registry.dom.test.tssrc/renderer/src/lib/terminal-runtime-registry.tstests/term-fidelity/README.mdtests/term-fidelity/harness.tstests/term-fidelity/oracle.tstests/term-fidelity/workloads.ts
| // pear#421: Claude's idle ESC[?6n loop must not permanently close the | ||
| // reconciler quiet gate. Lock the intentionally tiny safety boundary: exact, | ||
| // complete query-only output is neutral; anything mixed, partial, or merely | ||
| // similar remains activity so snapshot races are gated when in doubt. | ||
| describe('terminal-runtime-registry — reconciler-neutral PTY queries', () => { | ||
| it('accepts one or repeated complete DEC-private cursor-position queries', () => { | ||
| expect(registry.isReconcilerNeutralPtyOutput('\x1b[?6n')).toBe(true) | ||
| expect(registry.isReconcilerNeutralPtyOutput('\x1b[?6n\x1b[?6n\x1b[?6n')).toBe(true) | ||
| }) | ||
|
|
||
| it('rejects mixed, partial, non-private, and empty output', () => { | ||
| expect(registry.isReconcilerNeutralPtyOutput('\x1b[?6npaint')).toBe(false) | ||
| expect(registry.isReconcilerNeutralPtyOutput('paint\x1b[?6n')).toBe(false) | ||
| expect(registry.isReconcilerNeutralPtyOutput('\x1b[?6')).toBe(false) | ||
| expect(registry.isReconcilerNeutralPtyOutput('n')).toBe(false) | ||
| expect(registry.isReconcilerNeutralPtyOutput('\x1b[6n')).toBe(false) | ||
| expect(registry.isReconcilerNeutralPtyOutput('')).toBe(false) | ||
| }) | ||
|
|
||
| it('still delivers a neutral query byte-for-byte to xterm', async () => { | ||
| const runtime = registry.acquireTerminalRuntime({ | ||
| projectId: 'p', | ||
| agentName: 'query-agent', | ||
| terminalMode: 'drive', | ||
| theme: 'dark', | ||
| getInputSrtt: () => null | ||
| }) | ||
| const term = createdTerminals[0] | ||
| runtime.mount(makeLayoutContainer()) | ||
| await flushAsync() | ||
| await flushAsync() | ||
|
|
||
| const before = term.__writes.length | ||
| ptyBuffer.appendPtyChunk(runtime.key, '\x1b[?6n') | ||
| ptyBuffer.flushPtyChunksNow(runtime.key) | ||
|
|
||
| expect(term.__writes).toHaveLength(before + 1) | ||
| expect(term.__writes[before]).toBe('\x1b[?6n') | ||
| registry.disposeTerminalRuntime(runtime.key) | ||
| }) | ||
|
|
||
| it('keeps the reconciler quiet through queries but closes it for paint and mixed batches', async () => { | ||
| vi.useFakeTimers() | ||
| vi.setSystemTime(new Date('2026-01-01T00:00:00.000Z')) | ||
| Object.defineProperty(document, 'visibilityState', { | ||
| configurable: true, | ||
| value: 'visible' | ||
| }) | ||
| const runtime = registry.acquireTerminalRuntime({ | ||
| projectId: 'p', | ||
| agentName: 'activity-agent', | ||
| terminalMode: 'drive', | ||
| theme: 'dark', | ||
| getInputSrtt: () => null | ||
| }) | ||
| Object.defineProperty(runtime.host, 'clientWidth', { configurable: true, value: 800 }) | ||
| Object.defineProperty(runtime.host, 'clientHeight', { configurable: true, value: 600 }) | ||
| runtime.mount(makeLayoutContainer()) | ||
| await flushAsync() | ||
| await flushAsync() | ||
|
|
||
| const deps = reconcilerHarness.deps | ||
| expect(deps).not.toBeNull() | ||
| const baselineSerial = deps!.activitySerial() | ||
| expect(deps!.isQuiet()).toBe(true) | ||
|
|
||
| // One rAF drain containing repeated exact queries: bytes still flow, but | ||
| // visible state is unchanged, so the serial/quiet gate must stay open. | ||
| ptyBuffer.appendPtyChunk(runtime.key, '\x1b[?6n') | ||
| ptyBuffer.appendPtyChunk(runtime.key, '\x1b[?6n') | ||
| ptyBuffer.flushPtyChunksNow(runtime.key) | ||
| expect(deps!.activitySerial()).toBe(baselineSerial) | ||
| expect(deps!.isQuiet()).toBe(true) | ||
|
|
||
| ptyBuffer.appendPtyChunk(runtime.key, 'paint') | ||
| ptyBuffer.flushPtyChunksNow(runtime.key) | ||
| expect(deps!.activitySerial()).toBe(baselineSerial + 1) | ||
| expect(deps!.isQuiet()).toBe(false) | ||
|
|
||
| await vi.advanceTimersByTimeAsync(RECONCILE_QUIET_MS) | ||
| expect(deps!.isQuiet()).toBe(true) | ||
|
|
||
| // Query + paint in the same coalesced write is activity. This prevents a | ||
| // query prefix from laundering a real repaint through the race guard. | ||
| ptyBuffer.appendPtyChunk(runtime.key, '\x1b[?6n') | ||
| ptyBuffer.appendPtyChunk(runtime.key, 'fresh paint') | ||
| ptyBuffer.flushPtyChunksNow(runtime.key) | ||
| expect(deps!.activitySerial()).toBe(baselineSerial + 2) | ||
| expect(deps!.isQuiet()).toBe(false) | ||
|
|
||
| registry.disposeTerminalRuntime(runtime.key) | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a replay/duplicate-delivery regression case for the neutral-query gating.
writeChunks (the function under test here) is also invoked from the buffer-replay/catch-up paths in terminal-runtime-registry.ts (writeChunks(postSnapshotChunks), writeChunks(getPtyChunksSinceTotal(...))), not just the live subscribePtyBuffer callback exercised by these tests. None of the new cases cover a query-only or mixed batch delivered via replay (e.g. after a re-attach) to confirm the serial/quiet gate stays consistent when the same bytes are replayed rather than streamed live.
As per path instructions, "When changing broker start, event streaming, PTY buffering, spawned personas, or integration notifications, add regression tests covering duplicate and replay cases, not only successful first-run behavior."
🤖 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/renderer/src/lib/terminal-runtime-registry.dom.test.ts` around lines 247
- 340, Add a regression test covering replay or duplicate delivery through the
terminal runtime catch-up path, such as re-attachment or post-snapshot replay,
rather than only the live subscribePtyBuffer path. Verify that replayed
query-only batches preserve the reconciler activity serial and quiet state,
while replayed mixed query-and-paint batches increment the serial and close the
quiet gate. Anchor the test to writeChunks and the existing runtime/reconciler
harness symbols.
Source: Path instructions
1 participant
Summary
ESC[?6nsequences as reconciliation-neutral; mixed, partial, non-private, and empty output remains activityturn-boundary-follow-upworkload that mirrors the reported long markdown turn followed immediately by a short path questionRoot cause
Current Claude emits an exact five-byte
ESC[?6nquery roughly every 200 ms while its screen is idle. The sequence does not paint, erase, scroll, resize, or move the cursor, butterminal-runtime-registry.writeChunkspreviously resetlastOutputAtand incrementedactivitySerialfor every batch. That permanently closed the reconciler quiet gate, silently disabling Pear's only always-on divergence detector and repair backstop. A renderer divergence created by another transient path could therefore preserve stale prior-turn rows indefinitely.The production screenshot's
✻ Crunched for …line also matches Claude's UI. The installed inline Codex CLI does not retain that duration line after completion.Real-harness evidence
The new workload prints a 60-line markdown preview containing a remembered absolute path, waits for the CLI's completed-turn prompt, then submits the short path follow-up with no delay.
bufferType=alternate viewport=[0,0] cursorMatch=true rawBytes=128622 rawVsBroker=0 rawVsRenderer=0bufferType=normal viewport=[69,69] cursorMatch=true rawBytes=251998 rawVsBroker=0 rawVsRenderer=0Both focused real-Electron cases passed without reconciler divergence telemetry.
Validation
npm run lintnpm run typechecknpm run buildnpm run test:all(129 Node tests + 609 Vitest tests)npm run verify:mcp-resources-driftTERM_FIDELITY_SKIP_BUILD=1 TERM_FIDELITY_WORKLOAD=turn-boundary-follow-up npm run test:term-fidelity -- --cli=claudeTERM_FIDELITY_SKIP_BUILD=1 TERM_FIDELITY_WORKLOAD=turn-boundary-follow-up npm run test:term-fidelity -- --cli=codexFixes #421