Harden Desktop error telemetry and diagnostics - #1271
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR standardizes telemetry sanitization and failure tracking, preserves bounded Python traceback context, adds operation-specific updater errors, instruments adoption and migration phases, and improves process termination and port-conflict handling during launches. ChangesTelemetry and diagnostics
Adoption, migration, and launch flows
Sequence Diagram(s)sequenceDiagram
participant Launch
participant ChildProcess
participant PortManager
participant ExecutionTap
Launch->>ChildProcess: spawn ComfyUI process
Launch->>PortManager: wait for or reserve launch port
ChildProcess->>ExecutionTap: stream stderr and stdout
PortManager-->>Launch: report listener or port conflict
ChildProcess-->>Launch: emit close or exit
Launch->>Launch: retry port or return structured failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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
requested review from
AustinMroz,
christian-byrne and
deepme987
July 15, 2026 08:30
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
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/updater.ts (1)
408-417: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTrack the active updater operation explicitly.
This still derives the operation from cached state, so a revalidation check performed while state is
readyis reported as an installer failure; carry the initiatingcheck,download, orapply_restartoperation into the event handler instead.🤖 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/updater.ts` around lines 408 - 417, Update the updater event handling around the stage calculation and emitDesktopUpdateError call to use the initiating operation captured when the updater action starts, rather than deriving it from _appUpdateState.kind, _autoDownloadTriggeredFor, or wasUserInitiated. Propagate the active check, download, or apply_restart operation into this handler and emit that operation unchanged.
🤖 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/lib/executionTap.ts`:
- Around line 68-69: Update EXCEPTION_LINE and the traceback detection logic in
executionTap to recognize final exception lines structurally rather than
requiring class names to end with Error, Exception, Warning, or Interrupt.
Ensure names such as Boom, StopIteration, and ExceptionGroup trigger traceback
flushing through the existing some(...) checks, and add a regression test
covering a non-suffixed exception.
In `@src/main/lib/ipc/sessionActions/launch.test.ts`:
- Around line 81-86: Update the process event test around proc.emit and the
callback assertions to use distinct payloads for the exit and close events, then
assert that the callback is invoked once with the close event’s payload.
Preserve the multiple close emissions to verify the first close result is
preferred over exit and subsequent close events.
In `@src/main/lib/ipc/sessionActions/launch.ts`:
- Around line 823-830: Compute the explicit-port policy once in the shared
launch flow, treating a present portOverride as explicit alongside the --port
launch argument. Reuse that shared value in both conflict checks, including the
branches around the existing late-conflict logic, and remove their duplicate
local declarations while preserving the current portConflict behavior.
- Around line 129-134: Update the finish callback in the launch termination flow
to handle rejected promises returned by callback instead of discarding them with
void callback(...). Preserve the existing single-execution guard and timer
cleanup, and ensure callback failures are caught so they do not produce
unhandled rejections or interrupt lifecycle cleanup.
- Around line 1069-1083: In the port-conflict retry flow surrounding tryLaunch,
await killProcessTree(spawned.proc) before returning tryLaunch(). Keep the
existing port release, reassignment, reservation, and retry behavior unchanged,
ensuring the previous process tree is fully terminated before launching the
replacement.
In `@src/main/lib/processErrorHandlers.ts`:
- Around line 48-52: Update the options passed to mainTelemetry.captureException
in the process error handler to merge scrubbed.context alongside origin, source,
and level. Preserve the existing fields and allow captureException to normalize
the merged context so child/render process details are retained.
In `@src/main/lib/updater.ts`:
- Around line 302-317: Update the standard error-field construction in the
telemetry payload to use the original error value rather than only message.
Reuse errorObject when available, preserving Error.name and structured code for
updater failures, while retaining the existing message fallback when no Error
instance exists.
In `@src/renderer/src/lib/rendererBootstrap.ts`:
- Around line 167-174: Replace the renderer-local claimRendererTelemetryBudget
counter with a shared main-process IPC budget, ensuring each telemetry event is
emitted to Datadog only after main acknowledges acceptance. Update the related
renderer telemetry call sites, including the error path, so all
renderer-originated events use this centralized acceptance flow and cannot
bypass the global cap.
In `@src/shared/errorBucket.ts`:
- Around line 26-40: Update the detail extraction in the error-bucketing logic
around the raw input normalization so plain objects with a string record.message
use that value as their detail. Preserve existing Error and string handling,
allowing serialized errors such as { message: 'ECONNRESET' } to contribute to
raw instead of being classified as unknown.
In `@src/shared/errorEvent.ts`:
- Line 176: Update the missing-module replacement regex in error signature
normalization to preserve only explicitly allowlisted safe module identifiers;
replace non-allowlisted identifiers, including private package names, with <str>
to prevent unbounded grouping cardinality.
- Around line 83-93: Restrict structuredCode in src/shared/errorEvent.ts:83-93
to return only recognized Node, Chromium, or errno codes rather than arbitrary
uppercase tokens, including the nested cause path. In
src/shared/errorEvent.test.ts:102-105, add an uppercase private or
high-cardinality code case and assert it resolves to unknown.
---
Outside diff comments:
In `@src/main/lib/updater.ts`:
- Around line 408-417: Update the updater event handling around the stage
calculation and emitDesktopUpdateError call to use the initiating operation
captured when the updater action starts, rather than deriving it from
_appUpdateState.kind, _autoDownloadTriggeredFor, or wasUserInitiated. Propagate
the active check, download, or apply_restart operation into this handler and
emit that operation unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 10f0495a-0b35-4d22-b440-fa42c2e85238
📒 Files selected for processing (23)
src/main/lib/desktopAdopt.test.tssrc/main/lib/desktopAdopt.tssrc/main/lib/executionTap.test.tssrc/main/lib/executionTap.tssrc/main/lib/ipc/registerTelemetryHandlers.test.tssrc/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/ipc/sessionActions/launch.test.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/ipc/sessionActions/migrate.tssrc/main/lib/processErrorHandlers.tssrc/main/lib/standaloneMigration.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/updater.test.tssrc/main/lib/updater.tssrc/renderer/src/lib/rendererBootstrap.tssrc/shared/datadogMirroredEvents.test.tssrc/shared/datadogMirroredEvents.tssrc/shared/errorBucket.tssrc/shared/errorEvent.test.tssrc/shared/errorEvent.tssrc/shared/piiScrub.test.tssrc/shared/piiScrub.ts
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 (3)
src/main/lib/updater.ts (1)
657-713: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the duplicated
sourcelogic into a shared variable.Repeated ternary logic makes maintenance a fright, so share that variable and keep it tight! As per coding guidelines, check for duplicated logic and extract it into shared variables to prevent divergence.
♻️ Proposed refactor
export function installUpdate(userInitiated = true): void { + const updateSource = userInitiated ? 'install_call' : 'startup_install' if (isSessionEnding()) { // The OS is shutting down / logging off. Spawning the installer now risks // it being force-killed mid-write, corrupting the install — the exact // failure this whole flow exists to avoid. The staged update stays put and // applies on the next launch instead. return } const updater = getAutoUpdater() if (!updater) { emitDesktopUpdateError('apply_restart', UPDATER_UNAVAILABLE_MESSAGE, { userInitiated, - source: userInitiated ? 'install_call' : 'startup_install' + source: updateSource }) _broadcastToRenderer('app-update:user-action-failed', { message: UPDATER_UNAVAILABLE_MESSAGE }) return } emitTelemetry('comfy.desktop.app_update.install_triggered', { version: _appUpdateState.version, auto_update_setting: isAutoInstallEnabled() ? 'on' : 'off' }) try { _activeUpdateOperation = { operation: 'apply_restart', - source: userInitiated ? 'install_call' : 'startup_install', + source: updateSource, targetVersion: _appUpdateState.version, userInitiated, startedAt: Date.now() } setQuitReason('update-install') // macOS Squirrel quirk: if requestSingleInstanceLock is still held by // the quitting process, ShipIt swaps the .app bundle correctly but // the new Squirrel.Mac process cannot acquire the lock and exits // silently — the user sees the app close and nothing relaunches. // Electron's own docs call this out: quitAndInstall + single-instance // lock is a known footgun on darwin. Releasing the lock immediately // before restartAndInstall lets the next process come up cleanly. // Windows / Linux update paths don't have this contention and don't // need the release. if (process.platform === 'darwin') { app.releaseSingleInstanceLock() } // `isSilent: false` shows the NSIS progress window during the install (see // `isInstallerUIEnabled` — Windows-only, default on). Forced silent on // macOS/Linux, where `isSilent` has no effect anyway. updater.restartAndInstall({ isSilent: !isInstallerUIEnabled() }) } catch (err) { _activeUpdateOperation = null clearQuitReason() emitDesktopUpdateError('apply_restart', err, { userInitiated, - source: userInitiated ? 'install_call' : 'startup_install' + source: updateSource }) _broadcastToRenderer('app-update:user-action-failed', { message: err instanceof Error ? err.message : String(err) }) } }🤖 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/updater.ts` around lines 657 - 713, In installUpdate, extract the repeated userInitiated ? 'install_call' : 'startup_install' expression into one local source variable near the start of the function, then reuse it for _activeUpdateOperation.source and both emitDesktopUpdateError calls. Preserve the existing source values and behavior.Source: Coding guidelines
src/main/lib/telemetry.ts (1)
1013-1016: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated stack-length cap — extract a shared constant. Both sinks truncate
error.stackto the same magic16 * 1024, instead of reusing a shared constant the wayERROR_MESSAGE_MAXis already shared between main and renderer. One divergence away from the two caps quietly drifting.
src/main/lib/telemetry.ts#L1013-L1016: replace the inline16 * 1024incaptureExceptionwith a sharedERROR_STACK_MAXconstant (defined alongsideERROR_MESSAGE_MAX).src/renderer/src/lib/rendererBootstrap.ts#L536-L539: replace the inline16 * 1024inreportRendererErrorwith the same sharedERROR_STACK_MAXconstant.🤖 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/telemetry.ts` around lines 1013 - 1016, Define a shared ERROR_STACK_MAX constant alongside ERROR_MESSAGE_MAX, then update captureException in src/main/lib/telemetry.ts:1013-1016 and reportRendererError in src/renderer/src/lib/rendererBootstrap.ts:536-539 to use it instead of the inline 16 * 1024 cap.Source: Coding guidelines
src/renderer/src/lib/rendererBootstrap.ts (1)
201-212: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
scrubTelemetryContextsilently skips array values — Datadog-bound array fields bypass PII scrubbing.Unlike main-process
scrubProperties(which scrubs each string entry of array values too), this renderer-side helper doesif (typeof value !== 'string') continue, so any array-shaped context value sails through unscrubbed straight intodatadogRum.addAction/addError. Pre-existing gap, but this PR just widened what flows through this exact function (main-process events now round-trip here viaonTelemetryActionFromMain), so the blast radius grew. Bring it in line withscrubPropertiesto keep the two sinks' scrubbing symmetric — no need to let one scrub imp nap while the other one works overtime.🔒 Proposed fix
function scrubTelemetryContext(context: TelemetryContext): TelemetryContext { let mutated: TelemetryContext | null = null for (const key of Object.keys(context)) { const value = context[key] - if (typeof value !== 'string') continue - const cleaned = scrubAll(value) - if (cleaned === value) continue - if (!mutated) mutated = { ...context } - mutated[key] = cleaned + if (typeof value === 'string') { + const cleaned = scrubAll(value) + if (cleaned === value) continue + if (!mutated) mutated = { ...context } + mutated[key] = cleaned + } else if (Array.isArray(value)) { + const cleaned = value.map((entry) => (typeof entry === 'string' ? scrubAll(entry) : entry)) + if (cleaned.every((entry, index) => entry === value[index])) continue + if (!mutated) mutated = { ...context } + mutated[key] = cleaned + } } return mutated ?? context }🤖 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/rendererBootstrap.ts` around lines 201 - 212, Update scrubTelemetryContext to scrub array-valued fields as well as direct string values, applying scrubAll to each string element and preserving non-string elements. Keep the existing copy-on-change behavior and return the original context when no values require modification, matching scrubProperties’ handling.
♻️ Duplicate comments (1)
src/main/lib/processErrorHandlers.ts (1)
47-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
scrubbed.contextstill doesn't reach PostHog — and now bypasses it entirely for Datadog.Same gap flagged previously:
mainTelemetry.captureException(...)only receivesorigin/source/level, soexitCode,reason,name,serviceNamefromchild-process-gone/render-process-gonenever land in PostHog. Worse, this diff now spreadsscrubbed.contextintoforwardExceptionToRendererinstead — so the same fields that are missing from the "rich" sink partially leak toward the intentionally-thin Datadog sink, inverting the intended sink priority.🩹 Proposed fix
const accepted = mainTelemetry.captureException(err, { + ...(scrubbed.context || {}), origin: 'main-process', source: scrubbed.source, level: scrubbed.level ?? null })🤖 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/processErrorHandlers.ts` around lines 47 - 59, Update the error handling flow around mainTelemetry.captureException and forwardExceptionToRenderer so scrubbed.context is included in the PostHog capture payload, while the renderer/Datadog forwarding payload remains intentionally limited to origin, source, and level. Remove the context spread from forwardExceptionToRenderer and preserve the existing scrubbed field handling.
🤖 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/telemetry.ts`:
- Around line 1013-1016: Define a shared ERROR_STACK_MAX constant alongside
ERROR_MESSAGE_MAX, then update captureException in
src/main/lib/telemetry.ts:1013-1016 and reportRendererError in
src/renderer/src/lib/rendererBootstrap.ts:536-539 to use it instead of the
inline 16 * 1024 cap.
In `@src/main/lib/updater.ts`:
- Around line 657-713: In installUpdate, extract the repeated userInitiated ?
'install_call' : 'startup_install' expression into one local source variable
near the start of the function, then reuse it for _activeUpdateOperation.source
and both emitDesktopUpdateError calls. Preserve the existing source values and
behavior.
In `@src/renderer/src/lib/rendererBootstrap.ts`:
- Around line 201-212: Update scrubTelemetryContext to scrub array-valued fields
as well as direct string values, applying scrubAll to each string element and
preserving non-string elements. Keep the existing copy-on-change behavior and
return the original context when no values require modification, matching
scrubProperties’ handling.
---
Duplicate comments:
In `@src/main/lib/processErrorHandlers.ts`:
- Around line 47-59: Update the error handling flow around
mainTelemetry.captureException and forwardExceptionToRenderer so
scrubbed.context is included in the PostHog capture payload, while the
renderer/Datadog forwarding payload remains intentionally limited to origin,
source, and level. Remove the context spread from forwardExceptionToRenderer and
preserve the existing scrubbed field handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d7842d6-ef51-4301-9643-dca862c29c56
📒 Files selected for processing (11)
src/main/lib/ipc/registerTelemetryHandlers.test.tssrc/main/lib/ipc/registerTelemetryHandlers.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/processErrorHandlers.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/updater.test.tssrc/main/lib/updater.tssrc/renderer/src/lib/rendererBootstrap.tssrc/shared/datadogMirroredEvents.test.tssrc/shared/datadogMirroredEvents.ts
Member
Author
|
Addressed the actionable outside-diff review findings:
Validation: full typecheck, lint, build, and test suites pass. |
There was a problem hiding this comment.
Actionable comments posted: 4
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/ipc/sessionActions/launch.ts (1)
771-809: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract duplicated process exit logic.
The termination handling logic here is identical to the logic in
attachExitHandlerat lines 1359-1395 (aside fromgetStderr()vscurrentGetStderr()). As per coding guidelines, extract this duplicated logic into a shared helper function to prevent divergence. Don't be a goblin hoarding duplicate code! Extracting it makes the codebase gleam and puts less stress on the team.♻️ Proposed helper extraction
Define a helper function in
handleLaunch(above theskipPortWaitblock):async function handleProcessExited( code: number | null, signal: NodeJS.Signals | null, getStderrFn: () => string, logStream: WriteStream, execTap: ReturnType<typeof createExecutionTap>, hwTap: ReturnType<typeof createHardwareTap> ) { logStream.end() const crashed = _runningSessions.has(installationId) && isCrashedExit(code, signal) const lastStderr = lastNLines(getStderrFn(), 100) execTap.flushSummary() hwTap.flushSummary() const crashDiagnosis = crashed ? await diagnoseCrash(code) : {} _removeSession(installationId) const exitedPayload = { installationId, crashed, exitCode: code ?? undefined, signal: signal ?? undefined, installationName: inst.name, lastStderr, ...crashDiagnosis } telemetry.emit('comfy.desktop.comfyui.exited', { installation_id: installationId, crashed, exit_code: code ?? null, last_stderr: lastStderr ?? null }) if (crashed) { recordCrash(exitedPayload) _broadcastToRenderer('instance-crashed', exitedPayload) } if (!sender.isDestroyed()) { sender.send('comfy-exited', exitedPayload) } if (_onComfyExited) _onComfyExited({ installationId, crashed }) }Then replace this block (and the matching block in
attachExitHandlerat lines 1359-1395) with a single call:onProcessTerminated(proc, async (code, signal) => { await handleProcessExited(code, signal, getStderr, logStream, execTap, hwTap) })🤖 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/ipc/sessionActions/launch.ts` around lines 771 - 809, Extract the shared termination handling from the current exit callback and attachExitHandler into a local async handleProcessExited helper within handleLaunch, accepting the exit details, stderr getter, log stream, and execution/hardware taps. Move the common cleanup, crash diagnosis, payload creation, telemetry, crash recording, renderer notification, and _onComfyExited calls into that helper, preserving getStderr versus currentGetStderr and ending the corresponding log stream. Replace both duplicated blocks with calls to handleProcessExited through onProcessTerminated.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/lib/executionTap.ts`:
- Around line 68-69: Update the EXCEPTION_LINE regular expression in
executionTap.ts to allow uppercase letters in dotted module-name prefixes, while
preserving the existing matching behavior for exception class names and trailing
colon/whitespace.
In `@src/main/lib/process.ts`:
- Around line 91-103: Bound the retry loop in waitForGroupExit by tracking poll
attempts or elapsed time and stopping after a defined maximum, including
persistent EPERM cases. When the limit is reached, call done() so the launch
lifecycle cannot hang indefinitely; preserve the existing polling behavior
before the cap.
- Around line 193-197: In src/main/lib/process.ts lines 193-197, update
waitForPort to use a per-attempt boolean guard so the error and timeout handlers
schedule poll only once, including when req.destroy() emits error synchronously,
and ignore late errors after success. Apply the identical guard in waitForUrl at
src/main/lib/process.ts lines 232-236.
In `@src/main/lib/processErrorHandlers.ts`:
- Around line 54-58: Update the mainTelemetry.forwardExceptionToRenderer call in
process error handling to include scrubbed.context alongside origin, source, and
level, preserving the same merged context already used by captureException so
Datadog receives details such as exitCode and reason.
---
Outside diff comments:
In `@src/main/lib/ipc/sessionActions/launch.ts`:
- Around line 771-809: Extract the shared termination handling from the current
exit callback and attachExitHandler into a local async handleProcessExited
helper within handleLaunch, accepting the exit details, stderr getter, log
stream, and execution/hardware taps. Move the common cleanup, crash diagnosis,
payload creation, telemetry, crash recording, renderer notification, and
_onComfyExited calls into that helper, preserving getStderr versus
currentGetStderr and ending the corresponding log stream. Replace both
duplicated blocks with calls to handleProcessExited through onProcessTerminated.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: c223db9c-0f00-45eb-b06e-12400b645bd7
📒 Files selected for processing (15)
src/main/lib/executionTap.test.tssrc/main/lib/executionTap.tssrc/main/lib/ipc/sessionActions/launch.test.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/process.tssrc/main/lib/processErrorHandlers.tssrc/main/lib/telemetry.test.tssrc/main/lib/telemetry.tssrc/main/lib/updater.test.tssrc/main/lib/updater.tssrc/renderer/src/lib/rendererBootstrap.tssrc/renderer/src/lib/telemetry.tssrc/shared/errorBucket.tssrc/shared/errorEvent.test.tssrc/shared/errorEvent.ts
…ardening Amp-Thread-ID: https://ampcode.com/threads/T-019f9e5f-73ce-745e-bd14-4337a5f52ebd Co-authored-by: Amp <amp@ampcode.com> # Conflicts: # src/main/lib/ipc/sessionActions/launch.test.ts # src/main/lib/ipc/sessionActions/launch.ts # src/main/lib/standaloneMigration.ts # src/renderer/src/lib/rendererBootstrap.ts
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)
src/main/lib/ipc/sessionActions/launch.ts (1)
1225-1243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor explicit/manual port policy and wait for teardown before retrying.
Line 1218 starts termination without awaiting it, so either recursive retry can race the old process. Also, this new runtime-conflict branch switches ports even when the port is explicit or conflict mode is not
auto. The retry can hare off from the user’s selected port.Proposed fix
- killProcessTree(spawned.proc) + await killProcessTree(spawned.proc) @@ - if (isPortConflict && portRetries < PORT_RETRY_MAX) { + if ( + isPortConflict && + portConflictMode === 'auto' && + !portIsExplicit && + portRetries < PORT_RETRY_MAX + ) {🤖 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/ipc/sessionActions/launch.ts` around lines 1225 - 1243, Update the port-conflict retry flow in tryLaunch to await the existing termination operation before recursively retrying, preventing overlap with the prior process. Gate automatic port switching on the configured conflict mode being “auto” and the port not being explicit/manual; otherwise preserve the user-selected port and existing failure 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/main/lib/standaloneMigration.ts`:
- Around line 595-600: Update the restore-failure comment near the migration
flow to remove the numbered “4.” plan-step reference and the “#1255” historical
note. Keep only the concise rationale that the install remains bootable, the
migration should finish, and the failure must be reported through the caller’s
error event.
---
Outside diff comments:
In `@src/main/lib/ipc/sessionActions/launch.ts`:
- Around line 1225-1243: Update the port-conflict retry flow in tryLaunch to
await the existing termination operation before recursively retrying, preventing
overlap with the prior process. Gate automatic port switching on the configured
conflict mode being “auto” and the port not being explicit/manual; otherwise
preserve the user-selected port and existing failure 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: ASSERTIVE
Plan: Pro Plus
Run ID: 3e55a268-debc-42e9-8f9b-493842481170
📒 Files selected for processing (9)
src/main/lib/desktopAdopt.test.tssrc/main/lib/desktopAdopt.tssrc/main/lib/ipc/sessionActions/launch.test.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/ipc/sessionActions/migrate.tssrc/main/lib/standaloneMigration.tssrc/main/lib/telemetry.tssrc/renderer/src/lib/rendererBootstrap.tssrc/renderer/src/lib/telemetry.ts
- Recognize exceptions from uppercase module packages (PIL.*) in the execution tap traceback classifier - Bound killProcessTree's POSIX group-exit poll (5s) so an unkillable process cannot hang launch teardown forever - Guard waitForPort/waitForUrl retry scheduling: destroy-on-timeout emits 'error', which previously multiplied pollers per attempt - Forward scrubbed exception context (reason/exitCode/type) to the renderer Datadog mirror; keys stay allow-listed - Re-arm per-attempt boot phase observation on reboot/port retries so a failing retry flushes real phase rows instead of an empty buffer - Isolate the tolerated migrate restore_snapshot failure from the canonical flow scope and stop emitting a step error on cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019f9e5f-73ce-745e-bd14-4337a5f52ebd Co-authored-by: Amp <amp@ampcode.com>
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 (1)
src/main/lib/ipc/sessionActions/launch.ts (1)
1223-1246: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAwait teardown before either recursive retry.
Line 1222 starts termination but both retry paths can call
tryLaunch()first. The reboot path reuses the same port, and the replacement can overlap the old process tree and its stream handlers. AwaitkillProcessTree(spawned.proc)before branching into reboot or port retries.Proposed fix
- killProcessTree(spawned.proc) + await killProcessTree(spawned.proc) if (checkRebootMarker(sessionPath) && rebootRetries < REBOOT_RETRY_MAX) {🤖 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/ipc/sessionActions/launch.ts` around lines 1223 - 1246, In the retry handling around spawned.proc, await killProcessTree(spawned.proc) before either recursive tryLaunch() path. Ensure teardown completes before the reboot retry and before the port-conflict retry performs port reassignment and launches again, while preserving the existing retry limits and branching 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/main/lib/process.ts`:
- Around line 195-215: Update waitForPort in src/main/lib/process.ts lines
195-215 to settle its outer promise with rejection when signal.abort fires,
destroy the active request, and suppress both late responses and pending
retries. Apply the same global abort settlement to waitForUrl at
src/main/lib/process.ts lines 245-264, and add a test that aborts before the
mocked response arrives to verify no successful resolution or session
registration occurs.
In `@src/main/lib/standaloneMigration.ts`:
- Around line 623-630: Normalize the caught value in the restore-snapshot catch
block before assigning restoreError or calling buildErrorFields, using the
existing error-normalization utility or contract so null, undefined, and
primitive throws produce a safe, scrubbed, bounded Error representation.
Preserve the signal.aborted rethrow behavior, and use the normalized error
consistently for restoreError and telemetry.
---
Outside diff comments:
In `@src/main/lib/ipc/sessionActions/launch.ts`:
- Around line 1223-1246: In the retry handling around spawned.proc, await
killProcessTree(spawned.proc) before either recursive tryLaunch() path. Ensure
teardown completes before the reboot retry and before the port-conflict retry
performs port reassignment and launches again, while preserving the existing
retry limits and branching 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: ASSERTIVE
Plan: Pro Plus
Run ID: ab53055f-f1da-418d-b423-8c49b28d508d
📒 Files selected for processing (8)
src/main/lib/executionTap.test.tssrc/main/lib/executionTap.tssrc/main/lib/ipc/sessionActions/launch.tssrc/main/lib/launchProgress.test.tssrc/main/lib/launchProgress.tssrc/main/lib/process.tssrc/main/lib/processErrorHandlers.tssrc/main/lib/standaloneMigration.ts
…ardening Amp-Thread-ID: https://ampcode.com/threads/T-019f9e5f-73ce-745e-bd14-4337a5f52ebd Co-authored-by: Amp <amp@ampcode.com> # Conflicts: # src/main/lib/ipc/registerTelemetryHandlers.test.ts # src/main/lib/ipc/registerTelemetryHandlers.ts # src/main/lib/telemetry.test.ts # src/main/lib/telemetry.ts
- Abort now rejects the outer promise immediately, destroys the in-flight request, and suppresses pending retries, so a late response can't resolve after cancellation. - Snapshot-restore failures record a scrubbed, bounded error message via buildErrorFields instead of assuming an Error instance. Amp-Thread-ID: https://ampcode.com/threads/T-019f9e5f-73ce-745e-bd14-4337a5f52ebd Co-authored-by: Amp <amp@ampcode.com>
deepme987
reviewed
Jul 30, 2026
deepme987
left a comment
deepme987
left a comment
Collaborator
There was a problem hiding this comment.
Agent Review — focused on the privacy-critical part of this hardening: PII/secret scrubbing + size-bounding of error data before it leaves the box for PostHog/Datadog. It's well-designed; nothing blocks merge from my read.
What I verified:
- Scrubbing coverage (
piiScrub.ts) — home-dir usernames across Windows / macOS / Linux / WSL / UNC, plus the common credential shapes (OpenAIsk-, HFhf_, GitHub/GitLab/npm tokens, Bearer/Basic/Authorization, URL basic-auth, generickey=secret, URL-query secrets) and emails. Ordering is right:scrubSecretsbeforescrubPII, since URL creds can resemble emails. - Bounding + non-scalar drop —
normalizeExceptionContextcaps keys/items/string length and only admits scalars (+ scalar arrays), so a nested object can't smuggle unscrubbed data past the scrubber. (inline) - Final-sink safety net (the important one) —
captureExceptionscrubs+bounds the message, name, stack, and every context field viascrubAll/normalizeExceptionContextbefore the SDK sees them, and there's a scrub-every-string-property pass, so both PostHog and the Datadog-mirrored events are scrubbed regardless of whether the caller remembered to. Defense-in-depth (scrub at source and at sink). (inline)
Residuals (acceptable, honestly acknowledged as "best-effort"):
- Path patterns redact the username segment but keep the rest of the path, so a user who puts PII in a folder name below their home dir (e.g. a project folder named after themselves) would still leak that segment. Reasonable trade-off — the username is the clear identifier, and keeping the path structure preserves diagnostic value — just flagging the residual.
- Scrubbing is pattern-based/best-effort, so a novel token/PII shape can slip through; the 16 KB caps bound the blast radius. The module comment is honest about this.
Scope: I verified the scrubbing/bounding path (piiScrub.ts, the errorEvent.ts caps + class extraction, and the telemetry.ts sinks). I did not deep-read the diagnostics/robustness half — process.ts termination handling, executionTap.ts, launch.ts (365), updater.ts self-update detail, the migration/adoption dedup, or the ~600 lines of tests (2,826 pass per the PR).
| // Same default merge as capture() so exception events stay filterable by | ||
| // the shared axes (app_version, client, ...) instead of arriving bare. | ||
| const source = error instanceof Error ? error : new Error(String(error)) | ||
| const safeError = new Error(scrubAll(source.message || source.name).slice(0, ERROR_MESSAGE_MAX)) |
Collaborator
There was a problem hiding this comment.
This is the safety net that makes the privacy guarantee hold: even if a caller hands captureException an unscrubbed error, the message / name / stack and every context field are scrubAll'd and length-bounded here before the SDK (and the Datadog mirror) ever see them. Pairs with the scrub-at-source discipline as proper defense-in-depth — worth keeping this sink-side scrub unconditional in any refactor.
| export type SafeTelemetryValue = boolean | number | string | null | ||
|
|
||
| /** Normalize untrusted exception metadata before it reaches a telemetry SDK. */ | ||
| export function normalizeExceptionContext( |
Collaborator
There was a problem hiding this comment.
Nice that this only admits scalars (+ scalar arrays) and drops everything else — a nested object or function can't smuggle an unscrubbed string past the scrubber, and the key/item/length caps bound the payload. One thing to confirm: since only string values are scrubAll'd, make sure no PII ever rides in as a non-string that then gets stringified downstream (numbers/bools are fine; the risk would be if a caller pre-stringifies PII into a key name — keys are scrubbed here too, so that's covered).
…me port switching on conflict policy Amp-Thread-ID: https://ampcode.com/threads/T-019fb112-d404-7070-89a6-02b4f7372594 Co-authored-by: Amp <amp@ampcode.com>
Member
Author
|
Addressed the remaining outside-diff review findings in 41fe8ba:
Validation: full typecheck, lint, and test suites pass (224 files, 3683 tests). |
deepme987
approved these changes
Jul 30, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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
closeevents without hanging on inherited pipescheck,download, orapply_restart), running/target versions, provider, error source, stack, and Chinese-mirror cohortValidation
pnpm run typecheckpnpm run lintpnpm run buildpnpm run test(188 files, 2826 tests)Closes #1268
Closes #1269