Repository navigation
Deduplicate no-identity PTY chunks - #91
Conversation
|
CodeAnt AI is reviewing your PR. |
|
Warning Review limit reached
More reviews will be available in 8 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refines PTY chunk deduplication to support content-based dedup when events lack identity, adds socket-error detection for broker unreachability by inspecting error cause chains, and suppresses logging for broker-unreachable errors during fire-and-forget input transmission. ChangesPTY and Broker Reliability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
codeant-ai
Bot
added
the
size:M
![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 content-based deduplication for PTY chunks that lack an identity, using a shorter TTL of 1 second, and updates tests to verify this behavior. It also improves error handling by recognizing UND_ERR_SOCKET as an unreachable broker error and ignoring it in sendInputFireAndForget. Feedback on these changes highlights two main areas for improvement: optimizing the deduplication pruning loop to avoid O(N) iterations on every chunk arrival, and setting a size limit on chunks stored in the deduplication map to prevent high memory usage from large terminal outputs.
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.
| for (const [key, seenAt] of this.recentPtyChunks) { | ||
| const ttl = key.includes(':chunk:') | ||
| ? PTY_CHUNK_CONTENT_DEDUPE_TTL_MS | ||
| : PTY_CHUNK_IDENTITY_DEDUPE_TTL_MS | ||
| if ( | ||
| now - seenAt > PTY_CHUNK_IDENTITY_DEDUPE_TTL_MS || | ||
| now - seenAt > ttl || | ||
| this.recentPtyChunks.size > MAX_PTY_CHUNK_DEDUPE_ENTRIES | ||
| ) { | ||
| this.recentPtyChunks.delete(key) |
There was a problem hiding this comment.
Iterating over the entire recentPtyChunks map (which can contain up to 2,000 entries) on every single PTY chunk arrival introduces a significant performance bottleneck. Since PTY chunks can arrive at an extremely high frequency, this
By only running the pruning loop when the map size actually exceeds MAX_PTY_CHUNK_DEDUPE_ENTRIES, we can keep the common case
const shouldPrune = this.recentPtyChunks.size > MAX_PTY_CHUNK_DEDUPE_ENTRIES
for (const [key, seenAt] of shouldPrune ? this.recentPtyChunks : []) {
const ttl = key.includes(':chunk:')
? PTY_CHUNK_CONTENT_DEDUPE_TTL_MS
: PTY_CHUNK_IDENTITY_DEDUPE_TTL_MS
if (
now - seenAt > ttl ||
this.recentPtyChunks.size > MAX_PTY_CHUNK_DEDUPE_ENTRIES
) {
this.recentPtyChunks.delete(key)| const chunk = typeof eventRecord.chunk === 'string' ? eventRecord.chunk : '' | ||
| if (!identity && !chunk) return false |
There was a problem hiding this comment.
If a PTY chunk is extremely large (e.g., several kilobytes or megabytes of command output), storing the entire chunk string as a key in recentPtyChunks can consume significant memory and slow down map operations.
Since deduplication of no-identity chunks is primarily intended for small repeated terminal sequences (like spinner frames, prompt redraws, or cursor movements), we should skip deduplication for chunks exceeding a reasonable size threshold (e.g., 1024 characters).
const chunk = typeof eventRecord.chunk === 'string' ? eventRecord.chunk : ''
if (!identity && (!chunk || chunk.length > 1024)) return false
|
Reviewed PR #91 and made one focused fix. Changed:
Verified locally:
|
|
✅ pr-reviewer applied fixes — committed and pushed Reviewed PR #91 and made one focused fix. Changed:
Verified locally:
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const ttl = key.includes(':chunk:') | ||
| ? PTY_CHUNK_CONTENT_DEDUPE_TTL_MS | ||
| : PTY_CHUNK_IDENTITY_DEDUPE_TTL_MS |
There was a problem hiding this comment.
Suggestion: The TTL selection infers key type by checking whether the whole key string contains :chunk:. Because the key embeds sessionKey and agent name, a normal identity-key entry can be misclassified if either segment contains that substring, causing identity dedupe entries to expire after 1s instead of 60s. Use an explicit key-type marker (or store structured metadata) instead of substring matching on the composed key. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Terminal PTY output duplicates for specially named agents.
- ⚠️ Predictive echo reconciliation destabilized by duplicated chunks.Steps of Reproduction ✅
1. Spawn an agent with a custom name containing the literal substring `:chunk:` via the
renderer helper at `src/renderer/src/lib/spawn-agent.ts:10-18`, for example
`my:chunk:agent` (the code explicitly uses `trimmedCustom ?? nextAgentName(...)` and does
not sanitize out `:chunk:` before calling `pear.broker.spawnAgent`).
2. The preload bridge at `src/preload/index.ts:17-25` forwards that spawn to the main
process (`ipcMain.handle('broker:spawn-agent', ...)` in `src/main/ipc-handlers.ts:12-14`),
so the broker session now has an agent whose `event.name` includes `:chunk:` in the middle
of the string.
3. While that agent is running, generate PTY output (e.g. type commands in the terminal
attached at `src/renderer/src/hooks/use-terminal.ts:83-107`, which uses
`pear.broker.attachTerminal` and subscribes to PTY chunks). Each `worker_stream` PTY chunk
event is handled in `BrokerManager.attachClient` at `src/main/broker.ts:1890-1908`, which
calls `this.isDuplicatePtyChunk(sessionKey, event.name, event)` before emitting
`'broker:pty-chunk'` to the renderer.
4. Inside `BrokerManager.isDuplicatePtyChunk` at `src/main/broker.ts:1957-1996`,
identity-based entries use keys of the form ``${sessionKey}:${name}:${identity}``. Because
`name` contains `:chunk:`, these keys also contain `:chunk:` even though they represent
identity dedupe, so the cleanup loop at `1959-1965` classifies them as content-based
(`key.includes(':chunk:')`) and applies the short `PTY_CHUNK_CONTENT_DEDUPE_TTL_MS` of 1s
instead of the intended `PTY_CHUNK_IDENTITY_DEDUPE_TTL_MS` of 60s. If the broker replays
or re-sends the same `worker_stream` event (same `identity`) after more than 1s but less
than 60s—for example after an event-stream rebind in `rebindSessionEventStream` at
`src/main/broker.ts:1751-1807`—the stale identity entry has been prematurely evicted,
`previous` is undefined at `1987-1991`, and `isDuplicatePtyChunk` returns `false`, so the
duplicate PTY chunk is delivered again to the renderer and the terminal shows duplicated
output.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/broker.ts
**Line:** 1960:1962
**Comment:**
*Incorrect Condition Logic: The TTL selection infers key type by checking whether the whole key string contains `:chunk:`. Because the key embeds `sessionKey` and agent `name`, a normal identity-key entry can be misclassified if either segment contains that substring, causing identity dedupe entries to expire after 1s instead of 60s. Use an explicit key-type marker (or store structured metadata) instead of substring matching on the composed key.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // and the broker's transient "internal channel closed" while it's | ||
| // tearing down. Both surface as noise that's not actionable. | ||
| if (isMissingAgentError(err)) return | ||
| if (isBrokerUnreachableError(err)) return |
There was a problem hiding this comment.
Suggestion: This unconditionally suppresses unreachable transport errors in fire-and-forget mode, which makes real broker outages (ECONNREFUSED/ECONNRESET) silently drop user input with no warning signal. The shutdown-race suppression should stay narrowly scoped to known teardown-only errors, otherwise actionable failures become invisible. [logic error]
Severity Level: Critical 🚨
- ❌ Keystrokes dropped silently when local broker unreachable.
- ⚠️ Users lack visibility into broker input failures.Steps of Reproduction ✅
1. Open a project and attach a terminal in the renderer so that `useTerminal` at
`src/renderer/src/hooks/use-terminal.ts:180-199` is active; this hook defines `sendInput`
to call `pear.broker.sendInputFast(projectId, agentName!, data)` whenever the user types
or pastes (see also the global handlers at
`src/renderer/src/hooks/use-terminal.ts:500-563`).
2. On the preload/main boundary, `sendInputFast` in `src/preload/index.ts:24-27` forwards
input via `ipcRenderer.send('broker:send-input-fast', projectId, name, data)`, and the IPC
handler in `src/main/ipc-handlers.ts:40-42` calls
`brokerManager.sendInputFireAndForget(projectId, name, data)`, entering
`BrokerManager.sendInputFireAndForget` at `src/main/broker.ts:57-71` (PR hunk lines
2676-2690).
3. Induce a real broker outage—for example by killing the local Agent Relay broker process
while the session is still open or otherwise making the broker port unreachable. The next
keystroke from the terminal causes `sendInputFireAndForget` to call
`this.sendInput(projectId, trimmedName, data)` (at `src/main/broker.ts:62` in the
snippet), which ultimately calls `session.client.sendInput(trimmedName, data)` at
`src/main/broker.ts:49`. With the broker gone, the SDK surfaces a network error whose
cause has `code` `ECONNREFUSED`, `ECONNRESET`, or `UND_ERR_SOCKET`, so
`isBrokerUnreachableError` at `src/main/broker.ts:621-631` returns `true`.
4. That error is caught by the `.catch((err) => { ... })` in `sendInputFireAndForget` at
`src/main/broker.ts:62-71`; `isMissingAgentError(err)` is `false`, but the new `if
(isBrokerUnreachableError(err)) return` at PR line 2686 short-circuits the handler, so the
function returns without logging anything or surfacing an error. The keystroke is silently
dropped (no PTY write, no warning); yet the comment above (`2620-56`) still states that
"Errors get logged and the broker's existing stream-fallback logic in sendInput takes care
of reopening a broken stream." Recovery for unreachable brokers is implemented separately
in `withWedgeRecovery` at `src/main/broker.ts:2888-2973`, but `sendInputFireAndForget`
does not use that helper, so in this path broker outages result in unlogged, silently
discarded user input.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/broker.ts
**Line:** 2686:2686
**Comment:**
*Logic Error: This unconditionally suppresses unreachable transport errors in fire-and-forget mode, which makes real broker outages (`ECONNREFUSED`/`ECONNRESET`) silently drop user input with no warning signal. The shutdown-race suppression should stay narrowly scoped to known teardown-only errors, otherwise actionable failures become invisible.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
|
Implemented one additional PR hardening fix: rate-limited PTY diagnostics now emit for missing no-identity chunk IDs and suppressed PTY duplicates via Validated locally:
Checked local PR metadata/comments; no unresolved actionable bot review remained beyond the duplicate-hardening telemetry gap fixed here. |
|
✅ pr-reviewer applied fixes — committed and pushed Implemented one additional PR hardening fix: rate-limited PTY diagnostics now emit for missing no-identity chunk IDs and suppressed PTY duplicates via Validated locally:
Checked local PR metadata/comments; no unresolved actionable bot review remained beyond the duplicate-hardening telemetry gap fixed here. |
|
Implemented fixes for PR #91:
Validated bot comments against the current checkout. The TTL-prefix concern was stale: current code already uses explicit Verification run locally:
|
|
Implemented fixes for PR #91:
Validated bot comments against the current checkout. The TTL-prefix concern was stale: current code already uses explicit Verification run locally:
|
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
|
pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
2 similar comments
|
pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
|
pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
1 similar comment
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
|
pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
|
pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. pr-reviewer could not complete review for #91 in AgentWorkforce/pear. |
1 participant
User description
Summary
Tests
CodeAnt-AI Description
Deduplicate repeated no-identity PTY chunks while keeping distinct output and reducing shutdown noise
What Changed
Impact
✅ Fewer duplicate terminal lines✅ Correct split PTY output✅ Less shutdown noise in the console🔄 Retrigger CodeAnt AI Review
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.