From 749131125b2f15d9d488850f6fb7f032c564d625 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:26:45 -0700 Subject: [PATCH 1/3] fix(adapters): grant claude-code write permissions + fail on zero-file completion (#739) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --print mode silently denies Edit/Write/Bash, so the default ClaudeCodeAdapter (no allowlist) could only analyze, never modify — and P0.5 gates-on-unchanged-tree then marked tasks COMPLETED with zero code written. - ClaudeCodeAdapter default now passes --permission-mode bypassPermissions (acceptEdits alone leaves Bash denied); explicit allowlist path unchanged. - SubprocessAdapter gains opt-in require_file_changes (default False, so codex/opencode + plumbing tests are unaffected); ClaudeCodeAdapter defaults it True. A run that exits 0 but modifies no files is downgraded to failed. --- codeframe/core/adapters/claude_code.py | 26 +++++++-- codeframe/core/adapters/subprocess_adapter.py | 23 ++++++++ tests/core/adapters/test_claude_code.py | 57 ++++++++++++++++++- .../core/adapters/test_subprocess_adapter.py | 40 +++++++++++++ 4 files changed, 139 insertions(+), 7 deletions(-) diff --git a/codeframe/core/adapters/claude_code.py b/codeframe/core/adapters/claude_code.py index 20eae714..fd300537 100644 --- a/codeframe/core/adapters/claude_code.py +++ b/codeframe/core/adapters/claude_code.py @@ -17,21 +17,37 @@ class ClaudeCodeAdapter(SubprocessAdapter): https://docs.anthropic.com/en/docs/claude-code """ - def __init__(self, allowlist: list[str] | None = None) -> None: + def __init__( + self, + allowlist: list[str] | None = None, + require_file_changes: bool = True, + ) -> None: """Initialize the Claude Code adapter. Args: allowlist: Optional list of allowed tools/permissions. If provided, uses ``--allowedTools`` flag for each tool. - When omitted, no permission flags are added (the caller - is responsible for configuring permissions externally). + When omitted, the adapter runs with + ``--permission-mode bypassPermissions`` so Edit/Write/Bash + are auto-approved in non-interactive ``--print`` mode. + Without this, ``--print`` silently denies those tools and + the delegated agent can analyze but never modify files. (#739) + require_file_changes: If True (default), a run that exits 0 but touches + no files is downgraded to ``failed`` — a coding task that + writes nothing is a false completion. """ cli_args = ["--print"] if allowlist: for tool in allowlist: cli_args.extend(["--allowedTools", tool]) - - super().__init__(binary="claude", cli_args=cli_args) + else: + cli_args.extend(["--permission-mode", "bypassPermissions"]) + + super().__init__( + binary="claude", + cli_args=cli_args, + require_file_changes=require_file_changes, + ) self._allowlist = allowlist @property diff --git a/codeframe/core/adapters/subprocess_adapter.py b/codeframe/core/adapters/subprocess_adapter.py index 5f884aff..71aa77ea 100644 --- a/codeframe/core/adapters/subprocess_adapter.py +++ b/codeframe/core/adapters/subprocess_adapter.py @@ -33,6 +33,7 @@ def __init__( binary: str, cli_args: list[str] | None = None, timeout_s: int | None = None, + require_file_changes: bool = False, ) -> None: """Initialize with the binary name and default CLI args. @@ -40,6 +41,11 @@ def __init__( binary: Name of the CLI binary (e.g., 'claude', 'opencode') cli_args: Default CLI arguments appended to every invocation timeout_s: Max execution time in seconds (default: 1800, None = no limit) + require_file_changes: If True, a run that exits 0 but modifies no files + is downgraded to ``failed`` instead of ``completed``. Guards against + a delegated coding agent that "succeeds" without writing any code + (e.g. edits silently denied), which downstream gates would otherwise + pass on the unchanged tree. Default False (analysis-capable agents). Raises: EnvironmentError: If the binary is not found on PATH @@ -47,6 +53,7 @@ def __init__( self._binary = binary self._cli_args = cli_args or [] self._timeout_s = timeout_s if timeout_s is not None else self.DEFAULT_TIMEOUT_S + self._require_file_changes = require_file_changes resolved = shutil.which(binary) if resolved is None: @@ -202,6 +209,22 @@ def _write_stdin() -> None: workspace_path=workspace_path, ) result.modified_files = modified_files + + # A coding task that "succeeds" without touching any file is a false + # completion: edits were likely denied or the agent only analyzed. Fail + # hard so downstream gates don't pass on the unchanged tree. (#739) + if ( + self._require_file_changes + and result.status == "completed" + and not modified_files + ): + result.status = "failed" + result.error = ( + f"'{self._binary}' exited successfully but modified no files. " + "A coding task must change at least one file; the agent likely " + "lacked write permission or produced no edits." + ) + return result def _map_result( diff --git a/tests/core/adapters/test_claude_code.py b/tests/core/adapters/test_claude_code.py index def67c1b..a7c07454 100644 --- a/tests/core/adapters/test_claude_code.py +++ b/tests/core/adapters/test_claude_code.py @@ -40,17 +40,23 @@ def test_build_command_includes_print_flag(self) -> None: assert cmd[0] == "/usr/bin/claude" assert "--print" in cmd - def test_build_command_without_allowlist(self) -> None: + def test_build_command_without_allowlist_grants_permissions(self) -> None: + """Default (no allowlist) must pass a permission config so --print mode + does not silently deny Edit/Write/Bash. (#739)""" with patch("shutil.which", return_value="/usr/bin/claude"): adapter = ClaudeCodeAdapter() cmd = adapter.build_command("prompt", Path("/tmp")) assert "--allowedTools" not in cmd + idx = cmd.index("--permission-mode") + assert cmd[idx + 1] == "bypassPermissions" def test_build_command_with_allowlist(self) -> None: with patch("shutil.which", return_value="/usr/bin/claude"): adapter = ClaudeCodeAdapter(allowlist=["Edit", "Write"]) cmd = adapter.build_command("prompt", Path("/tmp")) assert "--allowedTools" in cmd + # Explicit allowlist path keeps its own permissions — no bypass mode. + assert "--permission-mode" not in cmd # Verify both tools are present after their respective flags idx_edit = cmd.index("Edit") idx_write = cmd.index("Write") @@ -76,11 +82,58 @@ def test_successful_execution(self) -> None: mock_process.returncode = 0 mock_process.wait.return_value = None - with patch("subprocess.Popen", return_value=mock_process): + with ( + patch("subprocess.Popen", return_value=mock_process), + patch.object( + ClaudeCodeAdapter, + "_detect_modified_files", + return_value=["src/main.py"], + ), + ): result = adapter.run("task-1", "fix the bug", Path("/tmp/repo")) assert result.status == "completed" assert "All tests pass" in result.output + assert result.modified_files == ["src/main.py"] + + def test_zero_modified_files_is_failed(self) -> None: + """A coding run that exits 0 but changes no files must fail, not + report a false completion. (#739)""" + with patch("shutil.which", return_value="/usr/bin/claude"): + adapter = ClaudeCodeAdapter() + + mock_process = MagicMock() + mock_process.stdout = iter(["I analyzed the code but made no changes\n"]) + mock_process.stderr = MagicMock() + mock_process.stderr.read.return_value = "" + mock_process.stdin = MagicMock() + mock_process.returncode = 0 + mock_process.wait.return_value = None + + # _no_git fixture already patches _detect_modified_files -> [] + with patch("subprocess.Popen", return_value=mock_process): + result = adapter.run("task-1", "fix the bug", Path("/tmp/repo")) + + assert result.status == "failed" + assert "modified no files" in result.error + + def test_require_file_changes_can_be_disabled(self) -> None: + """Opting out restores plain exit-code mapping for analysis-only runs.""" + with patch("shutil.which", return_value="/usr/bin/claude"): + adapter = ClaudeCodeAdapter(require_file_changes=False) + + mock_process = MagicMock() + mock_process.stdout = iter(["analysis complete\n"]) + mock_process.stderr = MagicMock() + mock_process.stderr.read.return_value = "" + mock_process.stdin = MagicMock() + mock_process.returncode = 0 + mock_process.wait.return_value = None + + with patch("subprocess.Popen", return_value=mock_process): + result = adapter.run("task-1", "analyze", Path("/tmp/repo")) + + assert result.status == "completed" def test_failed_execution(self) -> None: with patch("shutil.which", return_value="/usr/bin/claude"): diff --git a/tests/core/adapters/test_subprocess_adapter.py b/tests/core/adapters/test_subprocess_adapter.py index 7733aaa6..7155c2e1 100644 --- a/tests/core/adapters/test_subprocess_adapter.py +++ b/tests/core/adapters/test_subprocess_adapter.py @@ -416,6 +416,46 @@ def test_graceful_when_not_git_repo(self, adapter, tmp_path): assert result.status == "completed" assert result.modified_files == [] + def test_require_file_changes_fails_on_empty_diff(self, tmp_path): + """With require_file_changes=True, exit 0 + no changed files -> failed. (#739)""" + with patch("shutil.which", return_value="/usr/bin/test-agent"): + adapter = SubprocessAdapter("test-agent", require_file_changes=True) + mock_process = self._make_mock_process(stdout_lines=["done\n"], returncode=0) + with ( + patch("subprocess.Popen", return_value=mock_process), + patch( + "subprocess.run", + side_effect=[ + MagicMock(returncode=0, stdout=""), + MagicMock(returncode=0, stdout=""), + ], + ), + ): + result = adapter.run("task-1", "fix", tmp_path) + + assert result.status == "failed" + assert "modified no files" in result.error + + def test_require_file_changes_passes_when_files_changed(self, tmp_path): + """require_file_changes must not disturb a run that did change files.""" + with patch("shutil.which", return_value="/usr/bin/test-agent"): + adapter = SubprocessAdapter("test-agent", require_file_changes=True) + mock_process = self._make_mock_process(stdout_lines=["done\n"], returncode=0) + with ( + patch("subprocess.Popen", return_value=mock_process), + patch( + "subprocess.run", + side_effect=[ + MagicMock(returncode=0, stdout="src/main.py\n"), + MagicMock(returncode=0, stdout=""), + ], + ), + ): + result = adapter.run("task-1", "fix", tmp_path) + + assert result.status == "completed" + assert result.modified_files == ["src/main.py"] + def test_graceful_when_git_fails(self, adapter, tmp_path): """Should return empty modified_files if git diff fails.""" mock_process = self._make_mock_process( From 6726976672ef7e3e52c8226ade4fcca99bbdf3a9 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:35:33 -0700 Subject: [PATCH 2/3] fix(adapters): don't false-fail self-committed or non-git runs (#739 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third-party (codex) review flagged that require_file_changes judged work solely by `git diff HEAD`. With bypassPermissions now allowing Bash, an agent that commits its own work leaves an empty diff, and a non-git workspace can never show changes — both would spuriously downgrade a successful run to failed. The guard now only fires when it can positively confirm no work: a resolvable git repo whose HEAD did not advance during the run AND whose tree has no changes. Captures a pre-run HEAD baseline; a moved HEAD (self-commit) or unresolvable HEAD (non-git/unborn) is treated as 'cannot fail'. Verified end-to-end against a real git repo for all four cases. --- codeframe/core/adapters/subprocess_adapter.py | 57 ++++++++++++--- tests/core/adapters/test_claude_code.py | 20 ++++-- .../core/adapters/test_subprocess_adapter.py | 72 ++++++++++++------- 3 files changed, 109 insertions(+), 40 deletions(-) diff --git a/codeframe/core/adapters/subprocess_adapter.py b/codeframe/core/adapters/subprocess_adapter.py index 71aa77ea..edba0d18 100644 --- a/codeframe/core/adapters/subprocess_adapter.py +++ b/codeframe/core/adapters/subprocess_adapter.py @@ -41,11 +41,13 @@ def __init__( binary: Name of the CLI binary (e.g., 'claude', 'opencode') cli_args: Default CLI arguments appended to every invocation timeout_s: Max execution time in seconds (default: 1800, None = no limit) - require_file_changes: If True, a run that exits 0 but modifies no files - is downgraded to ``failed`` instead of ``completed``. Guards against - a delegated coding agent that "succeeds" without writing any code - (e.g. edits silently denied), which downstream gates would otherwise - pass on the unchanged tree. Default False (analysis-capable agents). + require_file_changes: If True, a run that exits 0 without producing any + work — no modified/untracked files and no new commit — is downgraded + to ``failed`` instead of ``completed``. Guards against a delegated + coding agent that "succeeds" without writing any code (e.g. edits + silently denied), which downstream gates would otherwise pass on the + unchanged tree. Only fires inside a resolvable git repo (a non-git + workspace can't be judged). Default False (analysis-capable agents). Raises: EnvironmentError: If the binary is not found on PATH @@ -102,6 +104,12 @@ def run( cmd = self.build_command(prompt, workspace_path) stdin_content = self.get_stdin(prompt) + # Baseline HEAD so a run that *commits* its work (bypassPermissions allows + # Bash) still counts as work despite an empty `git diff HEAD`. (#739) + head_before = ( + self._git_head(workspace_path) if self._require_file_changes else None + ) + stdout_lines: list[str] = [] stderr_chunks: list[str] = [] @@ -213,17 +221,24 @@ def _write_stdin() -> None: # A coding task that "succeeds" without touching any file is a false # completion: edits were likely denied or the agent only analyzed. Fail # hard so downstream gates don't pass on the unchanged tree. (#739) + # Only fire when we can *positively* confirm no work: a resolvable git + # repo whose HEAD didn't advance (self-committed work) and whose tree has + # no changes. A non-git workspace can't be judged, so we don't fail it. if ( self._require_file_changes and result.status == "completed" and not modified_files ): - result.status = "failed" - result.error = ( - f"'{self._binary}' exited successfully but modified no files. " - "A coding task must change at least one file; the agent likely " - "lacked write permission or produced no edits." - ) + head_after = self._git_head(workspace_path) + committed = head_after is not None and head_after != head_before + in_git_repo = head_after is not None + if in_git_repo and not committed: + result.status = "failed" + result.error = ( + f"'{self._binary}' exited successfully but modified no files. " + "A coding task must change at least one file; the agent likely " + "lacked write permission or produced no edits." + ) return result @@ -267,6 +282,26 @@ def _detect_modified_files(self, workspace_path: Path) -> list[str]: """Detect files modified by the subprocess via git diff.""" return detect_modified_files(workspace_path) + def _git_head(self, workspace_path: Path) -> str | None: + """Return the current HEAD commit sha, or None if HEAD is unresolvable. + + None means "not a git repo, git unavailable, or an unborn HEAD" — i.e. + a state where modified-file detection can't judge whether work happened. + """ + try: + result = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=str(workspace_path), + capture_output=True, + text=True, + timeout=10, + ) + if result.returncode != 0: + return None + return result.stdout.strip() or None + except (FileNotFoundError, OSError, subprocess.TimeoutExpired): + return None + def _extract_blocker_question(self, output: str) -> str: """Extract a meaningful blocker question from output.""" lines = [line.strip() for line in output.splitlines() if line.strip()] diff --git a/tests/core/adapters/test_claude_code.py b/tests/core/adapters/test_claude_code.py index a7c07454..f7b60d9a 100644 --- a/tests/core/adapters/test_claude_code.py +++ b/tests/core/adapters/test_claude_code.py @@ -14,8 +14,15 @@ class TestClaudeCodeAdapter: @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=[]): + """Prevent git introspection from calling real git / the patched Popen. + + _git_head -> None simulates a non-git workspace (guard won't fire); + tests that need the guard override it with an explicit sha. + """ + with ( + patch.object(ClaudeCodeAdapter, "_detect_modified_files", return_value=[]), + patch.object(ClaudeCodeAdapter, "_git_head", return_value=None), + ): yield def test_name(self) -> None: @@ -110,8 +117,13 @@ def test_zero_modified_files_is_failed(self) -> None: mock_process.returncode = 0 mock_process.wait.return_value = None - # _no_git fixture already patches _detect_modified_files -> [] - with patch("subprocess.Popen", return_value=mock_process): + # _no_git fixture already patches _detect_modified_files -> []. + # Stub HEAD to a stable sha so the guard sees a git repo with no new + # commit (before == after) and no working-tree changes. + with ( + patch("subprocess.Popen", return_value=mock_process), + patch.object(ClaudeCodeAdapter, "_git_head", return_value="sha1"), + ): result = adapter.run("task-1", "fix the bug", Path("/tmp/repo")) assert result.status == "failed" diff --git a/tests/core/adapters/test_subprocess_adapter.py b/tests/core/adapters/test_subprocess_adapter.py index 7155c2e1..9d7e77b7 100644 --- a/tests/core/adapters/test_subprocess_adapter.py +++ b/tests/core/adapters/test_subprocess_adapter.py @@ -416,46 +416,68 @@ def test_graceful_when_not_git_repo(self, adapter, tmp_path): assert result.status == "completed" assert result.modified_files == [] - def test_require_file_changes_fails_on_empty_diff(self, tmp_path): - """With require_file_changes=True, exit 0 + no changed files -> failed. (#739)""" - with patch("shutil.which", return_value="/usr/bin/test-agent"): - adapter = SubprocessAdapter("test-agent", require_file_changes=True) + def _run_with(self, adapter, tmp_path, *, modified, head_before, head_after): + """Run with _detect_modified_files and _git_head stubbed. + + head_before/head_after become the two _git_head() return values (pre-run + baseline, then the guard's post-run check). + """ mock_process = self._make_mock_process(stdout_lines=["done\n"], returncode=0) with ( patch("subprocess.Popen", return_value=mock_process), - patch( - "subprocess.run", - side_effect=[ - MagicMock(returncode=0, stdout=""), - MagicMock(returncode=0, stdout=""), - ], + patch.object( + type(adapter), "_detect_modified_files", return_value=modified + ), + patch.object( + type(adapter), "_git_head", side_effect=[head_before, head_after] ), ): - result = adapter.run("task-1", "fix", tmp_path) + return adapter.run("task-1", "fix", tmp_path) + def test_require_file_changes_fails_on_empty_diff(self, tmp_path): + """require_file_changes=True: exit 0, no changed files, HEAD unmoved -> failed. (#739)""" + with patch("shutil.which", return_value="/usr/bin/test-agent"): + adapter = SubprocessAdapter("test-agent", require_file_changes=True) + result = self._run_with( + adapter, tmp_path, modified=[], head_before="sha1", head_after="sha1" + ) assert result.status == "failed" assert "modified no files" in result.error def test_require_file_changes_passes_when_files_changed(self, tmp_path): - """require_file_changes must not disturb a run that did change files.""" + """A run that changed working-tree files stays completed.""" with patch("shutil.which", return_value="/usr/bin/test-agent"): adapter = SubprocessAdapter("test-agent", require_file_changes=True) - mock_process = self._make_mock_process(stdout_lines=["done\n"], returncode=0) - with ( - patch("subprocess.Popen", return_value=mock_process), - patch( - "subprocess.run", - side_effect=[ - MagicMock(returncode=0, stdout="src/main.py\n"), - MagicMock(returncode=0, stdout=""), - ], - ), - ): - result = adapter.run("task-1", "fix", tmp_path) - + result = self._run_with( + adapter, + tmp_path, + modified=["src/main.py"], + head_before="sha1", + head_after="sha1", + ) assert result.status == "completed" assert result.modified_files == ["src/main.py"] + def test_require_file_changes_accepts_self_committed_work(self, tmp_path): + """An agent that commits its own work (HEAD advances) is not a false + completion even though `git diff HEAD` is empty. (#739 review)""" + with patch("shutil.which", return_value="/usr/bin/test-agent"): + adapter = SubprocessAdapter("test-agent", require_file_changes=True) + result = self._run_with( + adapter, tmp_path, modified=[], head_before="sha1", head_after="sha2" + ) + assert result.status == "completed" + + def test_require_file_changes_skips_non_git_workspace(self, tmp_path): + """Outside a git repo, HEAD is unresolvable and we cannot judge work — + do not fail the run. (#739 review)""" + with patch("shutil.which", return_value="/usr/bin/test-agent"): + adapter = SubprocessAdapter("test-agent", require_file_changes=True) + result = self._run_with( + adapter, tmp_path, modified=[], head_before=None, head_after=None + ) + assert result.status == "completed" + def test_graceful_when_git_fails(self, adapter, tmp_path): """Should return empty modified_files if git diff fails.""" mock_process = self._make_mock_process( From 6cab7ea2164961bcaa72c09004ff7ba98a30e33d Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 4 Jul 2026 16:18:32 -0700 Subject: [PATCH 3/3] fix(adapters): require known HEAD baseline to credit a commit (#739 re-review) Internal re-review: a transient pre-run _git_head failure (head_before=None) plus a successful post-run read made 'committed' true via 'None != sha', silently reopening the false-completion hole. Require head_before is not None so an unknown baseline can't fake a commit; bias toward failing loudly. --- codeframe/core/adapters/subprocess_adapter.py | 11 ++++++++++- tests/core/adapters/test_subprocess_adapter.py | 11 +++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/codeframe/core/adapters/subprocess_adapter.py b/codeframe/core/adapters/subprocess_adapter.py index edba0d18..dd76f0fc 100644 --- a/codeframe/core/adapters/subprocess_adapter.py +++ b/codeframe/core/adapters/subprocess_adapter.py @@ -230,8 +230,17 @@ def _write_stdin() -> None: and not modified_files ): head_after = self._git_head(workspace_path) - committed = head_after is not None and head_after != head_before in_git_repo = head_after is not None + # Require a known baseline to credit a commit: if the pre-run HEAD + # read failed (head_before is None) we must not let `None != sha` + # masquerade as "committed" and silently pass a real zero-file run. + # Bias toward failing loudly. (ponytail: a rare `git init` mid-run + # false-fails here — acceptable; a false COMPLETED is worse.) + committed = ( + head_before is not None + and head_after is not None + and head_after != head_before + ) if in_git_repo and not committed: result.status = "failed" result.error = ( diff --git a/tests/core/adapters/test_subprocess_adapter.py b/tests/core/adapters/test_subprocess_adapter.py index 9d7e77b7..54744cd5 100644 --- a/tests/core/adapters/test_subprocess_adapter.py +++ b/tests/core/adapters/test_subprocess_adapter.py @@ -468,6 +468,17 @@ def test_require_file_changes_accepts_self_committed_work(self, tmp_path): ) assert result.status == "completed" + def test_require_file_changes_fails_when_pre_run_head_unavailable(self, tmp_path): + """A transient pre-run HEAD read failure must not let `None != sha` fake + a commit and pass a real zero-file run. Fail safe. (#739 re-review)""" + with patch("shutil.which", return_value="/usr/bin/test-agent"): + adapter = SubprocessAdapter("test-agent", require_file_changes=True) + # head_before=None (pre-run read failed), head_after="sha1" (post succeeds). + result = self._run_with( + adapter, tmp_path, modified=[], head_before=None, head_after="sha1" + ) + assert result.status == "failed" + def test_require_file_changes_skips_non_git_workspace(self, tmp_path): """Outside a git repo, HEAD is unresolvable and we cannot judge work — do not fail the run. (#739 review)"""