feat(adapters): detect modified files via git diff after subprocess execution - #436
Conversation
WalkthroughAdds detection of modified, staged, and untracked files to SubprocessAdapter by running git commands after subprocess execution and populating Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Review: feat(adapters): detect modified files via git diffGood, focused implementation. The graceful fallback strategy is well-designed and test coverage is solid. A few issues worth addressing. Bug: Duplicate files in The implementation calls Fix the test to use with patch(
"subprocess.run",
side_effect=[
MagicMock(returncode=0, stdout="src/main.py\ntests/test_main.py\n"), # git diff
MagicMock(returncode=0, stdout=""), # git ls-files (no untracked)
],
):Consider adding deduplication in the implementation as a defensive guard: return list(dict.fromkeys(files)) # preserves order, removes duplicatesMinor: Misleading test name The test asserts Edge case worth a comment: fresh git repo (no commits) On a repo with no commits, if result.returncode != 0:
# Also covers repos with no commits (HEAD does not exist yet)
return []Implementation is correct otherwise
Summary: The duplicate-file test bug and misleading test name should be fixed before merge. Deduplication in the implementation is a nice-to-have. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/core/adapters/test_subprocess_adapter.py (1)
251-267: This test doesn’t validate the untracked-files merge path yet.At Line 260,
subprocess.runis mocked with a single return value, but_detect_modified_files()performs two git calls. This can pass even if the untracked-file branch regresses.Suggested patch
with ( patch("subprocess.Popen", return_value=mock_process), patch( "subprocess.run", - return_value=MagicMock( - returncode=0, - stdout="src/main.py\ntests/test_main.py\n", - ), + side_effect=[ + MagicMock(returncode=0, stdout="src/main.py\n"), + MagicMock(returncode=0, stdout="tests/test_main.py\n"), + ], ), ): result = adapter.run("task-1", "fix", tmp_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/adapters/test_subprocess_adapter.py` around lines 251 - 267, The test test_populates_modified_files_on_success currently mocks subprocess.run with a single return value but _detect_modified_files() invokes two git commands; update the patch for subprocess.run in this test to use side_effect of two MagicMock results (both with returncode=0) where the first mock's stdout contains the git diff output (e.g., "src/main.py\n") and the second mock's stdout contains untracked files (e.g., "tests/test_main.py\n"), so both branches of _detect_modified_files() are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/adapters/test_claude_code.py`:
- Around line 15-20: This module's v2 adapter tests lack the required v2 marker;
add a module-level marker by defining pytestmark = pytest.mark.v2 at the top of
the test module (near the existing _no_git fixture and tests that exercise
ClaudeCodeAdapter) so the entire file is marked v2; alternatively, add
`@pytest.mark.v2` to each test function, but prefer the single module-level
pytestmark to ensure all tests (including fixtures like _no_git) run under v2
semantics.
In `@tests/core/adapters/test_opencode.py`:
- Around line 15-20: This test module adds v2-specific behavior but lacks the v2
marker; add a module-level pytest mark by inserting either a module decorator
`@pytest.mark.v2` above the tests or a top-level assignment pytestmark =
pytest.mark.v2 so the fixture _no_git and all tests in the module (e.g., the
_no_git autouse fixture) are executed under the v2 test configuration; ensure
you import pytest if not already present.
In `@tests/core/adapters/test_subprocess_adapter.py`:
- Around line 231-233: The new test class TestSubprocessAdapterModifiedFiles is
missing the v2 marker; update the test to be marked with pytest v2 by adding
either the decorator `@pytest.mark.v2` above the class definition or by setting
pytestmark = pytest.mark.v2 at module scope so the class is recognized as a v2
test (ensure pytest is imported as needed).
---
Nitpick comments:
In `@tests/core/adapters/test_subprocess_adapter.py`:
- Around line 251-267: The test test_populates_modified_files_on_success
currently mocks subprocess.run with a single return value but
_detect_modified_files() invokes two git commands; update the patch for
subprocess.run in this test to use side_effect of two MagicMock results (both
with returncode=0) where the first mock's stdout contains the git diff output
(e.g., "src/main.py\n") and the second mock's stdout contains untracked files
(e.g., "tests/test_main.py\n"), so both branches of _detect_modified_files() are
exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d729f0e4-72b5-48e8-9afc-77be6cc67c1e
📒 Files selected for processing (4)
codeframe/core/adapters/subprocess_adapter.pytests/core/adapters/test_claude_code.pytests/core/adapters/test_opencode.pytests/core/adapters/test_subprocess_adapter.py
| @pytest.fixture(autouse=True) | ||
| def _no_git(self): | ||
| """Prevent _detect_modified_files from calling real git.""" | ||
| with patch.object(ClaudeCodeAdapter, "_detect_modified_files", return_value=[]): | ||
| yield | ||
|
|
There was a problem hiding this comment.
Add a v2 marker for this test module.
The v2 adapter test updates introduced around Line 15 are in a module without @pytest.mark.v2 (or module-level pytestmark).
Suggested patch
import pytest
from codeframe.core.adapters.agent_adapter import AgentAdapter
from codeframe.core.adapters.claude_code import ClaudeCodeAdapter
+pytestmark = pytest.mark.v2
+
class TestClaudeCodeAdapter:As per coding guidelines: tests/**/*.py: Test files must use the @pytest.mark.v2 decorator or module-level pytestmark = pytest.mark.v2 for v2 functionality tests.
📝 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.
| @pytest.fixture(autouse=True) | |
| def _no_git(self): | |
| """Prevent _detect_modified_files from calling real git.""" | |
| with patch.object(ClaudeCodeAdapter, "_detect_modified_files", return_value=[]): | |
| yield | |
| import pytest | |
| from codeframe.core.adapters.agent_adapter import AgentAdapter | |
| from codeframe.core.adapters.claude_code import ClaudeCodeAdapter | |
| pytestmark = pytest.mark.v2 | |
| class TestClaudeCodeAdapter: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/adapters/test_claude_code.py` around lines 15 - 20, This module's
v2 adapter tests lack the required v2 marker; add a module-level marker by
defining pytestmark = pytest.mark.v2 at the top of the test module (near the
existing _no_git fixture and tests that exercise ClaudeCodeAdapter) so the
entire file is marked v2; alternatively, add `@pytest.mark.v2` to each test
function, but prefer the single module-level pytestmark to ensure all tests
(including fixtures like _no_git) run under v2 semantics.
| @pytest.fixture(autouse=True) | ||
| def _no_git(self): | ||
| """Prevent _detect_modified_files from calling real git.""" | ||
| with patch.object(OpenCodeAdapter, "_detect_modified_files", return_value=[]): | ||
| yield | ||
|
|
There was a problem hiding this comment.
Add a v2 marker for this test module.
The new v2-related adapter test behavior added around Line 15 is in a test module that is not marked with @pytest.mark.v2 (or module-level pytestmark).
Suggested patch
import pytest
from codeframe.core.adapters.agent_adapter import AgentAdapter
from codeframe.core.adapters.opencode import OpenCodeAdapter
+pytestmark = pytest.mark.v2
+
class TestOpenCodeAdapter:As per coding guidelines: tests/**/*.py: New v2 Python tests must be marked with @pytest.mark.v2 decorator or pytestmark = pytest.mark.v2.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/adapters/test_opencode.py` around lines 15 - 20, This test module
adds v2-specific behavior but lacks the v2 marker; add a module-level pytest
mark by inserting either a module decorator `@pytest.mark.v2` above the tests or a
top-level assignment pytestmark = pytest.mark.v2 so the fixture _no_git and all
tests in the module (e.g., the _no_git autouse fixture) are executed under the
v2 test configuration; ensure you import pytest if not already present.
| class TestSubprocessAdapterModifiedFiles: | ||
| """Tests for git diff file detection after execution.""" | ||
|
|
There was a problem hiding this comment.
Mark the newly added v2 tests with @pytest.mark.v2.
The new test class introduced at Line 231 is not marked as v2.
Suggested patch
+@pytest.mark.v2
class TestSubprocessAdapterModifiedFiles:
"""Tests for git diff file detection after execution."""As per coding guidelines: tests/**/*.py: New v2 Python tests must be marked with @pytest.mark.v2 decorator or pytestmark = pytest.mark.v2.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/adapters/test_subprocess_adapter.py` around lines 231 - 233, The
new test class TestSubprocessAdapterModifiedFiles is missing the v2 marker;
update the test to be marked with pytest v2 by adding either the decorator
`@pytest.mark.v2` above the class definition or by setting pytestmark =
pytest.mark.v2 at module scope so the class is recognized as a v2 test (ensure
pytest is imported as needed).
- Use side_effect for subprocess.run mocks to test git diff and git ls-files separately - Add dict.fromkeys deduplication as defensive guard - Rename misleading test name - Add comment about fresh git repos with no commits
Follow-up ReviewAll three issues from my initial review have been addressed:
One remaining item flagged by CodeRabbit that's still unaddressed: the new
All three test files should have Otherwise the implementation is clean and ready to merge once the marker is added. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/core/adapters/test_subprocess_adapter.py (1)
231-233:⚠️ Potential issue | 🟡 MinorMark the newly added v2 tests with
@pytest.mark.v2.The new test class
TestSubprocessAdapterModifiedFilesis missing the required v2 marker.Suggested fix
+@pytest.mark.v2 class TestSubprocessAdapterModifiedFiles: """Tests for git diff file detection after execution."""As per coding guidelines:
tests/**/*.py: New v2 Python tests must be marked with@pytest.mark.v2decorator orpytestmark = pytest.mark.v2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/adapters/test_subprocess_adapter.py` around lines 231 - 233, The new test class TestSubprocessAdapterModifiedFiles is missing the required v2 marker; add the marker by either decorating the class with `@pytest.mark.v2` or adding pytestmark = pytest.mark.v2 at module scope so the tests are correctly labeled as v2 per the test guidelines.
🧹 Nitpick comments (1)
tests/core/adapters/test_subprocess_adapter.py (1)
239-249: Consider extracting duplicated helper to module level.The
_make_mock_processhelper is duplicated betweenTestSubprocessAdapterRunandTestSubprocessAdapterModifiedFiles. You could extract it to module level or a shared fixture.def _make_mock_process(stdout_lines=None, stderr_text="", returncode=0): """Create a mock Popen process for subprocess tests.""" mock = MagicMock() mock.stdout = iter(stdout_lines or []) mock.stderr = MagicMock() mock.stderr.read.return_value = stderr_text mock.stdin = MagicMock() mock.returncode = returncode mock.wait.return_value = None return mockThis is a minor nit and acceptable to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/adapters/test_subprocess_adapter.py` around lines 239 - 249, Extract the duplicated _make_mock_process helper from inside TestSubprocessAdapterRun and TestSubprocessAdapterModifiedFiles to a module-level function or a shared pytest fixture so both test classes reuse the same implementation; move the existing implementation (creating MagicMock with stdout iterator, stderr.read, stdin, returncode, and wait) to a top-level def _make_mock_process(...) or convert it into a fixture (e.g., mock_process) and update test usages to call the top-level function or request the fixture instead of defining inline copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/core/adapters/test_subprocess_adapter.py`:
- Around line 231-233: The new test class TestSubprocessAdapterModifiedFiles is
missing the required v2 marker; add the marker by either decorating the class
with `@pytest.mark.v2` or adding pytestmark = pytest.mark.v2 at module scope so
the tests are correctly labeled as v2 per the test guidelines.
---
Nitpick comments:
In `@tests/core/adapters/test_subprocess_adapter.py`:
- Around line 239-249: Extract the duplicated _make_mock_process helper from
inside TestSubprocessAdapterRun and TestSubprocessAdapterModifiedFiles to a
module-level function or a shared pytest fixture so both test classes reuse the
same implementation; move the existing implementation (creating MagicMock with
stdout iterator, stderr.read, stdin, returncode, and wait) to a top-level def
_make_mock_process(...) or convert it into a fixture (e.g., mock_process) and
update test usages to call the top-level function or request the fixture instead
of defining inline copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8486f63-af7c-4df1-9e27-5a7c5a542884
📒 Files selected for processing (2)
codeframe/core/adapters/subprocess_adapter.pytests/core/adapters/test_subprocess_adapter.py
1 participant
Summary
Implements the remaining acceptance criterion for #411: [Phase 4] Claude Code Engine Adapter
The bulk of #411 was already implemented by PRs for #409 (AgentAdapter protocol), #414 (engine registry), and #415 (verification wrapper). The only missing piece was modified files detection via git diff —
AgentResult.modified_fileswas never populated for external engines.Changes
_detect_modified_files()toSubprocessAdapterbase class — runsgit diff --name-only HEAD+git ls-files --others --exclude-standardafter subprocess completionAgentResult.modified_filesfor ALL external engines (Claude Code, OpenCode, any future adapters)autousefixtures to prevent git calls in existing unit testsAcceptance Criteria (all met)
ClaudeCodeAdapterimplementsAgentAdapterprotocol ([Phase 4] Agent Adapter Protocol Definition #409)-pflag for long prompts)AgentResultStatus(SubprocessAdapter._map_result)AgentEvents (SubprocessAdapter)cf work start <id> --execute --engine claude-code([Phase 4] Engine Registry and Runtime Selection #414)Test Plan
_detect_modified_files()in TestSubprocessAdapterModifiedFilesCloses #411
Summary by CodeRabbit
New Features
Tests