Skip to content

fix(skill-creator): resolve Windows eval harness pipe reading and probe command leakage - #1694

Open
AbhiPra24 wants to merge 1 commit into
anthropics:mainfrom
AbhiPra24:fix/skill-creator-windows-eval-harness
Open

fix(skill-creator): resolve Windows eval harness pipe reading and probe command leakage#1694
AbhiPra24 wants to merge 1 commit into
anthropics:mainfrom
AbhiPra24:fix/skill-creator-windows-eval-harness

Conversation

@AbhiPra24

Copy link
Copy Markdown

Summary

  • Replaced select.select() on pipes in run_eval.py with a thread-safe queue.Queue reader thread, avoiding WinSock-only select() crashes on Windows anonymous pipes.
  • Updated find_project_root() to explicitly avoid matching Path.home() (~/.claude), ensuring probe command files are never written to the user's global command directory.
  • Added cleanup_probe_commands() invoked at start and in finally blocks across run_eval.py and run_loop.py to guarantee probe files (*-skill-*.md) are safely deleted even when runs are interrupted or fail.
  • Added explicit utf-8 encoding across file operations in run_eval.py, run_loop.py, improve_description.py, and utils.py to avoid Windows ANSI/cp1252 decoding errors.

Fixes #1692

cc @98zc5g5jyw-arch for review

@Hahaknight Hahaknight left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified on real Windows 11 (Python 3.13). This fixes defects 1, 2 and 4 from #1692 — but as written, defect 3 (twin collision capping recall at ~1/N) appears to remain, see last section.

Defect 1 (select on pipes) — fixed, verified. main still crashes in my environment: select.select([p.stdout], ...) raises OSError: [WinError 10093] (WSANOTINITIALISED — same class as the reporter's 10038: select() only supports sockets on Windows). This PR's thread-pump + queue.Queue pattern tested on the same platform: receives all subprocess output correctly. Cross-platform and keeps early-kill semantics.

Defect 2 (global ~/.claude/commands leak) — fixed, verified (3 cases):

  • from a git-clone subdir with no .claude: returns the repo root (via the new .git marker), not $HOME
  • from a bare directory with no .git/.claude anywhere below home: returns cwd, never walks up into $HOME
  • cleanup_probe_commands(root, "myproj") removes only myproj-skill-*.md and leaves other-skill-xyz.md / unrelated.md untouched; the no-name variant sweeps all *-skill-*.md ✅ (good orphan cleanup for previously-interrupted runs)

Defect 4 (encoding) — fixed. run_loop.py eval-set read and all report/JSON writes now explicit utf-8, matching #1687/#1690.

Defect 3 (parallel twins → recall ~1/N) — not fixed by this PR. run_single_query still writes {skill_name}-skill-{uuid}.md into the shared project_root/.claude/commands/, so while ProcessPoolExecutor runs ~10 probes concurrently, every claude -p session sees ~10 near-identical twins and invokes an arbitrary one — only the probe owning that UUID counts a hit. That's exactly the 11% train / 8% test plateau (~1/10) the reporter measured, which persisted "regardless of description". The reporter's local fix (per-probe tempfile.mkdtemp() root + rmtree in finally) took the same description from ~11% to 60/60 — worth adopting here or in a follow-up; it would also make the defect-2 leak structurally impossible rather than mitigated.

Minor: the reporter's last suggestion (loud failure when a large share of probes error, instead of feeding recall=0% into the improver) isn't addressed — a one-line guard in run_eval raising if e.g. >50% of futures raised would stop quota burn on invalid measurements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants