fix(adapters): harden claude-code require_file_changes + bypassPermissions (#819)#865
Conversation
…sions (#819) Four follow-ups from the #739 / PR #818 review. 1. Dangerous-command guard for bypassPermissions (security). The built-in ReAct engine filters every Bash command through is_dangerous_command(); the delegated claude CLI got unrestricted Bash with no CodeFrame-side equivalent, and #565 turns externally-authored issue bodies into task prompts. Register a PreToolUse hook (which fires even under bypassPermissions — the mode skips approval prompts, not hooks) that reuses is_dangerous_command verbatim, so both engines block the same set from one source of truth. Attached on the allowlist path too: an allowlist granting Bash has the same blast radius. 2. Route exit-0 clarifications to a blocker instead of hard-failing. --print cannot prompt, so a genuine ambiguity gets printed and the process exits clean; the zero-file guard then hard-failed it with a misleading "likely lacked write permission". Classify stdout first and raise a blocker a human can answer. Tactical questions still fail — by design those are the agent's own job. 3. Derive require_file_changes from what the allowlist grants, not from whether one was passed. A read-only allowlist means an analysis run, which legitimately writes nothing; it no longer silently hard-fails. 4. Log git errors in detect_modified_files / _git_head. Empty now means failed, so a transient git hiccup was indistinguishable from "the agent changed nothing".
|
Claude finished @frankbria's task in 7m 41s —— View job ✅ Bug-hunting review: no defects foundReviewed PR #865 for concrete defects (logic errors, security, data loss, race conditions). Focused only on the 4 source hunks and their integration points — style/coverage/test architecture are out of scope (covered elsewhere).
Integration points verified against the checked-out tree:
Candidate findings considered and rejected (no concrete failure scenario that isn't already an explicitly-disclosed limitation):
No inline comments posted. No files modified, no commits pushed. |
ReviewThis PR closes #819 with four hardening follow-ups to A few things worth a look:
Nothing here blocks merging — the stdout-only classification and |
…#819) Addresses post-PR review feedback on #865. Guard import weight (verified + measured). The guard is spawned as a fresh subprocess for every Bash call the delegated CLI makes, so its import cost is paid per command. Importing is_dangerous_command from core.executor pulled 930 modules including the openai SDK -- ~423ms per Bash call for what is a regex match. Two causes, both fixed: - core/executor.py imports codeframe.adapters.llm -> openai. Patterns extracted to a stdlib-only leaf, core/dangerous_commands.py; executor re-exports both names so the import path and the single source of truth are preserved. - core/adapters/__init__.py eagerly imports every adapter (and so the SDKs again), so any module in that package pays it. Guard moved to core/claude_code_guard.py, outside the package. 423ms -> 142ms per Bash call. A regression test asserts the guard imports no LLM SDK and that both call sites share one function object. require_file_changes polarity. _WRITE_TOOLS was a hardcoded set of write tools, so an unrecognized name (an MCP write tool, Task, anything added later) read as read-only -- silently disabling the zero-file guard and re-opening the #739 false completion. Inverted to _READ_ONLY_TOOLS: the unknown case now means write-capable, which fails loudly instead. A misread read-only tool only costs a noisy failure on an analysis run; a misread write tool costs a silent false pass. Re-verified live against the real claude CLI after the move: a stale hook path would fail open silently, so the deny path was re-tested end-to-end rather than trusted to the unit tests.
Post-PR cross-family review + feedback triageReviewer chain: opencode (GLM) is the preferred primary, but it timed out twice at 420s/540s returning zero bytes — unavailable in practice. Fell back to
Fixed (2)1. Guard import weight — confirmed, and worse than reported. Measured before assuming:
The guard is a fresh subprocess per Bash call, so this is paid every command — ~21s of pure overhead on a 50-command task. The bot identified 2. Rebutted (3)3. stderr not classified on the exit-0 path. stdout is where a 4. 5. Dedupe the two git Verification of the fixes
🤖 Generated with Claude Code |
|
Claude finished @frankbria's task in 9m 29s —— View job 🔍 Precision bug-hunting reviewScope: concrete defects only (logic errors, null derefs, security, data loss, races, breaking contract changes, resource leaks). Style/coverage/architecture are out of scope — covered by the general review.
✅ GLM review: no defects found. Read every source hunk ( Candidate concerns checked and cleared (each has no concrete failure scenario that isn't already an explicitly-disclosed limitation or pre-existing behavior):
The "false blocker on a benign stdout mention of an access term" case is real but only fires when the agent wrote nothing, routes to a human (the designed fallback), and is disclosed in the PR's Known Limitations — not a defect. No files modified, no commits pushed. |
Demo verification — outcome evidence per acceptance criterionRe-run against the final commit ( Item 1 — dangerous-command guard (live, against the real
|
| command | outcome |
|---|---|
dd if=/dev/zero of=/dev/null count=1 |
denied before reaching the shell — delegated agent received: CodeFrame blocked this command: dd writing to device. Destructive commands are refused for delegated runs; achieve the task another way. |
echo SAFE-OK |
allowed → returned SAFE-OK (guard discriminates; it isn't blanket-blocking) |
Re-verified on the new -m codeframe.core.claude_code_guard path after the module move — a stale hook path fails open silently, so unit tests passing would have proven nothing.
Item 2 — exit-0 clarification → blocker, not hard failure
requirements ambiguity -> blocked q='The specification unclear: should deletes cascade?'
access issue -> blocked q='Cannot continue: the credentials missing for the repo'
no signal -> failed (hard fail, unchanged)
tactical question -> failed (agent's own job, by design)
Item 3 — require_file_changes derived from what the allowlist grants (post-inversion)
allowlist=None -> True (bypass mode grants writes)
allowlist=['Read', 'Grep'] -> False (analysis run writes nothing)
allowlist=['Read', 'Write'] -> True
allowlist=['Bash(git *)'] -> True (rule-shaped entry parsed)
allowlist=['mcp__fs__write_file'] -> True <- unknown tool, fails SAFE
allowlist=['Task'] -> True <- unknown tool, fails SAFE
allowlist=['Read(src/**)'] -> False
explicit require_file_changes=True on read-only -> True (caller still wins)
Item 4 — git errors distinguishable from "agent did nothing"
LOG WARNING: git diff failed in /tmp/x (exit 128): not a git repository —
reporting no modified files, which callers may read as 'agent did nothing'.
Gates
- Local suite on clean source: 4275 passed, 12 skipped, 0 failed;
ruff check .clean. - All GitHub CI checks green on
a9d8606. - Test-mutation check: all 5 new behaviors fail when deliberately broken.
- Diff coverage 96.67% on changed modules.
- Docs sync: no stale docs (no doc describes the adapter permission model; CHANGELOG entry correctly omitted per repo convention — [P1.12]
claude-codeengine denies file edits by default (--printwith no permission config) #739/fix(adapters): grant claude-code write permissions + fail on zero-file completion (#739) #818 got none either).
🤖 Generated with Claude Code
1 participant
Closes #819. Four follow-ups from the #739 / PR #818 review — none blocked that merge.
1. Dangerous-command guard for
bypassPermissions(security)The built-in ReAct engine runs every Bash command through
is_dangerous_command()(core/executor.py:52, called atcore/tools.py:802).ClaudeCodeAdapter's default--permission-mode bypassPermissionshanded the delegatedclaudeCLI unrestricted Bash with no CodeFrame-side equivalent — and #565 turns externally-authored issue bodies into task prompts.Approach: a PreToolUse hook, not a narrower allowlist. Verified against the hooks docs: PreToolUse hooks fire even under
bypassPermissions— the mode skips approval prompts, not hooks. The alternatives lose:--allowedToolsdoes prefix matching, so it cannot express the existing regexes;Bash(rm:*)-style deny rules are simultaneously lossy and over-broad.bypassPermissionsregresses [P1.12]claude-codeengine denies file edits by default (--printwith no permission config) #739 (writes get silently denied in--print).The hook re-uses
is_dangerous_command()verbatim, so both engines block the same set from one source of truth. It's attached on the allowlist path too — an allowlist grantingBashcarries the same blast radius.2. Exit-0 clarifications route to a blocker, not a hard failure
--printcannot prompt, so a genuine ambiguity gets printed and the process exits clean. The zero-file guard then hard-failed it with a misleading "likely lacked write permission". Now stdout is classified first, and requirements/access/external-service ambiguity raises a blocker a human can answer.Tactical questions ("which approach…") still hard-fail —
classify_error_for_blockerreturnsNonefor those by design: they're the agent's own job, not a human's.3.
require_file_changesfollows what the allowlist grantsWas coupled to whether an allowlist was passed. A read-only allowlist (
["Read","Grep"]) means an analysis run that legitimately writes nothing, but still defaulted to hard-failing. Now derived from whether the allowlist grants a write tool (Edit/Write/MultiEdit/NotebookEdit/Bash, parsed off rule-shaped entries likeBash(git *)). ExplicitTrue/Falsestill wins. Not reachable today (engine_registrynever passesallowlist) — this closes the trap before the next caller hits it.4. Git errors are logged
detect_modified_files/_git_headswallowed every git error. Since empty→failed, a transient hiccup was indistinguishable from "the agent changed nothing".Verification
Full CI gate:
4270 passed, 12 skipped, 0 failed.ruff check .clean.Guard proven end-to-end against the real
claudeCLI — not just mocked stdin. With the adapter's actual--settingspayload,dd if=/dev/zero of=/dev/null(matches a dangerous pattern; harmless if it ran) was denied before reaching the shell, and the reason surfaced to the delegated agent:A safe command (
echo SAFE-OK) passed through the same hook, confirming the guard discriminates rather than blanket-blocking.codex reviewon the branch diff: no actionable correctness issues (it independently exercised the guard).Known limitations
head -c 512 /dev/zero > /dev/null) unprompted. It raises the floor for accidental and injected destructive commands; it does not stop an agent that means to evade it. Real containment needs the worktree/E2B isolation paths.sys.executable -m codeframe.core.claude_code_guard.sys.executableis the interpreter already running CodeFrame, socodeframeis importable in practice — but if the import did fail, Python exits 1, which Claude Code treats as a non-blocking hook error and the command proceeds. Exit code 2 (fail-closed) is unreachable from a module that failed to import.bypassPermissionspath not exercised live. The end-to-end demo ran under--allowedToolsbecause this dev sandbox blocks spawning a nestedclaude --permission-mode bypassPermissions. The hook is registered identically on both paths and the docs are explicit that hooks fire underbypassPermissions; the mechanism itself is proven.Review round (post-PR)
Full triage in this comment. Two fixes applied on top of the original commit:
claudebot, verified + measured): the guard is a fresh subprocess per Bash call, and importingis_dangerous_commandfromcore.executorpulled 930 modules incl. the openai SDK — ~423ms per Bash call. Patterns extracted to a stdlib-only leaf (core/dangerous_commands.py, re-exported byexecutorto keep one source of truth) and the guard moved out of the eagerly-importingcore/adapterspackage. 423ms → 142ms.require_file_changespolarity: inverted_WRITE_TOOLS→_READ_ONLY_TOOLSso an unrecognized tool (MCP write tool,Task, future tools) is assumed write-capable and fails loudly, rather than silently disabling the zero-file guard.Three findings rebutted in writing (stderr classification, last-line blocker extraction, git-logging dedupe). Cross-family review:
codex(opencode timed out twice with zero output — noted per the invariants). Guard re-verified live against the real CLI after the module move; test-mutation check passes on all 5 new behaviors; diff coverage 96.67%.🤖 Generated with Claude Code