Simplified Testing Strategy for Regression Prevention

[AndyMik90]

This task implements a focused, pragmatic testing strategy that targets the actual sources of regressions in Auto Claude, rather than pursuing arbitrary coverage metrics. Based on analysis from three independent critique agents, the original 9-week testing plan has been simplified to a 4-week targeted approach that adds ~83 tests to prevent 80%+ of current regressions. The strategy leverages existing infrastructure (2,098 backend tests, 84 frontend test files, Vitest, pytest, Electron MCP) without introducing new dependencies.

Summary by CodeRabbit

• New Features

  • Added verdict mapping and override logic for PR review statuses (affects merge/status decisions).

• Bug Fixes

  • Tightened CLI path validation to reject empty/whitespace and null-byte inputs.

• Tests

  • Vast expansion of tests: frontend E2E flows, agent end-to-end flows, platform/executable detection, auth routing, finding validation, implementation plans, recovery, and review-verdict behaviors.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Adds many new and expanded tests across frontend and backend, plus two new public platform functions. Frontend gains an Electron E2E smoke test and IPC/preload mocks; backend adds verdict-mapping helpers and platform/CLI path validation tweaks. Multiple test suites for agent flow, recovery, auth, finding validation, plans, and review verdicts were introduced or expanded.

Changes

Cohort / File(s)	Summary
Frontend E2E Smoke Tests apps/frontend/src/__tests__/e2e/smoke.test.ts	New comprehensive E2E smoke test file that mocks Electron IPC/preload APIs and exercises project/task/settings/QA/tab/log flows and many edge/error cases.
Frontend Test Environment apps/frontend/src/main/agent/agent-process.test.ts	Test setup now clears CLAUDE_CLI_PATH and GITHUB_CLI_PATH from process.env to ensure mocked detection.
Frontend Platform Tests apps/frontend/src/main/platform/__tests__/platform.test.ts	Expanded cross-platform tests for binary directories, shell configs, requiresShell rules, npm/npx detection, and isSecurePath behavior.
Platform Public APIs & Tests tests/test_platform.py	Adds public functions get_claude_detection_paths_structured() and find_executable() and extensive cross-platform tests for detection and executable lookup.
Agent Flow Integration Tests tests/test_agent_flow.py	Large new E2E-like tests covering planner→coder transitions, subtask lifecycle, post-session processing, QA loops, worktree isolation, and handoff preservation.
Auth Decryption Tests tests/test_auth.py	Adds platform-routing and error-handling tests for token decryption across macOS/Linux/Windows, env/keychain precedence, and many failure modes.
Finding Validation Tests tests/test_finding_validation.py	Extensive tests for evidence handling, deduplication, scope filtering, severity mapping, and validation edge cases.
Implementation Plan Tests tests/test_implementation_plan.py	Comprehensive schema, serialization, dependency resolution, state transition, and helper-factory tests for plans/phases/subtasks.
Recovery / Session Resilience Tests tests/test_recovery.py	Converts to pytest fixtures; adds tests for checkpoint persistence, restoration across sessions, multi-subtask checkpoints, recovery hints, and stuck-task handling.
Review Verdict Mapping apps/backend/runners/github/models.py, tests/test_review_verdict.py	Adds verdict helpers: verdict_from_severity_counts, apply_merge_conflict_override, apply_branch_behind_downgrade, apply_ci_status_override, and verdict_to_github_status, plus tests exercising mapping and overrides.
Backend Platform Validation apps/backend/core/platform/__init__.py	Tightened validate_cli_path to reject empty/whitespace paths and treat null bytes as dangerous in path validation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• fix(tests): isolate git operations in test fixtures from parent repository #1205 — Adjusts test fixtures and temp Git repo isolation; relates to added recovery/worktree and git-isolation tests.
• Fix/small fixes all around #645 — Changes to Electron mocks/preload surface; relates to the new frontend E2E smoke tests that mock Electron IPC/preload.
• Revert "Feat/Auto Fix Github issues and do extensive AI PR reviews" #251 — Modifies GitHub runners/models; closely related to added verdict-mapping helpers in apps/backend/runners/github/models.py.

Suggested labels

🔄 Checking, Missing AC Approval

Suggested reviewers

• AlexMadera

Poem

🐰 I hopped through tests both far and wide,
Mocking IPC, paths, and plans with pride.
Verdicts, recovery, tokens too—
Logs streamed, worktrees kept true.
A joyful thump: the burrow’s verified!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'Simplified Testing Strategy for Regression Prevention' directly summarizes the main objective of this changeset, which adds approximately 83 targeted tests across multiple layers (backend, frontend, platform) to prevent regressions using existing infrastructure.
Docstring Coverage	✅ Passed	Docstring coverage is 96.97% 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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[gemini-code-assist]

Summary of Changes

Hello @AndyMik90, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly strengthens the testing foundation of Auto Claude by introducing new end-to-end smoke tests for the user interface and expanding the backend's test coverage for agent workflows, platform compatibility, and PR review logic. The changes aim to proactively prevent regressions and ensure the reliability of critical system functionalities through a more targeted and comprehensive testing strategy.

Highlights

• New E2E Smoke Tests: Introduced a comprehensive suite of E2E smoke tests for the Electron frontend, covering critical user journeys like project/task management, settings, and QA review flows.
• Enhanced Platform Compatibility Testing: Significantly expanded test coverage for platform-specific CLI detection, binary directory resolution, shell configuration, and secure path validation across Windows, macOS, and Linux.
• Robust Backend Agent Flow Tests: Added a new Python test suite to validate the entire agent workflow, including planner-coder-QA state transitions, data handoffs, and worktree isolation.
• Improved PR Review Verdict Logic: Implemented a new test suite for the PR review verdict system, ensuring correct mapping of finding severities, merge conflicts, branch status, and CI results to merge verdicts.
• Comprehensive Implementation Plan Schema Validation: Introduced extensive tests for the JSON schema of implementation plans, covering valid structures, error handling for missing/invalid fields, and complex state transitions like blocked, stuck, and skipped subtasks.
• Advanced Token Decryption Testing: Added detailed tests for secure token decryption mechanisms, including platform-specific routing and robust error handling for various failure scenarios.
• Persistent Recovery Manager Checkpoints: Enhanced the RecoveryManager with tests for checkpoint persistence across sessions, ensuring state restoration, recovery hints, and stuck subtask management are robust.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[github-advanced-security]

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[gemini-code-assist]

Code Review

This pull request introduces a significant number of new, comprehensive tests across various critical components of the application, including E2E smoke tests, agent flow, authentication, finding validation, implementation plan schema, platform utilities, recovery mechanisms, and PR review verdict logic. The added tests greatly enhance the robustness, reliability, and maintainability of the codebase by covering numerous scenarios, edge cases, and platform-specific behaviors. The detailed testing of security-sensitive areas like path validation and token decryption is particularly commendable. This is a very positive and impactful change.

[gemini-code-assist]

Consider defining a more specific interface or type for Project instead of using the broad object type. This would improve type safety and make the test data structure clearer, aligning better with the actual Project type used in the application.

[gemini-code-assist]

Similar to createTestProject, defining a specific interface for Task would enhance type safety and readability, providing a clearer contract for the test data.

[gemini-code-assist]

For createTestSettings, using a dedicated Settings interface instead of object would improve type checking and make it easier to understand the expected structure of settings data.

[gemini-code-assist]

Using vi.resetModules() in beforeEach can sometimes lead to unexpected behavior or performance issues in larger test suites, as it clears the module cache for every test. While it ensures strong isolation, consider if more targeted mocking (e.g., vi.mock with mockImplementation or mockReturnValue) could achieve the desired isolation with less overhead. If this is strictly necessary for Electron preload behavior, it's acceptable, but it's worth noting the potential impact.

[coderabbitai]

Actionable comments posted: 10

🤖 Fix all issues with AI agents

In `@tests/test_agent_flow.py`:
- Around line 33-94: Extract the duplicated setup_test_environment and
cleanup_test_environment into tests/conftest.py as reusable pytest fixtures
(e.g., a tmp_git_repo fixture that yields temp_dir, spec_dir, project_dir and
restores env on teardown) and update tests/test_agent_flow.py and
tests/test_recovery.py to use that fixture instead of defining their own
functions; ensure the fixture contains the same logic (clearing git env vars,
setting GIT_CEILING_DIRECTORIES, initializing git, making initial commit, and
restoring saved_env) and remove the duplicate function definitions from both
test modules.
- Around line 268-281: Replace the nested async wrapper and asyncio.run call in
the test with a pytest-asyncio style async test: mark the test function
(test_completed_subtask_records_success) with `@pytest.mark.asyncio` and make it
async, then directly await post_session_processing(...) (referencing the
post_session_processing symbol and existing parameters like spec_dir,
project_dir, subtask_id, session_num, commit_before, recovery_manager,
linear_enabled) and assert the result; this removes the inner async def
run_test() and asyncio.run invocation for cleaner async test flow.
- Around line 1753-1790: The finally block should explicitly call
manager.cleanup_all() to remove git worktrees before tearing down the temp
environment; update the test_concurrent_worktree_operations_dont_conflict
finally clause to invoke WorktreeManager.cleanup_all() (via the local manager
instance) prior to calling cleanup_test_environment(temp_dir, saved_env) so
worktrees are properly removed and test isolation is ensured.

