feat(terminal): dump confirmed divergence corpus#400
Conversation
khaliqgant
added
the
no-agent-relay-review
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds a typed term-fidelity corpus IPC operation, main-process bundle writing with throttling and failure handling, and renderer hooks that capture confirmed terminal divergences with CLI and relay metadata. ChangesTerm fidelity corpus flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalReconciler
participant TerminalRuntimeRegistry
participant PearAPI
participant IpcHandlers
participant TermFidelityCorpusDumper
TerminalReconciler->>TerminalRuntimeRegistry: confirmed divergence snapshot
TerminalRuntimeRegistry->>PearAPI: dumpTermFidelityCorpus(input)
PearAPI->>IpcHandlers: term-fidelity:dump-corpus
IpcHandlers->>TermFidelityCorpusDumper: dump(input, capturePage, relayVersions)
TermFidelityCorpusDumper-->>IpcHandlers: dumped path or failure reason
Possibly related PRs
Suggested reviewers: Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
9abc7f0 to
a8e654b
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a terminal-fidelity corpus dumper (TermFidelityCorpusDumper) to capture and dump terminal states (renderer text, broker text, screenshots, and metadata) when terminal divergence is confirmed. This functionality is wired up via Electron IPC handlers and integrated into the terminal reconciler on the renderer side, complete with comprehensive tests. The review feedback suggests improving robustness by defensively validating the untrusted IPC payload in validateInput to prevent potential main-process crashes, and using optional chaining when accessing dependencies from the parsed package.json to handle cases where it might parse to null.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function validateInput(input: TermFidelityCorpusInput): void { | ||
| if (!input.agentName.trim()) throw new Error('Term-fidelity corpus agent name is required') | ||
| if (!input.cli.trim()) throw new Error('Term-fidelity corpus CLI is required') | ||
|
|
||
| for (const [label, grid] of [ | ||
| ['renderer', input.renderer], | ||
| ['broker', input.broker] | ||
| ] as const) { | ||
| if (!Number.isInteger(grid.rows) || grid.rows <= 0) { | ||
| throw new Error(`Term-fidelity corpus ${label} rows must be a positive integer`) | ||
| } | ||
| if (!Number.isInteger(grid.cols) || grid.cols <= 0) { | ||
| throw new Error(`Term-fidelity corpus ${label} cols must be a positive integer`) | ||
| } | ||
| if (typeof grid.text !== 'string') { | ||
| throw new Error(`Term-fidelity corpus ${label} text must be a string`) | ||
| } | ||
| } | ||
|
|
||
| if (!Array.isArray(input.telemetryLines) || input.telemetryLines.some((line) => typeof line !== 'string')) { | ||
| throw new Error('Term-fidelity corpus telemetry lines must be strings') | ||
| } | ||
| } |
There was a problem hiding this comment.
The validateInput function processes the input payload received from the renderer process via IPC. Since IPC payloads are untrusted and can be malformed or incomplete, we should defensively validate that input is a non-null object, and that its nested properties (agentName, cli, renderer, broker) exist and have the expected types before accessing their properties or calling methods like .trim(). This prevents potential unhandled TypeError crashes in the main process.
function validateInput(input: TermFidelityCorpusInput): void {
if (!input || typeof input !== 'object') {
throw new Error('Term-fidelity corpus input must be an object')
}
if (typeof input.agentName !== 'string' || !input.agentName.trim()) {
throw new Error('Term-fidelity corpus agent name is required and must be a string')
}
if (typeof input.cli !== 'string' || !input.cli.trim()) {
throw new Error('Term-fidelity corpus CLI is required and must be a string')
}
for (const [label, grid] of [
['renderer', input.renderer],
['broker', input.broker]
] as const) {
if (!grid || typeof grid !== 'object') {
throw new Error('Term-fidelity corpus ' + label + ' must be an object')
}
if (!Number.isInteger(grid.rows) || grid.rows <= 0) {
throw new Error('Term-fidelity corpus ' + label + ' rows must be a positive integer')
}
if (!Number.isInteger(grid.cols) || grid.cols <= 0) {
throw new Error('Term-fidelity corpus ' + label + ' cols must be a positive integer')
}
if (typeof grid.text !== 'string') {
throw new Error('Term-fidelity corpus ' + label + ' text must be a string')
}
}
if (!Array.isArray(input.telemetryLines) || input.telemetryLines.some((line) => typeof line !== 'string')) {
throw new Error('Term-fidelity corpus telemetry lines must be strings')
}
}| const manifest = JSON.parse( | ||
| await readFile(join(app.getAppPath(), 'package.json'), 'utf8') | ||
| ) as { dependencies?: Record<string, unknown> } | ||
| declaredRelayVersions = Object.fromEntries( | ||
| Object.entries(manifest.dependencies || {}).filter( | ||
| ([name, version]) => | ||
| (name === 'agent-relay' || name.startsWith('@agent-relay/')) && | ||
| typeof version === 'string' | ||
| ) | ||
| ) as Record<string, string> |
There was a problem hiding this comment.
If package.json parses to null (which is technically valid JSON), manifest will be null. Accessing manifest.dependencies would then throw a TypeError. Although this is wrapped in a try/catch block, using optional chaining (manifest?.dependencies) and typing manifest as nullable is more robust and prevents throwing unnecessary exceptions.
const manifest = JSON.parse(
await readFile(join(app.getAppPath(), 'package.json'), 'utf8')
) as { dependencies?: Record<string, unknown> } | null
declaredRelayVersions = Object.fromEntries(
Object.entries(manifest?.dependencies || {}).filter(
([name, version]) =>
(name === 'agent-relay' || name.startsWith('@agent-relay/')) &&
typeof version === 'string'
)
) as Record<string, string>
a8e654b to
f5cb6ea
Compare
|
Coordinator merge note: the manual E2E gate (synthetic divergence → bundle on disk) is WAIVED with rationale — the term-fidelity matrix exercises this path organically: any reconciler repair event during a matrix run on a build containing this PR triggers the real capturePage+disk path (an opencode typing-during-stream repair occurred in today's validation run). First organic bundle will be verified there. All other gates met: hook design verified in review, ordering-lock + rate-limit + malformed-payload tests, CI fully green, single clean commit. |
|
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: f5cb6eaafc
ℹ️ 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".
| await mkdir(bundleDir, { recursive: true }) | ||
| await Promise.all([ | ||
| writeFile(join(bundleDir, 'renderer.txt'), input.renderer.text), | ||
| writeFile(join(bundleDir, 'broker.txt'), input.broker.text), | ||
| writeFile(join(bundleDir, 'screen.png'), screenshot), | ||
| writeFile(join(bundleDir, 'meta.json'), `${JSON.stringify(meta, null, 2)}\n`) |
There was a problem hiding this comment.
When terminal divergence persists over days or affects many agents, this automatic path creates a new full-page PNG and three additional files every five minutes per project-agent, but nothing prunes old timestamp directories or enforces a global size/count limit. The userData/term-fidelity-corpus directory can therefore grow without bound and eventually exhaust user disk space; apply an age, count, or byte-size retention policy when adding bundles.
Useful? React with 👍 / 👎.
| void pear.app.dumpTermFidelityCorpus({ | ||
| projectId: opts.projectId, | ||
| agentName: opts.agentName, | ||
| cli: opts.cli?.trim() || 'unknown', |
There was a problem hiding this comment.
When an agent is initially discovered with cli: 'unknown' and a later broker synchronization supplies its real CLI, the hook reacquires the existing runtime, but acquireTerminalRuntime only updates its theme and terminal mode. This callback remains closed over the options from the first runtime creation, so subsequent corpus bundles are mislabeled as unknown or another stale CLI until that runtime is disposed; update the runtime's diagnostic CLI value when it is reacquired.
Useful? React with 👍 / 👎.
…ath IPC lock (#401) * fix(terminal): report reconciler divergence detection, not just repair The quiet-time reconciler is two things: the convergence backstop (repair) and the app's only always-on divergence detector. Every telemetry line hung off a completed repair, which conflated them. A divergence could be confirmed against the broker's ground truth and then go entirely unreported: repairs are rate-limited to one per 15s, the ansi re-fetch can return null, and output can race the snapshot or the final flush — each of those returned silently. The detector also can't speak until ~10s of quiet (1.5s quiet gate + up to a check interval to align + a second confirming check), so any observer sampling a shorter window reads zero telemetry off a fully diverged screen and concludes the pipeline is clean. That is what happened to the codex permission-prompt-repaint fidelity bundle: 401 differing cells at 42x139, quiet, dims matched, no predictions — and an empty reconcilerTelemetryLines array, because the checkpoint captured at 2.1s of quiet. The same session proves the module works: the resize workload sat quiet for 90s and the reconciler detected and repaired it down to 1 cell. Report at confirmed divergence, before deciding whether to repair, with the row count and the repair outcome. Both lines keep the documented `[terminal] viewport diverged from broker screen` prefix, whose documented meaning is "a creation vector exists upstream" — true at detection, not only at repair. The gates themselves are untouched: confirm-twice, the repair rate limit, the dims match and the activity-serial rechecks each guard a real re-corruption path. This only makes them audible. RECONCILE_DIVERGENCE_LOG_GAP_MS must stay strictly below RECONCILE_MIN_REPAIR_GAP_MS; the first draft used 15s for both, which re-silenced the repair-rate-limited case. A test locks the ordering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(broker): lock one-IPC-per-worker_stream-event on the PTY fast path The PTY fast path is the highest-volume IPC in the app, and the terminal fidelity matrix traced a codex renderer/broker screen divergence to somewhere in this pipeline. Whether a chunk can be delivered twice — or suppressed when it shouldn't be — had no coverage at all, so the question could only be answered by inference. It should be answerable by running a test. Three invariants, all currently held: - one worker_stream event produces exactly one broker:pty-chunk send - starting a project whose broker is already running (the reconnect path, which re-attaches a client) does not leave a second live listener double-sending to the same window - byte-identical consecutive chunks are BOTH delivered: TUIs legitimately emit identical repaint frames and repeated keystroke echoes, and suppressing on content alone drops real bytes (AGENTS.md "Duplicate Event Hardening") Why a doubled chunk would matter, for whoever reads this next: a duplicated non-idempotent append puts a row on the grid the PTY never emitted, and a diff-painting TUI then frames every later repaint against the wrong row — stale panels and a whole-screen row shift that no later output heals. These tests pass on main. That is the point: they were written to answer "does Pear double-send PTY chunks?", and they establish that it does not. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: keep corpus telemetryLines format stable across #400/#401 merge Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Verification
Coordination
The confirmed-divergence hook is deliberately small: one retained snapshot assignment at the confirm-twice point and one post-check queue in the outer finally block. This is intended to compose with the detection telemetry work on fix/codex-permission-divergence without changing its report ordering or repair gates.
No live broker instance or port 3889 was touched.