diff --git a/.github/workflows/claude-adversarial-review.yml b/.github/workflows/claude-adversarial-review.yml index e393a18..9b2ff30 100644 --- a/.github/workflows/claude-adversarial-review.yml +++ b/.github/workflows/claude-adversarial-review.yml @@ -182,7 +182,7 @@ jobs: id: claude # Pinned by full SHA; claude-review.yml's action step says why this # version and how the pin moves. - uses: anthropics/claude-code-action@a4f54ef2c58884867281bd8e2f8d63352ad019a9 # v1.0.229 + uses: anthropics/claude-code-action@a26b07d8c778b0c43f64618e5fd29fd0f8af72e8 # v1.0.250 # Attributes come from the base commit, never from the PR. `gh pr diff` # is GitHub's server-side diff, which ignores .gitattributes, but the # allowlisted `git log` prints patches too (-p), and git takes @@ -245,10 +245,18 @@ jobs: # posts. `grep` covers the searching. verifier-on-high-risk.yml's # allowlist comment lists the same gadgets (rg, sed, awk). Pinned by # selftest/test_review_lanes_comment_no_shell.py. + # + # --permission-mode default keeps that allowlist binding: the Agent + # SDK the action bundles stopped sending it at 0.3.286 (first in + # v1.0.238; v1.0.250 bundles 0.3.300), which leaves + # the mode to the base branch's .claude/settings.json. Not `auto`. + # claude-review.yml's claude_args comment says why. Guarded by + # selftest/test_claude_lanes_permission_mode.py. claude_args: | --model claude-sonnet-4-6 --max-turns 40 --allowedTools "Bash(gh pr diff:*),Bash(gh pr view:*),Bash(grep:*),Bash(git log:*)" + --permission-mode default # THE VERDICT IS POSTED HERE, NOT BY THE MODEL, for the reason # claude-review.yml's "Post review summary" step gives: posting through diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 8ec0099..1e4c24c 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -560,16 +560,28 @@ jobs: # guard below, which recovers whatever was posted before the cut. timeout-minutes: 30 # Pinned by full SHA and bumped by manual PR, for this lane and the - # adversarial and verifier lanes alike. v1.0.229 is the newest release - # reviewed: later ones bundle Claude Code 2.1.277+, which the - # adversarial and verifier lanes would run (they use the bundled CLI; - # this lane runs the version pinned above), and that CLI's handling of - # project files has not been reviewed for them yet. The pin moved off - # @v1 after v1.0.113/.114 broke the CLI install (ci-workflows#37). - # A bump must also re-check what the caller check relies on: a - # `github_token` input skips the OIDC exchange and is never revoked, - # and a validation skip still exits 0 with no `conclusion`. - uses: anthropics/claude-code-action@a4f54ef2c58884867281bd8e2f8d63352ad019a9 # v1.0.229 + # adversarial and verifier lanes alike; the pin moved off @v1 after + # v1.0.113/.114 broke the CLI install (ci-workflows#37). The Claude + # Code CLI a release bundles does not reach any lane: each runs its + # own pinned CLI through path_to_claude_code_executable + # (selftest/test_claude_lanes_cli_pin.py), so a CLI change is a + # CLAUDE_CODE_VERSION bump, never a side effect of this one. A bump + # must re-read the new release's source for: + # - what the caller and review-ran checks rely on: a `github_token` + # input skips the OIDC exchange (src/github/token.ts) and is never + # revoked (action.yml revokes only when it is ''), and a workflow + # validation skip exits 0 with no `conclusion` + # (src/entrypoints/run.ts). Then move the pin in + # selftest/test_claude_review_ran_check.sh; + # - the composite's step order: "Post buffered inline comments" + # still runs inside this step, before the lost-findings guard; + # - the permission mode the bundled Agent SDK sends. Up to 0.3.276 + # (v1.0.229) it sent `--permission-mode default` on its own; + # 0.3.300 (v1.0.250) sends none, which is why every lane's + # claude_args carries it (see below), and claude_args must still + # reach the CLI as `--permission-mode default` + # (base-action/src/parse-sdk-options.ts). + uses: anthropics/claude-code-action@a26b07d8c778b0c43f64618e5fd29fd0f8af72e8 # v1.0.250 with: anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} # The job token when this PR changes its calling workflow, or when @@ -666,12 +678,26 @@ jobs: # final message, which the "Post review summary" step posts. When # the model posted it through the shell, a code span in it ran as a # command (see that step). + # + # --permission-mode default: the mode the allowlist is written for. + # In it a tool that needs permission (Bash, edits, MCP tools) runs + # only when --allowedTools allows it, since a headless run has no one + # to ask. The Agent SDK the action bundles sent this flag on its own + # up to 0.3.276; 0.3.300 sends no mode, and the CLI then takes + # `permissions.defaultMode` from the caller's .claude/settings.json, + # which the action restores from the PR's base branch (not always + # the default branch). `bypassPermissions` or `acceptEdits` there + # would let tools outside the allowlist run. Not `auto` either: it + # runs whatever its classifier approves, allowlisted or not. The + # adversarial and verifier lanes pass the same flag. Guarded by + # selftest/test_claude_lanes_permission_mode.py. claude_args: | --model claude-sonnet-4-6 --max-turns ${{ inputs.max_turns }} --max-budget-usd ${{ inputs.max_budget_usd }} --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*)" --disallowedTools "Task,Agent,Skill,Workflow" + --permission-mode default # REVIEW-RAN CHECK. claude-code-action can end its step `success` having # reviewed nothing: on the workflow-validation skip (see "Caller check") diff --git a/.github/workflows/verifier-on-high-risk.yml b/.github/workflows/verifier-on-high-risk.yml index c24b81b..30455b7 100644 --- a/.github/workflows/verifier-on-high-risk.yml +++ b/.github/workflows/verifier-on-high-risk.yml @@ -521,7 +521,7 @@ jobs: # CLI install and well under 1 more on real runs) and the reporting # after it. Real runs take about 2 minutes in all. timeout-minutes: 10 - uses: anthropics/claude-code-action@a4f54ef2c58884867281bd8e2f8d63352ad019a9 # v1.0.229 + uses: anthropics/claude-code-action@a26b07d8c778b0c43f64618e5fd29fd0f8af72e8 # v1.0.250 # Attributes come from the base commit, never from the PR. The model # reads each file with `git diff` in the PR checkout, and git takes # .gitattributes from that checkout: a path the PR itself marks @@ -589,10 +589,19 @@ jobs: # on detection. Read-tool path denies cover the same surface for # the Read tool. The redactor + per-file-list artifact upload # are the last-line defenses. + # + # --permission-mode default keeps the allowlist and denylist above + # binding: the Agent SDK the action bundles stopped sending it at + # 0.3.286 (first in v1.0.238; v1.0.250 bundles 0.3.300), which leaves + # the mode to the base branch's + # .claude/settings.json. Not `auto`. claude-review.yml's claude_args + # comment says why. Guarded by + # selftest/test_claude_lanes_permission_mode.py. claude_args: | --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","transcript_markdown"],"properties":{"verdict":{"type":"string","enum":["PASS","FAIL"]},"transcript_markdown":{"type":"string","minLength":200,"maxLength":50000}}}' --allowedTools "Read,Bash(git diff:*),Bash(git log:*),Bash(git show:*),Bash(git ls-files:*),Bash(grep:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(wc:*),Bash(ls:*),Bash(find:*),Bash(file:*)" --disallowedTools "Write,Edit,MultiEdit,NotebookEdit,Read(//proc/**),Read(//sys/**),Read(//etc/**),Read(//var/**),Read(//root/**),Read(//tmp/**),Read(~/**)" + --permission-mode default - name: Materialize evidence from structured_output if: steps.classify.outputs.matched == 'yes' diff --git a/selftest/test_claude_lanes_permission_mode.py b/selftest/test_claude_lanes_permission_mode.py new file mode 100644 index 0000000..d767204 --- /dev/null +++ b/selftest/test_claude_lanes_permission_mode.py @@ -0,0 +1,237 @@ +"""Every Claude review lane runs the CLI in permission mode `default`. + +claude-code-action builds the CLI's arguments through the Agent SDK it bundles. +Up to SDK 0.3.276 (action v1.0.229) the SDK passed `--permission-mode default` +whenever the caller set no mode; from 0.3.286 (first in action v1.0.238; +v1.0.250 bundles 0.3.300) it passes nothing, and the CLI takes its mode from the settings it loads. The action +loads the user, project and local sources, and the project's +.claude/settings.json is the caller's, restored from the PR's base branch, +which is not always the default branch. A `permissions.defaultMode` there +(`bypassPermissions`, `acceptEdits`, `auto`, ...) would then pick the +reviewer's mode, and `bypassPermissions`, `acceptEdits` and `auto` each let +some tool outside the lane's --allowedTools run (`plan` and `dontAsk` are +stricter, but the lanes are written for `default`). So each lane passes `--permission-mode default` in +claude_args, the argument the SDK used to send. `auto` is no substitute: it +runs the tools its classifier approves, allowlisted or not. + +The lanes and the flag are written out here, not read from the workflows, so +dropping or changing the flag fails, and so does a new lane that lacks it. +claude_args is read the way the action reads it: lines starting with `#` are +dropped, the rest is split into shell words. +""" + +import shlex +from pathlib import Path + +import pytest +import yaml + +ROOT = Path(__file__).resolve().parents[1] +GITHUB = ROOT / ".github" +# Every anthropics/claude-code-action step there is, per file. +EXPECTED_STEPS = { + "workflows/claude-review.yml": 1, + "workflows/claude-adversarial-review.yml": 1, + "workflows/verifier-on-high-risk.yml": 1, +} +FLAG = "--permission-mode" +MODE = "default" +# Either one replaces the mode with bypassPermissions or offers it. +BYPASS_FLAGS = ( + "--dangerously-skip-permissions", + "--allow-dangerously-skip-permissions", +) +LINE = " --permission-mode default\n" + + +def shipped_texts(): + paths = sorted(GITHUB.glob("workflows/*.y*ml")) + sorted( + GITHUB.glob("actions/*/action.y*ml") + ) + return {str(path.relative_to(GITHUB)): path.read_text() for path in paths} + + +def action_steps(document): + """Every step of a workflow's jobs, or of a composite action.""" + steps = [] + for job in (document.get("jobs") or {}).values(): + steps += job.get("steps") or [] + steps += (document.get("runs") or {}).get("steps") or [] + return [ + step + for step in steps + if str(step.get("uses", "")).startswith("anthropics/claude-code-action@") + ] + + +def claude_words(claude_args): + lines = [ + line for line in claude_args.splitlines() if not line.strip().startswith("#") + ] + return shlex.split("\n".join(lines)) + + +def step_problems(name, step): + claude_args = (step.get("with") or {}).get("claude_args") + if not isinstance(claude_args, str): + return [f"{name}: the claude-code-action step has no claude_args"] + words = claude_words(claude_args) + problems = [] + modes = [i for i, word in enumerate(words) if word.startswith(FLAG)] + if len(modes) != 1: + problems.append( + f"{name}: claude_args carries {FLAG} {len(modes)} times, not once" + ) + for i in modes: + if words[i] != FLAG: + problems.append( + f"{name}: claude_args writes {words[i]!r}, not '{FLAG} {MODE}'" + ) + elif i + 1 >= len(words) or words[i + 1] != MODE: + value = words[i + 1] if i + 1 < len(words) else None + problems.append(f"{name}: {FLAG} is {value!r}, not {MODE!r}") + for word in words: + if word in BYPASS_FLAGS: + problems.append(f"{name}: claude_args carries {word}") + return problems + + +def all_problems(texts): + problems = [] + for name, text in sorted(texts.items()): + steps = action_steps(yaml.safe_load(text) or {}) + if name not in EXPECTED_STEPS: + if steps: + problems.append( + f"{name}: not an expected lane; pass {FLAG} {MODE} in its " + "claude_args and add it to EXPECTED_STEPS" + ) + elif len(steps) != EXPECTED_STEPS[name]: + problems.append( + f"{name}: {len(steps)} claude-code-action steps, not {EXPECTED_STEPS[name]}" + ) + for step in steps: + problems += step_problems(name, step) + missing = sorted(set(EXPECTED_STEPS) - set(texts)) + if missing: + problems.append(f"lanes not found: {missing}") + return problems + + +def test_every_lane_runs_in_permission_mode_default(): + assert all_problems(shipped_texts()) == [] + + +def swap(name, old, new): + def edit(texts): + assert texts[name].count(old) == 1, (name, old) + return {**texts, name: texts[name].replace(old, new)} + + return edit + + +def add_lane(texts): + lane = """name: New lane +on: pull_request +jobs: + look: + runs-on: ubuntu-latest + steps: + - uses: anthropics/claude-code-action@a26b07d8c778b0c43f64618e5fd29fd0f8af72e8 + with: + prompt: hi + claude_args: | + --max-turns 5 +""" + return {**texts, "workflows/new-lane.yml": lane} + + +def add_composite(texts): + action = """name: Wrapped review +runs: + using: composite + steps: + - uses: anthropics/claude-code-action@a26b07d8c778b0c43f64618e5fd29fd0f8af72e8 + with: + prompt: hi + claude_args: | + --permission-mode default +""" + return {**texts, "actions/wrapped-review/action.yml": action} + + +def drop_lane(name): + def edit(texts): + return {key: value for key, value in texts.items() if key != name} + + return edit + + +REVIEW = "workflows/claude-review.yml" +ADVERSARIAL = "workflows/claude-adversarial-review.yml" +VERIFIER = "workflows/verifier-on-high-risk.yml" +MUTANTS = { + "review lane without the flag": ( + swap(REVIEW, LINE, ""), + f"{REVIEW}: claude_args carries --permission-mode 0 times", + ), + "adversarial lane in auto": ( + swap(ADVERSARIAL, LINE, LINE.replace(MODE, "auto")), + f"{ADVERSARIAL}: --permission-mode is 'auto', not 'default'", + ), + "verifier lane in bypassPermissions": ( + swap(VERIFIER, LINE, LINE.replace(MODE, "bypassPermissions")), + f"{VERIFIER}: --permission-mode is 'bypassPermissions'", + ), + "flag with no value": ( + swap(REVIEW, LINE, " --permission-mode\n"), + f"{REVIEW}: --permission-mode is", + ), + "flag in --flag=value form": ( + swap(VERIFIER, LINE, " --permission-mode=default\n"), + f"{VERIFIER}: claude_args writes '--permission-mode=default'", + ), + "flag misspelled": ( + swap(ADVERSARIAL, LINE, " --permision-mode default\n"), + f"{ADVERSARIAL}: claude_args carries --permission-mode 0 times", + ), + "flag commented out": ( + swap(REVIEW, LINE, " # --permission-mode default\n"), + f"{REVIEW}: claude_args carries --permission-mode 0 times", + ), + "flag given twice": ( + swap(VERIFIER, LINE, LINE + " --permission-mode acceptEdits\n"), + f"{VERIFIER}: claude_args carries --permission-mode 2 times", + ), + "bypass flag added": ( + swap(ADVERSARIAL, LINE, LINE + " --dangerously-skip-permissions\n"), + f"{ADVERSARIAL}: claude_args carries --dangerously-skip-permissions", + ), + "a new lane without the flag": ( + add_lane, + "workflows/new-lane.yml: not an expected lane", + ), + "a composite action wrapping the action": ( + add_composite, + "actions/wrapped-review/action.yml: not an expected lane", + ), + "an expected lane gone": ( + drop_lane(VERIFIER), + f"lanes not found: ['{VERIFIER}']", + ), + "an expected lane's action step renamed away": ( + swap( + REVIEW, + "uses: anthropics/claude-code-action@", + "uses: anthropics/claude-code-action-fork@", + ), + f"{REVIEW}: 0 claude-code-action steps, not 1", + ), +} + + +@pytest.mark.parametrize("mutant", sorted(MUTANTS)) +def test_a_mutated_lane_fails_the_check_for_what_it_broke(mutant): + edit, expected = MUTANTS[mutant] + problems = all_problems(edit(shipped_texts())) + assert any(expected in problem for problem in problems), problems diff --git a/selftest/test_claude_review_ran_check.sh b/selftest/test_claude_review_ran_check.sh index fc5b7c8..dd4af0a 100644 --- a/selftest/test_claude_review_ran_check.sh +++ b/selftest/test_claude_review_ran_check.sh @@ -2,7 +2,7 @@ # Behavioral test for claude-review.yml's caller check and review-ran check: # a required `review / Claude Review` must never pass with nothing reviewed. # -# THE GAP. claude-code-action (pinned at v1.0.229) gets its GitHub token by +# THE GAP. claude-code-action (pinned at v1.0.250) gets its GitHub token by # exchanging the job's OIDC token for a Claude GitHub App token, and the # exchange refuses any run whose calling workflow differs from the default # branch's copy. The action then logs "Skipping action due to workflow @@ -287,7 +287,7 @@ else: ) # The caller check and the review-ran check rest on this release's # behaviour, read in its source; a bump must re-read it. - pin = "anthropics/claude-code-action@a4f54ef2c58884867281bd8e2f8d63352ad019a9" + pin = "anthropics/claude-code-action@a26b07d8c778b0c43f64618e5fd29fd0f8af72e8" if review.get("uses") != pin: problems.append( f"the review step uses {review.get('uses')!r}, not {pin!r}. The caller and review-ran checks " @@ -403,7 +403,7 @@ for p in problems: print(f"✗ {p}") sys.exit(1 if problems else 0) PY -[ "$failed" -eq 0 ] && pass "checkout (default merge ref) < caller < claude < ran < summary < guard; action pinned at a4f54ef; caller continue-on-error, env exact, permissions unchanged; github_token and REVIEW_BOT evaluated for changed=true/false/empty/unset; bot-skip, failed-review and cancelled paths never run the review-ran check" +[ "$failed" -eq 0 ] && pass "checkout (default merge ref) < caller < claude < ran < summary < guard; action pinned at a26b07d; caller continue-on-error, env exact, permissions unchanged; github_token and REVIEW_BOT evaluated for changed=true/false/empty/unset; bot-skip, failed-review and cancelled paths never run the review-ran check" # --------------------------------------------------------------------------- # 2. The shipped bash, extracted