In `@tests/test_platform.py`:
- Around line 709-714: Replace the no-op in
test_path_with_multiple_consecutive_separators by either removing the test or
adding concrete assertions: call the module's path validation/normalization
helpers used elsewhere in the test suite (e.g., the same
validate/is_safe/path_normalize functions used by neighboring tests) with
examples containing repeated separators (e.g., "foo//bar" or "foo///bar") and
assert the expected outcome (that OS normalization produces a safe/normalized
path or that the validator accepts/rejects per spec). Do the same for the other
placeholder tests flagged in the comment (the pass bodies at the other ranges) —
delete them if they are redundant, or replace each pass with targeted assertions
that mirror the intended security/normalization checks in the test suite.
- Around line 915-920: The test shows whitespace-only paths are currently
accepted; update validation to strip and reject them: change validate_cli_path
to trim the input (e.g., strip()) and return False for empty/whitespace-only
strings, then update the test_rejects_whitespace_only_path expectation to assert
validate_cli_path('   ') is False so the test reflects the tightened validation
behavior.

In `@tests/test_recovery.py`:
- Around line 567-568: Remove the redundant local import of subprocess inside
the try block in tests/test_recovery.py: since subprocess is already imported at
module level, delete the "import subprocess" line (and the surrounding
try/except if it only exists to guard that import) so the module uses the
top-level subprocess import instead.
- Around line 457-458: Remove the redundant local import statement "import
subprocess" inside the test (the one added near the "Get current commit"
comment); rely on the existing module-level subprocess import already present at
the top of tests/test_recovery.py, so delete the local import to avoid
duplication and potential confusion.

In `@tests/test_review_verdict.py`:
- Around line 93-110: Replace the duplicated inline verdict logic in
TestSeverityToVerdictMapping tests with calls to the production mapping
function: identify or create a single function (e.g.,
determine_verdict_from_counts) in the production code that accepts
(critical_count, high_count, medium_count, low_count) and returns a
MergeVerdict, then update tests like test_critical_severity_maps_to_blocked to
import and call determine_verdict_from_counts and assert its return equals
MergeVerdict.BLOCKED (do the same for other severity tests and remove the
duplicated if/elif/else blocks).
- Around line 20-31: Replace fragile sys.path manipulation in
tests/test_review_verdict.py (the block that sets _backend_dir, _github_dir,
_services_dir and inserts them into sys.path) by moving that path setup into
pytest configuration: add equivalent path prep to conftest.py (e.g., computing
Path(__file__).parent.parent / "apps" / "backend" and appending the subpaths) or
configure pythonpath in pyproject.toml so test imports no longer rely on runtime
sys.path edits; update or remove the sys.path code in
tests/test_review_verdict.py and ensure imports still resolve via the new
conftest.py or pyproject.toml settings.
- Around line 237-247: The test test_merge_conflict_overrides_needs_revision
defines an unused variable ai_verdict; either remove the ai_verdict assignment
or incorporate it into the test (for example set ai_verdict =
MergeVerdict.NEEDS_REVISION and assert that when has_merge_conflicts is True the
resulting verdict variable equals MergeVerdict.BLOCKED while ai_verdict remains
unchanged) — update the test body around has_merge_conflicts, ai_verdict, and
verdict to eliminate the unused variable or make it part of the assertion.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider using pytest-asyncio for cleaner async test patterns.

The current pattern of defining a nested async def run_test() and calling asyncio.run(run_test()) works but is verbose. Using pytest-asyncio would allow:

`@pytest.mark.asyncio`
async def test_completed_subtask_records_success(self):
    # ... setup ...
    result = await post_session_processing(...)
    assert result is True

This is an optional improvement for readability.

🤖 Prompt for AI Agents

In `@tests/test_agent_flow.py` around lines 268 - 281, Replace the nested async
wrapper and asyncio.run call in the test with a pytest-asyncio style async test:
mark the test function (test_completed_subtask_records_success) with
`@pytest.mark.asyncio` and make it async, then directly await
post_session_processing(...) (referencing the post_session_processing symbol and
existing parameters like spec_dir, project_dir, subtask_id, session_num,
commit_before, recovery_manager, linear_enabled) and assert the result; this
removes the inner async def run_test() and asyncio.run invocation for cleaner
async test flow.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, find the worktree.py file and check for cleanup methods
find . -name "worktree.py" -type f 2>/dev/null | head -5

Repository: AndyMik90/Auto-Claude

Length of output: 186

---

🏁 Script executed:

# Search for WorktreeManager class and cleanup method
rg -n "class WorktreeManager" --type=py -A 30 | head -50

Repository: AndyMik90/Auto-Claude

Length of output: 2238

---

🏁 Script executed:

# Check cleanup_test_environment implementation
rg -n "def cleanup_test_environment" --type=py -A 20

Repository: AndyMik90/Auto-Claude

Length of output: 2672

---

🏁 Script executed:

# Check how worktrees are created and what resources need cleanup
rg -n "def create_worktree" --type=py -A 15

