fix(cleanup): remove read-only files from scan temp directories

[kevin9327]

Problem

On Windows, every skillspector scan <git URL> leaves its clone behind in %TEMP%, with no warning. Reproduction (Windows 11, Python 3.12.10, main at 4148ab3). This lists the skillspector_* directories in %TEMP% before and after one scan:

PS> skillspector scan https://github.com/octocat/Hello-World.git --no-llm --format json
exit: 0
new leftover dirs: 1
C:\Users\...\AppData\Local\Temp\skillspector_agpgnip_ files=3
  \repo\.git\objects\pack\pack-22687ef7....idx  ro=True
  \repo\.git\objects\pack\pack-22687ef7....pack ro=True
  \repo\.git\objects\pack\pack-22687ef7....rev  ro=True

The clone is bounded only by INGEST_MAX_BYTES (100 MiB), so repeated URL scans, the MCP server and --transitive runs keep filling the temp drive.

Cause

cleanup_result (used by the CLI and the MCP server) and InputHandler.cleanup both call shutil.rmtree(temp_dir, ignore_errors=True). git clone writes pack and index files read-only. Windows will not delete a read-only file (PermissionError), and ignore_errors=True swallows that, so the pack files and every directory above them survive. Everything that is not read-only is removed, which is why the leftover holds only .git/objects/pack.

Fix

skillspector.cleanup.remove_temp_tree calls shutil.rmtree(path, onexc=...). When a removal fails, the handler adds the owner-write bit to the entry's current mode and retries once. On Windows that clears the read-only attribute. Both cleanup call sites use it now.

• It stays best-effort, like ignore_errors=True: if the retry also fails (a file still in use, or a path that is already gone), the error is swallowed and the scan result is unaffected.
• It skips the chmod for symlinks and junctions, because os.chmod follows links and must not change the permissions of anything outside the temp tree.
• It keeps the existing permission bits. The plain os.chmod(path, stat.S_IWRITE) from the shutil docs replaces the whole mode, which on POSIX strips read and search permission from a directory that still cannot be removed. The first CI run caught this with the best-effort test on Linux, and the second commit fixes it.

Tests (tests/unit/test_cleanup.py, new)

The tests patch os.unlink to refuse read-only files, which is Windows' behaviour, so they reproduce the leak on Linux CI too.

• test_cleanup_result_removes_read_only_git_objects: a temp clone with read-only .git/objects/pack files is fully removed by cleanup_result.
• test_input_handler_cleanup_removes_read_only_git_objects: the same for InputHandler.cleanup.
• test_cleanup_result_stays_best_effort_when_a_file_cannot_be_removed: a file that stays locked is left behind, the rest is removed, and nothing raises. This one passes before and after; it pins that the new handler keeps the old best-effort contract.

Against unmodified cleanup.py / input_handler.py:

>       assert not temp_dir.exists()
E       AssertionError: assert not True
FAILED tests/unit/test_cleanup.py::test_cleanup_result_removes_read_only_git_objects
FAILED tests/unit/test_cleanup.py::test_input_handler_cleanup_removes_read_only_git_objects
2 failed, 1 passed

With the fix: 3 passed. Branch coverage over cleanup.py shows no missed lines, including the swallowed-error path.

The real scan from the top, re-run on this branch: exit: 0, new leftover dirs: 0.

Suite

pytest -m "not integration and not provider" -p no:randomly, same machine:

	main	this branch
tests/unit, tests/opencode, tests/test_*.py	10 failed, 1721 passed	10 failed, 1724 passed
tests/nodes	3 failed, 3588 passed, 4 errors	3 failed, 3588 passed, 4 errors

The +3 are the new tests. The failures and errors are identical on both sides and Windows-only (release and compare_scan_accuracy harnesses, CRLF fixtures, a backslash inside a POSIX file name, and parametrized test IDs over the Windows environment-variable length limit).

ruff check src/ tests/: All checks passed. ruff format --check src/ tests/: 247 files already formatted.

🤖 Generated with Claude Code

[rng1995]

[SkillSpector Review]

The Windows cleanup direction is sound, but the generic onexc handler currently breaks its best-effort contract on POSIX failure paths. Please preserve existing permissions, distinguish permission failures from errors such as ENOTEMPTY, and ensure retry-callback failures cannot escape cleanup.

At review time, changes, lint, docker-smoke, DCO, and OpenCode TypeScript checks pass; test-unit is still running. GitHub reports no required checks for this branch.

[rng1995]

[P1] Preserve permissions and only retry supported permission failures

onexc is called for every rmtree failure, not only a read-only unlink. In the new locked-file test, the failed child remains, the final rmdir reports ENOTEMPTY, and this line changes the root directory from (for example) 0700 to 0200 on POSIX before the retry fails. The supposedly best-effort cleanup therefore leaves an unsearchable tree, and locked.exists() cannot reliably observe it. Also, function is platform/implementation-dependent (os.open is possible in fd-safe rmtree), so function(path) can raise a non-OSError such as TypeError, which the callback does not catch and rmtree will propagate. Gate the writable retry to the applicable permission-denied removal operations, OR S_IWRITE into the existing non-link mode instead of replacing it, and keep every remaining cleanup error non-fatal. Please make the locked-file case pass on POSIX as well as Windows.

[rng1995]

[SkillSpector Review]

Re-reviewed current head c9015f5f98efa5871e7753e9947b386a1b43aac0 against the prior finding, complete cleanup/test diff, Python shutil.rmtree(onexc=...) callback contract, and exact-head checks.

Preserving the existing mode before adding S_IWRITE fixes the permission-clobbering part. The best-effort contract is still not complete: onexc is invoked for removal, traversal, and directory errors, but _retry_writable() ignores the exception and callable type, mutates permissions for every failure, and catches only OSError. A callback such as os.open can raise TypeError when retried as function(path), which escapes remove_temp_tree() and can fail the scan/cleanup caller. Gate the writable retry to permission-denied unlink/rmdir operations, catch ordinary retry-callback exceptions so cleanup remains non-fatal, and add non-permission plus incompatible-callback regressions.

All six exact-head checks pass, but the remaining cleanup correctness issue, unresolved thread, active change requests, and GitHub BLOCKED state prevent merge.

Priority: P1 — shared cleanup must remain non-fatal across supported platforms and failure modes.