fix(cleanup): remove the temp directory of a scan that stops early

[kevin9327]

Problem

When a scan of a Git URL, file URL, .zip or single file is stopped after its input has been materialized, the temp directory is left in %TEMP% / /tmp. Stopping a scan can mean Ctrl+C during a long LLM scan, a cancelled MCP tool call, or an exception in a later node. A clone can be up to INGEST_MAX_BYTES (100 MiB), and each interrupted scan leaves its own directory.

Reproduction on main (8831219), Windows 11. I made the report step raise to stand in for the interrupt, then listed the skillspector_* directories created in the temp dir during the scan:

$ python repro.py skills/demo.zip terminal   # progress-bar path (interactive terminal)
leftover temp dirs: ['C:\\Users\\...\\AppData\\Local\\Temp\\skillspector_5najz7zm']
$ python repro.py skills/demo.zip pipe       # graph.invoke path (--verbose, pipes, CI)
leftover temp dirs: ['C:\\Users\\...\\AppData\\Local\\Temp\\skillspector_ap48m9e3']

With this change both lines print leftover temp dirs: [].

Cause

resolve_input creates the temp directory and reports it in the graph state as temp_dir_for_cleanup. The callers remove it with cleanup_result(result): scan() in cli.py (through _run_graph_scan) and run_scan() in mcp_server.py. That only works if the graph returns a result. When graph.invoke / graph.stream / graph.ainvoke raises, result is still None and nothing removes the directory.

resolve_input itself already cleans up when resolution fails, including on BaseException, with the comment "an interrupt or a cancellation that lands after the temp directory was allocated must clean up too". This PR covers the same case for the rest of the run.

Fix

• skillspector.cleanup.TempDirTracker, a small BaseCallbackHandler. Its on_chain_end records temp_dir_for_cleanup from a node output as soon as resolve_input finishes. It also has remove() and removing_on_error(), a context manager that removes the recorded directory on any BaseException and re-raises. The codebase already collects inference usage through a BaseCallbackHandler in the same way.
• cli._run_graph_scan passes the tracker in the run's callbacks. It wraps graph.invoke in removing_on_error(), and adds the same context manager to the with block of the progress-streaming path. Every CLI graph run goes through this function: single-skill, recursive children and transitive children.
• mcp_server.run_scan passes the tracker to graph.ainvoke. In the existing finally, it calls tracker.remove() when no result came back.

A scan that finishes is cleaned up exactly as before, through cleanup_result(result). The exception is always re-raised unchanged, so exit codes and error messages do not change.

Out of scope: skillspector baseline calls graph.invoke(state) directly and has the same gap. I left it alone because #643 and #657 are changing that function right now; it can take the same two lines once they land.

I used a callback rather than switching the non-interactive path to graph.stream. Many tests stub graph.invoke / graph.ainvoke directly, and a callback works with those stubs unchanged.

Tests

End-to-end with the real graph. A zipped skill is scanned, its skillspector_* temp directories are created under tmp_path, and nodes.report._format_json raises after input resolution:

• tests/unit/test_cli.py::test_scan_that_stops_early_removes_its_temp_dir: KeyboardInterrupt and RuntimeError, each with and without the progress stream (4 cases).
• tests/unit/test_mcp_server.py::test_run_scan_that_fails_removes_its_temp_dir: a RuntimeError during the scan.
• tests/unit/test_mcp_server.py::test_run_scan_that_is_cancelled_removes_its_temp_dir: the report step blocks, the test cancels the run_scan task and expects CancelledError.

All six fail on main with assert not True (the directory still exists) and pass with this change.

Suite

Windows 11, Python 3.12:

• tests/unit/test_cli.py tests/unit/test_mcp_server.py tests/unit/test_cleanup.py: 1 failed, 279 passed, 5 skipped. The failure is test_discovered_files_tree_renders_untrusted_names_literally, which also fails on main on Windows.
• pytest -m "not integration and not provider" -p no:randomly tests/unit tests/opencode tests/nodes tests/test_batch_scan_reports.py tests/test_multi_skill.py tests/test_structured_skill.py tests/test_models.py: 16 failed, 7882 passed, 43 skipped, 4 xfailed, 10 errors. These are the Windows-only failures and errors that main has too: /dev/null as the AWS config file, the gh and compare_scan_accuracy harnesses, a mocked os.scandir that Windows rmtree walks differently, a CRLF byte count, the Rich rendering test above, and test IDs over the Windows environment-variable length limit.

ruff check src/ tests/: All checks passed. ruff format --check src/ tests/: all files already formatted. mypy on cleanup.py, cli.py and mcp_server.py reports the same errors in those files before and after.

🤖 Generated with Claude Code

[rng1995]

[SkillSpector Review]

Hi @kevin9327, thank you for closing the temp-directory leak on interrupted and failed scans, and for the end-to-end tests that create real skillspector_* directories!

