diff --git a/.github/workflows/auto-reviewer-shared.yaml b/.github/workflows/auto-reviewer-shared.yaml index af63be5..f7c01b9 100644 --- a/.github/workflows/auto-reviewer-shared.yaml +++ b/.github/workflows/auto-reviewer-shared.yaml @@ -1039,12 +1039,17 @@ jobs: # User-scope setting: Read, Grep, and Glob outside the working directory are refused (Bash is # denied in claude_args); Claude Code keeps ~/.claude/skills readable. settings: '{"permissions":{"blockReadsOutsideWorkingDirectories":true}}' - prompt: | + prompt: &review_prompt | REPO: ${{ github.repository }} PR NUMBER: ${{ needs.gate.outputs.number }} BASE SHA: ${{ needs.gate.outputs.base_sha }} HEAD SHA: ${{ needs.gate.outputs.head_sha }} + Retry check: if .auto-reviewer/retry-error.txt exists, this is a second attempt after the + first draft failed validation. Do not redo the review. Read that file, then fix exactly the + listed errors in the existing .auto-reviewer/review.md, comments.json and review-actions.json + (Edit, keeping every other part unchanged) and finish. Without that file, ignore this paragraph. + Run the cr-request skill for this pull request on a prepared target. The target is the committed range from the merge base of BASE SHA and HEAD SHA to HEAD SHA, already captured in .auto-reviewer/target/: identity.txt (the target identity, tree included), log.txt @@ -1184,20 +1189,22 @@ jobs: # workspace. Write follows the Edit rules for the body and comments files. Bash is denied, # built-in read-only commands included: any allowed command can echo an expanded variable. .git/ # is denied for Read, Grep, and Glob: .git/config holds the action's token in the origin URL. - claude_args: >- + claude_args: &review_claude_args >- --model ${{ steps.model.outputs.model }} --effort ${{ steps.model.outputs.effort }} --max-turns 60 --allowedTools "Skill,Agent,Task,Grep,Glob,Read(./**),Read(~/.claude/skills/**),Edit(./.auto-reviewer/review.md),Write(./.auto-reviewer/review.md),Edit(./.auto-reviewer/comments.json),Write(./.auto-reviewer/comments.json),Edit(./.auto-reviewer/review-actions.json),Write(./.auto-reviewer/review-actions.json)" --disallowedTools "Bash,Read(./.git/**)" - # The duration is the agent step's wall time; the agent cannot measure it. Only the signature line - # among the last two non-blank lines is rewritten, so a quoted signature higher up stays as written; - # anything the agent put after the signature and a space there is replaced by the measured duration. + # The duration is the agent step's wall time; the agent cannot measure it. The closing lines are + # deterministic: any trailing signature and Next step lines the agent wrote are dropped and the + # canonical signature is appended, so a missing or malformed trailer cannot fail the run. A valid + # agent Next step is kept; otherwise it comes from the Summary table counts. # Under a minute the duration code span reads 0 min 42 sec. Usage comes from the action's result # execution file. Model, effort, grouped token count and cost each have their own code span. # Tokens include input, output, and cache tokens; a missing or unreadable file adds nothing, # and the merged summary then shows the usage as unknown. - - name: Add review duration + - &add_review_duration + name: Add review duration env: START: ${{ steps.start.outputs.epoch }} EXEC_FILE: ${{ steps.claude.outputs.execution_file }} @@ -1205,49 +1212,79 @@ jobs: USAGE_EFFORT: ${{ steps.model.outputs.effort }} SIGNATURE: 'Reviewed with: `auto-reviewer-shared.yaml` `${{ steps.workflow_identity.outputs.ref }}` workflow · `cr-request` `v${{ steps.skill.outputs.version }}` skill' SEP: ' · ' + BODY_FILE_PATH: .auto-reviewer/review.md run: | body=.auto-reviewer/review.md [ -f "$body" ] || exit 0 elapsed=$(( $(date +%s) - START )) export DURATION="In \`$(( elapsed / 60 )) min $(( elapsed % 60 )) sec\`" - if [ -n "${EXEC_FILE:-}" ] && [ -f "$EXEC_FILE" ]; then + # The usage of a retry run adds to the first run's; a file that is missing or unreadable adds nothing. + # The retry step is declared later, so its execution file path arrives through a runner file. + EXEC_FILE_RETRY="" + [ -f "${RUNNER_TEMP:-/nonexistent}/retry-exec-file" ] && EXEC_FILE_RETRY=$(cat "$RUNNER_TEMP/retry-exec-file") + total_tokens=0; total_cost=0; counted=0 + for exec_file in "${EXEC_FILE:-}" "${EXEC_FILE_RETRY:-}"; do + [ -n "$exec_file" ] && [ -f "$exec_file" ] || continue usage=$(jq -r '[.[] | select(type == "object" and .type == "result")] | last | select(. != null and (.total_cost_usd | type) == "number" and .total_cost_usd >= 0) | [((.usage.input_tokens // 0) + (.usage.output_tokens // 0) + (.usage.cache_creation_input_tokens // 0) + (.usage.cache_read_input_tokens // 0) | floor), - .total_cost_usd] | @tsv' "$EXEC_FILE" 2> /dev/null || true) + .total_cost_usd] | @tsv' "$exec_file" 2> /dev/null || true) if [[ "$usage" =~ ^([0-9]+)[[:space:]]([0-9.eE+-]+)$ ]]; then - cost=$(LC_ALL=C awk -v c="${BASH_REMATCH[2]}" 'BEGIN { printf "%.4f", c }') - tokens=$(LC_ALL=C awk -v n="${BASH_REMATCH[1]}" 'BEGIN { - out=""; while (length(n) > 3) { out="," substr(n,length(n)-2) out; n=substr(n,1,length(n)-3) } - print n out - }') - DURATION="$DURATION · \`$USAGE_MODEL\` \`$USAGE_EFFORT\` · \`$tokens tokens\` · \`\$$cost\`" + total_tokens=$(LC_ALL=C awk -v a="$total_tokens" -v b="${BASH_REMATCH[1]}" 'BEGIN { printf "%.0f", a + b }') + total_cost=$(LC_ALL=C awk -v a="$total_cost" -v b="${BASH_REMATCH[2]}" 'BEGIN { printf "%.10f", a + b }') + counted=1 fi + done + if [ "$counted" = 1 ]; then + cost=$(LC_ALL=C awk -v c="$total_cost" 'BEGIN { printf "%.4f", c }') + tokens=$(LC_ALL=C awk -v n="$total_tokens" 'BEGIN { + out=""; while (length(n) > 3) { out="," substr(n,length(n)-2) out; n=substr(n,1,length(n)-3) } + print n out + }') + DURATION="$DURATION · \`$USAGE_MODEL\` \`$USAGE_EFFORT\` · \`$tokens tokens\` · \`\$$cost\`" fi - tmp="$(mktemp)" - # Two passes over the body: the first finds the last two non-blank lines, the second rewrites. - awk ' - { sub(/\r$/, "") } - NR == FNR { if ($0 !~ /^[[:space:]]*$/) { prev = last; last = FNR } next } - FNR == prev || FNR == last { - line = $0; sub(/[[:space:]]+$/, "", line) - sig = ENVIRON["SIGNATURE"]; sep = ENVIRON["SEP"] - rest = substr(line, length(sig) + 1) - if (substr(line, 1, length(sig)) == sig && (rest == "" || substr(rest, 1, 1) == " ")) { - $0 = sig sep ENVIRON["DURATION"] - } - } - { print } - ' "$body" "$body" > "$tmp" - mv "$tmp" "$body" + export DURATION + python3 -I - <<'PY' + import os, re + from pathlib import Path + + path = Path(os.environ["BODY_FILE_PATH"]) + lines = path.read_text(encoding="utf-8").replace("\r", "").rstrip().split("\n") + # Drop every trailing model-written signature and Next step line; the model's Next step is + # kept only when it satisfies the same grammar the publish step validates. + next_re = re.compile( + r"^Next step: (`/[a-z0-9-]+( [^`\s]+)*`( -> `/[a-z0-9-]+`( [a-z0-9-]+)?)?( · \S.*)?|none( · \S.*)?)$") + model_next = None + while lines and (lines[-1].strip() == "" or lines[-1].startswith(("Reviewed with:", "Next step:"))): + line = lines.pop().rstrip() + if line.startswith("Next step:") and model_next is None and next_re.match(line): + model_next = line + body = "\n".join(lines) + # A Details block the agent opened but never closed is closed here, before the closing lines. + if (len(re.findall(r"^]*)?>Details[ \t]*$", body, re.M)) == 1 + and not re.search(r"^[ \t]*$", body, re.M)): + body += "\n\n" + if model_next is None: + # Fallback from the Summary table counts and a plain Ready; only the model's reasoned choice is lost. + counts = re.search(r"^\| *Intent *\|[^\n]*\n[^\n]*\n\|[^|]*\|[^|]*\|[^|]*\| *([0-9]+) *\| *([0-9]+) *\| *([0-9]+) *\|", body, re.M) + ready = re.search(r"^\*\*Readiness:\*\* [Rr]eady\s*$", body, re.M) + if counts and ready and sum(map(int, counts.groups())) == 0: + model_next = "Next step: `/verify -> /finalize squash`" + else: + number = os.environ.get("NUMBER", "") + model_next = "Next step: `/cr-received" + (" PR #" + number if number.isdigit() else "") + "`" + signature = os.environ["SIGNATURE"] + os.environ["SEP"] + os.environ["DURATION"] + path.write_text(body + "\n\n" + signature + "\n" + model_next + "\n", encoding="utf-8") + PY echo "Review duration and usage: $DURATION" # GitHub rejects a whole review when one inline comment points outside the diff, so only comments # on a head-side line of a diff.patch hunk stay; the rest of the findings live in the body. The # agent chooses the comments: the log gets counts only. -I keeps a json.py or re.py at the root of # the PR checkout from shadowing the standard library. - - name: Check inline comments + - &check_inline_comments + name: Check inline comments run: | python3 -I - <<'PY' import json, pathlib, re @@ -1301,6 +1338,145 @@ jobs: print(f"Inline comments: {len(kept)} kept of {len(proposed)}") PY + # Pre-validates the draft with the publish job's own actions validator plus a compact structure + # check. publish stays the authority; a failure here only triggers one retry with the errors. + - name: Pre-validate review draft + id: precheck + env: + REPO: ${{ github.repository }} + ACTIONS_FILE: .auto-reviewer/review-actions.json + BODY_FILE: .auto-reviewer/review.md + COMMENTS_FILE: .auto-reviewer/comments.json + VALIDATE_ACTIONS: &validate_review_actions | + python3 -I - <<'PY' + import json, os, re + from pathlib import Path + + def unique(pairs): + result = {} + for key, value in pairs: + if key in result: + raise ValueError('duplicate JSON key') + result[key] = value + return result + + def load(path): + return json.loads(Path(path).read_text(encoding='utf-8'), object_pairs_hook=unique) + + def require(value, message): + if not value: + raise ValueError(message) + + actions = Path(os.environ['ACTIONS_FILE']) + if not actions.is_file(): + print('::warning::No structured review actions; previous findings remain evidence gaps') + raise SystemExit(0) + d = load(actions) + require(isinstance(d, dict) and set(d) == {'schema_version', 'repository', 'pr_number', 'head_sha', + 'review_source_id', 'findings', 'previous_findings'}, 'invalid draft shape') + require(type(d['schema_version']) is int and d['schema_version'] == 1, 'invalid schema version') + require(d['repository'] == os.environ['REPO'] and type(d['pr_number']) is int + and d['pr_number'] == int(os.environ['NUMBER']) and d['head_sha'] == os.environ['HEAD_SHA'] + and re.fullmatch('[0-9a-f]{40}', d['head_sha']) and d['review_source_id'] is None, + 'draft target mismatch') + require(isinstance(d['findings'], list) and isinstance(d['previous_findings'], list), 'invalid findings') + base = {'origin_review_id', 'original_id', 'path', 'severity'} + extra = {'status', 'defer_decision', 'reason', 'owner'} + seen = set() + for current, entries in ((True, d['findings']), (False, d['previous_findings'])): + for f in entries: + require(isinstance(f, dict) and set(f) == (base if current else base | extra), 'invalid finding shape') + origin, ident, path = f['origin_review_id'], f['original_id'], f['path'] + require(origin is None if current else type(origin) is int and origin > 0, 'invalid origin') + require(isinstance(ident, str) and re.fullmatch(r'R[1-9][0-9]*-(?:[A-Za-z0-9_-]+-)?F[1-9][0-9]*', ident), 'invalid finding ID') + require(isinstance(path, str) and '\\' not in path and all(x not in ('', '.', '..', '.git', '.cross-review') for x in path.split('/')), 'invalid path') + require(f['severity'] in ('Critical', 'Important', 'Minor'), 'invalid severity') + key = (origin, ident) + require(key not in seen, 'duplicate finding') + seen.add(key) + if not current: + status, defer, reason, owner = f['status'], f['defer_decision'], f['reason'], f['owner'] + require(status in ('ADDRESSED', 'NOT_ADDRESSED') and defer in ('NONE', 'PROPOSED', 'ACCEPTED', 'DECLINED'), 'invalid previous state') + require(isinstance(reason, str) and bool(reason.strip()) and (owner is None or isinstance(owner, str) and bool(owner.strip())), 'invalid decision') + require(defer not in ('PROPOSED', 'ACCEPTED') or owner is not None, 'missing defer owner') + require(defer != 'ACCEPTED' or status == 'NOT_ADDRESSED' and f['severity'] != 'Critical', 'invalid accepted defer') + require(status != 'ADDRESSED' or defer == 'NONE', 'addressed finding has defer') + # New drafts use circles; historical thread formats are handled by the matchers. + circles = {r'$\color{red}◎$ High': 'Critical', r'$\color{orange}◎$ Medium': 'Important', r'$\color{green}◎$ Low': 'Minor'} + circle_re = '(?:' + '|'.join(re.escape(circle) for circle in circles) + ')' + report = Path(os.environ['BODY_FILE']).read_text(encoding='utf-8') + found = [(circles[m[1]], m[2], m[3]) for m in re.finditer( + r'^- ('+circle_re+r') (R[1-9][0-9]*-(?:[A-Za-z0-9_-]+-)?F[1-9][0-9]*) \[`([^`]+):[0-9]+`\]', report, re.M)] + expected = [(f['severity'], f['original_id'], f['path']) for f in d['findings']] + require(sorted(found) == sorted(expected), 'report findings differ from draft') + table = re.search(r'^\| *Intent *\| *System *\| *Craft *\| *Critical *\| *Important *\| *Minor *\|\s*\n[^\n]*\n\| *(?:PASS|FINDINGS|UNASSESSED) *\| *(?:PASS|FINDINGS|UNASSESSED) *\| *(?:PASS|FINDINGS|UNASSESSED) *\| *([0-9]+) *\| *([0-9]+) *\| *([0-9]+) *\|', report, re.M) + require(table is not None and tuple(map(int, table.groups())) == tuple(sum(f['severity']==severity for f in d['findings']) for severity in ('Critical','Important','Minor')), 'summary counts differ from draft') + comments_path = Path(os.environ['COMMENTS_FILE']) + comments = load(comments_path) if comments_path.is_file() else [] + require(isinstance(comments, list), 'invalid inline comments') + inline = set() + for comment in comments: + require(isinstance(comment, dict) and set(comment) == {'body', 'line', 'path'} + and type(comment['line']) is int and comment['line'] > 0 and isinstance(comment['body'], str), 'invalid inline comment') + m = re.match(r'^('+circle_re+r') (R[1-9][0-9]*-(?:[A-Za-z0-9_-]+-)?F[1-9][0-9]*)(?:\s|$)', comment['body']) + require(m is not None and (circles[m[1]], m[2], comment['path']) in expected, 'inline finding mismatch') + require(m[2] not in inline, 'duplicate inline finding') + inline.add(m[2]) + PY + run: | + [ -f "$BODY_FILE" ] || exit 0 + errors="" + if ! err=$(bash -eo pipefail -c "$VALIDATE_ACTIONS" 2>&1 > /dev/null); then + errors+="review-actions.json or inline comments: $(tail -n 1 <<< "$err")"$'\n' + fi + if ! err=$(python3 -I - 2>&1 <<'PY' + import os, re + from pathlib import Path + + source = Path(os.environ["BODY_FILE"]).read_text(encoding="utf-8").replace("\r", "") + opening = re.findall(r"^]*)?>Details[ \t]*$", source, re.M) + closing = re.findall(r"^[ \t]*$", source, re.M) + if len(opening) != 1 or len(closing) != 1 or len(re.findall(r" .auto-reviewer/retry-error.txt + echo "error=true" >> "$GITHUB_OUTPUT" + echo "::warning::The review draft failed pre-validation; retrying once" + printf '%s' "$errors" + fi + + - uses: anthropics/claude-code-action@925afb53776cf1a2b738c6bdbed37b4d4000ef13 # v1 + id: claude_retry + if: steps.precheck.outputs.error == 'true' + continue-on-error: true + with: + path_to_claude_code_executable: ${{ steps.claude_cli.outputs.path }} + github_token: ${{ github.token }} + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + settings: '{"permissions":{"blockReadsOutsideWorkingDirectories":true}}' + prompt: *review_prompt + claude_args: *review_claude_args + + - name: Record retry execution file + if: steps.claude_retry.outputs.execution_file != '' + env: + RETRY_EXEC_FILE: ${{ steps.claude_retry.outputs.execution_file }} + run: printf '%s' "$RETRY_EXEC_FILE" > "$RUNNER_TEMP/retry-exec-file" + + # After a retry the closing lines, usage and inline comments are produced again from the new draft. + - *add_review_duration + - *check_inline_comments + # The action hides the agent's output: list the paths it read and the calls the permission rules # refused (paths and commands only), and the result when it wrote no body. - name: Show file access and why no review was written @@ -1386,82 +1562,7 @@ jobs: - name: Validate review actions id: actions if: always() - run: | - python3 -I - <<'PY' - import json, os, re - from pathlib import Path - - def unique(pairs): - result = {} - for key, value in pairs: - if key in result: - raise ValueError('duplicate JSON key') - result[key] = value - return result - - def load(path): - return json.loads(Path(path).read_text(encoding='utf-8'), object_pairs_hook=unique) - - def require(value, message): - if not value: - raise ValueError(message) - - actions = Path(os.environ['ACTIONS_FILE']) - if not actions.is_file(): - print('::warning::No structured review actions; previous findings remain evidence gaps') - raise SystemExit(0) - d = load(actions) - require(isinstance(d, dict) and set(d) == {'schema_version', 'repository', 'pr_number', 'head_sha', - 'review_source_id', 'findings', 'previous_findings'}, 'invalid draft shape') - require(type(d['schema_version']) is int and d['schema_version'] == 1, 'invalid schema version') - require(d['repository'] == os.environ['REPO'] and type(d['pr_number']) is int - and d['pr_number'] == int(os.environ['NUMBER']) and d['head_sha'] == os.environ['HEAD_SHA'] - and re.fullmatch('[0-9a-f]{40}', d['head_sha']) and d['review_source_id'] is None, - 'draft target mismatch') - require(isinstance(d['findings'], list) and isinstance(d['previous_findings'], list), 'invalid findings') - base = {'origin_review_id', 'original_id', 'path', 'severity'} - extra = {'status', 'defer_decision', 'reason', 'owner'} - seen = set() - for current, entries in ((True, d['findings']), (False, d['previous_findings'])): - for f in entries: - require(isinstance(f, dict) and set(f) == (base if current else base | extra), 'invalid finding shape') - origin, ident, path = f['origin_review_id'], f['original_id'], f['path'] - require(origin is None if current else type(origin) is int and origin > 0, 'invalid origin') - require(isinstance(ident, str) and re.fullmatch(r'R[1-9][0-9]*-(?:[A-Za-z0-9_-]+-)?F[1-9][0-9]*', ident), 'invalid finding ID') - require(isinstance(path, str) and '\\' not in path and all(x not in ('', '.', '..', '.git', '.cross-review') for x in path.split('/')), 'invalid path') - require(f['severity'] in ('Critical', 'Important', 'Minor'), 'invalid severity') - key = (origin, ident) - require(key not in seen, 'duplicate finding') - seen.add(key) - if not current: - status, defer, reason, owner = f['status'], f['defer_decision'], f['reason'], f['owner'] - require(status in ('ADDRESSED', 'NOT_ADDRESSED') and defer in ('NONE', 'PROPOSED', 'ACCEPTED', 'DECLINED'), 'invalid previous state') - require(isinstance(reason, str) and bool(reason.strip()) and (owner is None or isinstance(owner, str) and bool(owner.strip())), 'invalid decision') - require(defer not in ('PROPOSED', 'ACCEPTED') or owner is not None, 'missing defer owner') - require(defer != 'ACCEPTED' or status == 'NOT_ADDRESSED' and f['severity'] != 'Critical', 'invalid accepted defer') - require(status != 'ADDRESSED' or defer == 'NONE', 'addressed finding has defer') - # New drafts use circles; historical thread formats are handled by the matchers. - circles = {r'$\color{red}◎$ High': 'Critical', r'$\color{orange}◎$ Medium': 'Important', r'$\color{green}◎$ Low': 'Minor'} - circle_re = '(?:' + '|'.join(re.escape(circle) for circle in circles) + ')' - report = Path(os.environ['BODY_FILE']).read_text(encoding='utf-8') - found = [(circles[m[1]], m[2], m[3]) for m in re.finditer( - r'^- ('+circle_re+r') (R[1-9][0-9]*-(?:[A-Za-z0-9_-]+-)?F[1-9][0-9]*) \[`([^`]+):[0-9]+`\]', report, re.M)] - expected = [(f['severity'], f['original_id'], f['path']) for f in d['findings']] - require(sorted(found) == sorted(expected), 'report findings differ from draft') - table = re.search(r'^\| *Intent *\| *System *\| *Craft *\| *Critical *\| *Important *\| *Minor *\|\s*\n[^\n]*\n\| *(?:PASS|FINDINGS|UNASSESSED) *\| *(?:PASS|FINDINGS|UNASSESSED) *\| *(?:PASS|FINDINGS|UNASSESSED) *\| *([0-9]+) *\| *([0-9]+) *\| *([0-9]+) *\|', report, re.M) - require(table is not None and tuple(map(int, table.groups())) == tuple(sum(f['severity']==severity for f in d['findings']) for severity in ('Critical','Important','Minor')), 'summary counts differ from draft') - comments_path = Path(os.environ['COMMENTS_FILE']) - comments = load(comments_path) if comments_path.is_file() else [] - require(isinstance(comments, list), 'invalid inline comments') - inline = set() - for comment in comments: - require(isinstance(comment, dict) and set(comment) == {'body', 'line', 'path'} - and type(comment['line']) is int and comment['line'] > 0 and isinstance(comment['body'], str), 'invalid inline comment') - m = re.match(r'^('+circle_re+r') (R[1-9][0-9]*-(?:[A-Za-z0-9_-]+-)?F[1-9][0-9]*)(?:\s|$)', comment['body']) - require(m is not None and (circles[m[1]], m[2], comment['path']) in expected, 'inline finding mismatch') - require(m[2] not in inline, 'duplicate inline finding') - inline.add(m[2]) - PY + run: *validate_review_actions # Runs with GITHUB_TOKEN and owns the status; the artifact exists only when the review job succeeded. # always(): after a workflow-level cancel a step without a status check is skipped, and the diff --git a/AUTO_REVIEWER_SHARED_TRANSPORT.md b/AUTO_REVIEWER_SHARED_TRANSPORT.md index c4307eb..34b1df6 100644 --- a/AUTO_REVIEWER_SHARED_TRANSPORT.md +++ b/AUTO_REVIEWER_SHARED_TRANSPORT.md @@ -7,7 +7,8 @@ to the read-only reviewer. The reusable engine is `.github/workflows/auto-reviewer-shared.yaml`, named `Auto Reviewer (Shared)`. The reviewer writes the internal draft footer with separate -workflow/ref spans and a Next step line; trusted publish validates it and renders the +workflow/ref spans and a Next step line; the review job rewrites those closing lines +deterministically (dropping any model-written ones), trusted publish validates them and renders the public comment. The public body shows `Readiness: **Ready**` (or the other validated readiness), the verdict table, one collapsed Details block, and a `Used:` footer. That footer links superscript skill/workflow versions, groups model and effort in one diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c884ce..6974da5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,26 @@ One release tag covers every workflow in this repository. Each release lists its changes per workflow, newest first. +## v0.0.7 + +### auto-reviewer-shared + +1. The review body's closing lines are deterministic. `Add review duration` drops any + trailing `Reviewed with:` and `Next step:` lines the model wrote and appends the + canonical signature (workflow ref, skill version, measured duration, usage) itself, so a + missing or malformed trailer no longer fails Publish with "The review body does not end + with the signature and Next step lines". A valid model `Next step` is kept; otherwise it + is derived from the Summary table and readiness (`/verify -> /finalize squash` only for + a plain Ready with no findings, else `/cr-received PR #`). The published format is unchanged. Fixes #11. +2. Missing ``: a Details block the agent opened but did not close is closed by the + same step before the closing lines. +3. One retry: `Pre-validate review draft` runs the publish job's own review-actions validator + (one shared definition) and a Details/readiness/Summary table check on the draft. On any + error it writes `.auto-reviewer/retry-error.txt` and a second `claude-code-action` run + (same prompt, which tells it to fix only the listed errors in the existing files) repairs + the draft; the closing lines, usage (summed over both runs) and inline comments are then + produced again. Publish validation is unchanged and still decides. + ## v0.0.6 ### auto-reviewer-shared diff --git a/README.md b/README.md index f6372fc..d0cc92b 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,10 @@ install as a plain directory without symbolic links, and must name its version. Published comments show readiness first, then the verdict table, a collapsed Details block, and a Used footer. The footer links skill and workflow versions in superscript, groups model and effort, and shows measured duration, tokens and cost. The public -comment omits Next step; the reviewer's internal draft retains its validated contract. +comment omits Next step; the reviewer's internal draft retains its validated contract. The +workflow itself appends the draft's signature and, when the model's is missing or invalid, +the Next step line, closes an unclosed Details block, and, when the draft still fails its +structural validation, asks the model once to fix the listed errors before publish. History and cycle summaries continue to read earlier review formats. ### Contract diff --git a/tests/test_auto_reviewer_shared.py b/tests/test_auto_reviewer_shared.py index d505493..45ae652 100644 --- a/tests/test_auto_reviewer_shared.py +++ b/tests/test_auto_reviewer_shared.py @@ -1244,7 +1244,7 @@ def run_duration(self, execution, model='claude-sonnet-5-5', effort='medium', ra root=Path(tmp); (root/'.auto-reviewer').mkdir() sig=f'Reviewed with: `auto-reviewer-shared.yaml` `{workflow_ref}` workflow · `cr-request` `v{version}` skill' (root/'.auto-reviewer/review.md').write_text('\nbody\n\n'+sig+'\nNext step: `/cr-received PR #1`\n') - env={**os.environ,'START':'0','SIGNATURE':sig,'SEP':' · ','USAGE_MODEL':model,'USAGE_EFFORT':effort} + env={**os.environ,'START':'0','SIGNATURE':sig,'SEP':' · ','USAGE_MODEL':model,'USAGE_EFFORT':effort,'BODY_FILE_PATH':'.auto-reviewer/review.md','NUMBER':'1'} if execution is not None: (root/'exec.json').write_text(raw if raw is not None else json.dumps(execution)); env['EXEC_FILE']=str(root/'exec.json') run=step('Add review duration').replace('elapsed=$(( $(date +%s) - START ))',f'elapsed={elapsed}') @@ -1273,6 +1273,184 @@ def test_missing_or_unreadable_usage_leaves_duration_only(self): self.assertEqual(self.run_duration([{'type':'system'}]),duration) self.assertEqual(self.run_duration([{'type':'result','usage':{}}]),duration) +class DeterministicTrailerTest(unittest.TestCase): + SIG='Reviewed with: `auto-reviewer-shared.yaml` `v0.0.6` workflow · `cr-request` `v0.0.11` skill' + NEXT='Next step: `/verify -> /finalize squash`' + TABLE='| Intent | System | Craft | Critical | Important | Minor |\n| --- | --- | --- | --- | --- | --- |\n| PASS | PASS | PASS | %s | %s | %s |' + + def finish(self, trailer, counts=(0,0,0)): + body='\n'+self.TABLE%counts+'\n\n**Readiness:** Ready\n\n
Details\n\nEvidence.\n\n
\n'+trailer + with tempfile.TemporaryDirectory() as tmp: + root=Path(tmp); (root/'.auto-reviewer').mkdir() + (root/'.auto-reviewer/review.md').write_text(body) + env={**os.environ,'START':'0','SIGNATURE':self.SIG,'SEP':' · ','USAGE_MODEL':'m','USAGE_EFFORT':'medium','BODY_FILE_PATH':'.auto-reviewer/review.md','NUMBER':'7'} + env.pop('EXEC_FILE',None) + run=step('Add review duration').replace('elapsed=$(( $(date +%s) - START ))','elapsed=125') + result=subprocess.run(['bash','-eo','pipefail','-c',run],cwd=root,env=env,capture_output=True,text=True) + self.assertEqual(result.returncode,0,result.stderr) + return (root/'.auto-reviewer/review.md').read_text() + + def test_missing_malformed_and_correct_trailers_publish_the_same_body(self): + good=self.SIG+' · In `9 min 9 sec`\n'+self.NEXT+'\n' + cases={'missing':'', + 'correct':good, + 'malformed signature':'Reviewed with: something else\n'+self.NEXT+'\n', + 'malformed next step':self.SIG+'\nNext step: verify it - then merge\n', + 'reversed order':self.NEXT+'\n'+self.SIG+' · In `1 min 1 sec`\n', + 'blank lines and CRLF':'\r\n'+self.SIG+'\r\n\r\n'+self.NEXT+'\r\n\r\n'} + results={name:self.finish(trailer) for name,trailer in cases.items()} + expected=results['correct'] + self.assertTrue(expected.endswith('\n\n'+self.SIG+' · In `2 min 5 sec`\n'+self.NEXT+'\n'),expected) + for name,body in results.items(): + with self.subTest(case=name): + self.assertEqual(body,expected) + + def test_validator_accepts_the_normalized_body(self): + script=step('Validate review body') + fragment='fail() { exit 1; }\n'+script[script.index('tail2=$(tr -d'):script.index('# The Next step line loses')] + for trailer in ('','Reviewed with: broken\n'): + with self.subTest(trailer=trailer),tempfile.TemporaryDirectory() as tmp: + path=Path(tmp)/'body'; path.write_text(self.finish(trailer)) + env={**os.environ,'BODY_FILE':str(path),'VERSION':'0.0.11','WORKFLOW_IDENTITY':'auto-reviewer-shared.yaml@v0.0.6'} + result=subprocess.run(['bash','-c',fragment],env=env,capture_output=True,text=True) + self.assertEqual(result.returncode,0,result.stderr) + + def test_fallback_next_step_follows_the_finding_counts(self): + self.assertTrue(self.finish('',(0,0,0)).endswith(self.NEXT+'\n')) + for counts in ((1,0,0),(0,2,0),(0,0,3)): + with self.subTest(counts=counts): + self.assertTrue(self.finish('',counts).endswith('Next step: `/cr-received PR #7`\n')) + + def test_fallback_next_step_needs_a_plain_ready(self): + trailer='
Details\n\nEvidence.\n\n
\n' + for readiness in ('Not ready','Ready with fixes'): + with self.subTest(readiness=readiness): + body='\n'+self.TABLE%(0,0,0)+'\n\n**Readiness:** '+readiness+'\n\n'+trailer + with tempfile.TemporaryDirectory() as tmp: + root=Path(tmp); (root/'.auto-reviewer').mkdir() + (root/'.auto-reviewer/review.md').write_text(body) + env={**os.environ,'START':'0','SIGNATURE':self.SIG,'SEP':' · ','USAGE_MODEL':'m','USAGE_EFFORT':'medium','BODY_FILE_PATH':'.auto-reviewer/review.md','NUMBER':'7'} + env.pop('EXEC_FILE',None) + run=step('Add review duration').replace('elapsed=$(( $(date +%s) - START ))','elapsed=1') + result=subprocess.run(['bash','-eo','pipefail','-c',run],cwd=root,env=env,capture_output=True,text=True) + self.assertEqual(result.returncode,0,result.stderr) + self.assertTrue((root/'.auto-reviewer/review.md').read_text().endswith('Next step: `/cr-received PR #7`\n')) + + def test_valid_agent_next_step_is_kept(self): + body=self.finish(self.SIG+'\nNext step: `/cr-received PR #7` · two findings\n',(0,1,0)) + self.assertTrue(body.endswith('Next step: `/cr-received PR #7` · two findings\n')) + + +def step_env(name): + ruby = os.environ.get('RUBY', 'ruby') + script = "require 'yaml'; require 'json'; w=YAML.load_file(ARGV[0], aliases: true); s=w['jobs'].values.flat_map{|j| j['steps'] || []}.find{|x| x['name']==ARGV[1]}; abort 'missing step' unless s; puts JSON.generate(s.fetch('env', {}))" + return json.loads(subprocess.check_output([ruby, '-e', script, str(WORKFLOW), name], text=True)) + + +class RetryTest(unittest.TestCase): + LOW = r'$\color{green}◎$ Low' + TABLE = '| Intent | System | Craft | Critical | Important | Minor |\n| --- | --- | --- | --- | --- | --- |\n| PASS | PASS | FINDINGS | 0 | 0 | 1 |' + + def body(self, details_close='', details_open='
Details', readiness='**Readiness:** Not ready'): + return ('\n\n' + self.TABLE + '\n\n' + readiness + '\n\n' + details_open + '\n\n- ' + self.LOW + + ' R2-F1 [`src/a.py:1`](url) issue\n\n' + details_close + '\n') + + def precheck(self, body, comment_id='R2-F1'): + draft = {'schema_version':1, 'repository':'owner/repo', 'pr_number':42, 'head_sha':'a'*40, + 'review_source_id':None, 'previous_findings':[], + 'findings':[{'origin_review_id':None, 'original_id':'R2-F1', 'path':'src/a.py', 'severity':'Minor'}]} + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp); (root/'.auto-reviewer').mkdir() + (root/'.auto-reviewer/review.md').write_text(body) + (root/'.auto-reviewer/review-actions.json').write_text(json.dumps(draft)) + (root/'.auto-reviewer/comments.json').write_text(json.dumps([{'path':'src/a.py','line':1,'body':self.LOW+' '+comment_id+' - issue'}])) + output = root/'output' + env = {**os.environ, **step_env('Pre-validate review draft'), 'NUMBER':'42', 'HEAD_SHA':'a'*40, 'REPO':'owner/repo', + 'GITHUB_OUTPUT':str(output)} + result = subprocess.run(['bash','-eo','pipefail','-c',step('Pre-validate review draft')], cwd=root, env=env, capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stderr) + error = (root/'.auto-reviewer/retry-error.txt').read_text() if (root/'.auto-reviewer/retry-error.txt').exists() else '' + return (output.read_text() if output.exists() else ''), error + + def test_precheck_uses_the_publish_actions_validator_verbatim(self): + self.assertEqual(step_env('Pre-validate review draft')['VALIDATE_ACTIONS'], step('Validate review actions')) + + def test_valid_draft_needs_no_retry(self): + self.assertEqual(self.precheck(self.body()), ('', '')) + + def test_inline_finding_mismatch_requests_a_retry_with_the_error(self): + output, error = self.precheck(self.body(), comment_id='R2-F9') + self.assertEqual(output, 'error=true\n') + self.assertIn('inline finding mismatch', error) + + def test_structure_errors_request_a_retry(self): + cases = {'no closing line': self.body(details_close=''), + 'two details': self.body().replace('
', '
Other\n
\n'), + 'no readiness': self.body(readiness='')} + for name, body in cases.items(): + with self.subTest(case=name): + output, error = self.precheck(body) + self.assertEqual(output, 'error=true\n') + self.assertIn('review body structure', error) + + def test_precheck_agrees_with_the_publish_renderer_on_details_counts(self): + for details_close in ('', ''): + with self.subTest(close=details_close): + body = self.body(details_close=details_close) + precheck_failed = self.precheck(body)[0] != '' + self.assertEqual(precheck_failed, details_close == '') + + def test_unclosed_details_is_closed_by_the_workflow(self): + broken = self.body(details_close='') + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp); (root/'.auto-reviewer').mkdir() + (root/'.auto-reviewer/review.md').write_text(broken) + sig = DeterministicTrailerTest.SIG + env = {**os.environ, 'START':'0', 'SIGNATURE':sig, 'SEP':' · ', 'USAGE_MODEL':'m', 'USAGE_EFFORT':'medium', + 'BODY_FILE_PATH':'.auto-reviewer/review.md', 'NUMBER':'42'} + env.pop('EXEC_FILE', None) + run = step('Add review duration').replace('elapsed=$(( $(date +%s) - START ))', 'elapsed=3') + result = subprocess.run(['bash','-eo','pipefail','-c',run], cwd=root, env=env, capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stderr) + fixed = (root/'.auto-reviewer/review.md').read_text() + self.assertIn('issue\n\n\n\n' + sig, fixed) + self.assertEqual(self.precheck(fixed), ('', '')) + + def test_retry_usage_adds_to_the_first_run(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp); (root/'.auto-reviewer').mkdir() + (root/'.auto-reviewer/review.md').write_text(self.body()) + for name, tokens, cost in (('one.json', 100, 0.25), ('two.json', 50, 0.125)): + (root/name).write_text(json.dumps([{'type':'result','total_cost_usd':cost,'usage':{'input_tokens':tokens}}])) + (root/'retry-exec-file').write_text(str(root/'two.json')) + env = {**os.environ, 'START':'0', 'SIGNATURE':DeterministicTrailerTest.SIG, 'SEP':' · ', 'USAGE_MODEL':'m', 'USAGE_EFFORT':'medium', + 'BODY_FILE_PATH':'.auto-reviewer/review.md', 'NUMBER':'42', 'EXEC_FILE':str(root/'one.json'), 'RUNNER_TEMP':str(root)} + run = step('Add review duration').replace('elapsed=$(( $(date +%s) - START ))', 'elapsed=3') + result = subprocess.run(['bash','-eo','pipefail','-c',run], cwd=root, env=env, capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn('`m` `medium` · `150 tokens` · `$0.3750`', (root/'.auto-reviewer/review.md').read_text()) + + def test_retry_step_runs_once_on_the_precheck_error_with_the_same_prompt(self): + ruby = os.environ.get('RUBY', 'ruby') + script = "require 'yaml'; require 'json'; w=YAML.load_file(ARGV[0], aliases: true); puts JSON.generate(w['jobs']['review']['steps'])" + steps = json.loads(subprocess.check_output([ruby, '-e', script, str(WORKFLOW)], text=True)) + first = next(s for s in steps if s.get('id') == 'claude') + retry = next(s for s in steps if s.get('id') == 'claude_retry') + self.assertEqual(retry['if'], "steps.precheck.outputs.error == 'true'") + self.assertTrue(retry['continue-on-error']) + self.assertEqual(retry['uses'], first['uses']) + self.assertEqual(retry['with']['prompt'], first['with']['prompt']) + self.assertEqual(retry['with']['claude_args'], first['with']['claude_args']) + self.assertIn('.auto-reviewer/retry-error.txt', first['with']['prompt']) + names = [s.get('id') or s.get('name') for s in steps] + self.assertEqual(names.count('claude_retry'), 1) + self.assertLess(names.index('claude'), names.index('precheck')) + self.assertLess(names.index('precheck'), names.index('claude_retry')) + after = names[names.index('claude_retry'):] + self.assertIn('Add review duration', after) + self.assertIn('Check inline comments', after) + + class TokenFilterTest(unittest.TestCase): def test_atlassian_prefix_is_blocked_in_every_transport(self): script=step('Validate review body')