Repository: AndyMik90/Auto-Claude

Length of output: 2634

---

🏁 Script executed:

# Search for cleanup or remove methods in WorktreeManager
rg -n "def (cleanup|remove).*worktree" --type=py -i -A 10

Repository: AndyMik90/Auto-Claude

Length of output: 8535

---

🏁 Script executed:

# Check the full WorktreeManager class methods
rg -n "^\s+def " apps/backend/core/worktree.py | grep -E "(cleanup|remove|destroy|prune)"

Repository: AndyMik90/Auto-Claude

Length of output: 278

---

🏁 Script executed:

# Look at the structure of WorktreeManager more completely - all public methods
ast-grep --pattern 'class WorktreeManager {
  $$$
}'

Repository: AndyMik90/Auto-Claude

Length of output: 47

---

🏁 Script executed:

# Check test_agent_flow.py around line 1753-1790 to see full context of the test
sed -n '1750,1800p' tests/test_agent_flow.py

Repository: AndyMik90/Auto-Claude

Length of output: 2265

---

Call manager.cleanup_all() explicitly to ensure proper test isolation.

While cleanup_test_environment does remove the temporary directory, explicitly calling manager.cleanup_all() in the finally block before cleanup_test_environment provides clearer test isolation and properly manages git worktrees. The WorktreeManager class provides a cleanup_all() method (not cleanup()) specifically for this purpose.

finally:
    manager.cleanup_all()  # Clean up git worktrees explicitly
    cleanup_test_environment(temp_dir, saved_env)

🤖 Prompt for AI Agents

In `@tests/test_agent_flow.py` around lines 1753 - 1790, The finally block should
explicitly call manager.cleanup_all() to remove git worktrees before tearing
down the temp environment; update the
test_concurrent_worktree_operations_dont_conflict finally clause to invoke
WorktreeManager.cleanup_all() (via the local manager instance) prior to calling
cleanup_test_environment(temp_dir, saved_env) so worktrees are properly removed
and test isolation is ensured.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider using pytest's path configuration instead of sys.path manipulation.

The manual sys.path manipulation is fragile and can lead to import issues in different environments. Consider configuring the path in conftest.py or using pyproject.toml's pythonpath setting for pytest.

🤖 Prompt for AI Agents

In `@tests/test_review_verdict.py` around lines 20 - 31, Replace fragile sys.path
manipulation in tests/test_review_verdict.py (the block that sets _backend_dir,
_github_dir, _services_dir and inserts them into sys.path) by moving that path
setup into pytest configuration: add equivalent path prep to conftest.py (e.g.,
computing Path(__file__).parent.parent / "apps" / "backend" and appending the
subpaths) or configure pythonpath in pyproject.toml so test imports no longer
rely on runtime sys.path edits; update or remove the sys.path code in
tests/test_review_verdict.py and ensure imports still resolve via the new
conftest.py or pyproject.toml settings.

[AndyMik90]

🤖 Auto Claude PR Review

Merge Verdict: 🔴 BLOCKED

🔴 Blocked - 5 CI check(s) failing. Fix CI before merge.

Blocked: 5 CI check(s) failing. Fix CI before merge.

Risk Assessment

Factor	Level	Notes
Complexity	High	Based on lines changed
Security Impact	None	Based on security findings
Scope Coherence	Good	Based on structural review

🚨 Blocking Issues (Must Fix)

• Branch Out of Date: PR branch is behind the base branch and needs to be updated
• CI Failed: CI Complete
• CI Failed: test-python (3.13, ubuntu-latest)
• CI Failed: test-python (3.12, ubuntu-latest)
• CI Failed: test-python (3.12, windows-latest)
• CI Failed: test-python (3.12, macos-latest)

Findings Summary

• High: 3 issue(s)
• Medium: 3 issue(s)
• Low: 2 issue(s)

---

Generated by Auto Claude PR Review

Findings (8 selected of 8 total)

🟠 [ecc3eafa3128] [HIGH] [Potential] Duplicated setup_test_environment function - should use conftest.py fixture

📁 tests/test_agent_flow.py:33

The setup_test_environment() function (lines 33-80) duplicates logic already available in conftest.py's temp_git_repo fixture (lines 172-240). Both functions perform identical operations: clearing git environment variables, setting GIT_CEILING_DIRECTORIES, initializing a git repo, configuring user, and creating initial commit. The same duplication exists in test_recovery.py (lines 26-75). Changes to git isolation logic must be maintained in three places.

Suggested fix:

Replace setup/cleanup functions with pytest fixtures that use the existing temp_git_repo fixture from conftest.py. Refactor tests to use fixture-based setup/teardown.

🟠 [d175f0eed58e] [HIGH] [Potential] Duplicated setup_test_environment - identical to test_agent_flow.py

