diff --git a/.github/workflows/deps-verify.yml b/.github/workflows/deps-verify.yml index 19254ff..891495c 100644 --- a/.github/workflows/deps-verify.yml +++ b/.github/workflows/deps-verify.yml @@ -582,6 +582,8 @@ jobs: # run-agent writes a needs-manual verdict naming the cause whenever # the agent leaves no valid one, so publish reads one file always. report-file: ${{ runner.temp }}/deps-verdict.json + # Written outside the checkout; see run-agent's header. + write-files: ${{ runner.temp }}/deps-bumps.json ${{ runner.temp }}/deps-verdict.md report-valid: '.verdict | IN("verified", "fixed", "needs-manual")' report-fallback: '{"verdict": "needs-manual"}' allowed-tools: 'Bash "Bash(gh:*)" Read Write Edit Grep Glob WebFetch' diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index c1028f5..f981439 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -414,6 +414,8 @@ jobs: plugin-marketplaces: ${{ needs.gate.outputs.mode != 'scoped' && 'https://github.com/anthropics/claude-code.git' || '' }} plugins: ${{ needs.gate.outputs.mode != 'scoped' && 'code-review@claude-code-plugins' || '' }} report-file: ${{ runner.temp }}/review-verdict.json + # Written outside the checkout; see run-agent's header. + write-files: ${{ runner.temp }}/review-body.md report-valid: '(.verdict | IN("approve", "changes")) and ((.blocking // []) | type == "array")' report-fallback: '{"verdict": "changes", "blocking": []}' # The org default plus the inline-comment tool. Variadic, space diff --git a/.github/workflows/self-test.yml b/.github/workflows/self-test.yml index 61290dd..3b8fa6c 100644 --- a/.github/workflows/self-test.yml +++ b/.github/workflows/self-test.yml @@ -497,8 +497,9 @@ jobs: run: | set -euo pipefail python3 -c " - import sys, yaml - steps = yaml.safe_load(open('.github/workflows/pr-review.yml'))['jobs']['review']['steps'] + import sys + from lib import load, D + steps = load('.github/workflows/pr-review.yml')['jobs']['review']['steps'] agent = [s for s in steps if 'run-agent@' in str(s.get('uses', ''))][0] env = agent.get('env') or {} @@ -514,6 +515,13 @@ jobs: sys.exit('::error::the PR Review prompt lost --comment. Inline comments come from the ' 'plugin steps that flag unlocks.') + w = agent.get('with') or {} + named = str(w.get('report-file', '')) + ' ' + str(w.get('write-files', '')) + for f in ('review-body.md', 'review-verdict.json'): + if D + 'runner.temp }}/' + f not in named: + sys.exit('::error::PR Review asks the agent to write %s but does not pass it to run-agent as ' + 'report-file or write-files, so Claude Code refuses the write.' % f) + verdict = [s for s in steps if s.get('id') == 'verdict'][0] if 'review-body.md' not in verdict['run']: sys.exit('::error::the verdict step no longer reads review-body.md, so the agent writes a ' diff --git a/actions/run-agent/action.yml b/actions/run-agent/action.yml index 74df9e4..c1a5676 100644 --- a/actions/run-agent/action.yml +++ b/actions/run-agent/action.yml @@ -7,7 +7,7 @@ # other condition, and public repos and fork PRs never touch a self-hosted # runner. # -# Six footguns are repeated below because the code reads as wrong without them: +# The footguns below are repeated because the code reads as wrong without them: # # `--allowed-tools` is VARIADIC, space-separated, each entry quoted if it # contains parentheses. A comma-joined list parses as one meaningless token: @@ -40,6 +40,13 @@ # prompt, and the run "succeeds" after 0 turns. So every run passes # `--settings '{"disableAllHooks":true}'`. It has to be the flag: the # `settings` input is user settings, which a project's own value overrides. +# +# RUNNER_TEMP is outside the checkout on every runner, and Claude Code +# refuses a Write call or a Bash `> file` to a path outside its working +# directories unless an `Edit` allow rule names it. A headless run cannot +# ask, so it refuses. Hence one `Edit(//path)` rule per report file. Never +# `--add-dir` RUNNER_TEMP instead: the agent's settings file, secrets +# included, lives there. name: Run agent description: Invoke claude-code-action with the org's tool allowlist, model policy and reporting defaults. @@ -168,6 +175,12 @@ inputs: `report-valid` rejects, it writes `report-fallback` with a `why` field naming the cause. Callers read one file on every path. default: '' + write-files: + description: >- + Other files the prompt tells the agent to write, as absolute paths, + space or newline separated. Each one, and `report-file`, becomes an + `Edit(//path)` allow rule for that exact file. See the header. + default: '' report-valid: description: 'jq expression that is `true` for an acceptable report, e.g. `.verdict | IN("a", "b")`.' default: 'true' @@ -245,6 +258,25 @@ runs: AGENT_ENV_SECRET: ${{ inputs.agent-env-secret }} run: '"$GITHUB_ACTION_PATH/agent-env.sh"' + # See the header. A path is refused rather than quoted when it holds a + # character the rule would read as a pattern or the tool list would split on. + - id: writable + if: inputs.report-file != '' || inputs.write-files != '' + shell: bash + env: + FILES: ${{ inputs.report-file }} ${{ inputs.write-files }} + run: | + set -euf -o pipefail + rules='' + for f in $FILES; do + if ! [[ $f =~ ^/[A-Za-z0-9._/+@-]+$ ]] || [[ $f/ == */./* || $f/ == */../* ]]; then + echo "::error title=Agent write path refused::$f is not a plain absolute path, so no exact allow rule can name it." + exit 1 + fi + rules+="${rules:+ }\"Edit(/$f)\"" + done + echo "rules=$rules" >> "$GITHUB_OUTPUT" + # Fail fast on the combination that silently posts nothing. See the header. - name: Reject a sticky comment agent mode will ignore if: inputs.sticky-comment == 'true' && inputs.prompt != '' @@ -343,7 +375,7 @@ runs: claude_args: >- --model ${{ steps.model.outputs.model }} --settings '{"disableAllHooks":true}' - --allowed-tools ${{ inputs.allowed-tools }} + --allowed-tools ${{ inputs.allowed-tools }} ${{ steps.writable.outputs.rules }} ${{ steps.model.outputs.max-turns != '' && format('--max-turns {0}', steps.model.outputs.max-turns) || '' }} ${{ format('--append-system-prompt "{0} {1}"', inputs.house-style, inputs.append-system-prompt) }} @@ -383,6 +415,11 @@ runs: ((.num_turns // 0) == 0 and ((.modelUsage // {}) | length) == 0), (.num_turns // ""), ((.result // ((.errors // []) | map(tostring) | join("; "))) | gsub("[\\t\\r\\n]+"; " ") | .[:300])] | @tsv' <<<"$r") cost=$(jq -r '.total_cost_usd // empty' <<<"$r") + # Tool calls Claude Code refused, named by tool and file only: a + # command line or file content can carry what a log should not. + denied=$(jq -r '[(.permission_denials // [])[] | [.tool_name, .tool_input.file_path // empty] | join(" ")] + | unique | join(", ")' <<<"$r") + [ -z "$denied" ] || echo "::warning title=Agent tool calls refused::$denied" why='' if [ "$subtype" = error_max_turns ]; then why="The agent reached its turn cap after $turns turns." elif [ "$is_error" = true ]; then why="The agent stopped with an error after $turns turn(s): ${text:-no message}." @@ -391,6 +428,7 @@ runs: { echo "turns=$turns" echo "cost=$cost" + echo "denied=$denied" echo "why=$why" } >> "$GITHUB_OUTPUT" # One line per run, the same shape everywhere, for comparing models. @@ -408,12 +446,14 @@ runs: FALLBACK: ${{ inputs.report-fallback }} WHY: ${{ steps.outcome.outputs.why || steps.validate.outputs.why }} TURNS: ${{ steps.outcome.outputs.turns }} + DENIED: ${{ steps.outcome.outputs.denied }} run: | set -euo pipefail if [ -s "$FILE" ] && [ "$(jq -r "$VALID" "$FILE" 2>/dev/null || true)" = true ]; then echo "source=agent" >> "$GITHUB_OUTPUT"; exit 0 fi why="${WHY:-The agent finished${TURNS:+ after $TURNS turn(s)} without writing a valid $(basename "$FILE").}" + why+="${DENIED:+ Claude Code refused: $DENIED.}" mkdir -p "$(dirname "$FILE")" jq -n --argjson f "$FALLBACK" --arg why "$why" '$f + {why: $why}' > "$FILE" { diff --git a/actions/run-agent/test.sh b/actions/run-agent/test.sh index 81230ce..a7b9412 100755 --- a/actions/run-agent/test.sh +++ b/actions/run-agent/test.sh @@ -105,6 +105,22 @@ EOF } expect "run-agent turns the repo's hooks off with --settings" hooks_off +# The files the agent writes outside the checkout reach the tool list. That +# pr-review names both of its own is a Self test check. +writable_wired() { +python3 - "$ROOT" <<'EOF' +import sys, yaml +steps = yaml.safe_load(open(sys.argv[1] + '/actions/run-agent/action.yml'))['runs']['steps'] +ids = [s.get('id') for s in steps] +w = steps[ids.index('writable')] +assert ids.index('writable') < ids.index('agent'), ids +assert w['env']['FILES'] == '${{ inputs.report-file }} ${{ inputs.write-files }}', w['env'] +call = steps[ids.index('agent')]['with'] +assert '--allowed-tools ${{ inputs.allowed-tools }} ${{ steps.writable.outputs.rules }}' in call['claude_args'], call['claude_args'] +EOF +} +expect "run-agent allows report-file and write-files by exact path" writable_wired + # 2. Lift the steps out of the workflow. The push itself is actions/push-round, # tested there. python3 - "$ROOT" "$WORK" <<'EOF' || { echo 'cannot read the deps-verify steps'; exit 1; } @@ -116,7 +132,7 @@ fix = job['env'] for sid in ('publish', 'actor', 'renovate'): open('%s/%s.sh' % (work, sid), 'w').write(steps[sid]['run']) ra = {s.get('id'): s for s in yaml.safe_load(open(root + '/actions/run-agent/action.yml'))['runs']['steps']} -for sid in ('outcome', 'validate', 'report'): +for sid in ('outcome', 'validate', 'report', 'writable'): open('%s/%s.sh' % (work, sid), 'w').write(ra[sid]['run']) open(work + '/fix.env', 'w').write('FIX_NAME=%r\nFIX_EMAIL=%r\n' % (fix['FIX_NAME'], fix['FIX_EMAIL'])) push = steps['push']['with'] @@ -200,7 +216,7 @@ report() { [ -n "${RAW:-}" ] && printf '%s' "$RAW" > "$WORK/deps-verdict.json" ( cd "$WORK" && env -i PATH="$PATH" HOME="$WORK" FILE="$WORK/deps-verdict.json" \ VALID='.verdict | IN("verified", "fixed", "needs-manual")' FALLBACK='{"verdict": "needs-manual"}' \ - WHY="${1:-}" TURNS="${TURNS:-}" GITHUB_OUTPUT="$WORK/out" bash "$WORK/report.sh" ) > "$WORK/log" 2>&1 + WHY="${1:-}" TURNS="${TURNS:-}" DENIED="${DENIED:-}" GITHUB_OUTPUT="$WORK/out" bash "$WORK/report.sh" ) > "$WORK/log" 2>&1 } field() { jq -r ".$1" "$WORK/deps-verdict.json"; } @@ -215,6 +231,10 @@ REPORT=bogus report expect "an invalid verdict word is replaced" test "$(field verdict)" = needs-manual RAW='not json' TURNS=12 report expect "unreadable JSON is replaced, naming the turns" grep -q 'finished after 12 turn(s) without writing a valid deps-verdict.json' <(field why) +TURNS=11 DENIED='Write /x/_temp/deps-verdict.json' report +expect "a missing report names the writes Claude Code refused" test "$(field why)" = 'The agent finished after 11 turn(s) without writing a valid deps-verdict.json. Claude Code refused: Write /x/_temp/deps-verdict.json.' +REPORT=verified DENIED='Write /x/other.md' report +expect "a refusal beside a valid report changes nothing" grep -qx 'source=agent' "$WORK/out" # 8b. Publish words what run-agent wrote, and fails only on a real failure. printf '{"verdict":"needs-manual","why":"claude-code-action refuses to run while x.yml differs"}' > "$WORK/mismatch.json" @@ -263,11 +283,43 @@ outcome success '[{"type":"result","subtype":"success","is_error":false,"num_tur expect "with the text the session left, if any" grep -q 'blocked the prompt. It said: UserPromptSubmit operation blocked' <(out_why) outcome success '[{"type":"result","subtype":"success","is_error":false,"num_turns":0,"modelUsage":{"m":{"inputTokens":9}},"result":"done"}]' expect "0 turns with model usage is a normal finish" test -z "$(out_why)" +outcome success '[{"type":"result","subtype":"success","is_error":false,"num_turns":11,"result":"merge","permission_denials":[ + {"tool_name":"Write","tool_input":{"file_path":"/w/_temp/review-body.md","content":"s3cret"}}, + {"tool_name":"Write","tool_input":{"file_path":"/w/_temp/review-verdict.json","content":"{}"}}, + {"tool_name":"Bash","tool_input":{"command":"echo s3cret > /w/_temp/x"}}]}]' +expect "refused tool calls are named by tool and file" grep -qx 'denied=Bash, Write /w/_temp/review-body.md, Write /w/_temp/review-verdict.json' "$WORK/out" +expect "and warned about, without content or command text" bash -c 'grep -q "::warning title=Agent tool calls refused::" "$1" && ! grep -q s3cret "$1" "$2"' _ "$WORK/log" "$WORK/out" +expect "a refusal alone is not a why" test -z "$(out_why)" +outcome success '[{"type":"result","subtype":"success","is_error":false,"num_turns":3,"result":"ok"}]' +expect "no refusals, no warning" bash -c 'grep -qx "denied=" "$1" && ! grep -q "calls refused" "$2"' _ "$WORK/out" "$WORK/log" outcome success expect "no result after a success is the action skipping itself" grep -q 'skipped itself' <(out_why) outcome failure expect "no result after a failure says the action failed first" grep -q 'failed before the agent produced a result' <(out_why) +# 8d1. run-agent's writable step: one exact Edit rule per report file, wherever +# RUNNER_TEMP is. A self-hosted slot keeps it under its own work root, beside +# the workspace rather than inside it, and the rule must not care. +# writable FILES: prints the rules, exit status is the step's. +writable() { + : > "$WORK/out" + ( cd "$WORK" && env -i PATH="$PATH" HOME="$WORK" FILES="$1" GITHUB_OUTPUT="$WORK/out" bash "$WORK/writable.sh" ) > "$WORK/log" 2>&1 + local rc=$?; sed -n 's/^rules=//p' "$WORK/out"; return $rc +} +# The tool list as claude-code-action splits it: shell words. +tools() { python3 -c 'import shlex, sys; print("\n".join(shlex.split(sys.argv[1])))' "$1"; } +slot=/var/lib/github-runner/slots/1/work/_temp +rules=$(writable "$slot/review-verdict.json $slot/review-body.md") +expect "a RUNNER_TEMP outside the workspace gets one absolute rule per file" test "$rules" = "\"Edit(/$slot/review-verdict.json)\" \"Edit(/$slot/review-body.md)\"" +expect "and they split into tool-list entries of their own" test "$(tools "Bash \"Bash(gh:*)\" Read $rules" | tail -2)" = "Edit(/$slot/review-verdict.json)"$'\n'"Edit(/$slot/review-body.md)" +expect "a hosted RUNNER_TEMP the same way" test "$(writable /home/runner/work/_temp/review-verdict.json)" = '"Edit(//home/runner/work/_temp/review-verdict.json)"' +expect "newline-separated paths too" test "$(writable $'/t/a.json\n/t/b.md')" = '"Edit(//t/a.json)" "Edit(//t/b.md)"' +expect "no files, no rules" test -z "$(writable ' ')" +for p in 'review.json' '/t/*.md' '/t/../etc/passwd' '/t/./a' '/t/a)b' '/t/a"b' '/t/[ab]'; do + writable "$p" > /dev/null; rc=$? + expect "$p is refused, and fails the step" bash -c '[ "$1" != 0 ] && grep -q "Agent write path refused" "$2" && ! grep -q "^rules=" "$3"' _ "$rc" "$WORK/log" "$WORK/out" +done + # 8d2. agent-env.sh: the org's plugins on top of the caller's, and env for the agent alone. # agent_env [ENV=value ...]: runs it, outputs in $WORK/out, log in $WORK/log. agent_env() {