Value and readiness: The leak is real. A Git URL or archive scan that raises or is cancelled after resolve_input leaves up to INGEST_MAX_BYTES in the temp dir each time, and the MCP server is a long-lived process where these add up. Recording the directory with a callback and removing it on BaseException works for both graph.invoke and graph.stream in the CLI and for graph.ainvoke in MCP. The cleanup itself is safe. One CLI early-exit path is still not covered: in a --transitive scan, the root's temp directory still leaks if the scan stops after the root graph returns. That path is in the PR's stated scope and needs a small fix before this is ready.

Material findings

1. [Blocker] src/skillspector/cli.py:2409 (_scan_skill → _scan_transitive): the tracker only guards the graph run. Once the root graph returns, its temp_dir_for_cleanup lives only in the local result. scan() removes it only when _scan_skill returns (finally: if result is not None: cleanup_result(result), cli.py:762-764). Failing input: skillspector scan <git-url-or-skill.zip> --transitive whose SKILL.md references an external skill. Press Ctrl+C while a transitive child is scanning, which can be the longest part of the run. The child's tracker removes the child's directory, but KeyboardInterrupt is not caught by except Exception at cli.py:2189. It propagates out of _scan_transitive and _scan_skill before anything returns, so the root clone or extraction stays in /tmp. The same happens if report(...) or finalize_ledger(...) raises in the merge step (cli.py:1842, 2313, 2317). This is the "Ctrl+C during a long LLM scan" case from the PR body. Expected fix: in _scan_skill, remove the root result's directory when the transitive phase raises, for example:

   try:
       return _scan_transitive(initial_result=result, ...)
   except BaseException:
       cleanup_result(result)
       raise

   On success the returned report_result carries the same temp_dir_for_cleanup, so nothing is removed twice. Please add a test that interrupts a transitive child scan of a zipped root and asserts the root skillspector_* directory is gone.
2. [Non-blocking] src/skillspector/cleanup.py:65-70: TempDirTracker.on_chain_end accepts temp_dir_for_cleanup from any chain output in the run, and that value later goes to rmtree. Today it is safe. Only resolve_input emits that key, and its value comes from tempfile.mkdtemp(prefix="skillspector_"). LLM runnables inside nodes are invoked with an explicit callbacks=[collector] (llm_utils.py:484/502), which replaces the inherited callbacks, and they return Pydantic objects, not mappings. Still, this callback is now the only guard between a mapping key and recursive deletion. A future node, subgraph or dict-returning chain would silently become a deletion source. Consider recording only from the resolve_input node run (for example, remember its run_id from on_chain_start via metadata["langgraph_node"]). Or require the path to be an existing, non-symlink directory whose name starts with skillspector_.
3. [Non-blocking] tests/unit/test_cleanup.py: the tracker's own contract has no unit tests. Small cases would pin it: a later temp_dir_for_cleanup: None (or a non-string) does not erase a recorded path; remove() on an already-removed directory is a no-op; removing_on_error() re-raises the original exception unchanged. The recording_mkdtemp helper is duplicated in test_cli.py and test_mcp_server.py and could live in a shared fixture.

PIC tradeoffs: None identified. On merge order: #583 also changes the same finally blocks in cli.py / mcp_server.py to call cleanup_result(None) on failure. Whichever PR lands second needs a rebase that keeps both behaviors.

Verification and gaps: I traced every graph entry point at the PR head. _run_graph_scan (all CLI single-skill, recursive-child and transitive-child runs, both the invoke and stream paths) and mcp_server.run_scan are covered. skillspector baseline (cli.py:3219) is out of scope as the PR body says. Cleanup safety: the tracked value only ever comes from InputHandler._get_temp_dir() (mkdtemp(prefix="skillspector_")). For local inputs resolve_input emits None, which the tracker ignores, so a user directory is never recorded. remove_temp_tree uses shutil.rmtree(onexc=_retry_writable). rmtree refuses a symlinked root and does not follow links inside the tree, and _retry_writable never chmods through a link. Removing an already-deleted directory reaches onexc from os.lstat and is ignored, so double removal is a no-op. On success nothing changes: removing_on_error only acts on an exception, and the caller's cleanup_result runs as before. Exceptions are re-raised unchanged, so exit codes do not change. Explicit callbacks in the trace config does not disable env-configured LangSmith tracing. One residual risk I could not verify: after a cancellation, sync node threads may still be reading the directory when it is removed. On Windows that removal is best-effort and could leave a partial directory. All 6 CI checks are green and the branch is mergeable. Per policy I did not run the tests locally.

---

Decision: Changes Requested (reviewed head e99aeb2cdf22c8dafde3307f5ef7bd2386758866)

[kevin9327]

@rng1995 Thanks for tracing every entry point. Addressed in c1a25bf:

