Repository navigation
fix(skillopt-sleep): surface Claude CLI spawn failures instead of silent zero scores - #126
Merged
Yifan Yang (Yif-Yang) merged 2 commits intoJul 12, 2026
Conversation
…ent zero scores In _call and attempt_with_tools, the bare 'except Exception: return ""' swallows FileNotFoundError (e.g. bare 'claude' on Windows with npm .cmd shim) and any other spawn failure, returning an empty string that the trainer treats as a legitimate model response that scores 0.0 everywhere. Now the exception is caught explicitly: last_call_error is set, a warning is logged, and the empty-string return is preserved for backward compatibility of the control flow. This mirrors the pattern from microsoft#92 (codex backend) which fixed the same class of 'dead CLI masquerades as nothing to learn' bug. Issue: microsoft#121
…re surfaced Add two tests to TestClaudeCliBackendBare: - test_spawn_failure_sets_last_call_error: _call sets last_call_error and returns '' when subprocess.run raises FileNotFoundError. - test_attempt_tools_spawn_failure_sets_last_call_error: same for attempt_with_tools. These prove the fix from the parent commit (surface spawn failures instead of silently scoring 0) and guard against regressions.
Contributor
Author
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make
ClaudeCliBackendreport subprocess spawn failures (e.g.FileNotFoundErroron Windows) instead of silently swallowing them and returning an empty string that scores 0.0 everywhere.Motivation
Two stacked problems (detailed in #121):
Path resolution. On Windows,
claudeinstalled via npm is a.cmdshim.subprocess.run(["claude", ...])raisesFileNotFoundErrorbecauseCreateProcessdoesn't resolve.cmdby bare name. (PR fix: make ClaudeCliBackend work on Windows (.cmd shim resolution + argv length) #98 addresses this withshutil.which().)Silent swallow. Regardless of why the CLI fails to spawn, both
_callandattempt_with_toolswrap the subprocess call in a bareexcept Exception: return ""that never setslast_call_erroror logs a warning. The empty string is then treated as a legitimate model answer, so the run proceeds and all scores come back 0.0 — indistinguishable from "nothing to optimize".This PR fixes problem 2, following the same pattern established in #92 for the codex backend.
Changes
Engine
_call(line 623): Catch the exception explicitly, setlast_call_error, log a warning, then return"". Control flow preserved.attempt_with_tools(line 687): Same treatment.Both ensure that a dead or broken CLI can never masquerade as a 0.0-scoring model.
Tests
test_spawn_failure_sets_last_call_error(new): Verifies_callsetslast_call_errorand returns""whensubprocess.runraisesFileNotFoundError.test_attempt_tools_spawn_failure_sets_last_call_error(new): Same forattempt_with_tools.Testing
All 5 tests pass (3 pre-existing + 2 new).
Full suite:
\vs/), completely unrelated to this change.Related issue
Closes #121