Fix Telegram markdown and agent responsiveness#1668
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis change adds Telegram Markdown table conversion and entity-parse fallback, implements a steer pending-input lane with yielding in the agent loop, moves background exec into a utility-process RPC proxy, relocates Electron bootstrap to appMain, updates renderer pending-input capacity, and adjusts tests to cover the new flows. ChangesTelegram Markdown + Agent Steer + Exec Isolation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
test/main/presenter/remoteControlPresenter/telegramPoller.test.ts (1)
469-555: ⚡ Quick winAdd a regression test for fallback edit “not modified” handling.
The new cases validate fallback on
sendMessage, but not theeditMessageTextfallback branch. A targeted test here would lock in the fix for the Line 849 fallback edit path.🤖 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 `@test/main/presenter/remoteControlPresenter/telegramPoller.test.ts` around lines 469 - 555, Add a regression test in telegramPoller.test.ts that mirrors the existing "retries formatted markdown chunks as plain text" case but exercises the edit fallback path: mock client.editMessageText (not just sendMessage) to throw a TelegramApiRequestError representing the "message is not modified" / Bad Request response, ensure the TelegramPoller receives an update and router.conversation.getSnapshot returns a completed snapshot that would normally trigger editMessageText, then assert that after the editMessageText error the poller falls back to calling client.sendMessage with plain-text content; target symbols: TelegramPoller, client.editMessageText, client.sendMessage, TelegramApiRequestError, poller.start()/poller.stop(), and router.conversation.getSnapshot to locate and implement the test.
🤖 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/appMain.ts`:
- Around line 49-52: Remove the unconditional TLS bypass by deleting or gating
the call to app.commandLine.appendSwitch('ignore-certificate-errors') so it only
runs in a controlled dev/debug mode (e.g., guarded by an isDev or explicit env
flag) and add a clear comment explaining why it is restricted; also inspect the
function/method pullModel in ollamaProvider (look for the insecure: true flag)
and change it to false or similarly gate it behind the same explicit
development-only condition to avoid broadly weakening TLS in production.
In `@src/main/lib/agentRuntime/backgroundExecSessionManager.ts`:
- Around line 1037-1065: waitForCompletionOrYield and getCompletionResult call
this.request unconditionally and therefore return a "session not found" RPC
error when the utility process crashed; detect and return the stored crash
result instead of calling RPC when handleHostExit moved the session into
crashedSessions. In both methods (waitForCompletionOrYield and
getCompletionResult) first check this.crashedSessions for sessionId and if
present return that SessionCompletionResult/WaitForCompletionOrYieldResult (and
still clean up this.activeSessions/delete as appropriate), otherwise proceed to
call this.request(...) as today.
- Around line 1157-1167: BackgroundExecUtilityProxy.startHost() is forking the
current module (via fileURLToPath(import.meta.url)) so the child never runs the
host entrypoint (runBackgroundExecUtilityHostIfRequested) wired elsewhere;
change startHost() to fork the actual utility-host entrypoint module used by the
main bootstrap so the child enters host mode and services background-exec RPCs.
Also update waitForCompletionOrYield() and getCompletionResult() to consult the
crashedSessions fallback (the same map used by list/poll/log) when the utility
RPC is unavailable or the process has exited so completion APIs return results
or errors for crashed sessions instead of hanging on RPC. Reference:
BackgroundExecUtilityProxy.startHost, runBackgroundExecUtilityHostIfRequested,
waitForCompletionOrYield, getCompletionResult, crashedSessions.
In `@src/main/presenter/agentRuntimePresenter/pendingInputCoordinator.ts`:
- Around line 126-145: The four methods (releaseClaimedQueueInput,
releaseClaimedInput, consumeQueuedInput, consumeSteerInput) currently mutate by
itemId only and must revalidate that the item is owned by the caller sessionId
before changing it; update each method to verify ownership (either by calling an
existing this.store accessor like getById/getPendingInput or by adding
store-level guarded methods such as releaseClaimedQueueInputIfOwned(sessionId,
itemId) / releaseClaimedInputIfOwned(sessionId, itemId) /
consumeQueueInputIfOwned(sessionId, itemId) /
consumeSteerInputIfOwned(sessionId, itemId)) and only perform the
mutate+emitUpdated(sessionId) when the fetched record.sessionId === sessionId
(otherwise throw or return without mutating). Ensure the checks use the
sessionId parameter and keep emitUpdated(sessionId) behavior unchanged when the
mutation succeeds.
In `@src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts`:
- Around line 848-854: The fallback editMessageText call inside the parse-mode
error branch can throw a Telegram "400: message is not modified" and break
delivery sync; update the block that calls this.deps.client.editMessageText({
target, messageId, text: chunk.fallbackText }) (inside the condition checking
chunk.parseMode && this.isTelegramEntityParseError(error)) to catch errors and
ignore only the "message is not modified" case (same check used elsewhere in
this file), while rethrowing other errors so normal error handling still
applies.
In `@test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts`:
- Around line 152-155: The mock for countActiveBySession currently counts any
non-consumed row and therefore includes 'steer' items and already-claimed queue
rows; update it to only count actual pending/unclaimed input rows by filtering
pendingRows for rows with the matching session_id that are in the
unclaimed/pending state and not of type 'steer' (e.g. change the predicate to
something like row.session_id === sessionId && row.state === 'pending' &&
row.type !== 'steer' to exclude steer and claimed/consumed rows).
In `@test/main/presenter/agentSessionPresenter/integration.test.ts`:
- Around line 1071-1073: Replace the fixed 120ms setTimeout waits with a
condition-based wait that polls until providerInstance.coreStream has been
called the expected number of times; specifically, remove the await new
Promise((r) => setTimeout(r, 120)) and instead await a helper like waitFor or a
short-poll loop that repeatedly checks
expect(providerInstance.coreStream).toHaveBeenCalledTimes(3) (and the other
occurrence expecting a call count around lines 1153-1156) until success or a
timeout, so the test waits deterministically for providerInstance.coreStream to
reach the desired call count.
---
Nitpick comments:
In `@test/main/presenter/remoteControlPresenter/telegramPoller.test.ts`:
- Around line 469-555: Add a regression test in telegramPoller.test.ts that
mirrors the existing "retries formatted markdown chunks as plain text" case but
exercises the edit fallback path: mock client.editMessageText (not just
sendMessage) to throw a TelegramApiRequestError representing the "message is not
modified" / Bad Request response, ensure the TelegramPoller receives an update
and router.conversation.getSnapshot returns a completed snapshot that would
normally trigger editMessageText, then assert that after the editMessageText
error the poller falls back to calling client.sendMessage with plain-text
content; target symbols: TelegramPoller, client.editMessageText,
client.sendMessage, TelegramApiRequestError, poller.start()/poller.stop(), and
router.conversation.getSnapshot to locate and implement the test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7862e914-2c56-442a-a7f1-7f234e67a8b9
📒 Files selected for processing (31)
docs/features/telegram-markdown-rendering/plan.mddocs/features/telegram-markdown-rendering/spec.mddocs/features/telegram-markdown-rendering/tasks.mddocs/issues/agent-loop-input-exec-responsiveness/plan.mddocs/issues/agent-loop-input-exec-responsiveness/spec.mddocs/issues/agent-loop-input-exec-responsiveness/tasks.mdpackage.jsonsrc/main/appMain.tssrc/main/index.tssrc/main/lib/agentRuntime/backgroundExecSessionManager.tssrc/main/lib/agentRuntime/backgroundExecUtilityHost.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/pendingInputCoordinator.tssrc/main/presenter/agentRuntimePresenter/pendingInputStore.tssrc/main/presenter/agentRuntimePresenter/process.tssrc/main/presenter/agentRuntimePresenter/types.tssrc/main/presenter/remoteControlPresenter/telegram/telegramClient.tssrc/main/presenter/remoteControlPresenter/telegram/telegramOutbound.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/main/presenter/toolPresenter/agentTools/agentBashHandler.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/renderer/src/stores/ui/pendingInput.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/process.test.tstest/main/presenter/agentSessionPresenter/integration.test.tstest/main/presenter/remoteControlPresenter/telegramClient.test.tstest/main/presenter/remoteControlPresenter/telegramOutbound.test.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.tstest/renderer/components/PendingInputLane.test.tstest/renderer/stores/pendingInputStore.test.ts
Comment on lines
+49
to
+52
| app.commandLine.appendSwitch('autoplay-policy', 'no-user-gesture-required') // Allow video autoplay | ||
| app.commandLine.appendSwitch('webrtc-max-cpu-consumption-percentage', '100') // Set WebRTC max CPU usage | ||
| app.commandLine.appendSwitch('js-flags', '--max-old-space-size=4096') // Set V8 heap memory size | ||
| app.commandLine.appendSwitch('ignore-certificate-errors') // Ignore certificate errors (for dev or specific scenarios) |
Contributor
There was a problem hiding this comment.
Remove unconditional TLS bypass (ignore-certificate-errors) from the Electron main process
src/main/appMain.ts (lines 49-52) unconditionally appends ignore-certificate-errors, which disables TLS validation for every HTTPS request the app makes. Remove it or gate it behind an explicit development-only path.
Suggested fix
app.commandLine.appendSwitch('autoplay-policy', 'no-user-gesture-required') // Allow video autoplay
app.commandLine.appendSwitch('webrtc-max-cpu-consumption-percentage', '100') // Set WebRTC max CPU usage
app.commandLine.appendSwitch('js-flags', '--max-old-space-size=4096') // Set V8 heap memory size
-app.commandLine.appendSwitch('ignore-certificate-errors') // Ignore certificate errors (for dev or specific scenarios)
+if (process.env.NODE_ENV === 'development') {
+ app.commandLine.appendSwitch('ignore-certificate-errors')
+}Also review src/main/presenter/llmProviderPresenter/providers/ollamaProvider.ts where pullModel() sets insecure: true to ensure it isn’t broadly weakening TLS without the same controlled intent.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.commandLine.appendSwitch('autoplay-policy', 'no-user-gesture-required') // Allow video autoplay | |
| app.commandLine.appendSwitch('webrtc-max-cpu-consumption-percentage', '100') // Set WebRTC max CPU usage | |
| app.commandLine.appendSwitch('js-flags', '--max-old-space-size=4096') // Set V8 heap memory size | |
| app.commandLine.appendSwitch('ignore-certificate-errors') // Ignore certificate errors (for dev or specific scenarios) | |
| app.commandLine.appendSwitch('autoplay-policy', 'no-user-gesture-required') // Allow video autoplay | |
| app.commandLine.appendSwitch('webrtc-max-cpu-consumption-percentage', '100') // Set WebRTC max CPU usage | |
| app.commandLine.appendSwitch('js-flags', '--max-old-space-size=4096') // Set V8 heap memory size | |
| if (process.env.NODE_ENV === 'development') { | |
| app.commandLine.appendSwitch('ignore-certificate-errors') | |
| } |
🤖 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/appMain.ts` around lines 49 - 52, Remove the unconditional TLS
bypass by deleting or gating the call to
app.commandLine.appendSwitch('ignore-certificate-errors') so it only runs in a
controlled dev/debug mode (e.g., guarded by an isDev or explicit env flag) and
add a clear comment explaining why it is restricted; also inspect the
function/method pullModel in ollamaProvider (look for the insecure: true flag)
and change it to false or similarly gate it behind the same explicit
development-only condition to avoid broadly weakening TLS in production.
Comment on lines
+152
to
+155
| countActiveBySession: vi.fn( | ||
| (sessionId: string) => | ||
| pendingRows.filter((row) => row.session_id === sessionId && row.state !== 'consumed').length | ||
| ), |
Contributor
There was a problem hiding this comment.
Align countActiveBySession mock with queue-capacity semantics.
Line 152 currently counts all non-consumed rows, which includes steer items and claimed queue rows. That diverges from the updated queue-capacity model and can mask regressions in pending-input flow tests.
Suggested fix
countActiveBySession: vi.fn(
(sessionId: string) =>
- pendingRows.filter((row) => row.session_id === sessionId && row.state !== 'consumed').length
+ pendingRows.filter(
+ (row) =>
+ row.session_id === sessionId &&
+ row.mode === 'queue' &&
+ row.state !== 'consumed' &&
+ row.state !== 'claimed'
+ ).length
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| countActiveBySession: vi.fn( | |
| (sessionId: string) => | |
| pendingRows.filter((row) => row.session_id === sessionId && row.state !== 'consumed').length | |
| ), | |
| countActiveBySession: vi.fn( | |
| (sessionId: string) => | |
| pendingRows.filter( | |
| (row) => | |
| row.session_id === sessionId && | |
| row.mode === 'queue' && | |
| row.state !== 'consumed' && | |
| row.state !== 'claimed' | |
| ).length | |
| ), |
🤖 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 `@test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts`
around lines 152 - 155, The mock for countActiveBySession currently counts any
non-consumed row and therefore includes 'steer' items and already-claimed queue
rows; update it to only count actual pending/unclaimed input rows by filtering
pendingRows for rows with the matching session_id that are in the
unclaimed/pending state and not of type 'steer' (e.g. change the predicate to
something like row.session_id === sessionId && row.state === 'pending' &&
row.type !== 'steer' to exclude steer and claimed/consumed rows).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts (1)
806-814:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap fallback editMessageText in try-catch to handle "message is not modified" errors.
The fallback
editMessageTextcall at Line 807 lacks protection against Telegram's "400: message is not modified" error. If the plain-text retry produces the same rendered output, Telegram will reject it and break delivery sync.🛡️ Proposed fix
if (this.isTelegramEntityParseError(error)) { - await this.deps.client.editMessageText({ - target, - messageId: action.messageId, - text: action.text, - replyMarkup: action.replyMarkup ?? undefined - }) + try { + await this.deps.client.editMessageText({ + target, + messageId: action.messageId, + text: action.text, + replyMarkup: action.replyMarkup ?? undefined + }) + } catch (fallbackError) { + if (this.isMessageNotModifiedError(fallbackError)) { + return + } + throw fallbackError + } return }🤖 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/presenter/remoteControlPresenter/telegram/telegramPoller.ts` around lines 806 - 814, The fallback editMessageText call inside telegramPoller.ts (in the isTelegramEntityParseError branch) must be wrapped in a try-catch: call this.deps.client.editMessageText({ target, messageId: action.messageId, text: action.text, replyMarkup: action.replyMarkup ?? undefined }) inside the try, catch errors and suppress/ignore the Telegram "400: message is not modified" case (or check error.code/status/message) so it doesn't break delivery sync, and rethrow or log other errors; update the isTelegramEntityParseError handling to use this guarded call.
🤖 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.
Duplicate comments:
In `@src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts`:
- Around line 806-814: The fallback editMessageText call inside
telegramPoller.ts (in the isTelegramEntityParseError branch) must be wrapped in
a try-catch: call this.deps.client.editMessageText({ target, messageId:
action.messageId, text: action.text, replyMarkup: action.replyMarkup ??
undefined }) inside the try, catch errors and suppress/ignore the Telegram "400:
message is not modified" case (or check error.code/status/message) so it doesn't
break delivery sync, and rethrow or log other errors; update the
isTelegramEntityParseError handling to use this guarded call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68f1d177-4ae2-4951-9892-ff8525923bf2
📒 Files selected for processing (7)
docs/issues/telegram-message-markdown-render/plan.mddocs/issues/telegram-message-markdown-render/spec.mddocs/issues/telegram-message-markdown-render/tasks.mdsrc/main/presenter/remoteControlPresenter/telegram/telegramMarkdown.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tstest/main/presenter/remoteControlPresenter/telegramMarkdown.test.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/issues/telegram-message-markdown-render/plan.md
Collaborator
Author
|
Handled the latest review feedback in 9ebb6e9.\n\nUpdated:\n- Gated global TLS bypass and Ollama insecure pull behind dev/explicit debug flag.\n- Fixed background exec utility host entrypoint resolution and crashed-session completion fallback.\n- Revalidated pending input ownership before release/consume operations.\n- Ignored Telegram not-modified errors on plain edit fallback.\n- Replaced fixed test sleeps with condition-based waits and added regression coverage.\n\nValidation:\n- pnpm vitest run test/main/lib/agentRuntime/backgroundExecSessionManager.test.ts test/main/presenter/agentRuntimePresenter/pendingInputCoordinator.test.ts test/main/presenter/agentRuntimePresenter/pendingInputStore.test.ts test/main/presenter/remoteControlPresenter/telegramPoller.test.ts test/main/presenter/llmProviderPresenter/ollamaProvider.test.ts test/main/presenter/agentSessionPresenter/integration.test.ts test/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.ts\n- pnpm run format\n- pnpm run i18n\n- pnpm run lint\n- pnpm run typecheck |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/lib/agentRuntime/backgroundExecSessionManager.ts (1)
1130-1154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout for utility-process RPCs.
pendingRequestsare only rejected on host exit or synchronouspostMessagefailure. If the utility process stays alive but stops replying, every proxied operation can hang indefinitely.Suggested fix
private async request<T = void>(method: BackgroundExecRpcMethod, args: unknown[]): Promise<T> { const host = await this.ensureHost() const id = `exec_rpc_${++this.requestId}` return await new Promise<T>((resolve, reject) => { + const timeoutId = setTimeout(() => { + this.pendingRequests.delete(id) + reject(new Error(`Background exec RPC timed out: ${method}`)) + }, 30_000) + this.pendingRequests.set(id, { - resolve: (value) => resolve(value as T), - reject + resolve: (value) => { + clearTimeout(timeoutId) + resolve(value as T) + }, + reject: (error) => { + clearTimeout(timeoutId) + reject(error) + } }) const payload: BackgroundExecRpcRequest = { type: 'background-exec:request', id, @@ try { host.postMessage(payload) } catch (error) { + clearTimeout(timeoutId) this.pendingRequests.delete(id) reject(error) } }) }🤖 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/lib/agentRuntime/backgroundExecSessionManager.ts` around lines 1130 - 1154, The request method can hang indefinitely if the utility process never replies; add a per-request timeout: when creating the pendingRequests entry in request() (which uses requestId, pendingRequests, ensureHost, and host.postMessage to send a BackgroundExecRpcRequest), set a setTimeout that deletes the pendingRequests entry and calls its reject with a clear timeout error after a configurable TIMEOUT_MS; store the timer reference in the pendingRequests value so that both the resolve and reject wrappers clearTimeout when the request completes; ensure the timeout is cleaned up on synchronous postMessage failure as well.
♻️ Duplicate comments (2)
src/main/lib/agentRuntime/backgroundExecSessionManager.ts (1)
1042-1075:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the crashed-session fallback after RPC rejection too.
The new pre-check only handles sessions already present in
crashedSessions. If the utility host exits after that check but before the RPC resolves,request()rejects and callers still see an exception instead of the synthesized terminal crash result.Suggested fix
async waitForCompletionOrYield( conversationId: string, sessionId: string, yieldMs = getConfig().backgroundMs ): Promise<WaitForCompletionOrYieldResult> { const crashed = this.getCrashedCompletionResult(conversationId, sessionId) if (crashed) { return { kind: 'completed', result: crashed } } - const result = await this.request<WaitForCompletionOrYieldResult>('waitForCompletionOrYield', [ - conversationId, - sessionId, - yieldMs - ]) + const result = await this.request<WaitForCompletionOrYieldResult>('waitForCompletionOrYield', [ + conversationId, + sessionId, + yieldMs + ]).catch((error) => { + const crashedAfterExit = this.getCrashedCompletionResult(conversationId, sessionId) + if (crashedAfterExit) { + return { + kind: 'completed', + result: crashedAfterExit + } + } + throw error + }) if (result.kind === 'completed') { this.activeSessions.delete(sessionId) } return result } async getCompletionResult( conversationId: string, sessionId: string, previewChars = FOREGROUND_PREVIEW_CHARS ): Promise<SessionCompletionResult> { const crashed = this.getCrashedCompletionResult(conversationId, sessionId) if (crashed) { return crashed } - const result = await this.request<SessionCompletionResult>('getCompletionResult', [ - conversationId, - sessionId, - previewChars - ]) + const result = await this.request<SessionCompletionResult>('getCompletionResult', [ + conversationId, + sessionId, + previewChars + ]).catch((error) => { + const crashedAfterExit = this.getCrashedCompletionResult(conversationId, sessionId) + if (crashedAfterExit) { + return crashedAfterExit + } + throw error + }) this.activeSessions.delete(sessionId) return result }🤖 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/lib/agentRuntime/backgroundExecSessionManager.ts` around lines 1042 - 1075, The RPC calls in getCompletionResult (and the earlier waitForCompletionOrYield path where this.request(...) is used) need to be wrapped in a try/catch so that if this.request rejects (utility host exits after the pre-check), you call getCrashedCompletionResult(conversationId, sessionId) inside the catch and return that synthesized terminal crash result instead of letting the exception propagate; specifically, wrap the await this.request<...>('getCompletionResult', [...]) (and the await this.request<WaitForCompletionOrYieldResult>('waitForCompletionOrYield', [...]) call) in try/catch, on success keep the existing behavior (including activeSessions.delete(sessionId) for completed), and on catch return the value from getCrashedCompletionResult(conversationId, sessionId) if present, otherwise rethrow.src/main/presenter/agentRuntimePresenter/pendingInputCoordinator.ts (1)
60-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
mergeItemIdagainstsessionIdbefore appending.
appendSteerInput()only checks mode/state, so a stale or wrongmergeItemIdcan merge text/files into another session’s steer item whileemitUpdated(sessionId)notifies the caller’s session instead of the mutated owner.Suggested fix
let record: PendingSessionInputRecord if (options?.mergeItemId) { + this.assertSteerInputForSession(sessionId, options.mergeItemId) record = this.store.appendSteerInput(options.mergeItemId, normalizeInput(input)) } else { record = this.store.createSteerInput(sessionId, normalizeInput(input)) }🤖 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/presenter/agentRuntimePresenter/pendingInputCoordinator.ts` around lines 60 - 65, Validate that options?.mergeItemId actually belongs to the current sessionId before calling appendSteerInput: when mergeItemId is provided, look up the steer item owner from the store (e.g., a method like this.store.getSteerItemOwner or this.store.findSteerItem by id) and if the owner !== sessionId treat it as invalid (either fall back to createSteerInput(sessionId, normalizeInput(input)) or return an error). If you do append to the found steer item, ensure emitUpdated is invoked with the mutated item’s true owner session id (not always the incoming sessionId) so notifications go to the correct session; keep the existing normalizeInput and record handling otherwise.
🤖 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.
Outside diff comments:
In `@src/main/lib/agentRuntime/backgroundExecSessionManager.ts`:
- Around line 1130-1154: The request method can hang indefinitely if the utility
process never replies; add a per-request timeout: when creating the
pendingRequests entry in request() (which uses requestId, pendingRequests,
ensureHost, and host.postMessage to send a BackgroundExecRpcRequest), set a
setTimeout that deletes the pendingRequests entry and calls its reject with a
clear timeout error after a configurable TIMEOUT_MS; store the timer reference
in the pendingRequests value so that both the resolve and reject wrappers
clearTimeout when the request completes; ensure the timeout is cleaned up on
synchronous postMessage failure as well.
---
Duplicate comments:
In `@src/main/lib/agentRuntime/backgroundExecSessionManager.ts`:
- Around line 1042-1075: The RPC calls in getCompletionResult (and the earlier
waitForCompletionOrYield path where this.request(...) is used) need to be
wrapped in a try/catch so that if this.request rejects (utility host exits after
the pre-check), you call getCrashedCompletionResult(conversationId, sessionId)
inside the catch and return that synthesized terminal crash result instead of
letting the exception propagate; specifically, wrap the await
this.request<...>('getCompletionResult', [...]) (and the await
this.request<WaitForCompletionOrYieldResult>('waitForCompletionOrYield', [...])
call) in try/catch, on success keep the existing behavior (including
activeSessions.delete(sessionId) for completed), and on catch return the value
from getCrashedCompletionResult(conversationId, sessionId) if present, otherwise
rethrow.
In `@src/main/presenter/agentRuntimePresenter/pendingInputCoordinator.ts`:
- Around line 60-65: Validate that options?.mergeItemId actually belongs to the
current sessionId before calling appendSteerInput: when mergeItemId is provided,
look up the steer item owner from the store (e.g., a method like
this.store.getSteerItemOwner or this.store.findSteerItem by id) and if the owner
!== sessionId treat it as invalid (either fall back to
createSteerInput(sessionId, normalizeInput(input)) or return an error). If you
do append to the found steer item, ensure emitUpdated is invoked with the
mutated item’s true owner session id (not always the incoming sessionId) so
notifications go to the correct session; keep the existing normalizeInput and
record handling otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e64c829-cac8-4fbf-b92f-a6afb573eae7
📒 Files selected for processing (13)
src/main/appMain.tssrc/main/lib/agentRuntime/backgroundExecSessionManager.tssrc/main/lib/insecureTls.tssrc/main/presenter/agentRuntimePresenter/pendingInputCoordinator.tssrc/main/presenter/agentRuntimePresenter/pendingInputStore.tssrc/main/presenter/llmProviderPresenter/providers/ollamaProvider.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tstest/main/lib/agentRuntime/backgroundExecSessionManager.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/pendingInputCoordinator.test.tstest/main/presenter/agentSessionPresenter/integration.test.tstest/main/presenter/llmProviderPresenter/ollamaProvider.test.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.ts
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.
Summary
dev, including upstream Telegram HTML markdown rendering from fix(telegram): render markdown as html #1667.<pre>text and plain-text retry when Telegram rejects converted HTML entities.Tests
pnpm vitest run test/main/presenter/remoteControlPresenter/telegramMarkdown.test.ts test/main/presenter/remoteControlPresenter/telegramClient.test.ts test/main/presenter/remoteControlPresenter/telegramPoller.test.tspnpm run formatpnpm run i18npnpm run lintpnpm run typecheckSummary by CodeRabbit
New Features
Improvements