feat(cli): add structured memory diagnostics JSON#3785
Conversation
|
@copilot pls review for me |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Here's the review summary: Overall: The PR is well-structured with good test coverage, clean interfaces, and safe degradation when Linux-only probes are unavailable. Two bugs were fixed (commit
A new test was added to assert that Minor observations (not changed — low-impact in a diagnostics context):
All 3 core tests and 9 CLI tests pass; typecheck is clean. |
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] formatMemoryUsage is duplicated identically in packages/core/src/utils/formatters.ts and packages/cli/src/ui/utils/formatters.ts. If someone fixes a bug or adds a tier (e.g., TB) in one copy, the other silently diverges. Consider having the CLI's formatters.ts re-export from @qwen-code/qwen-code-core instead of re-declaring an identical function.
— pai/glm-5.1 via Qwen Code /review
Code ReviewOverviewAdds What's good
Issues / suggestions1. ACP UX: parent 2. Vacuous assertion in expect(result?.type === 'message' ? result.content : '').not.toContain('0.00 MiB');
3. Duplicate 4. Heap-pressure threshold may be noisy for legitimately heavy workloads. 5. 6. Minor: 7. Coverage gap: heap-spaces rendering. Risk assessment
Suggested actions before merge
Overall: ship-worthy as a first diagnostics layer for #3000. The test discipline is unusually good for a feature this size. |
|
Updated this PR after re-reviewing the memory diagnostics feedback. Changes since the previous draft:
Local validation:
I kept the parent |
|
Updated again after syncing with the latest What changed in this round:
Local validation after the merge:
GitHub now reports the PR as mergeable; CI has started on the new head and I am not waiting on the long OS matrix here. |
wenshao
left a comment
There was a problem hiding this comment.
Cross-file concerns (files not in this diff):
/doctor memory --json warnings exit with code 0 in nonInteractiveCli.ts:321 — The PR widens messageType to include 'warning' in nonInteractiveCliCommands.ts, but nonInteractiveCli.ts:321 only treats messageType === 'error' as failure (isError: false, exit 0). CI pipelines running health checks will miss memory warnings. The handler at :321 and :326 should also treat 'warning' as a non-zero exit.
Session.ts:2249 does not distinguish warning from info — ACP #processSlashCommandResult renders both warning and info identically through emitAgentMessage. ACP clients (Zed) cannot visually differentiate memory risk warnings.
wenshao
left a comment
There was a problem hiding this comment.
The warning messageType was added to NonInteractiveSlashCommandResult, but the exit-code logic in nonInteractiveCli.ts still only checks messageType === 'error'. /doctor memory --json returns messageType: 'warning' when risks are detected, yet the non-interactive CLI exits with code 0. CI scripts that rely solely on exit codes will miss memory warnings — they must parse the JSON output to discover risks.
Test coverage gaps: heapSizeLimit === 0 guard in analyzeMemoryDiagnostics is not tested. Interactive-mode /doctor memory --json path is not tested.
— gpt-5.5 via Qwen Code /review
yiliang114
left a comment
There was a problem hiding this comment.
Addressed the remaining review feedback:
- Removed the incorrect
maxRSS * 1024multiplier on Linux (Node.js >=14.10 returns bytes on all platforms) - Switched
heapRatioto usev8HeapStats.usedHeapSizeinstead ofmemoryUsage.heapUsedfor internal consistency - Updated test expectations accordingly
a50c9c2 to
146d165
Compare
Verification Report — PR #3785
Environment
Test Matrix
Build Pipeline
E2E ScenariosTest 1:
|
|
@yiliang114 这个 PR 3 个 Test 平台全失败了（run 25968245065）。失败不是 tsc 类型错，是 i18n 严格 parity 检查没过：
修复新增 slash 命令时同步加翻译到这两个 locale 的命令描述。看一下别的
本地复现命令： cd packages/cli && npx vitest run src/i18n/mustTranslateKeys.test.ts补完翻译后这条会从 |
wenshao
left a comment
There was a problem hiding this comment.
Code Review: /doctor memory --json in ACP tools
Summary
PR adds a dedicated /doctor memory --json path that exposes machine-readable
memory diagnostics through ACP tools. The implementation moves the core
MemoryDiagnostics type and collectMemoryDiagnostics() from CLI into packages/core,
adds /doctor memory --json as a direct handler in doctorCommand.ts (bypassing the
historical 2-message ACP stream), wires /doctor memory --json into
list_directory and read_file tool failures, and covers the change with
CLI + Core unit tests and two integration tests.
Positive findings:
- Core module (
packages/core/src/utils/memoryDiagnostics.test.ts) tests are
comprehensive: positive path, error/missing data, and boundary conditions (539 lines) - Error handling with graceful degradation across all probes
- Rate limiting and disk space checks for heap snapshots
- Abort signal support throughout the async path
Findings
[non-blocking] 12 new t() calls reference 8 keys absent from ALL locale files
doctorCommand.ts introduces 8 new t() keys that are missing from every
locale file (en.js, zh.js, zh-TW.js, ca.js, de.js, fr.js, ja.js, pt.js, ru.js):
Show current process memory diagnosticsUnknown argument(s)UsageFailed to collect memory diagnosticsunavailableavailablenoneMemory Diagnostics
The t() function falls back to the key itself, so English users see the raw
key text -- which happens to be English, masking the gap. But zh and zh-TW
users also see English text for these strings.
Additionally, doctorChecks.ts adds t('system') (line 170) which is also
absent from all locale files.
Recommendation: Add all 9 keys to en.js (as identity mappings) and to at
least zh.js/zh-TW.js with proper translations. The other locale files can
be left for native speakers to fill in.
[non-blocking] Dual diagnostic paths -- parent action vs subcommand action
doctorCommand.action (the parent) handles /doctor memory inline using the
CLI-local getMemoryDiagnostics() + formatMemoryDiagnostics() (structured
CLI output). The memory subcommand's own action (memoryDoctorAction) uses
the core collectMemoryDiagnostics() + formatCoreDiagnostics() (plain-text
key-value output).
Both paths are exercised by tests, but they produce different output formats
for the same logical operation:
| Path | Source | Format |
|---|---|---|
Parent action (doctorCommand.action) |
CLI-local getMemoryDiagnostics() |
Structured object with formatMemoryDiagnostics() |
Subcommand action (memoryDoctorAction) |
Core collectMemoryDiagnostics() |
Plain-text key-value via formatCoreDiagnostics() |
If the command dispatcher ever routes /doctor memory to the subcommand's
action directly (rather than through the parent), the output format and data
source would differ silently.
Recommendation: Consider making the parent action delegate to the
subcommand's action for the memory case, or document the routing contract
explicitly to prevent future drift.
[non-blocking] memoryDoctorAction does not handle --sample or --snapshot
The subcommand's action only recognizes --json:
const subSubCommand = args?.trim() ?? '';
const isJsonMode = subSubCommand === '--json';If the command dispatcher routes /doctor memory --sample or
/doctor memory --snapshot to the subcommand's action, it returns an "Unknown
argument" error. The --sample and --snapshot paths are only handled by the
parent action.
Recommendation: Either add --sample/--snapshot handling to the
subcommand's action, or add an explicit guard comment explaining that these
flags are parent-action-only.
[non-blocking] acceptsInput inference bypasses subcommand hint
Session.ts's acceptsInput inference checks cmd.argumentHint and
cmd.subCommands, so doctorCommand (which has both) correctly gets
acceptsInput = true. However, the memory subcommand also has
argumentHint: '[--json]' -- the inference logic only runs on top-level
commands passed to buildAvailableCommands, so subcommand acceptsInput is
never evaluated. This is fine for the current ACP protocol but could cause
confusion if subcommands are ever exposed directly.
Recommendation: No action needed now; just noting the implicit contract.
[nit] formatCoreDiagnostics risks/recommendation indentation inconsistency
Risks are indented with - (2-space indent + dash), but the recommendation
line has no indentation prefix:
risks:
- heap-pressure: Heap pressure detected.
recommendation: WARNING: 1 potential leak indicator(s) found.
The recommendation key appears at the same level as risks:, which is
correct for the key-value format, but the visual gap between the indented
risk items and the unindented recommendation may confuse parsers expecting
consistent indentation.
[nit] formatMemoryUsage uses mixed units
The function uses decimal MB for values >= 1 MiB:
return `${(bytes / (1024 * 1024)).toFixed(1)} MB`;But uses binary KiB for values < 1 MiB:
return `${(bytes / 1024).toFixed(1)} KiB`;This mixes decimal (MB) and binary (KiB) units in the same output. The CLI's
formatBytes function consistently uses binary MiB. Consider using a single
unit system (binary MiB/KiB or decimal MB/KB) throughout.
Suggested fix (in packages/core/src/utils/formatters.ts):
export const formatMemoryUsage = (bytes: number): string => {
if (bytes < 1024) {
return `${bytes} B`;
}
if (bytes < 1024 * 1024) {
return `${(bytes / 1024).toFixed(1)} KiB`;
}
return `${(bytes / (1024 * 1024)).toFixed(1)} MiB`;
};[nit] doctorChecks.ts -- normalizeConfigDir does not expand ~ on Linux
The function handles macOS's /Users/name prefix but not Linux's /home/name:
if (process.platform === 'darwin' && !configDir.startsWith('/Users/')) {
configDir = path.join(os.homedir(), configDir);
}On Linux, a relative configDir (e.g., '.config/qwen-code') would not be
expanded. The existing code has the same gap, but since this PR modifies the
file, it is worth noting.
[nit] checkGoogleApplicationCredentials receives raw (non-normalized) configDir
checkAuthentication normalizes configDir via normalizeConfigDir() but then
passes the original (non-normalized) configDir to checkGoogleApplicationCredentials:
const configDir = config.getConfigDir();
const normalizedDir = normalizeConfigDir(configDir);
// ...
const googleAppCredsResult = await checkGoogleApplicationCredentials(configDir);This is fine because checkGoogleApplicationCredentials only uses it for the
config.json existence check, but it is a subtle inconsistency worth harmonizing.
[scope] Suggested improvement -- expose --sample and --snapshot through ACP
The PR description mentions exposing /doctor memory --json through ACP tools.
The --sample (memory pressure over time) and --snapshot (heap dump) features
are powerful diagnostics that would also benefit ACP tool consumers. Consider
following up with a PR that makes these available through the ACP protocol.
Test Coverage Notes
Well-covered areas:
- Core
collectMemoryDiagnostics: positive path, error/missing data, boundary
conditions (539 lines) - CLI
doctorCommand: interactive + non-interactive modes, abort signals,
--json,--sample,--snapshot, error handling - Integration tests: headless
--json, tmux readable output, stdout isolation
Gaps:
- No standalone test for
formatCoreDiagnostics(tested indirectly through
doctorCommand.action) - No test for the
acceptsInputinference inSession.tswith the new
acceptsInputfield - No test for
normalizeConfigDirwith Linux paths
wenshao
left a comment
There was a problem hiding this comment.
Missing i18n keys: Several t() calls in the new memoryDoctorAction + formatters use keys not registered in locale files: 'Usage', 'Unknown argument(s)', 'Failed to collect memory diagnostics', 'Memory Diagnostics', 'unavailable', 'available'. While t() falls back to the key itself (works for English), non-English locales will see untranslated English text. Please add translations to en.js, zh.js, and zh-TW.js.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
Build & Test: All passing (tsc ✅, eslint ✅, 180/180 tests ✅, i18n parity ✅)
Verdict: Request changes — 2 Critical findings must be fixed before merge.
| # | Severity | Issue | Location |
|---|---|---|---|
| 1 | Critical | maxRSS unit mismatch: code assumes bytes, Node returns KB (1024× under-report) | memoryDiagnostics.ts:134 |
| 2 | Critical | warning messageType exits 0 in non-interactive CLI; CI cannot detect memory risks |
nonInteractiveCli.ts:326 |
| 3 | Suggestion | MemoryResourceUsage fields lack JSDoc unit docs (microseconds for CPU, KB for maxRSS) |
memoryDiagnostics.ts:56-63 |
| 4 | Suggestion | --json silently dropped when combined with --sample/--snapshot |
doctorCommand.ts:266 |
| 5 | Suggestion | No timeout on optionalProbe; NFS/FUSE /proc can cause indefinite hang |
memoryDiagnostics.ts:257 |
| 6 | Suggestion | Parent /doctor action: --snapshot/--sample flags parsed but unreachable outside memory subcommand |
doctorCommand.ts:89 |
| 7 | Suggestion | Arena dialog callbacks + ACP stream_messages lack warning branch; risk warnings silently swallowed |
ArenaStopDialog.tsx:43, Session.ts:2285 |
| 8 | Suggestion | countOpenFileDescriptors allocates full string array via readdir; opendir async iterator is more memory-efficient |
memoryDiagnostics.ts:226 |
| 9 | Suggestion | formatSmapsRollup can use regex instead of split+map+find |
doctorCommand.ts:339 |
Finding 1 — Critical: maxRSS unit mismatch
Empirically verified on this machine (Node v22.22.2, macOS ARM64):
process.resourceUsage().maxRSS = 40816 (kilobytes)
process.memoryUsage().rss = 41844736 (bytes ≈ 39.9 MB)
maxRSS * 1024 / rss = 0.998 (matches!)
maxRSS / rss = 0.00098 (off by 1024×)
The code comment at line 132 states "Node.js >=14.10.0 returns maxRSS in bytes on all platforms" — this contradicts the official Node.js documentation which states maxRSS returns kilobytes on Linux/macOS. The result: formatMemoryUsage(maxRSSBytes) displays 39.9 KB instead of the correct 39.9 MB.
Fix: const maxRSSBytes = resourceUsage.maxRSS * 1024; (and update the comment).
Note: Commit 146d165a00 ("correct maxRSS byte handling") removed a * 1024 multiplier, which appears to have introduced this regression.
Finding 2 — Critical: warning exits 0 in non-interactive CLI
nonInteractiveCli.ts:326:
return slashCommandResult.messageType === 'error' ? 1 : 0;When /doctor memory detects risks, it returns messageType: 'warning'. In non-interactive mode, this maps to exit code 0 — indistinguishable from a clean run. CI pipelines that check exit codes will silently pass.
Fix: Add a warning branch:
if (slashCommandResult.messageType === 'error') return 1;
if (slashCommandResult.messageType === 'warning') return 2;
return 0;Note: This finding was partially flagged in a prior review round as "warning exit code logic not updated." The current code still has the issue.
Finding 3 — Missing JSDoc units on MemoryResourceUsage
memoryDiagnostics.ts:56-63 — the interface fields have no unit documentation:
export interface MemoryResourceUsage {
maxRSS: number; // KB (per Node.js docs)
userCPUTime: number; // microseconds (per Node.js docs)
systemCPUTime: number; // microseconds
}Future consumers (tests, integrations, Bun compatibility) will silently get values wrong by orders of magnitude.
Finding 4 — --json silently dropped with --sample/--snapshot
doctorCommand.ts:266 — when shouldSampleMemory || shouldWriteHeapSnapshot, the code strips --json and delegates to the parent action, which returns plain text. No error or warning is emitted. Automated scripts expecting JSON will receive unparseable text.
Fix: Reject the combination explicitly:
if (tokens.includes('--json') && (shouldSampleMemory || shouldWriteHeapSnapshot)) {
return { type: 'message', messageType: 'error', content: t('--json is not supported with --sample or --snapshot') };
}Finding 5 — No timeout on optionalProbe
memoryDiagnostics.ts:257 — await probe() has no timeout. On NFS-mounted /proc or hung FUSE filesystems, readdir('/proc/self/fd') or readFile('/proc/self/smaps_rollup') can hang indefinitely.
Fix: Add a Promise.race with a 5-second timeout inside optionalProbe.
Finding 6 — Parent /doctor dead code
doctorCommand.ts:89-103 — shouldSampleMemory and shouldWriteHeapSnapshot are parsed at the parent level, but the snapshot/sample execution block is inside the if (subCommand === MEMORY_SUBCOMMAND) branch. When a user types /doctor --snapshot (without memory), subCommand is '--snapshot', which doesn't match, so the flags are silently ignored.
Finding 7 — Warning handling missing in Arena + ACP paths
ArenaStopDialog.tsx:43andArenaSelectDialog.tsx:42type callbacks as'info' | 'error'—warningmaps toMessageType.ERROR.Session.ts:2285(ACPstream_messages): onlyerrortriggersthrow;warningis emitted as a plainagentMessagewith no severity signal.
Finding 8 — countOpenFileDescriptors allocates full string array
memoryDiagnostics.ts:226 — readdir('/proc/self/fd') returns Promise<string[]>, allocating a string per fd just to read .length. On a process with fd leaks (the exact scenario this diagnostic targets), this could be thousands of strings. Using fs.opendir with an async iterator avoids the allocation.
Finding 9 — formatSmapsRollup optimization
doctorCommand.ts:339 — the function splits all lines, maps each to a trimmed string, then finds the Rss: line. A regex smapsRollup.match(/^Rss:\s*.+$/m) would be simpler and avoid intermediate allocations.
Previously flagged items now resolved: i18n missing keys ✅, formatError duplication ✅, probe failure logging ✅, getActiveHandlesCount/getActiveRequestsCount deduplication ✅.
Open items from prior review still not addressed: 7 inline comments from @wenshao remain open (see individual comment threads).
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
Build & Test: All passing (tsc ✅, eslint ✅, 180/180 tests ✅, i18n parity ✅)
Verdict: Request changes — 2 Critical findings must be fixed before merge.
| # | Severity | Issue | Location |
|---|---|---|---|
| 1 | Critical | maxRSS unit mismatch: code assumes bytes, Node returns KB (1024× under-report) | memoryDiagnostics.ts:134 |
| 2 | Critical | warning messageType exits 0 in non-interactive CLI; CI cannot detect memory risks |
nonInteractiveCli.ts:326 |
| 3 | Suggestion | MemoryResourceUsage fields lack JSDoc unit docs (microseconds for CPU, KB for maxRSS) |
memoryDiagnostics.ts:56-63 |
| 4 | Suggestion | --json silently dropped when combined with --sample/--snapshot |
doctorCommand.ts:266 |
| 5 | Suggestion | No timeout on optionalProbe; NFS/FUSE /proc can cause indefinite hang |
memoryDiagnostics.ts:257 |
| 6 | Suggestion | Parent /doctor action: --snapshot/--sample flags parsed but unreachable outside memory subcommand |
doctorCommand.ts:89 |
| 7 | Suggestion | Arena dialog callbacks + ACP stream_messages lack warning branch; risk warnings silently swallowed |
ArenaStopDialog.tsx:43, Session.ts:2285 |
| 8 | Suggestion | countOpenFileDescriptors allocates full string array via readdir; opendir async iterator is more memory-efficient |
memoryDiagnostics.ts:226 |
| 9 | Suggestion | formatSmapsRollup can use regex instead of split+map+find |
doctorCommand.ts:339 |
Finding 1 — Critical: maxRSS unit mismatch
Empirically verified on this machine (Node v22.22.2, macOS ARM64):
process.resourceUsage().maxRSS = 40816 (kilobytes)
process.memoryUsage().rss = 41844736 (bytes ≈ 39.9 MB)
maxRSS * 1024 / rss = 0.998 (matches!)
maxRSS / rss = 0.00098 (off by 1024×)
The code comment at line 132 states "Node.js >=14.10.0 returns maxRSS in bytes on all platforms" — this contradicts the official Node.js documentation which states maxRSS returns kilobytes on Linux/macOS. The result: formatMemoryUsage(maxRSSBytes) displays 39.9 KB instead of the correct 39.9 MB.
Fix: const maxRSSBytes = resourceUsage.maxRSS * 1024; (and update the comment).
Note: Commit 146d165a00 ("correct maxRSS byte handling") removed a * 1024 multiplier, which appears to have introduced this regression.
Finding 2 — Critical: warning exits 0 in non-interactive CLI
nonInteractiveCli.ts:326:
return slashCommandResult.messageType === 'error' ? 1 : 0;When /doctor memory detects risks, it returns messageType: 'warning'. In non-interactive mode, this maps to exit code 0 — indistinguishable from a clean run. CI pipelines that check exit codes will silently pass.
Fix: Add a warning branch:
if (slashCommandResult.messageType === 'error') return 1;
if (slashCommandResult.messageType === 'warning') return 2;
return 0;Note: This finding was partially flagged in a prior review round as "warning exit code logic not updated." The current code still has the issue.
Finding 3 — Missing JSDoc units on MemoryResourceUsage
memoryDiagnostics.ts:56-63 — the interface fields have no unit documentation:
export interface MemoryResourceUsage {
maxRSS: number; // KB (per Node.js docs)
userCPUTime: number; // microseconds (per Node.js docs)
systemCPUTime: number; // microseconds
}Future consumers (tests, integrations, Bun compatibility) will silently get values wrong by orders of magnitude.
Finding 4 — --json silently dropped with --sample/--snapshot
doctorCommand.ts:266 — when shouldSampleMemory || shouldWriteHeapSnapshot, the code strips --json and delegates to the parent action, which returns plain text. No error or warning is emitted. Automated scripts expecting JSON will receive unparseable text.
Fix: Reject the combination explicitly:
if (tokens.includes('--json') && (shouldSampleMemory || shouldWriteHeapSnapshot)) {
return { type: 'message', messageType: 'error', content: t('--json is not supported with --sample or --snapshot') };
}Finding 5 — No timeout on optionalProbe
memoryDiagnostics.ts:257 — await probe() has no timeout. On NFS-mounted /proc or hung FUSE filesystems, readdir('/proc/self/fd') or readFile('/proc/self/smaps_rollup') can hang indefinitely.
Fix: Add a Promise.race with a 5-second timeout inside optionalProbe.
Finding 6 — Parent /doctor dead code
doctorCommand.ts:89-103 — shouldSampleMemory and shouldWriteHeapSnapshot are parsed at the parent level, but the snapshot/sample execution block is inside the if (subCommand === MEMORY_SUBCOMMAND) branch. When a user types /doctor --snapshot (without memory), subCommand is '--snapshot', which doesn't match, so the flags are silently ignored.
Finding 7 — Warning handling missing in Arena + ACP paths
ArenaStopDialog.tsx:43andArenaSelectDialog.tsx:42type callbacks as'info' | 'error'—warningmaps toMessageType.ERROR.Session.ts:2285(ACPstream_messages): onlyerrortriggersthrow;warningis emitted as a plainagentMessagewith no severity signal.
Finding 8 — countOpenFileDescriptors allocates full string array
memoryDiagnostics.ts:226 — readdir('/proc/self/fd') returns Promise<string[]>, allocating a string per fd just to read .length. On a process with fd leaks (the exact scenario this diagnostic targets), this could be thousands of strings. Using fs.opendir with an async iterator avoids the allocation.
Finding 9 — formatSmapsRollup optimization
doctorCommand.ts:339 — the function splits all lines, maps each to a trimmed string, then finds the Rss: line. A regex smapsRollup.match(/^Rss:\s*.+$/m) would be simpler and avoid intermediate allocations.
Previously flagged items now resolved: i18n missing keys ✅, formatError duplication ✅, probe failure logging ✅, getActiveHandlesCount/getActiveRequestsCount deduplication ✅.
Open items from prior review still not addressed: 7 inline comments from @wenshao remain open (see individual comment threads).
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
Build & Test: All passing (tsc ✅, eslint ✅, 180/180 tests ✅, i18n parity ✅)
Verdict: Request changes — 2 Critical findings must be fixed before merge.
| # | Severity | Issue | Location |
|---|---|---|---|
| 1 | Critical | maxRSS unit mismatch: code assumes bytes, Node returns KB (1024× under-report) | memoryDiagnostics.ts:134 |
| 2 | Critical | warning messageType exits 0 in non-interactive CLI; CI cannot detect memory risks |
nonInteractiveCli.ts:326 |
| 3 | Suggestion | MemoryResourceUsage fields lack JSDoc unit docs (microseconds for CPU, KB for maxRSS) |
memoryDiagnostics.ts:56-63 |
| 4 | Suggestion | --json silently dropped when combined with --sample/--snapshot |
doctorCommand.ts:266 |
| 5 | Suggestion | No timeout on optionalProbe; NFS/FUSE /proc can cause indefinite hang |
memoryDiagnostics.ts:257 |
| 6 | Suggestion | Parent /doctor action: --snapshot/--sample flags parsed but unreachable outside memory subcommand |
doctorCommand.ts:89 |
| 7 | Suggestion | Arena dialog callbacks + ACP stream_messages lack warning branch; risk warnings silently swallowed |
ArenaStopDialog.tsx:43, Session.ts:2285 |
| 8 | Suggestion | countOpenFileDescriptors allocates full string array via readdir; opendir async iterator is more memory-efficient |
memoryDiagnostics.ts:226 |
| 9 | Suggestion | formatSmapsRollup can use regex instead of split+map+find |
doctorCommand.ts:339 |
Finding 1 — Critical: maxRSS unit mismatch
Empirically verified on this machine (Node v22.22.2, macOS ARM64):
process.resourceUsage().maxRSS = 40816 (kilobytes)
process.memoryUsage().rss = 41844736 (bytes ≈ 39.9 MB)
maxRSS * 1024 / rss = 0.998 (matches!)
maxRSS / rss = 0.00098 (off by 1024×)
The code comment at line 132 states "Node.js >=14.10.0 returns maxRSS in bytes on all platforms" — this contradicts the official Node.js documentation which states maxRSS returns kilobytes on Linux/macOS. The result: formatMemoryUsage(maxRSSBytes) displays 39.9 KB instead of the correct 39.9 MB.
Fix: const maxRSSBytes = resourceUsage.maxRSS * 1024; (and update the comment).
Note: Commit 146d165a00 ("correct maxRSS byte handling") removed a * 1024 multiplier, which appears to have introduced this regression.
Finding 2 — Critical: warning exits 0 in non-interactive CLI
nonInteractiveCli.ts:326:
return slashCommandResult.messageType === 'error' ? 1 : 0;When /doctor memory detects risks, it returns messageType: 'warning'. In non-interactive mode, this maps to exit code 0 — indistinguishable from a clean run. CI pipelines that check exit codes will silently pass.
Fix: Add a warning branch:
if (slashCommandResult.messageType === 'error') return 1;
if (slashCommandResult.messageType === 'warning') return 2;
return 0;Note: This finding was partially flagged in a prior review round as "warning exit code logic not updated." The current code still has the issue.
Finding 3 — Missing JSDoc units on MemoryResourceUsage
memoryDiagnostics.ts:56-63 — the interface fields have no unit documentation:
export interface MemoryResourceUsage {
maxRSS: number; // KB (per Node.js docs)
userCPUTime: number; // microseconds (per Node.js docs)
systemCPUTime: number; // microseconds
}Future consumers (tests, integrations, Bun compatibility) will silently get values wrong by orders of magnitude.
Finding 4 — --json silently dropped with --sample/--snapshot
doctorCommand.ts:266 — when shouldSampleMemory || shouldWriteHeapSnapshot, the code strips --json and delegates to the parent action, which returns plain text. No error or warning is emitted. Automated scripts expecting JSON will receive unparseable text.
Fix: Reject the combination explicitly:
if (tokens.includes('--json') && (shouldSampleMemory || shouldWriteHeapSnapshot)) {
return { type: 'message', messageType: 'error', content: t('--json is not supported with --sample or --snapshot') };
}Finding 5 — No timeout on optionalProbe
memoryDiagnostics.ts:257 — await probe() has no timeout. On NFS-mounted /proc or hung FUSE filesystems, readdir('/proc/self/fd') or readFile('/proc/self/smaps_rollup') can hang indefinitely.
Fix: Add a Promise.race with a 5-second timeout inside optionalProbe.
Finding 6 — Parent /doctor dead code
doctorCommand.ts:89-103 — shouldSampleMemory and shouldWriteHeapSnapshot are parsed at the parent level, but the snapshot/sample execution block is inside the if (subCommand === MEMORY_SUBCOMMAND) branch. When a user types /doctor --snapshot (without memory), subCommand is '--snapshot', which doesn't match, so the flags are silently ignored.
Finding 7 — Warning handling missing in Arena + ACP paths
ArenaStopDialog.tsx:43andArenaSelectDialog.tsx:42type callbacks as'info' | 'error'—warningmaps toMessageType.ERROR.Session.ts:2285(ACPstream_messages): onlyerrortriggersthrow;warningis emitted as a plainagentMessagewith no severity signal.
Finding 8 — countOpenFileDescriptors allocates full string array
memoryDiagnostics.ts:226 — readdir('/proc/self/fd') returns Promise<string[]>, allocating a string per fd just to read .length. On a process with fd leaks (the exact scenario this diagnostic targets), this could be thousands of strings. Using fs.opendir with an async iterator avoids the allocation.
Finding 9 — formatSmapsRollup optimization
doctorCommand.ts:339 — the function splits all lines, maps each to a trimmed string, then finds the Rss: line. A regex smapsRollup.match(/^Rss:\s*.+$/m) would be simpler and avoid intermediate allocations.
Previously flagged items now resolved: i18n missing keys ✅, formatError duplication ✅, probe failure logging ✅, getActiveHandlesCount/getActiveRequestsCount deduplication ✅.
Open items from prior review still not addressed: 7 inline comments from @wenshao remain open (see individual comment threads).
- Extract platform detection before building diagnostics so the correct unit conversion can be applied: multiply by 1024 on Linux (where process.resourceUsage().maxRSS is in KB) but leave the value unchanged on macOS/Windows (where it is already in bytes). - Correct the native-memory-pressure risk message to accurately state that the threshold is 2× heap used, not just "larger than heapUsed". - Add a dedicated test to assert that maxRSS is not multiplied on a non-Linux platform (darwin). All 3 core and 9 CLI tests pass; typecheck clean. Agent-Logs-Url: https://github.com/QwenLM/qwen-code/sessions/9b413337-68ed-4d5c-af99-0d42378900c3
…output - Add 64MB absolute floor on native-memory-pressure so cold processes don't trip the 2x ratio check; raise active-handles threshold from 100 to 256 - Show detachedContexts, nativeContexts, maxRSS, CPU times, smapsRollup availability, and v8HeapSpaces summary in the readable /doctor memory output - Validate unknown memory subcommand args with a usage hint instead of silently dropping them - Wrap human-readable strings in t(...) for i18n parity with the rest of doctor - Advertise the memory subcommand via /doctor argumentHint while keeping acceptsInput false so the parent still auto-submits - Document _getActiveHandles/_getActiveRequests as undocumented Node internals - Update tests for new thresholds, expanded output, unknown-arg path, and abort-during-json
- Remove incorrect * 1024 multiplier for maxRSS on Linux (Node.js >=14.10 returns bytes on all platforms) - Use v8HeapStats.usedHeapSize for heapRatio to avoid cross-API inconsistency - Update test expectations and rename "does not multiply" test
- Rename local formatMemoryDiagnostics to formatCoreDiagnostics to avoid naming conflict with the imported utility from memoryDiagnostics.js - Update Session.test.ts to use objectContaining for _meta field added in recent main commits - Align doctorCommand.test.ts assertions with current parent command state (argumentHint includes --sample/--snapshot from main)
…cate active count helpers - optionalProbe/optionalSyncProbe now return null on failure so JSON.stringify preserves the keys instead of silently omitting them. - Merge getActiveHandlesCount/getActiveRequestsCount into a single parameterized getProcessInternalCount helper. - Update MemoryDiagnostics interface: v8HeapSpaces, openFileDescriptors, smapsRollup are now T | null instead of T | undefined.
55c4fad to
b3921c2
Compare
wenshao
left a comment
There was a problem hiding this comment.
Suggestion: Dual memory diagnostics code paths with different thresholds
The PR introduces a new collectMemoryDiagnostics() path (core layer, heap-pressure threshold 0.75) but leaves the existing /doctor memory --sample / --snapshot path using the old getMemoryDiagnostics() (cli layer, threshold 0.85). The two paths produce different data shapes, use different risk thresholds, and the old path never returns 'warning' message type. Over time these paths will diverge — changes to the core diagnostics won't be reflected in --sample/--snapshot output.
Consider migrating --sample/--snapshot to use collectMemoryDiagnostics() as the unified data source in a follow-up PR.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (shouldSampleMemory || shouldWriteHeapSnapshot) { | ||
| return doctorCommand.action?.( | ||
| context, | ||
| [MEMORY_SUBCOMMAND, ...tokens.filter((token) => token !== '--json')].join( |
There was a problem hiding this comment.
[Suggestion] --json is silently dropped when combined with --sample/--snapshot
memoryDoctorAction filters out --json before delegating to the parent doctorCommand.action, which never returns JSON. The user receives text output with no error. And argumentHint lists [--json] [--sample] [--snapshot], implying they can be combined.
| [MEMORY_SUBCOMMAND, ...tokens.filter((token) => token !== '--json')].join( | |
| if (shouldSampleMemory || shouldWriteHeapSnapshot) { | |
| if (tokens.includes('--json')) { | |
| return { | |
| type: 'message' as const, | |
| messageType: 'error' as const, | |
| content: `${t('--json is not compatible with --sample or --snapshot')}. ${t('Usage')}: ${MEMORY_USAGE_HINT}`, | |
| }; | |
| } | |
| return doctorCommand.action?.( | |
| context, | |
| [MEMORY_SUBCOMMAND, ...tokens].join(' '), | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Summary
What changed:
collectMemoryDiagnostics()in core for structured Node/V8/process memory snapshots with session id, CLI version, resource usage, optional Linux/procprobes, and heuristic risk analysis./doctor memory --jsonto return the structured diagnostics while preserving the latestmainbehavior for readable/doctor memory,--sample, and--snapshotoutput.warningthrough the slash-command message pipeline.Why it changed:
mainnow owns the human-readable/doctor memorypath; this PR focuses on structured JSON output and warning-aware handling.Reviewer focus:
/doctor memory --jsonexposes the right fields for issue/debug attachment./procis unavailable.Validation
Commands run:
Expected result:
/doctor memorykeeps the latest main readable diagnostics behavior./doctor memory --sampleand/doctor memory --snapshotkeep the latest main behavior./doctor memory --jsonreturns structured JSON with session and version metadata.analysis.risksand render as warning messages rather than command failures.Observed result:
npm run lint: passed.npm run build: passed.npm run typecheck: passed.npm run bundle: passed.git diff --cached --check: passed.Scope / Risk
Main risk or tradeoff:
Not covered / not validated:
mainand is not changed here.Breaking changes / migration notes:
Linked Issues / Bugs
Progress on #3000