fix: lint gate returns SKIPPED when linter binary is missing (#372)

[frankbria]

Summary

• Fixes [Phase 2.5] Per-edit lint gate returns FAILED when linter binary is missing #372: run_lint_on_file() now returns GateStatus.SKIPPED (not FAILED) when the linter binary isn't installed in the target project's dependencies
• Detects tool-not-found via stderr patterns from uv run: "Failed to spawn", "command not found", "No such file or directory"
• Prevents ReactAgent from wasting all 30 iterations trying to fix phantom lint errors

Changes

• codeframe/core/gates.py: Added tool-not-found detection between subprocess result capture and pass/fail decision
• tests/core/test_gates_observability.py: Added test_uv_run_missing_tool_returns_skipped covering the uv run failure scenario

Test Plan

• New test: test_uv_run_missing_tool_returns_skipped — mocks uv run ruff returning exit code 2 with "Failed to spawn" stderr, verifies SKIPPED
• Existing test: test_missing_linter_binary_skips — still passes (covers binary not on PATH at all)
• All 22 gate observability tests pass
• All 60 ReactAgent tests pass (no regressions)
• Linting clean (ruff check)

Closes #372

Summary by CodeRabbit

• Bug Fixes
  • Linting gate now detects when a configured linter binary cannot be spawned or is not installed and gracefully returns a skipped result with a clear explanatory message and duration instead of failing.
• Tests
  • Added a test covering the missing-tool scenario to ensure the lint gate returns a skipped result with appropriate output.

[coderabbitai]

Walkthrough

The per-file lint gate now detects tool-not-found failures (e.g., "Failed to spawn", "No such file or directory") after running the linter and returns GateStatus.SKIPPED with an explanatory message and duration. A test was added for the uv run + missing tool scenario.

Changes

Cohort / File(s)	Summary
Lint gate logic codeframe/core/gates.py	Extend run_lint_on_file() to inspect stderr for tool-not-found patterns (e.g., "Failed to spawn", "No such file or directory", "command not found") and return GateStatus.SKIPPED with an explanatory message and duration. Update docstring to document SKIPPED cases.
Test coverage tests/core/test_gates_observability.py	Add test_uv_run_missing_tool_returns_skipped that patches subprocess.run and shutil.which, simulates uv run failing to spawn the linter (exit code 2 with "not found" stderr), and asserts the lint gate result is SKIPPED with output mentioning the tool was not found.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat: language-aware per-edit lint gate in ReactAgent #366: Modifies the same per-file linting logic (run_lint_on_file) and touches related handling for missing lint tools.
• Issue #372: Tracks the problem where the lint gate returned FAILED when the linter binary was missing; this PR implements the proposed fix.

Poem

🐰 I hopped through code at break of day,
Sniffed stderr where the ruff would play.
"Failed to spawn" — I gave a wink,
Skipped the gate instead of sink.
No phantom errors now, hip-hip-hooray! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix: lint gate now returns SKIPPED when linter binary is missing, directly addressing the core change.
Linked Issues check	✅ Passed	The PR fully addresses #372's objectives: detects tool-not-found conditions via stderr patterns, returns SKIPPED instead of FAILED, and includes test coverage for the uv run failure scenario.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the linked issue: modifications to run_lint_on_file for tool-not-found detection and new test coverage for the specific failure scenario.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/issue-372-lint-gate-missing-binary

---

No actionable comments were generated in the recent review. 🎉

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[macroscopeapp]

Return SKIPPED from codeframe.core.gates.run_lint_on_file when the linter binary is missing to fix incorrect failure reporting

Add stderr-based detection of tool-not-found errors and return a SKIPPED GateCheck when uv or the shell cannot spawn the linter; add a unit test covering the SKIPPED path in tests/core/test_gates_observability.py.

📍Where to Start

Start with codeframe.core.gates.run_lint_on_file in codeframe/core/gates.py.

---

Macroscope summarized 2a4c52d.

[claude]

Code Review Summary

This PR successfully addresses issue #372 by improving the lint gate's error detection to distinguish between "linter not found" and "actual lint errors". The implementation is clean, well-tested, and follows the repository's patterns.

✅ Strengths

1. Correct Problem Diagnosis

The fix addresses the root cause: when uv run ruff fails because ruff isn't in project dependencies, the stderr contains "Failed to spawn" but was being treated as a lint failure rather than a missing tool.

2. Clean Implementation (codeframe/core/gates.py:678-691)

• Pattern detection is case-insensitive and comprehensive
• Covers multiple error patterns: "failed to spawn", "command not found", "no such file or directory"
• Early return prevents unnecessary processing
• Consistent with existing SKIPPED patterns (lines 648, 654, 711)

3. Comprehensive Test Coverage (test_gates_observability.py:259-279)

• Properly mocks both shutil.which and subprocess.run
• Realistic error message from uv
• Tests the exact scenario described in issue [Phase 2.5] Per-edit lint gate returns FAILED when linter binary is missing #372
• Assertion validates both status and output message

4. Documentation Updated

Docstring now clearly explains all SKIPPED scenarios including the new tool-not-found case.

🔍 Observations

Code Quality

• Pattern tuple: Using a tuple for _NOT_FOUND patterns is appropriate (lines 682-683)
• Error handling: Placed at the right location - after subprocess execution but before pass/fail determination
• Backward compatibility: Existing FileNotFoundError catch (line 710) and binary check (line 652-655) remain intact

Performance

• Minimal overhead: only checks stderr when returncode != 0
• Short-circuit evaluation with any() is efficient

Security

• No security concerns: uses existing subprocess execution with proper timeouts
• No injection risks: patterns are literals, not user input

⚠️ Minor Considerations

1. Test Assertion (line 279)

The assertion is somewhat redundant:

assert "not found" in check.output.lower() or "not found" in (check.output or "").lower()

The second part (check.output or "").lower() is unnecessary since if check.output is None/empty, the first check would already fail. Consider simplifying to:

assert "not found" in check.output.lower()

However, this is very minor and doesn't affect functionality.

2. Exit Code Specificity

The fix uses generic stderr pattern matching rather than checking exit code 2 specifically. This is actually better because:

• More robust across different tools and environments
• Exit codes can vary by tool and OS
• Pattern matching is more reliable for detecting "tool not found" vs "lint errors"

3. Pattern Coverage

The three patterns chosen are excellent and cover the main scenarios. Consider whether you might want to also detect:

• "executable file not found" (another common shell error)
• Tool-specific messages if other linters have different error formats

But the current patterns are sufficient for the immediate use case with ruff/uv.

🧪 Test Coverage

Looking at the test structure:

• ✅ Tests both PATH-missing scenario (test_missing_linter_binary_skips)
• ✅ Tests uv-dependency-missing scenario (test_uv_run_missing_tool_returns_skipped)
• ✅ Maintains separation of concerns between the two cases

🎯 Impact Assessment

Positive Impact

• ReactAgent efficiency: No more wasted iterations on phantom errors
• User experience: Clear SKIPPED status instead of confusing FAILED
• Consistency: Aligns with existing gate behavior patterns

Risk Assessment

• Low risk: Change is localized to error detection logic
• Backward compatible: Doesn't change behavior for actual lint errors
• Well-tested: New test specifically covers the regression scenario

📋 Alignment with Repository Standards

Per CLAUDE.md guidelines:

• ✅ Headless core: No FastAPI imports, pure domain logic
• ✅ Minimal changes: Only touches what's necessary for the fix
• ✅ Test coverage: Follows existing test patterns in test_gates_observability.py
• ✅ No over-engineering: Simple, focused solution

Recommendation

✅ APPROVE with minor suggestion

This is a high-quality fix that solves a real problem affecting ReactAgent usability. The implementation is clean, well-tested, and follows repository patterns. The only suggestion is the minor test assertion cleanup mentioned above, but this is optional.

The PR is ready to merge as-is.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@codeframe/core/gates.py`:
- Around line 678-691: The SKIPPED detection is too broad because it treats any
"no such file or directory" as the tool being missing; update the check in the
error-handling block that builds _NOT_FOUND so that "no such file or directory"
only triggers SKIPPED when the tool name (cfg.name) or the binary basename
appears in stderr_lower (or when stderr contains a spawn-related phrase), e.g.,
keep "failed to spawn" and "command not found" as global matches but require
that "no such file or directory" is accompanied by cfg.name or the executable
name found in result.args; return GateCheck(GateStatus.SKIPPED, ...) only when
one of these tightened conditions is met (use stderr_lower, cfg.name, and
result.args to locate the binary).

[claude]

Code Review - Second Review ✅

I've reviewed the latest changes (commit 225f3f0) that address the previous feedback. The improvements look excellent!

---

✅ Improvements Implemented

1. Narrowed Tool-Not-Found Detection (gates.py:686-690)

The fix for the false positive concern is well-implemented:

or ("no such file or directory" in stderr_lower and tool_name in stderr_lower)

Why this is good:

• "Failed to spawn" and "command not found" remain global matches (correct - these are unambiguous)
• "No such file or directory" now requires the tool name (cfg.cmd[0]) to also appear in stderr
• This prevents false SKIPPED when the linter reports a missing target file (e.g., ruff check nonexistent.py)

Example scenarios covered:

• ✅ uv run ruff + stderr contains "ruff" + "no such file" → SKIPPED (tool missing)
• ✅ ruff check missing.py + stderr contains "missing.py" + "no such file" → FAILED (file missing, not tool)

2. Simplified Test Assertion (test_gates_observability.py:279)

The redundant assertion was removed:

# Before: assert "not found" in check.output.lower() or "not found" in (check.output or "").lower()
# After: assert "not found" in check.output.lower()

This is cleaner and sufficient.

---

🔍 Code Quality Assessment

Implementation Correctness

Location: codeframe/core/gates.py:678-697

Strengths:

1. Proper tool name extraction: Uses cfg.cmd[0] which correctly gets the base command (e.g., "ruff" from ["ruff", "check", ...])
2. Case-insensitive matching: All comparisons use .lower() for robustness
3. Short-circuit evaluation: Only checks stderr when result.returncode != 0 (performance)
4. Early return pattern: Returns SKIPPED immediately, preventing unnecessary processing
5. Consistent with existing patterns: Matches the existing SKIPPED returns at lines 648, 654, 711

Edge Cases Considered

✅ Empty stderr: Handled by if result.returncode != 0 and result.stderr: guard
✅ Case variations: All pattern matching is case-insensitive
✅ Multiple error patterns: Supports various tool-not-found messages across platforms
✅ Tool name variations: Uses actual command from config, not hardcoded names

Potential False Negatives

The current implementation might miss some tool-not-found scenarios:

• Different error messages from other package managers (e.g., pipenv run, poetry run)
• Localized error messages (non-English environments)

However, these are acceptable trade-offs because:

• The three patterns cover the most common cases (uv, shell, OS errors)
• Adding more patterns risks false positives
• Worst case: returns FAILED instead of SKIPPED (agent tries to fix, which is safe)

---

🧪 Test Coverage

Test Quality (test_gates_observability.py:259-279)

Strengths:

1. Realistic stderr: Uses actual uv error message format
2. Proper mocking: Mocks both shutil.which and subprocess.run appropriately
3. Clear documentation: Docstring explains the scenario being tested
4. Two-part assertion: Validates both status and output message

Coverage Analysis:
The test suite now covers:

• ✅ Binary not on PATH at all (test_missing_linter_binary_skips)
• ✅ Binary on PATH but tool not in project dependencies (test_uv_run_missing_tool_returns_skipped)
• ✅ Actual lint errors (existing tests)

Missing test cases (optional, not blocking):

• Tool name + "no such file" scenario (to verify the narrowed detection)
• Different error patterns ("command not found", shell variations)

These aren't critical since the main regression scenario is well-covered.

---

📊 Performance & Security

Performance Impact

• Minimal: Only adds 3 string pattern checks when returncode != 0
• No regression: Doesn't affect the success path (when linter passes)
• Efficient: Uses in operator on lowercased strings (O(n) but fast in practice)

Security Considerations

• ✅ No injection risks: Patterns are literals, not user input
• ✅ No subprocess changes: Uses existing subprocess execution
• ✅ No sensitive data exposure: Error messages are already captured

---

🎯 Impact on ReactAgent

Before Fix

# Every file creation → GateStatus.FAILED
# Agent sees: "Lint errors detected" (phantom)
# Result: Wastes all 30 iterations trying to fix

After Fix

# File creation → GateStatus.SKIPPED
# Agent sees: Empty string (line 637-640 in react_agent.py)
# Result: Continues with next step

Verified: The existing ReactAgent code (react_agent.py:637-640) correctly handles SKIPPED by returning empty string, so this fix requires no changes to the agent logic.

---

📋 Repository Standards Compliance

Per CLAUDE.md guidelines:

• ✅ Headless core: No FastAPI imports, pure domain logic
• ✅ Minimal changes: Only touches what's necessary for the fix
• ✅ Test coverage: Follows existing test patterns
• ✅ No over-engineering: Simple, focused solution
• ✅ Backward compatible: Doesn't change behavior for actual lint errors

---

🚀 Recommendation

✅ APPROVE - Ready to Merge

This PR successfully resolves issue #372 with:

1. Correct implementation that distinguishes tool-not-found from lint errors
2. Improved false positive prevention via narrowed "no such file" detection
3. Comprehensive test coverage for the regression scenario
4. Clean, maintainable code following repository patterns
5. No breaking changes or performance concerns

The latest commit (225f3f0) effectively addresses the CodeRabbit feedback about false positives. The implementation is production-ready.

---

📝 Minor Suggestions for Future (Not Blocking)

If you want to further improve this area in the future:

1. Logging: Consider adding debug logging for the tool-not-found path to help diagnose edge cases
2. Metrics: Track SKIPPED vs FAILED rates to detect if patterns need adjustment
3. Configuration: Allow projects to customize tool-not-found patterns via config (probably overkill)

But these are nice-to-haves, not requirements.

---

Great work addressing the feedback! The narrowed detection logic is exactly what was needed. 🎉

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@codeframe/core/gates.py`:
- Around line 678-691: The current "no such file or directory" check only looks
for cfg.cmd[0] (tool_name) which misses wrapper invocations (e.g., npx eslint);
update the matching to consider both the wrapper and the real tool by building a
set of candidate names from cfg.cmd (e.g., cfg.cmd[0] plus cfg.cmd[1] if
present, or any parsed subcommand) and check if any candidate appears in
stderr_lower before treating the error as a tool-not-found; adjust the condition
around result.returncode/result.stderr and the existing variables (stderr_lower,
tool_name) to use this multi-name membership test.

[claude]

Code Review - Third Review

I've reviewed the latest commit (2a4c52d) which addresses CodeRabbit's feedback about wrapper invocations. The implementation continues to improve!

Latest Improvement: Multi-Name Matching

The detection now checks both cfg.cmd[0] and cfg.name when matching tool-not-found errors (gates.py:686).

This handles wrapper commands like npx where cfg.cmd[0]=npx but cfg.name=eslint. If stderr mentions eslint, the code now catches it.

Code Quality Assessment

Implementation strengths:

• Set-based matching for efficient O(1) lookups
• Checks both wrapper and tool names comprehensively
• Minimal overhead (only runs on failure path)
• Maintains false positive protection

Edge cases now covered:

• Wrapper commands (npx, pipx, poetry run, pipenv run)
• Direct uv commands (uv run ruff)
• Cases where cfg.name differs from cfg.cmd[0]

The approach is sufficient because cfg.cmd[0] (executable) and cfg.name (tool) cover the two names that appear in error messages. Checking cfg.cmd[1] (args like check) would be overkill.

Final Recommendation

APPROVE - Production Ready

This PR evolved through three iterations:

1. Initial: Core tool-not-found detection
2. Second: Narrowed no-such-file to require tool name (prevent false positives)
3. Third: Added cfg.name to catch wrapper scenarios (prevent false negatives)

The implementation is:

• Robust: Handles direct invocations and wrappers
• Precise: Avoids false positives with targeted matching
• Comprehensive: Covers the reported issue and edge cases
• Well-tested: Has targeted test coverage
• Production-ready: No known issues or concerns

Ready to merge!