fix(adapters): grant claude-code write permissions + fail on zero-file completion (#739)

[frankbria]

Closes #739

Problem

ClaudeCodeAdapter built ["--print"] with no permission config. In non-interactive
--print mode Claude Code auto-denies Edit/Write/Bash, so the delegated agent could analyze
but never modify files. Combined with P0.5's gates-on-unchanged-tree, tasks were marked
COMPLETED with zero code written — the exact failure the audit flagged.

Fix

1. Permission config (claude_code.py)
Default path (no allowlist — the only production caller) now appends
--permission-mode bypassPermissions, so Edit/Write/Bash are auto-approved in --print mode.
Chose bypassPermissions over the acceptEdits example in the issue because acceptEdits
alone still leaves Bash denied. The explicit-allowlist API path is unchanged.

2. Zero-modified-files hard error (subprocess_adapter.py)
Base SubprocessAdapter gains an opt-in require_file_changes flag (default False, so
codex/opencode and all plumbing tests are unaffected). ClaudeCodeAdapter defaults it True:
a run that exits 0 but modifies no files is downgraded completed → failed with a clear error,
so downstream gates can no longer pass on an unchanged tree.

Acceptance criteria

• Engine passes an explicit permission config covering Edit/Write/Bash
• Hard error surfaces when the agent reports zero modified files on a coding task

Verification

• Built command inspected: claude --print --permission-mode bypassPermissions — accepted by the real CLI (claude --help confirms the flag/mode).
• require_file_changes=True guard exercised in unit tests (zero-file → failed, files-changed → completed); opt-out restores plain exit-code mapping.
• Tests: full tests/core suite 2380 passed; adapters+registry 151 green; ruff check clean.

Known limitations

• bypassPermissions grants full Bash to the delegated agent — appropriate for an autonomous
  coder running in the workspace, matching the built-in ReAct engine's tool access.
• require_file_changes is enabled only for claude-code; codex/opencode keep exit-code
  mapping until a false-completion is observed there (default-off flag, opt-in later).

Summary by CodeRabbit

• New Features

  • Added a safer default permission mode for runs without an allowlist, enabling non-interactive approvals for common actions.
  • Added an option to require actual file changes before a run is considered successful.

• Bug Fixes

  • Runs that report success but make no changes now fail instead of appearing completed.
  • Analysis-only runs can still be allowed to complete when file changes are not required.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e51b23a8-6ab4-42bb-8d3e-7cdd77ed9a1b

📥 Commits

Reviewing files that changed from the base of the PR and between efc4e93 and e64ee70.

📒 Files selected for processing (4)

• codeframe/core/adapters/claude_code.py
• codeframe/core/adapters/subprocess_adapter.py
• tests/core/adapters/test_claude_code.py
• tests/core/adapters/test_subprocess_adapter.py

---

Walkthrough

ClaudeCodeAdapter now accepts a require_file_changes parameter and defaults to injecting --permission-mode bypassPermissions when no allowlist is set. SubprocessAdapter adds git HEAD tracking to detect and fail runs that report success but produce no file changes. Tests cover both behaviors.

Changes

Permission defaults and file-change enforcement

Layer / File(s)	Summary
ClaudeCodeAdapter permission mode and require_file_changes wiring codeframe/core/adapters/claude_code.py	__init__ accepts require_file_changes: bool = True, forwards it to SubprocessAdapter, and adds --permission-mode bypassPermissions when no allowlist is provided instead of adding no permission flags.
SubprocessAdapter require_file_changes flag and git HEAD capture codeframe/core/adapters/subprocess_adapter.py	__init__ stores a new require_file_changes flag (default False); run() captures head_before via a new _git_head() helper that safely runs git rev-parse HEAD.
Post-run validation for empty diffs codeframe/core/adapters/subprocess_adapter.py	run() downgrades a completed result with zero modified files to failed unless git HEAD advanced (self-committed work) or the workspace is non-git.
ClaudeCodeAdapter tests for permissions and file-change enforcement tests/core/adapters/test_claude_code.py	Tests assert default bypassPermissions, allowlist behavior, modified_files propagation, failure on zero modified files, and completion when require_file_changes=False.
SubprocessAdapter tests for git HEAD-based validation tests/core/adapters/test_subprocess_adapter.py	New _run_with helper and tests cover empty-diff failure, passing on modified files, self-committed HEAD advancement, unavailable pre-run HEAD, and non-git workspace skip.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

• [P1.12] claude-code engine denies file edits by default (--print with no permission config) #739: This PR directly fixes the reported issue by adding a default permission mode and a hard failure when zero files are modified on successful runs.
• [P2.23] claude-code engine: harden require_file_changes + bypassPermissions follow-ups (from #739 review) #819: Overlaps directly with the same ClaudeCodeAdapter bypassPermissions and require_file_changes behavior.

Possibly related PRs

• frankbria/codeframe#436: Builds on the git-diff-based modified-files detection this PR extends with require_file_changes and _git_head().

Poem

A rabbit hopped through code so tight,
"No more silent fails," it cheered with delight,
HEAD before, HEAD after, watched with care,
bypassPermissions now floats in the air,
Edits land safe, no work left behind — 🐇✨

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/739-claude-code-permissions

---

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

[claude]

Review: fix(adapters): grant claude-code write permissions + fail on zero-file completion (#739)

This is a well-scoped fix for a real bug (COMPLETED tasks with zero code written), and both halves of the change are covered by targeted new tests. No prior CodeRabbit review has posted yet (it hit a rate limit), so this is the first substantive review on the PR — no earlier comments to reconcile with.

Findings

1. bypassPermissions grants broader Bash access than the ReAct engine it's compared to (security — worth a follow-up issue, not a blocker)
The PR description says this "match[es] the built-in ReAct engine's tool access," but that's not quite accurate: the built-in engine's Bash tool runs every command through is_dangerous_command() (codeframe/core/executor.py:52, used at codeframe/core/tools.py:802-807) to block destructive patterns (e.g. rm -rf, disk-wiping commands) before execution. --permission-mode bypassPermissions hands the delegated claude CLI subprocess unrestricted Bash with no equivalent CodeFrame-side filter — the external CLI has no knowledge of that blocklist. Given the GitHub Issues import feature (#565) can turn externally-authored issue bodies into task prompts, this widens the prompt-injection blast radius for the claude-code engine specifically vs. react. This may well be an acceptable tradeoff (the "Known limitations" section already flags bypassPermissions as broad), but I'd soften the "matching ReAct" framing in the description/docs and consider tracking the missing dangerous-command guard as a fast-follow rather than treating it as equivalent.

2. Zero-file detection failures and zero-file truth are indistinguishable (correctness edge case)
_detect_modified_files -> detect_modified_files() (codeframe/core/adapters/git_utils.py) silently returns [] on any git error — non-repo workspace, git binary missing, or a subprocess.TimeoutExpired/transient failure (e.g. lock contention). Before this PR that was fine because modified_files was purely informational. Now, for claude-code (require_file_changes=True by default), an empty list is treated as proof the agent wrote nothing and downgrades a genuinely successful completed run to failed. A transient git diff/git ls-files hiccup (or a workspace that briefly isn't a clean git repo) would now cause a real false failure, not just a missing detail. Worth at least logging when detection itself errors out (vs. cleanly returning empty), so this failure mode is distinguishable from "the agent truly changed nothing" in logs/diagnostics.

3. require_file_changes default is coupled to "no allowlist," not to allowlist content
ClaudeCodeAdapter(require_file_changes=True) is the default regardless of whether an allowlist is passed. The PR description says "the explicit-allowlist API path is unchanged" (true for the permission-mode flag), but require_file_changes still defaults on even if a caller passes a read-only allowlist (e.g. ["Read", "Grep"]) for an analysis-only task. Not exploitable today since engine_registry.get_external_adapter is the only production caller and never passes allowlist (confirmed via grep), but any future caller using the allowlist path for a read-only task will silently get hard failures unless it also remembers to pass require_file_changes=False. Might be worth deriving the default from whether the allowlist grants write tools, or at least calling out this coupling in the docstring.

Other notes

• Test coverage is solid: default-permission-mode, explicit-allowlist (no bypass), zero-file->failed, and opt-out->completed are all covered at both the ClaudeCodeAdapter and base SubprocessAdapter levels.
• No test exercises the "detection itself failed" vs. "detection succeeded and found nothing" distinction raised in (2) — worth adding if the logging suggestion above is taken.
• The stderr/stdout draining and stdin-write threading in subprocess_adapter.py is untouched by this diff and looks pre-existing — not part of this review.

Nothing here should block merging; (1) and (2) are the ones I'd actually want a follow-up ticket for.

[frankbria]

Third-party review (codex, GLM/opencode timed out)

Ran codex review on the branch diff. One real finding, now fixed in 0f-review commit:

[High → fixed] 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.

Resolution: the guard now captures a pre-run HEAD baseline and only fires when it can positively confirm no work — a resolvable git repo whose HEAD did not advance AND whose tree has no changes. 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:

scenario	result
no-op run	failed ✓
writes a file	completed ✓
self-commits (empty git diff HEAD, HEAD moved)	completed ✓
non-git workspace	completed ✓

No codex/opencode adapter regression (their require_file_changes defaults False).

[claude]

Re-review: fix(adapters): grant claude-code write permissions + fail on zero-file completion (#739)

This is a follow-up review of the updated diff (commit 0f-review), which addresses the high-severity finding from the third-party codex review (self-committed work / non-git workspace spuriously downgraded to failed) via the new _git_head pre/post-run baseline. That fix is sound for the two scenarios it targets, and the test additions (test_require_file_changes_accepts_self_committed_work, test_require_file_changes_skips_non_git_workspace) directly cover them. Nothing here should block merging — these are residual, lower-severity notes on the new guard logic.

Findings

1. subprocess_adapter.py:227-241 — the hard-failure path can misfire on a legitimate need-clarification response, not just a permission failure.
   _map_result only runs classify_error_for_blocker on a non-zero exit code; a --print run that exits 0 after printing "I need clarification on X, please confirm before I proceed" (a very plausible non-interactive response to an ambiguous task) was previously mapped to completed with no blocker created, and now gets hard-failed here instead, since modified_files is empty and head_after == head_before. The result is a task marked failed (no blocker_question, no entry in blockers, per runtime.py:817-830) instead of routed through the blocker flow a human could actually answer. The error message text ("likely lacked write permission or produced no edits") is hedged but still steers whoever reads it toward the wrong cause. Worth considering: peek at stdout for a question/blocker-shaped response before hard-failing, or at least route this case through classify_error_for_blocker too.

2. subprocess_adapter.py:107-111, 232-234 — a transient failure on the pre-run _git_head call can silently reopen the false-completion hole this PR is fixing.
   committed = head_after is not None and head_after != head_before. If the pre-run baseline call hits a transient error (e.g. git lock contention right as the subprocess is about to start) and returns None, while the post-run call succeeds and returns the unchanged current sha, committed evaluates True purely because None != <sha> — even though nothing was actually committed. In that narrow window, a genuine zero-file completion (the exact case this PR exists to catch) would silently pass through as completed again. Rare (requires the two _git_head calls to differ in reliability within one run), but it means the class of "transient git failure undermines the guard" concern from the prior review isn't fully closed, just shifted to a narrower trigger. Neither _git_head nor detect_modified_files logs when they hit an actual error (as opposed to "not a git repo") — a logger.debug/warning in the except branches would make this failure mode visible in diagnostics instead of indistinguishable from "nothing to detect."

Already-noted, still open (no new action needed here)

• require_file_changes still defaults to True regardless of whether a future caller passes a read-only allowlist (raised in my prior review as finding 3) — still unaddressed, still low-priority since engine_registry.get_external_adapter remains the only production caller and never passes allowlist.
• The bypassPermissions vs. ReAct-engine Bash-filtering gap (prior finding 1) is unchanged by this diff — still worth a follow-up issue per the PR's own "Known limitations" section, not a blocker.

Test coverage for the new HEAD-baseline logic is solid (empty-diff-but-committed, non-git-workspace, and the original zero-file/opt-out cases are all exercised at both ClaudeCodeAdapter and base SubprocessAdapter levels).

[frankbria]

Triaged internal-review findings. Addressed now: the re-review's HEAD-baseline false-pass hardening (commit 6cab7ea — require head_before is not None to credit a commit, fail-safe). The remaining advisory findings (bypassPermissions dangerous-command gap, exit-0 clarification→blocker routing, require_file_changes/allowlist coupling, git-error logging) are non-blocking design follow-ups tracked in #819.

[claude]

Re-review: fix(adapters): grant claude-code write permissions + fail on zero-file completion (#739)

Third pass, covering the latest commit (6cab7ea) plus the triage comment. This commit does exactly one thing: closes finding (2) from my prior re-review — the transient pre-run _git_head failure that let None != sha masquerade as "committed."

Verified fix is correct. committed now requires head_before is not None in addition to head_after is not None and head_after != head_before (subprocess_adapter.py:239-243). Traced through all four cases:

• head_before="sha1", head_after="sha1" (no-op) → not committed, in repo → failed ✓
• head_before="sha1", head_after="sha2" (self-commit) → committed → completed ✓
• head_before=None, head_after="sha1" (transient pre-run failure, the bug this commit fixes) → head_before is not None is False → not committed → in_git_repo is True (head_after resolved) → correctly fails instead of silently passing ✓
• head_before=None, head_after=None (non-git workspace) → in_git_repo is False → guard doesn't fire → completed ✓

All four are directly exercised by the new tests (test_require_file_changes_fails_when_pre_run_head_unavailable, test_require_file_changes_skips_non_git_workspace, plus the pre-existing empty-diff and self-commit cases). Good, tight coverage — this is exactly the kind of test I'd want for a fail-safe boolean flip.

One residual asymmetry, not a regression, worth being aware of: because committed now requires both head_before and head_after to resolve, a transient pre-run hiccup combined with a genuine self-commit (head_before=None, agent commits, head_after="shaX") will now be misclassified as failed even though real work happened — trading the prior false-completed for an occasional false-failed. The inline comment already calls this out explicitly ("a rare git init mid-run false-fails here — acceptable; a false COMPLETED is worse"), so this is a deliberate, documented tradeoff rather than an oversight. Given the goal of this PR (stop false completions), biasing toward a loud failure over a silent pass is the right call — no action needed.

Everything else already triaged. The three other advisory findings from my previous reviews (bypassPermissions vs. ReAct's is_dangerous_command() gap, exit-0-with-a-clarification-question misrouted to failed instead of blocked, and require_file_changes defaulting on regardless of a future read-only allowlist) are unchanged by this commit and are already tracked in #819 per the owner's triage comment above — not repeating them here.

No blocking issues. This is ready to merge as far as the adapter logic goes.

[claude]

Re-review: fix(adapters): grant claude-code write permissions + fail on zero-file completion (#739)

Checked the current head against my last re-review (commit 6cab7ea). The only new commit is the merge of origin/main into this branch, which doesn't touch any of the four changed files (claude_code.py, subprocess_adapter.py, and their tests) — the adapter logic, HEAD-baseline guard, and test coverage are byte-identical to what I already verified and signed off on.

No new findings. Standing position unchanged: no blocking issues, ready to merge as far as the adapter logic goes. The four advisory items from my earlier passes (bypassPermissions vs. ReAct's is_dangerous_command() gap, exit-0-with-clarification misrouted to failed instead of blocked, require_file_changes/read-only-allowlist coupling, and git-error-vs-truly-empty logging) remain correctly triaged as non-blocking follow-ups tracked in #819 per the owner's earlier comment — not repeating the detail here since nothing has changed.