📋 Review Summary

This PR adds a well-structured debugging skill for diagnosing memory leaks using heap snapshots and the chrome-devtools CLI. The documentation is clear, the workflow is logical, and the included example demonstrates real-world applicability (the ink 7 PerformanceMeasure leak). The shell script is simple and effective. Overall, this is a valuable addition to the debugging toolkit.

🔍 General Feedback

• Strong documentation structure: The step-by-step workflow is clear, with appropriate prerequisites, notes, and cleanup instructions
• Real-world validation: The worked example from the ink 7 upgrade provides credibility and a template for future investigations
• Good integration: Leverages existing tmux-real-user-testing infrastructure appropriately
• Practical focus: Emphasizes retained-size analysis over shallow-size, which is the correct approach for memory leak diagnosis
• Platform limitation acknowledged: macOS-only testing is clearly stated, though Linux/Windows paths are marked as untested

🎯 Specific Feedback

🔵 Low

• SKILL.md:line 23 — The eval pattern with export statements is correct, but the note about "shell environment does not persist across separate tool calls" seems specific to Qwen Code's agent context. Consider clarifying this is about tool call boundaries, not tmux pane boundaries.

• SKILL.md:line 38 — The find-leaf-node.sh script walks the process tree to find the innermost node child. Consider adding a brief explanation of why this is necessary (e.g., "npm spawns a process chain; we need the actual CLI process for USR2 signals").

• SKILL.md:line 45 — For production bundle profiling, you mention NODE_PID=$(tmux list-panes -t "<session-name>" -F '#{pane_pid}'). This assumes the pane PID is the node process. Consider adding a verification step or note that this works because node dist/cli.js is the direct command (no npm/tsx intermediary).

• SKILL.md:line 80 — The get_memory_snapshot_details output format is described as CSV with specific columns. Consider noting that column order may vary by chrome-devtools version, or suggest using --format=csv explicitly if available.

• SKILL.md:line 92 — The retainer chain command uses <nodeId> from the id field. Consider clarifying the distinction between uid (class aggregate identifier) and id (instance identifier) earlier, perhaps in a glossary or inline note.

• find-leaf-node.sh:line 1 — The shebang uses #!/usr/bin/env bash, which is correct for portability. However, the script is marked executable (mode 100755), which is good. Consider adding a brief comment about the algorithm: "Walks down the process tree until no more node children exist."

• find-leaf-node.sh:line 11 — The || true pattern prevents exit on failure, which is correct here. Consider adding a comment explaining this is intentional (we expect pgrep to return non-zero when no children exist).

• react-reconciler-performance-measure-leak.md:line 4 — The symptom description mentions "300+ MB" heap growth. Consider adding the timeframe (e.g., "over ~25 minutes of moderate usage") to set expectations for leak detection sensitivity.

• react-reconciler-performance-measure-leak.md:line 38 — The fix shows the esbuild define config. Consider linking to the actual commit dbdc94be9 as a reference, or noting that this is already applied (since the example is retrospective).

🟢 Medium

• SKILL.md:line 17 — The prerequisite mentions installing chrome-devtools-mcp@latest -g. Consider adding a version constraint or minimum version if the memory tools require specific chrome-devtools features (e.g., --experimentalMemory flag availability).

• SKILL.md:line 56 — The snapshot naming pattern Heap.<timestamp>.<pid>.<seq>.heapsnapshot is mentioned. Consider suggesting a renaming or organization strategy for multi-session investigations (e.g., "Consider prefixing with session name for clarity").

• SKILL.md:line 100 — The "Common patterns" section lists four leak types. Consider adding a fifth: "Circular references preventing GC" — while less common in modern V8, it still occurs with certain native bindings or weak reference patterns.

• react-reconciler-performance-measure-leak.md:line 26 — The retainer chain output description mentions (object elements) → Array. Consider showing the actual chrome-devtools output format for clarity (e.g., a truncated example of the JSON or text output).

🟡 High

• SKILL.md:line 23-25 — The eval pattern captures SESSION and OUTDIR, but the subsequent find-leaf-node.sh call requires <session-name> as a literal string replacement. This is a potential footgun: users might forget to substitute the actual session name. Consider adding an explicit example:

  # After eval, use the actual session name:
  NODE_PID=$(bash .qwen/skills/memory-leak-debug/scripts/find-leaf-node.sh "$SESSION")

• SKILL.md:line 62-65 — The snapshot workflow shows three snapshots but doesn't explicitly state the expected outcome. Consider adding: "Compare maxRetainedSize for each class across snapshots — leaking classes will show monotonic growth."

✅ Highlights

• Excellent worked example: The PerformanceMeasure leak case study is thorough, showing symptom → diagnosis → root cause → fix → verification. This is exactly the kind of documentation that helps future investigators.

• Clear prerequisite chain: The skill explicitly states Node.js 22+ requirement for --heapsnapshot-signal, and provides installation instructions for chrome-devtools.

• Smart use of tmux: The tmux-based approach allows interactive debugging while capturing snapshots — much more practical than automated scripting for exploratory leak hunting.

• Retainer chain emphasis: The skill correctly prioritizes retained-size analysis and retainer-chain tracing, which are the key techniques for finding root causes (not just symptoms).

• Verification step included: Step 6 (Verify Fix) closes the loop, ensuring the skill supports the full debugging lifecycle, not just diagnosis.

• find-leaf-node.sh is elegant: The process-tree-walking algorithm is simple, robust, and handles arbitrary nesting depth. The || true pattern is correctly used to handle the "no children" termination case.

It is intentionally written as <session-name>, since shell variable does not suvive tool call boundary. All agents I know have this limitation.