feat(cli): add baseline /doctor memory diagnostics#4180
Conversation
📋 Review SummaryThis PR adds a new 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
|
Addressed the review feedback in
Verification: cd packages/cli
npx vitest run src/ui/commands/doctorCommand.test.ts src/utils/memoryDiagnostics.test.ts
npx eslint src/ui/commands/doctorCommand.ts src/ui/commands/doctorCommand.test.ts src/utils/memoryDiagnostics.ts src/utils/memoryDiagnostics.test.ts
npm run typecheck --workspace @qwen-code/qwen-codeAll passed locally. |
|
Started the next /doctor memory phase and pushed Added an
Validation: cd packages/cli
npx vitest run src/ui/commands/doctorCommand.test.ts src/utils/memoryDiagnostics.test.ts
npx eslint src/ui/commands/doctorCommand.ts src/ui/commands/doctorCommand.test.ts src/utils/memoryDiagnostics.ts src/utils/memoryDiagnostics.test.ts
npm run typecheck --workspace @qwen-code/qwen-codeResult: 13 tests passed, eslint passed, typecheck passed. |
|
Started the next /doctor memory phase and pushed Added opt-in heap snapshot capture:
Validation: cd packages/cli
npx vitest run src/ui/commands/doctorCommand.test.ts src/utils/memoryDiagnostics.test.ts
npx eslint src/ui/commands/doctorCommand.ts src/ui/commands/doctorCommand.test.ts src/utils/memoryDiagnostics.ts src/utils/memoryDiagnostics.test.ts
npm run typecheck --workspace @qwen-code/qwen-codeResult: 15 tests passed, eslint passed, typecheck passed. |
|
Next phase update: added opt-in memory pressure sampling for What changed:
Validation:
|
|
Snapshot hardening update for What changed:
Validation:
|
| now?: () => Date; | ||
| memoryUsage?: () => NodeJS.MemoryUsage; | ||
| wait?: (ms: number) => Promise<void>; | ||
| } |
There was a problem hiding this comment.
[Critical] v8.writeHeapSnapshot() is synchronous — blocks the event loop for seconds with zero progress feedback. On a process near its heap limit (the exact scenario /doctor memory is meant to diagnose), the snapshot itself can trigger OOM and crash the process.
Suggested fix: Show a progress message before the blocking call ("Writing heap snapshot, this may take a moment..."). In buildMemoryInsights, when heapPressure >= 0.85, warn the user or refuse the snapshot.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| arrayBuffers: number; | ||
| } | ||
|
|
||
| export interface CollectMemoryPressureSamplesOptions { |
There was a problem hiding this comment.
[Critical] No disk space protection for heap snapshots. Each call writes a full heap dump (potentially hundreds of MB), files accumulate indefinitely in ~/.qwen/memory-snapshots/ with no size estimation, rate limiting, or auto-cleanup. A malicious script or agent-mode prompt injection could fill the user's disk.
Suggested fix: Add rate limiting (max 1/min), auto-cleanup (keep last N snapshots), and warn before writing if estimated size exceeds a threshold.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| if (index < count) { | ||
| await wait(intervalMs); | ||
| } |
There was a problem hiding this comment.
[Critical] buildMemoryInsights ignores diagnostics.v8.unavailable. When v8.getHeapStatistics() throws, getMemoryDiagnostics sets v8.unavailable = true, but buildMemoryInsights downgrades it to an empty object via heapStatistics ?? {}, resulting in status: 'ok' with zero signals — while all V8 fields display "unavailable". The data says "broken", the assessment says "fine".
| } | |
| function buildMemoryInsights(diagnostics: MemoryDiagnostics): MemoryInsights { | |
| if (diagnostics.v8.unavailable) { | |
| return { | |
| status: 'warn', | |
| signals: ['V8 heap statistics are unavailable; pressure assessment skipped.'], | |
| recommendations: ['The Node.js V8 heap statistics API failed. Verify the Node.js version is supported.'], | |
| }; | |
| } | |
| const heapStatistics = diagnostics.v8.heapStatistics ?? {}; | |
| // ...existing logic... | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return; | ||
| } | ||
|
|
||
| const diagnostics = getMemoryDiagnostics(); |
There was a problem hiding this comment.
[Critical] Interactive mode success paths for --sample and --snapshot are completely untested. Only abort/error paths exist for interactive mode; success tests only cover non-interactive. The primary UI code path at lines 97-103 (context.ui.addItem) is not verified for either flag in interactive mode.
Suggested fix: Add interactive-mode success tests for --sample, --snapshot, and --sample --snapshot combo, asserting that context.ui.addItem is called with the expected report text.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| messageType = 'error'; | ||
| report = `${report}\n\nHeap snapshot failed: ${formatErrorMessage(error)}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] In interactive mode, messageType is set to 'error' on snapshot failure (line 87) but addItem hardcodes type: 'info' (line 99). A failed snapshot displays identically to a successful one — the error state is invisible visually.
| } | |
| context.ui.addItem( | |
| { | |
| type: messageType === 'error' ? 'error' : 'info', | |
| text: report, | |
| }, | |
| Date.now(), | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| export function formatMemoryPressureSamples( | ||
| samples: MemoryPressureSample[], | ||
| ): string { | ||
| const first = samples[0]; |
There was a problem hiding this comment.
[Suggestion] formatMemoryPressureSamples uses first && last to compute deltas, but this is truthy for a single-element array (both reference the same object). The result displays "0.0 MiB" instead of "unavailable", misleadingly implying memory changed by exactly zero.
| const first = samples[0]; | |
| const rssDelta = | |
| first && last && samples.length > 1 ? last.rss - first.rss : undefined; | |
| const heapUsedDelta = | |
| first && last && samples.length > 1 ? last.heapUsed - first.heapUsed : undefined; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ) { | ||
| // These process methods are undocumented Node.js internals. They provide | ||
| // useful diagnostic counts, but may change across Node.js major versions; if | ||
| // unavailable or unstable, report `unavailable` instead of failing /doctor. |
There was a problem hiding this comment.
[Suggestion] countProcessInternals returns { count: 0, unavailable: false } when the internal API returns a non-array value. This silently reports a precise-but-wrong count of 0 instead of honestly marking it as unavailable. Since these are undocumented Node.js internals (_ prefixed), their return format may change across versions.
| // unavailable or unstable, report `unavailable` instead of failing /doctor. | |
| const entries = (getter as () => unknown[])(); | |
| if (!Array.isArray(entries)) { | |
| return { count: 0, unavailable: true }; | |
| } | |
| return { count: entries.length, unavailable: false }; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| wait?: (ms: number) => Promise<void>; | ||
| } | ||
|
|
||
| function defaultHeapSnapshotDir(): string { |
There was a problem hiding this comment.
[Suggestion] Heap snapshot files are written with default OS permissions (directory: 0755, file: 0644). Heap snapshots contain a full serialization of all in-memory JavaScript objects — including API keys held in environment variables, conversation prompts, authentication tokens, and file contents previously read into memory. On shared machines (CI runners, multi-user dev laptops, SSH bastion hosts), any local user can read ~/.qwen/memory-snapshots/*.heapsnapshot.
| function defaultHeapSnapshotDir(): string { | |
| fs.mkdirSync(outputDir, { recursive: true, mode: 0o700 }); | |
| try { fs.chmodSync(outputDir, 0o700); } catch { /* already exists */ } |
And after writeSnapshot(filePath), tighten the file:
try { fs.chmodSync(result, 0o600); } catch { /* best effort */ }— glm-5.1 via Qwen Code /review
| } | ||
| } | ||
|
|
||
| if (abortSignal?.aborted) { |
There was a problem hiding this comment.
[Suggestion] After a successful heap snapshot write, this abort check can silently discard the entire report — including the sensitive-data warning. The .heapsnapshot file persists on disk containing prompts, file contents, and tool results, but the user receives no notification that it was created. This undermines the purpose of the warning.
Move the abort check before the snapshot write, or skip it when a snapshot file exists on disk so the report (with the sensitive-data warning) is always delivered:
| if (abortSignal?.aborted) { | |
| if (shouldWriteHeapSnapshot && !abortSignal?.aborted) { | |
| try { | |
| const heapSnapshotPath = writeMemoryHeapSnapshot(); | |
| report = `${report}\n\nHeap snapshot written: ${heapSnapshotPath}\n${HEAP_SNAPSHOT_SENSITIVE_DATA_WARNING}`; | |
| } catch (error) { | |
| messageType = 'error'; | |
| report = `${report}\n\nHeap snapshot failed: ${formatErrorMessage(error)}`; | |
| } | |
| } | |
| // Deliver report even if aborted — user may have a snapshot on disk. |
— glm-5.1 via Qwen Code /review
|
Addressed the latest review feedback in
Verification: cd packages/cli && npx vitest run src/ui/commands/doctorCommand.test.ts src/utils/memoryDiagnostics.test.ts
cd packages/cli && npx eslint src/ui/commands/doctorCommand.ts src/ui/commands/doctorCommand.test.ts src/utils/memoryDiagnostics.ts src/utils/memoryDiagnostics.test.ts
cd packages/cli && npm run typecheck --workspace @qwen-code/qwen-codeResult: 32 tests passed; eslint passed; typecheck passed. |
wenshao
left a comment
There was a problem hiding this comment.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
[Critical] hasHighHeapPressure(diagnostics) uses stale data when --sample and --snapshot are combined.
getMemoryDiagnostics() is called at line 86, then collectMemoryPressureSamples runs for ~3 seconds. If heap pressure crosses the 85% threshold during sampling, the pre-sample diagnostics won't reflect it, and the OOM safeguard is bypassed.
| try { | |
| if (hasHighHeapPressure(getMemoryDiagnostics())) { |
Re-fetch diagnostics after sampling completes so the check sees current heap state.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| if (abortSignal?.aborted && !report.includes('Heap snapshot written:')) { |
There was a problem hiding this comment.
[Critical] The abort gate !report.includes('Heap snapshot written:') silently discards completed --sample results when --snapshot is not also specified.
When a user runs /doctor memory --sample (no --snapshot), the 3-second sampling completes and formats its output into report. But because the report never contains the string "Heap snapshot written:", the abort check at line 135 always returns undefined, throwing away the valid sample data.
Replace the string-based gate with an explicit boolean flag:
| if (abortSignal?.aborted && !report.includes('Heap snapshot written:')) { | |
| if (abortSignal?.aborted && shouldWriteHeapSnapshot && !snapshotWritten) { | |
| return; | |
| } |
Set let snapshotWritten = false before the try block and snapshotWritten = true inside the success branch.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| fs.chmodSync(writtenPath, 0o600); | ||
| } catch { | ||
| // Best-effort hardening; the report warns that snapshots are sensitive. | ||
| } |
There was a problem hiding this comment.
[Critical] If cleanupOldHeapSnapshots throws (e.g., transient filesystem error), the exception propagates and doctorCommand reports "Heap snapshot failed" — but the snapshot file was already successfully written at line 225.
Oncall engineers would waste time re-running the command and investigating a nonexistent failure.
| } | |
| recordHeapSnapshotWrite(outputDir, now); | |
| try { | |
| cleanupOldHeapSnapshots(outputDir, maxSnapshots); | |
| } catch { | |
| // Best-effort cleanup; snapshot already written successfully. | |
| } | |
| return writtenPath; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (abortSignal?.aborted) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] setPendingItem('Writing heap snapshot...') is never cleared after the snapshot write completes (success or failure). The default doctor flow clears it in a finally block (lines 193-196), but the memory subcommand path lacks this cleanup.
Add context.ui.setPendingItem(null) after the try-catch block to prevent stale pending state in the UI.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| function hasHighHeapPressure( | ||
| diagnostics: ReturnType<typeof getMemoryDiagnostics>, | ||
| ): boolean { |
There was a problem hiding this comment.
[Suggestion] hasHighHeapPressure here and buildMemoryInsights in memoryDiagnostics.ts:322 independently compute the same heapUsed / heapSizeLimit >= 0.85 threshold. If the threshold is ever changed, the snapshot gate and the diagnostic report assessment would silently diverge.
Extract a shared isHighHeapPressure(diagnostics) function (or a constant) from memoryDiagnostics.ts so both call sites use a single source of truth.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| name.startsWith('qwen-code-heap-') && name.endsWith('.heapsnapshot'), | ||
| ) | ||
| .map((name) => path.join(outputDir, name)) | ||
| .sort((a, b) => b.localeCompare(a)); |
There was a problem hiding this comment.
[Suggestion] cleanupOldHeapSnapshots sorts snapshot files by full path using localeCompare. Filenames have format qwen-code-heap-{pid}-{timestamp}.heapsnapshot. PID "999" sorts after "12345" lexicographically, so newer snapshots from a low-numeric PID could be deleted while older snapshots from a high-numeric PID are retained.
Extract and compare only the timestamp portion (ISO-8601) for chronological ordering:
| .sort((a, b) => b.localeCompare(a)); | |
| .sort((a, b) => { | |
| const tsA = path.basename(a).match(/qwen-code-heap-\d+-(.+)\.heapsnapshot/)?.[1] ?? ''; | |
| const tsB = path.basename(b).match(/qwen-code-heap-\d+-(.+)\.heapsnapshot/)?.[1] ?? ''; | |
| return tsB.localeCompare(tsA); | |
| }) |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return DOCTOR_SUBCOMMANDS.filter((candidate) => | ||
| candidate.startsWith(trimmed), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
[Suggestion] The new completion() function is completely untested. As a public API that drives tab-completion behavior, it should have tests covering: empty partial arg → returns ['memory'], partial match 'mem' → ['memory'], no match 'x' → [].
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ) { | ||
| signals.push( | ||
| 'Active handle count is high; long-lived timers, sockets, or file watchers may be accumulating.', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The high active-handle-count signal path (activeHandles.count >= 1000) in buildMemoryInsights is untested. Add a test case with activeHandles: { count: 1500, unavailable: false } and assert the report includes both the "Active handle count is high" signal and the corresponding MCP/watcher recommendation.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| diagnostics.memory.heapUsed / heapSizeLimit >= 0.85 | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] hasHighHeapPressure edge cases are untested: heap_size_limit: 0, undefined, or heapStatistics entirely missing. Add parameterized tests to verify the function returns false (safe to snapshot) in these cases.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Test (windows-latest, Node 22.x). Static review found 1 Critical and 7 Suggestions below.
| function getAvailableBytes(outputDir: string): number { | ||
| const stats = fs.statfsSync(outputDir); | ||
| return stats.bavail * stats.bsize; | ||
| } |
There was a problem hiding this comment.
[Critical] getAvailableBytes calls fs.statfsSync(outputDir) without try/catch. On filesystems that don't support statfs (certain FUSE mounts, network filesystems), this throws and crashes the entire writeMemoryHeapSnapshot call — the user gets an unhandled error instead of a graceful fallback.
| } | |
| function getAvailableBytes(outputDir: string): number { | |
| try { | |
| const stats = fs.statfsSync(outputDir); | |
| return stats.bavail * stats.bsize; | |
| } catch { | |
| return Number.MAX_SAFE_INTEGER; // skip disk check on unsupported FS | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| for (const filePath of snapshots.slice(maxSnapshots)) { | ||
| fs.rmSync(filePath, { force: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] cleanupOldHeapSnapshots uses .sort((a, b) => b.localeCompare(a)) on filenames like qwen-code-heap-{PID}-{timestamp}.heapsnapshot. Variable-length PIDs cause lexicographic sort to order incorrectly across process runs (e.g., PID 9 vs PID 12345), so newer snapshots may be deleted while older ones are preserved.
| } | |
| .sort((a, b) => { | |
| const aStat = fs.statSync(a); | |
| const bStat = fs.statSync(b); | |
| return bStat.mtimeMs - aStat.mtimeMs; | |
| }); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| function hasHighHeapPressure( | ||
| diagnostics: ReturnType<typeof getMemoryDiagnostics>, | ||
| ): boolean { |
There was a problem hiding this comment.
[Suggestion] hasHighHeapPressure() independently computes heapUsed / heapSizeLimit >= 0.85, duplicating the identical logic in buildMemoryInsights() in memoryDiagnostics.ts. If the threshold changes in one place but not the other, snapshot gating and diagnostic reporting will diverge. Reuse buildMemoryInsights:
| ): boolean { | |
| function hasHighHeapPressure(diagnostics: MemoryDiagnostics): boolean { | |
| const insights = buildMemoryInsights(diagnostics); | |
| return insights.heapPressure !== undefined && insights.heapPressure >= 0.85; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| throw new Error( | ||
| 'Heap snapshot skipped: V8 heap pressure is already high, and writing a synchronous heap snapshot could make the process unresponsive or trigger OOM. Restart Qwen Code first if it is unstable, or retry before memory pressure reaches the warning threshold.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The abort guard uses report.includes('Heap snapshot written:') — control flow is coupled to a formatted message string. If the success message text changes, the guard silently breaks and successfully-written snapshot reports are discarded on abort. Use an explicit boolean flag instead:
| } | |
| let heapSnapshotWritten = false; | |
| // in try block after writeMemoryHeapSnapshot: heapSnapshotWritten = true; | |
| // ... | |
| if (abortSignal?.aborted && !heapSnapshotWritten) { | |
| return; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| .filter( | ||
| (name) => | ||
| name.startsWith('qwen-code-heap-') && name.endsWith('.heapsnapshot'), | ||
| ) |
There was a problem hiding this comment.
[Suggestion] cleanupOldHeapSnapshots calls readdirSync/rmSync without try/catch. If the directory is deleted between the successful snapshot write and cleanup, the exception kills the call and the user sees "Heap snapshot failed: ENOENT" — even though a valid .heapsnapshot file was already written to disk. Wrap cleanup in try/catch as best-effort.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const getter = (process as unknown as Record<string, unknown>)[name]; | ||
| if (typeof getter !== 'function') { | ||
| return { count: 0, unavailable: true }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] process._getActiveHandles() is called unconditionally in getMemoryDiagnostics(). On processes with thousands of handles (MCP servers, watchers), this synchronous call blocks the event loop for hundreds of ms to seconds, causing a noticeable UI freeze during /doctor memory. Consider making handle counting opt-in behind a --verbose flag, or at minimum log elapsed time when it exceeds ~100ms.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (shouldWriteHeapSnapshot) { | ||
| if (abortSignal?.aborted) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] In interactive mode, setPendingItem('Writing heap snapshot...') is called before the write but never cleared afterward. The "Writing..." message persists permanently in the UI alongside the final result. Add context.ui.setPendingItem(null) before addItem.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const MEMORY_SUBCOMMAND = 'memory'; | ||
| const DOCTOR_SUBCOMMANDS = [MEMORY_SUBCOMMAND] as const; | ||
| const HEAP_SNAPSHOT_SENSITIVE_DATA_WARNING = | ||
| 'Heap snapshot may contain prompts, file contents, tool results, and other sensitive data. Do not share it publicly without reviewing it first.'; |
There was a problem hiding this comment.
[Suggestion] HEAP_SNAPSHOT_SENSITIVE_DATA_WARNING and inline error messages in the action handler are hardcoded English strings, while other strings in the same file use t() for i18n. Wrap user-facing strings in t() for consistency with project i18n conventions.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| candidate.startsWith(trimmed), | ||
| ); | ||
| }, | ||
| action: async (context, args) => { |
There was a problem hiding this comment.
[Critical] completion function has zero test coverage. Tab-completion logic (/doctor m → memory) has no test exercising it. If completion breaks, no test will catch it.
| action: async (context, args) => { | |
| // Add test for completion function: | |
| // '' → ['memory'], 'm' → ['memory'], 'xyz' → [] |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| if ( | ||
| diagnostics.activeHandles.count >= 1000 && |
There was a problem hiding this comment.
[Critical] High active-handle-count signal path (activeHandles.count >= 1000 → 'Active handle count is high') is untested. This is a key resource-leak diagnostic. No test fixture passes a high count.
| diagnostics.activeHandles.count >= 1000 && | |
| // Add test with activeHandles: { count: 1000, unavailable: false } | |
| // Assert output includes 'Active handle count is high' |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| function estimateHeapSnapshotBytes(): number { | ||
| const heapStats = v8.getHeapStatistics(); | ||
| return Math.max(heapStats.total_heap_size, process.memoryUsage().heapTotal); |
There was a problem hiding this comment.
[Critical] estimateHeapSnapshotBytes uses a 1x multiplier — real V8 heap snapshots are typically 2–5× larger (per-object metadata, edge tables, string tables). The disk-space guard at line 213 can pass when the actual snapshot would fill remaining disk. Additionally, the v8.getHeapStatistics() call lacks try/catch, unlike getMemoryDiagnostics() which wraps the same call. If V8 throws, the error propagates as unhelpful 'Heap snapshot failed: <raw V8 error>'.
| return Math.max(heapStats.total_heap_size, process.memoryUsage().heapTotal); | |
| const OVERHEAD_FACTOR = 3; | |
| function estimateHeapSnapshotBytes(): number { | |
| try { | |
| const heapStats = v8.getHeapStatistics(); | |
| return Math.max(heapStats.total_heap_size, process.memoryUsage().heapTotal) * OVERHEAD_FACTOR; | |
| } catch { | |
| return process.memoryUsage().heapTotal * OVERHEAD_FACTOR; | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ); | ||
|
|
||
| const writtenPath = writeSnapshot(filePath); | ||
| try { |
There was a problem hiding this comment.
[Critical] When v8.writeHeapSnapshot(filePath) throws mid-write, a partial .heapsnapshot file remains on disk indefinitely. cleanupOldHeapSnapshots is only called on success (line 234) — failed writes skip cleanup. The corrupt partial file wastes disk space and occupies a maxSnapshots slot.
| try { | |
| let writtenPath: string; | |
| try { | |
| writtenPath = writeSnapshot(filePath); | |
| } catch (err) { | |
| try { fs.rmSync(filePath, { force: true }); } catch {} | |
| throw err; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| const lastHeapSnapshotWriteByDir = new Map<string, number>(); |
There was a problem hiding this comment.
[Critical] Module-level Map lastHeapSnapshotWriteByDir is only set()-ed, never delete()-d. In long-running processes this is a slow memory leak (one entry per unique directory). Also, stale rate-limit state persists across test runs with no cleanup export, breaking test isolation.
| const lastHeapSnapshotWriteByDir = new Map<string, number>(); | |
| export function clearHeapSnapshotRateLimit(): void { | |
| lastHeapSnapshotWriteByDir.clear(); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }; | ||
| } | ||
|
|
||
| export interface WriteMemoryHeapSnapshotOptions { |
There was a problem hiding this comment.
[Suggestion] The getMemoryDiagnostics catch branch (where v8.getHeapStatistics() throws, setting v8Unavailable = true) has zero test coverage. Only the happy path is tested.
| export interface WriteMemoryHeapSnapshotOptions { | |
| // Add test: mock v8.getHeapStatistics to throw, | |
| // assert v8.unavailable === true, heapStatistics === undefined |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| const writtenPath = writeSnapshot(filePath); | ||
| try { | ||
| fs.chmodSync(writtenPath, 0o600); |
There was a problem hiding this comment.
[Suggestion] cleanupOldHeapSnapshots is called after recordHeapSnapshotWrite without its own try/catch. If cleanup throws (e.g., readdirSync EACCES), the error propagates and masks the fact that the heap snapshot was successfully written and rate-limited. Users see 'Heap snapshot failed: <cleanup error>'.
| fs.chmodSync(writtenPath, 0o600); | |
| recordHeapSnapshotWrite(outputDir, now); | |
| try { | |
| cleanupOldHeapSnapshots(outputDir, maxSnapshots); | |
| } catch { | |
| // Best-effort cleanup; snapshot was already written successfully. | |
| } | |
| return writtenPath; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| const diagnostics = getMemoryDiagnostics(); | ||
|
|
There was a problem hiding this comment.
[Suggestion] When --sample --snapshot is combined, hasHighHeapPressure(diagnostics) uses the diagnostics captured before sampling. collectMemoryPressureSamples runs ~3s — heap usage can climb from safe (<85%) to dangerous (>85%) during this window, defeating the OOM-prevention guard.
| if (shouldWriteHeapSnapshot) { | |
| // Re-fetch diagnostics after sampling for current heap pressure | |
| const latestDiagnostics = getMemoryDiagnostics(); | |
| if (hasHighHeapPressure(latestDiagnostics)) { | |
| throw new Error('Heap snapshot skipped: heap pressure is high.'); | |
| } | |
| // ... write snapshot | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| let v8Unavailable = false; | ||
|
|
||
| try { | ||
| heapStatistics = v8.getHeapStatistics() as unknown as Record< |
There was a problem hiding this comment.
[Suggestion] getMemoryDiagnostics wraps both v8.getHeapStatistics() and v8.getHeapSpaceStatistics() in a single try/catch. If getHeapStatistics() succeeds but getHeapSpaceStatistics() throws, v8Unavailable is set to true while heapStatistics is populated. Downstream buildMemoryInsights reports 'V8 unavailable' while formatMemoryDiagnostics still displays the heap data — a contradictory report.
| heapStatistics = v8.getHeapStatistics() as unknown as Record< | |
| // Use separate try/catch for each V8 call, or track per-statistic availability | |
| let heapStatistics: v8.HeapStatistics | undefined; | |
| try { | |
| heapStatistics = v8.getHeapStatistics(); | |
| } catch { /* heapStatistics stays undefined */ } | |
| let heapSpaceStatistics: v8.HeapSpaceStatistics[] | undefined; | |
| try { | |
| heapSpaceStatistics = v8.getHeapSpaceStatistics(); | |
| } catch { /* heapSpaceStatistics stays undefined */ } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| heapUsed: number; | ||
| external: number; | ||
| arrayBuffers: number; | ||
| } |
There was a problem hiding this comment.
[Suggestion] collectMemoryPressureSamples does not accept or check an abortSignal. The sampling loop (for + await wait(intervalMs)) runs ~3s without any abort check. doctorCommand.ts checks abortSignal before and after but cannot interrupt during sampling.
| } | |
| export async function collectMemoryPressureSamples( | |
| options: CollectMemoryPressureSamplesOptions & { signal?: AbortSignal } = {} | |
| ): Promise<MemoryPressureSample[]> { | |
| // ... | |
| for (let i = 0; i < count; i++) { | |
| if (signal?.aborted) break; | |
| // ... sample | |
| if (i < count - 1) { | |
| await wait(intervalMs); | |
| } | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ) | ||
| .map((name) => path.join(outputDir, name)) | ||
| .sort((a, b) => { | ||
| const aStat = fs.statSync(a); |
There was a problem hiding this comment.
[Critical] fs.statSync follows symlinks — a broken symlink in ~/.qwen/memory-snapshots/ throws in the sort comparator, and the outer try/catch silently swallows it. From that point on, every future cleanup fails silently and snapshots accumulate without bound.
| const aStat = fs.statSync(a); | |
| .sort((a, b) => { | |
| const getMtime = (p: string) => { | |
| try { | |
| return fs.lstatSync(p).mtimeMs; | |
| } catch { | |
| return 0; | |
| } | |
| }; | |
| const mtimeDelta = getMtime(b) - getMtime(a); | |
| if (mtimeDelta !== 0) return mtimeDelta; | |
| return ( | |
| extractHeapSnapshotTimestamp(b).localeCompare( | |
| extractHeapSnapshotTimestamp(a), | |
| ) || path.basename(b).localeCompare(path.basename(a)) | |
| ); | |
| }) |
Also wrap individual fs.rmSync calls in the deletion loop so one stuck file doesn't block the rest.
— glm-5.1 via Qwen Code /review
| if (mtimeDelta !== 0) return mtimeDelta; | ||
|
|
||
| return ( | ||
| extractHeapSnapshotTimestamp(b).localeCompare( |
There was a problem hiding this comment.
[Suggestion] The new tiebreaker branch (extractHeapSnapshotTimestamp + basename fallback) is the only behavioral change in this PR, but the existing test uses different mtimes so mtimeDelta !== 0 always short-circuits. Add a test with two snapshots having identical mtime to exercise the new code path:
it('resolves equal-mtime ties by filename timestamp', () => {
const older = path.join(outputDir, 'qwen-code-heap-1-2026-05-15T11-00-00-000Z.heapsnapshot');
const newer = path.join(outputDir, 'qwen-code-heap-1-2026-05-15T12-00-00-000Z.heapsnapshot');
fs.writeFileSync(older, 'a');
fs.writeFileSync(newer, 'b');
const sameMtime = new Date('2026-05-15T12:00:00.000Z');
fs.utimesSync(older, sameMtime, sameMtime);
fs.utimesSync(newer, sameMtime, sameMtime);
writeMemoryHeapSnapshot({
outputDir, now: new Date('2026-05-15T13:00:00.000Z'), maxSnapshots: 2,
writeSnapshot: (p) => { fs.writeFileSync(p, 'new'); return p; },
});
expect(fs.existsSync(older)).toBe(false);
expect(fs.existsSync(newer)).toBe(true);
});— glm-5.1 via Qwen Code /review
|
|
||
| function extractHeapSnapshotTimestamp(filePath: string): string { | ||
| return ( | ||
| path |
There was a problem hiding this comment.
[Suggestion] The filename template at ~line 266 (qwen-code-heap-${pid}-${timestamp}.heapsnapshot) and this parsing regex are ~65 lines apart with no shared constant. A future change to the template (e.g., adding a session ID) would silently break sort semantics. Extract shared constants:
| path | |
| const SNAPSHOT_PREFIX = 'qwen-code-heap'; | |
| const SNAPSHOT_EXT = '.heapsnapshot'; | |
| // Filename construction: | |
| `${SNAPSHOT_PREFIX}-${process.pid}-${formatSnapshotTimestamp(now)}${SNAPSHOT_EXT}` | |
| // Parsing regex (derived from the same constants): | |
| const SNAPSHOT_RE = new RegExp(`^${SNAPSHOT_PREFIX}-\\d+-(.+)\\${SNAPSHOT_EXT}$`); |
— glm-5.1 via Qwen Code /review
| function getAvailableBytes(outputDir: string): number { | ||
| try { | ||
| const stats = fs.statfsSync(outputDir); | ||
| return stats.bavail * stats.bsize; |
There was a problem hiding this comment.
[Suggestion] fs.statfsSync is unavailable on Windows. The catch returns MAX_SAFE_INTEGER, making the disk-space guard a complete no-op on that platform. Consider adding a Windows-specific disk query, or at minimum log a warning so the gap is observable:
| return stats.bavail * stats.bsize; | |
| } catch { | |
| // statfsSync unavailable on this platform (e.g., Windows); | |
| // disk-space guard is inactive. | |
| return Number.MAX_SAFE_INTEGER; | |
| } |
— glm-5.1 via Qwen Code /review
| return t('Run installation and environment diagnostics'); | ||
| }, | ||
| kind: CommandKind.BUILT_IN, | ||
| supportedModes: ['interactive', 'non_interactive', 'acp'] as const, |
There was a problem hiding this comment.
[Critical] ACP mode allows external agents to trigger heap snapshot writes.
supportedModes includes 'acp', and the action handler has no mode-based restriction on --snapshot. An external MCP/ACP agent issuing /doctor memory --snapshot executes writeMemoryHeapSnapshot() unimpeded, dumping all process memory (including API keys, prompts, and file contents) to a predictable path on disk. The snapshot path is returned verbatim to the agent.
| supportedModes: ['interactive', 'non_interactive', 'acp'] as const, | |
| // In the action handler, before the --snapshot block: | |
| if (executionMode === 'acp' && shouldWriteHeapSnapshot) { | |
| return { | |
| type: 'message' as const, | |
| messageType: 'error', | |
| content: t('Heap snapshots are not available in agent (ACP) mode for security reasons.'), | |
| }; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| function getAvailableBytes(outputDir: string): number { |
There was a problem hiding this comment.
[Critical] getAvailableBytes silently bypasses disk-space protection when fs.statfsSync throws.
When fs.statfsSync fails (e.g., macOS where it's unavailable, permission errors, FUSE mounts), the catch block returns Number.MAX_SAFE_INTEGER, making the disk-space guard availableBytes - estimatedSnapshotBytes < minFreeBytesAfterSnapshot always evaluate to false. The protection is silently disabled with no log, no warning, and no user-visible signal.
| function getAvailableBytes(outputDir: string): number { | |
| function getAvailableBytes(outputDir: string): number | undefined { | |
| try { | |
| const stats = fs.statfsSync(outputDir); | |
| return stats.bavail * stats.bsize; | |
| } catch { | |
| return undefined; | |
| } | |
| } | |
| // At call site: | |
| const availableBytes = getAvailableBytes(outputDir); | |
| if (availableBytes === undefined) { | |
| throw new Error(t('Cannot determine available disk space for heap snapshot. The filesystem may not support this operation.')); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| if (shouldSampleMemory) { | ||
| const samples = await collectMemoryPressureSamples({ |
There was a problem hiding this comment.
[Suggestion] Missing try/catch around collectMemoryPressureSamples await.
If collectMemoryPressureSamples throws (unlikely in practice, but the default wait/memoryUsage injectors could throw in edge cases), the report variable already containing formatMemoryDiagnostics(diagnostics) with RSS, heap usage, active handles, and process metadata is lost. The user sees only a raw error message instead of the base diagnostics that were successfully collected.
| const samples = await collectMemoryPressureSamples({ | |
| let samples: MemoryPressureSample[] | undefined; | |
| try { | |
| samples = await collectMemoryPressureSamples({ | |
| sampleCount: 3, | |
| intervalMs: 1000, | |
| signal: abortSignal, | |
| }); | |
| } catch (err) { | |
| messageType = 'error'; | |
| report = `${report}\n\n${t('Memory pressure sampling failed:')} ${formatErrorMessage(err)}`; | |
| } | |
| if (samples) { | |
| report = `${report}\n\n${formatMemoryPressureSamples(samples)}`; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| '/doctor memory --sample', | ||
| '/doctor memory --snapshot', | ||
| ], | ||
| completion: async (_context, partialArg) => { |
There was a problem hiding this comment.
[Suggestion] trimStart() doesn't handle trailing whitespace, causing a completion dead-end.
When a user types /doctor memory (with trailing space after the full subcommand), partialArg.trimStart() produces 'memory ' with trailing space intact. The filter candidate.startsWith('memory ') returns [] — no completions are offered, and the user cannot discover --sample or --snapshot flags via Tab.
| completion: async (_context, partialArg) => { | |
| const trimmed = partialArg.trim(); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return error instanceof Error ? error.message : String(error); | ||
| } | ||
|
|
||
| function formatHeapSnapshotErrorMessage(error: unknown): string { |
There was a problem hiding this comment.
[Suggestion] formatHeapSnapshotErrorMessage uses string prefix matching (message.startsWith('Heap snapshot')) that is coupled to another module's error message wording.
Errors from memoryDiagnostics.ts like 'Insufficient free disk space for heap snapshot' don't start with 'Heap snapshot', producing redundant output: "Heap snapshot failed: Insufficient free disk space for heap snapshot". If error messages in memoryDiagnostics.ts are reworded, this logic silently breaks. Consider using a structured error type instead.
| function formatHeapSnapshotErrorMessage(error: unknown): string { | |
| // Use a custom error class or discriminated result rather than string prefix matching: | |
| class HeapSnapshotError extends Error { | |
| constructor(message: string) { | |
| super(message); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| const estimatedSnapshotBytes = estimateSnapshotBytesOption(); | ||
| const availableBytes = getAvailableBytesOption(outputDir); | ||
| if (availableBytes - estimatedSnapshotBytes < minFreeBytesAfterSnapshot) { |
There was a problem hiding this comment.
[Suggestion] fs.chmodSync catch-all silently swallows EACCES/EPERM without differentiation from benign failures.
Both fs.chmodSync(outputDir, 0o700) and fs.chmodSync(writtenPath, 0o600) use empty catch {} blocks. "Best-effort hardening" is the stated intent, but EACCES/EPERM (real permission errors the user should know about) are silenced identically to ENOSYS (filesystem doesn't support chmod). The user is told permissions are "private" when they may not be.
| if (availableBytes - estimatedSnapshotBytes < minFreeBytesAfterSnapshot) { | |
| // Log or warn on EACCES/EPERM, silently ignore ENOSYS/ENOTSUP | |
| try { | |
| fs.chmodSync(outputDir, 0o700); | |
| } catch (err: any) { | |
| if (err?.code === 'EACCES' || err?.code === 'EPERM') { | |
| // Re-throw or return a warning — permissions are not as expected | |
| throw err; | |
| } | |
| // ENOSYS, ENOTSUP — best-effort, silently skip | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ...(heapSpaceLines.length > 0 ? heapSpaceLines : [' - unavailable']), | ||
| '', | ||
| 'Runtime internals', | ||
| ` Active handles: ${formatActiveCount(diagnostics.activeHandles)}`, |
There was a problem hiding this comment.
[Suggestion] Zero heap_size_limit renders as misleading "0.0 MiB".
When V8 reports heap_size_limit: 0 ("no explicit limit"), formatBytes renders it as "0.0 MiB", implying the heap has zero capacity — which is false and alarming. getHeapPressure already correctly handles this by returning undefined, but the raw display bypasses that guard.
| ` Active handles: ${formatActiveCount(diagnostics.activeHandles)}`, | |
| const heapSizeLimit = asFiniteNumber(heapStatistics['heap_size_limit']); | |
| const displayLimit = (heapSizeLimit !== undefined && heapSizeLimit > 0) | |
| ? formatBytes(heapSizeLimit) | |
| : 'unavailable'; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| report = `${report}\n\n${formatMemoryPressureSamples(samples)}`; | ||
| } | ||
|
|
||
| if (shouldWriteHeapSnapshot) { |
There was a problem hiding this comment.
[Suggestion] ACP mode output format is inconsistent between /doctor and /doctor memory.
The original /doctor returns structured JSON via JSON.stringify({ checks, summary }). The new /doctor memory subcommand returns human-readable plain text in the content field of the same { type: 'message' } envelope. ACP consumers that parse /doctor output as JSON will silently break when receiving the plain-text memory report.
| if (shouldWriteHeapSnapshot) { | |
| // Wrap in JSON for consistency with /doctor: | |
| return { | |
| type: 'message' as const, | |
| messageType, | |
| content: JSON.stringify({ type: 'memory', report, heapSnapshotWritten }), | |
| }; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| try { | ||
| fs.rmSync(filePath, { force: true }); | ||
| } catch { | ||
| // Best-effort cleanup for partial snapshots after a failed write. |
There was a problem hiding this comment.
[Suggestion] Heap snapshot filename is fully predictable: qwen-code-heap-{process.pid}-{timestamp}.heapsnapshot.
PID and timestamp are easily obtainable. A local attacker who gains access to the machine can find snapshots in ~/.qwen/memory-snapshots/ without discovery effort — just wait for or trigger a /doctor memory --snapshot. Combined with best-effort chmod, this makes information gathering trivial.
| // Best-effort cleanup for partial snapshots after a failed write. | |
| // Add a random component to the filename: | |
| const snapshotId = crypto.randomUUID().slice(0, 8); | |
| const filePath = path.join( | |
| outputDir, | |
| `qwen-code-heap-${process.pid}-${formatSnapshotTimestamp(now)}-${snapshotId}.heapsnapshot`, | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| now = () => new Date(), | ||
| memoryUsage = process.memoryUsage, | ||
| wait = defaultWait, | ||
| }: CollectMemoryPressureSamplesOptions = {}): Promise<MemoryPressureSample[]> { |
There was a problem hiding this comment.
[Suggestion] Invalid sampleCount values (negative, zero) are silently normalized to 1, inconsistent with NaN/Infinity treatment.
normalizeSampleCount uses Math.max(1, Math.floor(sampleCount)), silently converting sampleCount: 0 or sampleCount: -5 to 1 sample. But NaN and Infinity are normalized to the default of 3. A caller passing sampleCount: 0 (intending "no sampling") gets 1 sample with misleading results instead of an error or the default.
| }: CollectMemoryPressureSamplesOptions = {}): Promise<MemoryPressureSample[]> { | |
| function normalizeSampleCount(sampleCount: number): number { | |
| if (!Number.isFinite(sampleCount) || sampleCount <= 0) { | |
| return 3; // default | |
| } | |
| return Math.floor(sampleCount); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
One untested coverage gap: getAvailableBytes catch fallback (returns MAX_SAFE_INTEGER) and estimateHeapSnapshotBytes catch fallback (returns heapTotal * 3) have zero test coverage — no test exercises statfsSync throwing or v8.getHeapStatistics() throwing within these functions.
| } | ||
| } | ||
|
|
||
| function getAvailableBytes(outputDir: string): number { |
There was a problem hiding this comment.
[Critical] getAvailableBytes silently bypasses disk-space protection when fs.statfsSync throws.
When fs.statfsSync(outputDir) throws (non-standard filesystems, FUSE mounts, permission issues), the function returns Number.MAX_SAFE_INTEGER instead of propagating the error. The disk-space guard (availableBytes - estimatedSnapshotBytes < minFreeBytesAfterSnapshot) becomes mathematically impossible to trigger — the protection is completely bypassed just when it's needed most.
The old code (before this PR) let statfsSync throw naturally (fail-closed). The new try/catch converts it to fail-open. Disk-space protection should fail-closed: if you can't verify available space, skip the snapshot rather than silently writing to a potentially full disk.
| function getAvailableBytes(outputDir: string): number { | |
| function getAvailableBytes(outputDir: string): number { | |
| try { | |
| const stats = fs.statfsSync(outputDir); | |
| return stats.bavail * stats.bsize; | |
| } catch (error) { | |
| // If we can't verify available space, skip the snapshot to be safe. | |
| throw new Error( | |
| `Unable to check available disk space: ${formatErrorMessage(error)}`, | |
| ); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }); | ||
|
|
||
| if (abortSignal?.aborted) { | ||
| return; |
There was a problem hiding this comment.
[Critical] Abort after --sample silently discards the already-generated diagnostics report.
When a user runs /doctor memory --sample, getMemoryDiagnostics() and formatMemoryDiagnostics() have already completed successfully. After collectMemoryPressureSamples() finishes (which may have taken ~3s), the abort check at this line returns with zero output — the fully-computed report is thrown away.
The user waited for sampling and pressed Ctrl+C, but gets absolutely nothing back. The abort should return the partial results that were already computed, not discard everything.
| return; | |
| if (abortSignal?.aborted) { | |
| // Return the diagnostics we already computed, even if sampling was interrupted. | |
| const result = shouldSampleMemory | |
| ? `${report}\n\n${formatMemoryPressureSamples(samples)}` | |
| : report; | |
| return { | |
| type: 'doctor', | |
| content: result, | |
| messageType: 'info', | |
| }; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| function estimateHeapSnapshotBytes(): number { | ||
| const overheadFactor = 3; | ||
| try { |
There was a problem hiding this comment.
[Suggestion] Undocumented overheadFactor = 3 magic number in estimateHeapSnapshotBytes.
The 3x multiplier used to estimate heap snapshot size has no comment explaining its origin. Without documentation, future maintainers may reduce it (thinking it's overly conservative) and cause real OOMs in disk-constrained environments. Add a brief comment explaining the heuristic.
| try { | |
| // Conservative 3x estimate: V8 heap snapshots include per-object metadata, | |
| // edge tables, and string tables, typically 1.5–3× the live heap size. | |
| const overheadFactor = 3; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| function formatHeapSnapshotErrorMessage(error: unknown): string { | ||
| const message = formatErrorMessage(error); |
There was a problem hiding this comment.
[Suggestion] formatHeapSnapshotErrorMessage uses hardcoded English startsWith('Heap snapshot') check that will break when i18n translations are added.
The check message.startsWith('Heap snapshot') assumes error messages always begin with English text. But throw new Error(t('Heap snapshot skipped: ...')) wraps the message through t(). If a non-English translation is ever added, the startsWith check fails, and the function prepends a translated t('Heap snapshot failed:') prefix — producing a double-prefixed message like "[translated failed:] [translated skipped: ...]".
Consider using a custom error type, an error code property, or checking against the untranslated key instead of the translated message.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| function defaultHeapSnapshotDir(): string { | ||
| return path.join(os.homedir(), '.qwen', 'memory-snapshots'); | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] os.homedir() may throw without a fallback in defaultHeapSnapshotDir().
os.homedir() throws ERR_INVALID_ARG_VALUE when HOME is unset (headless Docker containers, restricted CI environments). The error propagates from writeMemoryHeapSnapshot()'s default parameter evaluation, and the user sees a confusing "Heap snapshot failed: ..." with no hint about the missing home directory.
The codebase has precedent for handling this: packages/core/src/core/prompts.ts wraps os.homedir() in try/catch with a fallback. Consider using os.homedir() || os.tmpdir() or wrapping in try/catch with a fallback to path.resolve('.qwen', 'memory-snapshots').
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| function normalizeSampleCount(sampleCount: number): number { | ||
| if (!Number.isFinite(sampleCount)) { | ||
| return 3; | ||
| } |
There was a problem hiding this comment.
[Suggestion] collectMemoryPressureSamples does not respond to abort during await wait(intervalMs).
The abort signal is only checked at the start of each loop iteration. If abort arrives during await wait(intervalMs) (default 1000ms), the function doesn't respond until wait completes — up to 1s of unresponsiveness. While minor relative to the ~3s total sampling duration, it violates abort semantics (operations should stop promptly).
Consider using an abort-aware wait wrapper that races setTimeout against the signal's abort event, or reducing the sampling interval.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No new high-confidence issues found. All previously identified Critical issues have been addressed — the code has improved significantly since the last review. 5 low-confidence findings noted for author consideration (TOCTOU window on snapshot file permissions, untested error branches in countProcessInternals, and minor duplication). Tests pass (44/44), CI green. LGTM! ✅ — DeepSeek/deepseek-v4-pro via Qwen Code /review
Pre-merge Verification Report ✅Date: 2026-05-16 | Tester: maintainer Build & Quality
tmux Real-User TUI Testing
Key Observations
Artifacts
Ready to merge. |
wenshao
left a comment
There was a problem hiding this comment.
LGTM. All tests pass (44/44 unit + full tmux real-user TUI matrix), typecheck/lint clean, heap snapshot permission safety and rate-limiting verified.
Summary
/doctor memoryas the first lightweight memory diagnostic slice.Test Plan
npm test --workspace=packages/cli -- --run src/utils/memoryDiagnostics.test.ts src/ui/commands/doctorCommand.test.tsnpm run typecheck --workspace=packages/clicurrently fails on pre-existing unrelated package/core API drift; nomemoryDiagnosticsordoctorCommanderrors remain.Part of #3000
Closes #4179