📁 tests/test_recovery.py:26

The setup_test_environment() function in test_recovery.py is nearly character-for-character identical to test_agent_flow.py and duplicates conftest.py's temp_git_repo fixture. This is copy-paste code that creates maintenance burden.

Suggested fix:

Remove the duplicated functions and use conftest.py fixtures. Create a shared fixture if additional setup is needed.

🟠 [23ff3c23373b] [HIGH] [Potential] Tests reimplement production logic instead of testing actual production functions

📁 tests/test_review_verdict.py:93

The TestSeverityToVerdictMapping class contains tests that reimplement the verdict determination logic inline instead of calling actual production functions. The comment explicitly states 'Simulating the verdict logic from followup_reviewer'. This means if production logic changes, tests would still pass with their outdated copy. The same pattern repeats 6+ times in this file.

Suggested fix:

Import and call the actual production verdict function. If no such function exists, extract the verdict logic into a testable function in production code first:
```python
from followup_reviewer import determine_verdict
verdict = determine_verdict(critical=1, high=0, medium=0, low=0)
assert verdict == MergeVerdict.BLOCKED

#### 🟡 [1758bb5be6c4] [MEDIUM] [Potential] Whitespace-only CLI paths pass validation when they should be rejected
📁 `tests/test_platform.py:919`

The test documents that validate_cli_path('   ') returns True with comment 'May pass validation but fail file check'. Whitespace-only paths should be rejected during validation, not allowed to proceed. This is a validation gap that could lead to confusing error messages downstream.

**Suggested fix:**

Fix the production validate_cli_path() to reject whitespace-only paths (add .strip() check). Then update test to: assert validate_cli_path(' ') is False

#### 🟡 [b26ad0296423] [MEDIUM] [Potential] No-op test provides zero coverage - test_path_with_multiple_consecutive_separators
📁 `tests/test_platform.py:709`

The test method has only a `pass` statement with comment 'No explicit test needed - OS handles this'. This provides zero test coverage and inflates test counts without value.

**Suggested fix:**

Either delete the test method entirely or add actual assertions:

assert validate_cli_path('/usr//bin//python') is True

#### 🟡 [b2ec0da199b3] [MEDIUM] [Potential] No-op test provides zero coverage - test_rejects_null_byte_injection
📁 `tests/test_platform.py:751`

The test method has only a `pass` statement for null byte injection testing. This provides zero coverage for a security-relevant test case.

**Suggested fix:**

Add actual assertions:

assert validate_cli_path('cmd\x00evil') is False

#### 🔵 [89d5130e693d] [LOW] [Potential] Redundant subprocess import inside function
📁 `tests/test_recovery.py:43`

subprocess is imported at module level (line 18) but also has local import inside setup_test_environment() at line 43. Multiple test functions have the same redundant import pattern.

**Suggested fix:**

Remove all function-local subprocess imports since the module-level import is sufficient.

#### 🔵 [f6dac3e6a3be] [LOW] [Potential] Factory functions return generic 'object' type instead of specific interfaces
📁 `apps/frontend/src/__tests__/e2e/smoke.test.ts:47`

The factory functions createTestProject(), createTestTask(), and createTestSettings() all return `object` type instead of specific TypeScript interfaces. The codebase has proper interfaces in /apps/frontend/src/shared/types/. While intentional for mock flexibility, this reduces type safety.

**Suggested fix:**

Consider using Partial or a TestProject interface for better type safety while maintaining flexibility.

---
*This review was generated by Auto Claude.*

In general, to fix an unused local variable, either remove the variable (and possibly just call the expression for its side effects) or, if it is intentionally unused, rename it to follow an “unused” naming convention (for example, _ or unused_result). Here, the test only verifies that mock_isfile was called, so the actual value returned by find_executable is irrelevant.

The best minimal fix without changing existing functionality is to remove the assignment and call find_executable('node') as a standalone statement. This keeps the side effects (any internal logic and calls to os.path.isfile) while removing the unused variable. Concretely, in tests/test_platform.py around line 311, replace:

311:         result = find_executable('node')

with:

311:         find_executable('node')

No new imports, methods, or definitions are required.

To fix this, we should keep the side effect of calling RecoveryManager(spec_dir, project_dir) while avoiding introducing a named local variable that is never read. The safest approach that does not alter functionality is to remove the left-hand side of the assignment and invoke the constructor as a standalone expression. This preserves any side effects in __init__ (such as creating directories and files) while eliminating the unused variable.

Concretely, in tests/test_recovery.py, in test_initialization, replace the line _manager = RecoveryManager(spec_dir, project_dir) with a bare call RecoveryManager(spec_dir, project_dir). No additional imports or definitions are required, and no other parts of the file need to change. The rest of the test continues to validate the created directories and files as before.

To fix the problem, we should ensure that the second is_circular assignment is actually used, or, if the value is intentionally ignored, change the variable name to indicate it is unused or drop the assignment. Since this is a test function and the comment explicitly discusses behavior (“This might be detected as circular if word overlap is high / But ‘callback-based’ is sufficiently different from ‘async await’”), the most appropriate fix is to assert the expected boolean result for the second call.

Concretely, in tests/test_recovery.py within test_circular_fix_detection, after line 131 where is_circular is reassigned with the result of manager.is_circular_fix("subtask-1", "Using completely different callback-based approach"), we should add an assertion that verifies this scenario is not detected as circular. For example:

assert not is_circular, "Non-circular fix incorrectly detected as circular"

This uses the existing variable, removes the unused-variable warning, and strengthens the test by checking both positive and negative cases of is_circular_fix. No new imports or helper methods are needed.

In general, to fix an unused local variable, either (a) remove the variable (and possibly the entire assignment) if its value is not needed, or (b) use the variable meaningfully (e.g., in an assertion, return value, or logging). If the variable is intentionally unused, rename it to an accepted unused-style name (like _ or unused_is_circular).

Here, the comment # Check circular fix detection with restored data strongly suggests the intent is to validate the behavior of is_circular_fix. The best fix without changing existing functionality (and in fact making the test complete) is to add an assertion that uses is_circular. Because the note says “May or may not detect as circular depending on word overlap”, we cannot assert a specific boolean value, but we can at least assert that the method returns a boolean, confirming that the call works on restored data and consuming the variable so it’s not unused.

Concretely, in tests/test_recovery.py around lines 471–472, keep the assignment to is_circular and add an assertion such as assert isinstance(is_circular, bool) (or equivalent) after the note. No new imports are needed, and we stay within the provided snippet. This both satisfies CodeQL and makes the test actually verify something about the returned value.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[cursor]

CI failure doesn't block NEEDS_REVISION verdicts as documented

Medium Severity

The apply_ci_status_override function's docstring states "Failing CI -> BLOCKED" without any condition, but the code only returns BLOCKED when the current verdict is READY_TO_MERGE or MERGE_WITH_CHANGES. If the verdict is NEEDS_REVISION and CI is failing, the function incorrectly returns NEEDS_REVISION instead of BLOCKED. The asymmetry in the docstring (failing CI has no qualifier while pending CI explicitly has one) indicates failing CI was intended to always result in BLOCKED.

 

[coderabbitai]

Actionable comments posted: 5

🤖 Fix all issues with AI agents

In `@tests/test_agent_flow.py`:
- Around line 507-518: The git add/commit subprocess.run calls in the test (the
calls that create the new commit after recovery_manager.record_good_commit and
before asserting HEAD) do not check return codes, so a failed git command can
leave HEAD unchanged and the test false-positive; update the subprocess.run
invocations that run ["git", "add", "."] and ["git", "commit", "-m", "Add new
file"] to either pass check=True or capture the CompletedProcess and assert
result.returncode == 0 (or raise) to fail the test on git errors.

In `@tests/test_platform.py`:
- Around line 876-881: The test method
test_allows_literal_percent_in_valid_context currently contains only a pass and
should either be removed or converted into a concrete assertion: decide whether
the platform should allow a lone '%' (not %VAR%) and implement an assertion that
calls the relevant function(s) (e.g., the env-parsing or regex-matching routine
used elsewhere in tests) to assert the actual observed behavior, or if you want
to keep it as documentation move the explanatory comment into the method
docstring and delete the empty test body so it no longer runs as a no-op test;
update or remove test_allows_literal_percent_in_valid_context accordingly.

In `@tests/test_recovery.py`:
- Around line 126-132: The local variable assignments to is_circular from
manager.is_circular_fix are unused; either assert the expected boolean result or
drop the variable and call the method for side effects. Update both occurrences
where you assign is_circular (e.g., the second occurrence in the shown block and
the other one referenced) to either assert manager.is_circular_fix("subtask-1",
"Using completely different callback-based approach") with the expected value,
or simply call manager.is_circular_fix(...) without assigning to is_circular if
no assertion is needed.
- Around line 54-55: The assignment to the unused variable `_manager` should be
removed; instead instantiate RecoveryManager without assigning it so the
side-effect (directory creation) still occurs. Locate the line creating the
instance (using RecoveryManager(spec_dir, project_dir)) and change it to a bare
instantiation (no `_manager = `) or otherwise remove the variable so static
analysis no longer flags an unused variable.
- Around line 543-569: The manual runner in run_all_tests currently prints
messages then returns True, giving a false green; change it to invoke pytest
programmatically instead: import pytest (and sys if you prefer to sys.exit),
call pytest.main(["-q", "tests/test_recovery.py"]) (or similar args used
elsewhere) and propagate/exit with the returned exit code so the process fails
on test failures; ensure the stubbed prints remain for the pytest suggestion but
remove the unconditional return True in the run_all_tests function and use the
pytest exit code to determine success/failure.

♻️ Duplicate comments (3)

apps/backend/runners/github/models.py (1)

157-182: Failing CI doesn’t always force BLOCKED despite the docstring.
On Line 176–178, failing CI only blocks when the current verdict is READY_TO_MERGE or MERGE_WITH_CHANGES; NEEDS_REVISION stays unchanged. If the intended rule is “Failing CI → BLOCKED,” this should always return BLOCKED whenever failing_count > 0 (or the docstring should be updated).

🔧 Suggested fix (align logic with docstring)

 def apply_ci_status_override(
     verdict: MergeVerdict,
     failing_count: int = 0,
     pending_count: int = 0,
 ) -> MergeVerdict:
@@
-    if failing_count > 0:
-        if verdict in (MergeVerdict.READY_TO_MERGE, MergeVerdict.MERGE_WITH_CHANGES):
-            return MergeVerdict.BLOCKED
+    if failing_count > 0:
+        return MergeVerdict.BLOCKED
     elif pending_count > 0:
         if verdict in (MergeVerdict.READY_TO_MERGE, MergeVerdict.MERGE_WITH_CHANGES):
             return MergeVerdict.NEEDS_REVISION
     return verdict

tests/test_review_verdict.py (1)

22-32: Avoid direct sys.path manipulation in tests.
This block is brittle across environments; prefer pytest config or a conftest.py path setup.

tests/test_platform.py (1)

301-314: Use the result variable to strengthen the assertion.

The result variable is assigned but never used (flagged by static analysis). While the test verifies mock behavior, asserting on the result would make the test more explicit about expected outcomes.

💡 Suggested fix

         mock_isfile.side_effect = isfile_side_effect
 
         result = find_executable('node')
 
         # Should have tried to find with extension
         assert mock_isfile.called
+        # Verify the found path contains expected Windows location
+        assert result is None or 'Program Files' in result

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fail fast on git command errors.
git add/commit calls ignore returncode, so a failed commit can leave HEAD unchanged while the test still passes. Consider check=True (or assert returncode == 0) for these git commands in this file.

🐛 Suggested hardening (example)

-        subprocess.run(["git", "add", "."], cwd=project_dir, capture_output=True)
-        subprocess.run(["git", "commit", "-m", "Add new file"], cwd=project_dir, capture_output=True)
+        subprocess.run(["git", "add", "."], cwd=project_dir, capture_output=True, check=True)
+        subprocess.run(["git", "commit", "-m", "Add new file"], cwd=project_dir, capture_output=True, check=True)

🤖 Prompt for AI Agents

In `@tests/test_agent_flow.py` around lines 507 - 518, The git add/commit
subprocess.run calls in the test (the calls that create the new commit after
recovery_manager.record_good_commit and before asserting HEAD) do not check
return codes, so a failed git command can leave HEAD unchanged and the test
false-positive; update the subprocess.run invocations that run ["git", "add",
"."] and ["git", "commit", "-m", "Add new file"] to either pass check=True or
capture the CompletedProcess and assert result.returncode == 0 (or raise) to
fail the test on git errors.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider converting this to a concrete assertion or removing it.

The pass body with a comment documents implementation-specific behavior, but doesn't provide actual test coverage. If the behavior is worth documenting, consider either:

1. Adding a concrete assertion that demonstrates the actual behavior
2. Moving the note to a docstring without the test method

💡 Suggested approach

     def test_allows_literal_percent_in_valid_context(self):
         """Single percent signs (not env vars) context varies by platform."""
-        # A single percent not forming %VAR% pattern might be allowed
-        # depending on the regex - test actual behavior
-        # Note: Our pattern is r"%[^%]+%" which requires %...%
-        pass  # Implementation-specific behavior
+        # Single percent (not forming %VAR% pattern) is allowed
+        # Our pattern r"%[^%]+%" requires opening AND closing %
+        assert validate_cli_path('file%name.exe') is True  # No closing %
+        assert validate_cli_path('50%discount') is True

🤖 Prompt for AI Agents

In `@tests/test_platform.py` around lines 876 - 881, The test method
test_allows_literal_percent_in_valid_context currently contains only a pass and
should either be removed or converted into a concrete assertion: decide whether
the platform should allow a lone '%' (not %VAR%) and implement an assertion that
calls the relevant function(s) (e.g., the env-parsing or regex-matching routine
used elsewhere in tests) to assert the actual observed behavior, or if you want
to keep it as documentation move the explanatory comment into the method
docstring and delete the empty test body so it no longer runs as a no-op test;
update or remove test_allows_literal_percent_in_valid_context accordingly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Remove the unused _manager assignment.
Static analysis flags this as unused; instantiating without assignment keeps behavior and clears the warning.

🔧 Suggested fix

-    _manager = RecoveryManager(spec_dir, project_dir)
+    RecoveryManager(spec_dir, project_dir)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	# Initialize manager to trigger directory creation (manager instance not needed)
	_manager = RecoveryManager(spec_dir, project_dir)
	# Initialize manager to trigger directory creation (manager instance not needed)
	RecoveryManager(spec_dir, project_dir)

🧰 Tools

🪛 GitHub Check: CodeQL

[notice] 55-55: Unused local variable
Variable _manager is not used.

🤖 Prompt for AI Agents

In `@tests/test_recovery.py` around lines 54 - 55, The assignment to the unused
variable `_manager` should be removed; instead instantiate RecoveryManager
without assigning it so the side-effect (directory creation) still occurs.
Locate the line creating the instance (using RecoveryManager(spec_dir,
project_dir)) and change it to a bare instantiation (no `_manager = `) or
otherwise remove the variable so static analysis no longer flags an unused
variable.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Drop unused is_circular locals (or assert expected behavior).
These assignments are unused; either assert a value or just call the method.

🔧 Suggested fix (apply to both locations)

-    is_circular = manager.is_circular_fix("subtask-1", "Using async await pattern once more")
+    manager.is_circular_fix("subtask-1", "Using async await pattern once more")

Also applies to: 471-472

🧰 Tools

🪛 GitHub Check: CodeQL

[notice] 131-131: Unused local variable
Variable is_circular is not used.

🤖 Prompt for AI Agents

In `@tests/test_recovery.py` around lines 126 - 132, The local variable
assignments to is_circular from manager.is_circular_fix are unused; either
assert the expected boolean result or drop the variable and call the method for
side effects. Update both occurrences where you assign is_circular (e.g., the
second occurrence in the shown block and the other one referenced) to either
assert manager.is_circular_fix("subtask-1", "Using completely different
callback-based approach") with the expected value, or simply call
manager.is_circular_fix(...) without assigning to is_circular if no assertion is
needed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

run_all_tests reports success without running tests.
Right now it returns True after printing a note, so python tests/test_recovery.py yields a false green. Either invoke pytest (like tests/test_agent_flow.py) or exit non‑zero with a clear message.

✅ Proposed fix (pytest runner)

 def run_all_tests():
     """Run all tests."""
-    print("=" * 70)
-    print("SMART ROLLBACK AND RECOVERY - TEST SUITE")
-    print("=" * 70)
-    print()
-
-    # Note: This manual runner is kept for backwards compatibility.
-    # Prefer running tests with pytest: pytest tests/test_recovery.py -v
-
-    tests = [
-        ("test_initialization", test_initialization),
-        ("test_record_attempt", test_record_attempt),
-        ("test_circular_fix_detection", test_circular_fix_detection),
-        ("test_failure_classification", test_failure_classification),
-        ("test_recovery_action_determination", test_recovery_action_determination),
-        ("test_good_commit_tracking", test_good_commit_tracking),
-        ("test_mark_subtask_stuck", test_mark_subtask_stuck),
-        ("test_recovery_hints", test_recovery_hints),
-        # Session checkpoint and restoration tests
-        ("test_checkpoint_persistence_across_sessions", test_checkpoint_persistence_across_sessions),
-        ("test_restoration_after_failure", test_restoration_after_failure),
-        ("test_checkpoint_multiple_subtasks", test_checkpoint_multiple_subtasks),
-        ("test_restoration_with_build_commits", test_restoration_with_build_commits),
-        ("test_checkpoint_recovery_hints_restoration", test_checkpoint_recovery_hints_restoration),
-        ("test_restoration_stuck_subtasks_list", test_restoration_stuck_subtasks_list),
-        ("test_checkpoint_clear_and_reset", test_checkpoint_clear_and_reset),
-    ]
-
-    print("Note: Running with manual test runner for backwards compatibility.")
-    print("For full pytest integration with fixtures, run: pytest tests/test_recovery.py -v")
-    print()
-    print("Manual test runner cannot use fixtures - please run with pytest.")
-    return True
+    import pytest
+    return pytest.main([__file__, "-v", "--tb=short"]) == 0

As per coding guidelines, tests under tests/ should follow pytest conventions.

🤖 Prompt for AI Agents

In `@tests/test_recovery.py` around lines 543 - 569, The manual runner in
run_all_tests currently prints messages then returns True, giving a false green;
change it to invoke pytest programmatically instead: import pytest (and sys if
you prefer to sys.exit), call pytest.main(["-q", "tests/test_recovery.py"]) (or
similar args used elsewhere) and propagate/exit with the returned exit code so
the process fails on test failures; ensure the stubbed prints remain for the
pytest suggestion but remove the unconditional return True in the run_all_tests
function and use the pytest exit code to determine success/failure.