1. Transitive root leak: _scan_skill now removes the root result's temp dir when _scan_transitive raises, KeyboardInterrupt included, as you suggested. On success the merged result still carries the same temp_dir_for_cleanup, so nothing is removed twice. The new test_transitive_scan_stopped_in_a_child_removes_the_root_temp_dir scans a zipped root whose SKILL.md references an external skill. It stops the first transitive child (KeyboardInterrupt) or the merge step (RuntimeError), then asserts the root skillspector_* directory is gone. It fails without the change.
2. Tracker hardening: TempDirTracker now records a path only if it is an existing, non-symlink directory whose name starts with skillspector_, so another mapping key can't become a deletion target.
3. Tracker tests: added to tests/unit/test_cleanup.py. A later None or non-string doesn't erase a recorded path, a second remove() is a no-op, removing_on_error() re-raises the original exception, and a successful run leaves the directory for cleanup_result.

I left the shared recording_mkdtemp fixture out to keep this diff focused, and I'm happy to fold it in if you prefer. Noted on #583: if it lands first, I'll rebase and keep both behaviors.

[rng1995]

[SkillSpector Review]

Hi @kevin9327, thank you for the quick, well-tested follow-up and for hardening the tracker right where it feeds rmtree!

Value and readiness: The PR now removes the temp directory of every CLI and MCP scan that stops early, including the --transitive root that still leaked last round. A scan that finishes is cleaned up exactly as before. Exceptions still propagate unchanged, so exit codes do not move. The tracker now records only an existing, non-symlink skillspector_* directory. I found no required change, and the PR is ready for final review. The branch was not rebased, but it merges cleanly with current main and the functions it edits are unchanged there. The last CI run predates #702, #436 and #423, so a fresh run on current main before merge is advisable (for example via update-branch).

Previous findings:

• [Blocker] The transitive root's temp dir leaked when a child scan was interrupted or the merge step raised (_scan_skill → _scan_transitive): Resolved.

  • src/skillspector/cli.py:2409-2431 wraps _scan_transitive in try/except BaseException, calls cleanup_result(result) and re-raises.
  • On success, both return paths of _scan_transitive still copy the root's temp_dir_for_cleanup into the returned result (also on current main), so nothing is removed twice.
  • test_transitive_scan_stopped_in_a_child_removes_the_root_temp_dir scans a zipped root with the real graph. It stops the first child with KeyboardInterrupt, and the merge step with RuntimeError. It asserts that a child was reached and that the root skillspector_* directory is gone. By tracing, both cases leave the directory behind without the fix.

• [Non-blocking] The tracker accepted temp_dir_for_cleanup from any chain output: Resolved. _is_scan_temp_dir (src/skillspector/cleanup.py:53-63) accepts only a non-empty string that names an existing directory whose name starts with skillspector_ and that is not a symlink or junction. A later invalid value never replaces a path already recorded. This is the second option I offered. It also works on macOS, where the temp root sits under the /var symlink, because only the last path component is checked.

• [Non-blocking] The tracker contract had no unit tests: Resolved. tests/unit/test_cleanup.py now covers:

  • recording a scan temp dir;
  • later None, "" or non-string values;
  • paths that are not scan temp dirs (a user directory, a missing directory, a file);
  • a symlink with the skillspector_ prefix;
  • calling remove() twice;
  • re-raising the original KeyboardInterrupt or RuntimeError;
  • leaving the directory in place on success.

  The shared recording_mkdtemp helper was not extracted and now exists in three copies. That suggestion was optional and is fine to leave.

Material findings

1. [Non-blocking] skillspector baseline (src/skillspector/cli.py:3227) still calls graph.invoke(state) without a tracker, as the PR body says. Since then #643 has been closed, so only #657 still touches that function. Please open a follow-up issue so this last entry point is tracked.

PIC tradeoffs: None identified. Merge order:

• #583 has not been updated since 2026-09-28. It already conflicts with main in artifacts.py and cleanup.py, and with this PR it also conflicts in mcp_server.py. Whichever lands second must keep both behaviours.
• #736 and #744 merge cleanly with this PR.

Verification and gaps:

• Diff and merge: I read the full diff and the new commit c1a25bf. The branch is still based on 8831219, 236 commits behind main, and git merge-tree against current main is clean.
• Merged tree: since the PR's base, main changed _scan_skill, _run_graph_scan and _run_graph_scan_for_source only through #635's exclude_patterns plumbing, and the merge keeps it alongside the tracker. graph in cli.py is now #436's lazy proxy, which still forwards invoke and stream. In MCP, run_scan only gained restore_package_graph_export(). The new meta_review_required lines in _scan_transitive do not touch the exception path the new test uses. _format_json and _ensure_required_failure_events, which the tests patch, still exist.
• Lint: ruff check and ruff format --check pass on the six touched files in the merged tree.
• Entry points: _run_graph_scan covers single, recursive-child and transitive-child CLI runs, and mcp_server.run_scan is covered. baseline is the only uncovered entry point.
• CI: all six checks pass on c1a25bf. That run tested the merge with fd211ac (main at 2026-10-04 09:53Z). #702, #436, #423, #621 and #642 landed after it.
• Residual risk from last round: after an MCP cancellation, sync node threads may still be reading the directory while it is removed.
• Contributor tests were not run locally, per policy.

---

Decision: Approved (reviewed head c1a25bfd08c83ab080cecea751f3dc6c3d38995a)