diff --git a/.github/actions/README.md b/.github/actions/README.md index 1bf413f..0c62ae8 100644 --- a/.github/actions/README.md +++ b/.github/actions/README.md @@ -79,6 +79,13 @@ The `pr-review` action additionally accepts: Automatic mode runs the independent production-failure analysis for initial and high-risk reviews, but skips it for ordinary incremental updates. `on` always enables it and `off` disables it. +- `state_artifact` - optional, defaults to `true`. + Uploads the run's `.pr-review` state directory as an artifact. Set it to `false` only if the + repository forbids artifacts. +- `state_artifact_retention_days` - optional, defaults to `14`. +- `debug_logs` - deprecated and ignored. The analysis session's full output is now always + printed. Consumers still passing it are unaffected; drop it at your convenience. + The semantic-analysis stage runs under a turn budget: 12 for a low-risk incremental review, 44 for a strong-tier one, and 56 for `deep`. Set `max_turns` to override any of these. @@ -155,8 +162,46 @@ Questions published before the `
` shape existed fall back to a single-l A question that is already open is never re-asked; the original stays the copy the author answers. +### Inspecting a review run + +The published review says what the reviewer concluded. These say how it got there, and they +are the starting point for tuning the rubric, the prompt, or the turn budget. + +**Step summary** (the run's front page) carries the routing decision and its reason, the head +range, the model and turn budget the analysis actually ran with, the model tier, high-risk and +pre-mortem flags, and this round's counts — new findings split into inline and review-body, +new questions, prior findings resolved. It then folds in two blocks: the **pipeline trace** and +the **raw model output before compilation**. A failed run gets the same trace, which shows how +far the round got before it stopped. + +**Job log** groups, in step order: + +- `Review routing` (prepare) — mode and why, prior state source and whether its version still + matches, previous and current head, the compare status and file count, both diff sizes, how + much of the PR conversation was included versus truncated, the paths in scope, and every open + prior finding and question the model was handed. +- `Analysis settings` (compose) — model, tier, depth, pre-mortem, turn budgets, allowed tools. +- The analysis step itself prints the session's full output, always: every tool call, which + files it opened, and which it never read. The step is collapsed until you expand it. +- `Model output (raw, before compilation)` and `Compilation decisions` (compile) — one line per + model result the compiler accepted, suppressed as a duplicate of an open finding, dropped at + the per-severity cap of five, or rerouted to the review body because its line is not + commentable, plus each prior finding and question disposition. +- `Publication` (publish) — review ID, inline comments requested versus posted, whether inline + publication fell back to the review body, threads resolved, and the sticky comment ID. + +**Run artifact** `pr-review-state---` holds the bytes themselves, for 14 days +by default: `review-input.json` (everything the model was given), `review.diff` and `full.diff`, +`analysis-transcript.json` (the session's turn-by-turn record, plus a `-retry-` twin when the +retry ran), `structured-output.json`, `model-output.json`, `review-payload.json` (the compiled +review, including its decision trace), `publish-result.json`, and `trace.log`. + +"The model missed it" and "the pipeline dropped it" look identical in the published review and +different in these. Compare `model-output.json` against the `Compilation decisions` group first. + ### Comment-triggered reconciliation (opt-in) + By default the review only runs on `pull_request` events, so an author who answers an open question in a PR comment sees nothing happen until the next push. A consumer can also let a comment drive a reconcile round by adding an `issue_comment` trigger: diff --git a/.github/actions/claude-pr-review/action.yml b/.github/actions/claude-pr-review/action.yml index 6cb89af..f040880 100644 --- a/.github/actions/claude-pr-review/action.yml +++ b/.github/actions/claude-pr-review/action.yml @@ -53,10 +53,24 @@ inputs: required: false default: "false" description: > - `true` prints the analysis session's full output, including its tool - calls, into the job log. Off by default because the log is long and the - review itself is the product; turn it on to answer why a review reached - the verdict it did — which files it opened, and which it never read. + Deprecated and ignored. The analysis session's full output — its tool + calls, the files it opened, and the ones it never read — is now always + printed, and the whole `.pr-review` state directory plus the session + transcript are uploaded as a run artifact. Kept only so consumers that + still pass it do not break. + state_artifact: + required: false + default: "true" + description: > + `true` uploads the `.pr-review` state directory — routing input, both + diffs, the raw model output, the compiled payload, the pipeline trace, + and the analysis session transcript — as a run artifact. This is the + only copy that outlives the runner, so leave it on unless the repository + forbids artifacts. + state_artifact_retention_days: + required: false + default: "14" + description: Retention in days for the review state artifact. premortem: required: false default: auto @@ -172,6 +186,21 @@ runs: echo "allowed_tools=${allowed_tools}" >> "$GITHUB_OUTPUT" echo "runtime_flags=${runtime_flags}" >> "$GITHUB_OUTPUT" echo "retry_runtime_flags=${retry_runtime_flags}" >> "$GITHUB_OUTPUT" + # Surfaced so the run report can state the settings the analysis + # actually used. Reading them back out of the flag string later would + # be guesswork. + echo "selected_model=${selected_model:-default}" >> "$GITHUB_OUTPUT" + echo "max_turns=${max_turns}" >> "$GITHUB_OUTPUT" + echo "retry_max_turns=${retry_turns}" >> "$GITHUB_OUTPUT" + + echo "::group::Analysis settings" + echo "model: ${selected_model:-Claude Code default}" + echo "model tier: ${MODEL_TIER}" + echo "review depth: ${REVIEW_DEPTH}" + echo "pre-mortem: ${RUN_PREMORTEM}" + echo "max turns: ${max_turns} (retry ${retry_turns})" + echo "allowed tools: ${allowed_tools}" + echo "::endgroup::" - name: Analyze and verify findings id: review @@ -182,7 +211,10 @@ runs: claude_code_oauth_token: ${{ inputs.claude_code_oauth_token }} allowed_bots: ${{ inputs.allowed_bots }} display_report: false - show_full_output: ${{ inputs.debug_logs == 'true' }} + # Always on. The review body says what the model concluded; only this + # says how it got there — which files it opened, and which it never + # read. The step is collapsed in the job log until you expand it. + show_full_output: true additional_permissions: | actions: read claude_args: | @@ -323,7 +355,7 @@ runs: claude_code_oauth_token: ${{ inputs.claude_code_oauth_token }} allowed_bots: ${{ inputs.allowed_bots }} display_report: false - show_full_output: ${{ inputs.debug_logs == 'true' }} + show_full_output: true additional_permissions: | actions: read claude_args: | @@ -359,6 +391,46 @@ runs: Markdown, or a fenced JSON block. Empty arrays are correct for a clean change. + - name: Capture analysis session transcript + if: always() + continue-on-error: true + shell: bash + env: + EXECUTION_FILE: ${{ steps.review.outputs.execution_file }} + RETRY_EXECUTION_FILE: ${{ steps.review_retry.outputs.execution_file }} + SESSION_ID: ${{ steps.review.outputs.session_id }} + RETRY_SESSION_ID: ${{ steps.review_retry.outputs.session_id }} + STRUCTURED_OUTPUT: ${{ steps.review.outputs.structured_output }} + RETRY_STRUCTURED_OUTPUT: ${{ steps.review_retry.outputs.structured_output }} + STATE_DIR: ${{ github.workspace }}/.pr-review + run: | + set -uo pipefail + # The execution file is the model's own turn-by-turn record: every + # tool call, its arguments, and its result. It lives in a temp path + # that dies with the runner, so copy it into the state directory the + # artifact step uploads. + mkdir -p "$STATE_DIR" + copy_transcript() { + local source="$1" name="$2" session="$3" + if [[ -n "$source" && -f "$source" ]]; then + cp "$source" "$STATE_DIR/$name" + echo "captured $name from $source (session ${session:-unknown})" + else + echo "no transcript available for $name" + fi + } + copy_transcript "$EXECUTION_FILE" "analysis-transcript.json" "$SESSION_ID" + copy_transcript "$RETRY_EXECUTION_FILE" "analysis-retry-transcript.json" "$RETRY_SESSION_ID" + # Keep the raw structured output beside the transcript. `compile` also + # writes a parsed copy, but it never runs when compilation is what + # failed, and this is exactly the case you want the bytes for. + if [[ -n "$STRUCTURED_OUTPUT" ]]; then + printf '%s' "$STRUCTURED_OUTPUT" > "$STATE_DIR/structured-output.json" + fi + if [[ -n "$RETRY_STRUCTURED_OUTPUT" ]]; then + printf '%s' "$RETRY_STRUCTURED_OUTPUT" > "$STATE_DIR/structured-output-retry.json" + fi + - name: Compile and validate review id: compile shell: bash @@ -399,7 +471,25 @@ runs: PUBLISH_OUTCOME: ${{ steps.publish.outcome }} PUBLISH_PUBLISHED: ${{ steps.publish.outputs.published }} PUBLISH_STALE: ${{ steps.publish.outputs.stale }} + ANALYSIS_MODEL: ${{ steps.compose.outputs.selected_model }} + ANALYSIS_MAX_TURNS: ${{ steps.compose.outputs.max_turns }} + ANALYSIS_RETRY_MAX_TURNS: ${{ steps.compose.outputs.retry_max_turns }} + ANALYSIS_REVIEW_DEPTH: ${{ inputs.review_depth }} + ANALYSIS_ALLOWED_TOOLS: ${{ steps.compose.outputs.allowed_tools }} run: | set -euo pipefail python3 "$GITHUB_ACTION_PATH/review_pipeline.py" report \ --state-dir "$STATE_DIR" + + - name: Upload review state for inspection + if: always() && inputs.state_artifact == 'true' + continue-on-error: true + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + # Run ID and attempt keep reruns from colliding; the PR number keeps + # concurrent reviews in one run distinguishable. + name: pr-review-state-${{ github.event.pull_request.number || github.event.issue.number }}-${{ github.run_id }}-${{ github.run_attempt }} + path: ${{ github.workspace }}/.pr-review + if-no-files-found: warn + retention-days: ${{ inputs.state_artifact_retention_days }} + include-hidden-files: true diff --git a/.github/actions/claude-pr-review/review_pipeline.py b/.github/actions/claude-pr-review/review_pipeline.py index 819b764..9985824 100644 --- a/.github/actions/claude-pr-review/review_pipeline.py +++ b/.github/actions/claude-pr-review/review_pipeline.py @@ -70,6 +70,9 @@ GITHUB_LINK_RETRY_ATTEMPTS = 5 GITHUB_LINK_RETRY_DELAY_SECONDS = 1 FAILURE_FILENAME = "failure.json" +TRACE_FILENAME = "trace.log" +MODEL_OUTPUT_FILENAME = "model-output.json" +TRACE_LIST_LIMIT = 40 DEFAULT_REMEDIATIONS = { "prepare": ( @@ -254,7 +257,62 @@ def sanitize_text(value: Any, *, maximum: int) -> str: return text[:maximum].rstrip() +def log(message: str) -> None: + """Write one line to the job log, unbuffered so ordering survives.""" + print(message, flush=True) + + +def emit_trace( + state_dir: Path | None, + title: str, + lines: list[str], +) -> None: + """Print a collapsible trace block and persist it beside the state. + + The job log is where someone looks first when a review surprises them, and + the persisted copy is what the run's artifact still carries once the runner + is gone. Both are rendered from the same lines so they cannot disagree. + """ + if not lines: + return + log(f"::group::{title}") + for line in lines: + log(line) + log("::endgroup::") + if state_dir is None: + return + try: + with (state_dir / TRACE_FILENAME).open( + "a", encoding="utf-8" + ) as handle: + handle.write(f"### {title}\n\n") + handle.write("\n".join(lines).rstrip() + "\n\n") + except OSError: + # Tracing is diagnostic. Never fail a review because the log could not + # be written. + pass + + +def read_trace(state_dir: Path) -> str: + try: + return (state_dir / TRACE_FILENAME).read_text(encoding="utf-8") + except OSError: + return "" + + +def trace_list(label: str, items: list[str]) -> list[str]: + """Render a bounded bullet list, saying so when it had to truncate.""" + if not items: + return [f"{label}: (none)"] + lines = [f"{label} ({len(items)}):"] + lines.extend(f" - {item}" for item in items[:TRACE_LIST_LIMIT]) + if len(items) > TRACE_LIST_LIMIT: + lines.append(f" - … {len(items) - TRACE_LIST_LIMIT} more") + return lines + + def read_json_file(path: Path) -> dict[str, Any] | None: + try: value = json.loads(path.read_text(encoding="utf-8")) except (OSError, json.JSONDecodeError): @@ -1084,7 +1142,103 @@ def manifest_has_open_items(manifest: dict[str, Any]) -> bool: return False +def open_items(container: Any) -> list[tuple[str, dict[str, Any]]]: + if not isinstance(container, dict): + return [] + return [ + (str(item_id), item) + for item_id, item in container.items() + if isinstance(item, dict) and item.get("status") == "open" + ] + + +def prepare_trace( + input_value: dict[str, Any], + *, + prior_state_source: str, + version_matches: bool, + comparison_status: str | None, + comparison_files: int, + full_diff_bytes: int, + review_diff_bytes: int, +) -> list[str]: + """Explain how this round was routed and what the model was handed. + + Routing is the single most opaque decision in the pipeline: whether a run + reviewed everything, only a delta, or nothing at all determines what the + model could possibly have found, and until now that reasoning existed only + inside `review-input.json` on a runner that is about to disappear. + """ + scope = input_value["review_scope"] + manifest = input_value.get("manifest") or {} + conversation = input_value.get("conversation") or {} + threads = input_value.get("review_threads") or [] + findings = manifest.get("findings") or {} + questions = manifest.get("questions") or {} + open_findings = open_items(findings) + open_questions = open_items(questions) + previous = scope.get("previous_reviewed_head") or "(none)" + + lines = [ + f"pull request: {input_value['repository']}" + f"#{input_value['pull_request']}", + f"publisher login: {input_value.get('publisher_login') or '(unknown)'}", + f"pipeline version: {input_value.get('pipeline_version')}" + f" · rubric version: {input_value.get('rubric_version')}", + f"prior state: {prior_state_source}" + f" · version match: {str(version_matches).lower()}", + f"previous reviewed head: {previous}", + f"current head: {scope['current_head']}", + "compare previous..current: " + f"status={comparison_status or '(not compared)'}" + f" files={comparison_files}", + f"MODE: {scope['mode']} — {scope['reason']}", + f"model tier: {scope['model_tier']}" + f" · high risk: {str(scope['high_risk']).lower()}" + f" · pre-mortem: {str(scope['run_premortem']).lower()}", + f"thread resolution: " + f"{'enabled' if input_value.get('thread_resolution_enabled') else 'disabled'}", + f"review.diff: {review_diff_bytes} bytes" + f" · full.diff: {full_diff_bytes} bytes", + f"conversation: {conversation.get('included_entries', 0)} of " + f"{conversation.get('total_entries', 0)} timeline entries" + f" · {conversation.get('included_body_chars', 0)} chars" + f" · truncated={str(bool(conversation.get('truncated'))).lower()}", + f"review threads: {len(threads)}", + f"previous automated review: " + f"{'present' if conversation.get('previous_automated_review') else 'none'}", + ] + lines.extend( + trace_list("changed paths in scope", list(scope["changed_paths"])) + ) + if scope["mode"] == "incremental": + lines.append( + f"full PR paths (context only): {len(scope['full_pr_paths'])}" + ) + lines.extend( + trace_list( + "open prior findings", + [ + f"{item_id} [{item.get('severity')}] " + f"{item.get('path')}:{item.get('line')} — {item.get('title')}" + for item_id, item in open_findings + ], + ) + ) + lines.extend( + trace_list( + "open prior questions", + [ + f"{item_id} [{item.get('confidence')}] {item.get('question')}" + for item_id, item in open_questions + ], + ) + ) + return lines + + def prepare(args: argparse.Namespace) -> None: + action_dir = Path(__file__).resolve().parent state_dir = Path(args.state_dir) state_dir.mkdir(parents=True, exist_ok=True) @@ -1145,6 +1299,13 @@ def prepare(args: argparse.Namespace) -> None: publisher_login=publisher_login, ) prior_state = sticky_state or review_state + prior_state_source = ( + "sticky status comment" + if sticky_state is not None + else "previous review body" + if review_state is not None + else "none (first review of this PR)" + ) manifest = ( copy.deepcopy(prior_state) if prior_state is not None @@ -1366,6 +1527,24 @@ def prepare(args: argparse.Namespace) -> None: json.dumps(input_value, indent=2, sort_keys=True) + "\n", encoding="utf-8", ) + emit_trace( + state_dir, + "Review routing", + prepare_trace( + input_value, + prior_state_source=prior_state_source, + version_matches=version_matches, + comparison_status=( + str(comparison.get("status")) + if isinstance(comparison, dict) + else None + ), + comparison_files=len(comparison_files), + full_diff_bytes=len(full_diff.encode("utf-8")), + review_diff_bytes=len(incremental_diff.encode("utf-8")), + ), + ) + schema = json.loads( (action_dir / "review-output.schema.json").read_text(encoding="utf-8") ) @@ -1516,6 +1695,10 @@ def compile_review( f"" ) + # Every branch below that silently drops, dedupes, or reroutes a model + # result appends its reason here. Without it, "the model reported it but + # the PR never showed it" is unanswerable after the fact. + trace: list[str] = [] if scope["mode"] == "skip": model_output = { @@ -1606,12 +1789,24 @@ def compile_review( # already closed this finding. That human action is authoritative: # a concordant `resolved` is a no-op, and an `open` must not reopen # a thread a reviewer deliberately closed. + trace.append( + f"prior finding {item_id}: ignored '{status}' disposition — " + "already resolved on GitHub" + ) print( f"ignoring {status} disposition for {item_id}: already " "resolved on GitHub", file=sys.stderr, ) continue + trace.append( + f"prior finding {item_id}: {status}" + + ( + f" — {public_text(disposition.get('reason'), maximum=200)}" + if disposition.get("reason") + else "" + ) + ) item["status"] = status item["last_checked_sha"] = head if status == "resolved": @@ -1723,19 +1918,46 @@ def compile_review( and manifest["findings"][prior_finding_id].get("status") == "open" ): manifest["findings"][prior_finding_id]["last_checked_sha"] = head + trace.append( + f"finding {index} suppressed: restates open prior finding " + f"{prior_finding_id} ({finding['path']}:{line})" + ) continue finding["finding_id"] = finding_id(finding) existing = manifest["findings"].get(finding["finding_id"]) if isinstance(existing, dict) and existing.get("status") == "open": existing["last_checked_sha"] = head + trace.append( + f"finding {index} suppressed: same fingerprint as open " + f"finding {finding['finding_id']} ({finding['path']}:{line})" + ) continue finding["inline"] = line in commentable.get(path, set()) + anchor = ( + "inline" + if finding["inline"] + else "review body — line is not commentable in the PR diff" + ) + trace.append( + f"finding {index} accepted: [{severity}] " + f"{finding['path']}:{line} ({anchor}) — {finding['title']}" + ) findings.append(finding) - high = [f for f in findings if f["severity"] in {"critical", "major"}][:5] - low = [f for f in findings if f["severity"] in {"minor", "nit"}][:5] + + high_all = [f for f in findings if f["severity"] in {"critical", "major"}] + low_all = [f for f in findings if f["severity"] in {"minor", "nit"}] + high = high_all[:5] + low = low_all[:5] + for dropped in high_all[5:] + low_all[5:]: + trace.append( + f"finding dropped by per-severity cap of 5: " + f"[{dropped['severity']}] {dropped['path']}:{dropped['line']} — " + f"{dropped['title']}" + ) findings = high + low + question_state = manifest.setdefault("questions", {}) open_question_ids = { item_id @@ -1775,6 +1997,14 @@ def compile_review( ) item["status"] = status item["last_checked_sha"] = head + trace.append( + f"prior question {item_id}: {status}" + + ( + f" — {public_text(disposition.get('reason'), maximum=200)}" + if disposition.get("reason") + else "" + ) + ) if status != "open": item["closed_sha"] = head item["disposition_reason"] = public_text( @@ -1787,6 +2017,11 @@ def compile_review( item["annotation"] = "pending" questions: list[dict[str, Any]] = [] + if len(model_output["open_questions"]) > 3: + trace.append( + f"open questions truncated to 3 of " + f"{len(model_output['open_questions'])} returned" + ) for index, raw in enumerate(model_output["open_questions"][:3]): if not isinstance(raw, dict): raise PipelineError( @@ -1823,9 +2058,17 @@ def compile_review( # Already asked and still unanswered. Re-asking would orphan the # original, which is the copy the author is expected to answer. existing["last_checked_sha"] = head + trace.append( + f"question {index} suppressed: already open as " + f"{question['question_id']}" + ) continue + trace.append( + f"question {index} accepted: [{confidence}] {question['question']}" + ) questions.append(question) + for question in questions: question_state[question["question_id"]] = { "question_id": question["question_id"], @@ -2023,10 +2266,26 @@ def compile_review( f"{STATE_MARKER_PREFIX}{encode_state(review_manifest)} -->" ) + trace.append( + f"verdict: {round_value['result']}" + f" · new findings: {len(findings)}" + f" ({len(inline_comments)} inline, {len(body_only)} in review body)" + f" · new questions: {len(questions)}" + f" · prior findings resolved this round: {resolved_count}" + f" · open findings after this round: {len(open_findings)}" + ) + trace.append( + f"submits a review: {str(bool(findings or questions)).lower()}" + f" · threads to resolve: {len(set(resolution_ids))}" + f" · question annotations queued: {len(question_annotations)}" + ) + return { "schema_version": SCHEMA_VERSION, "repository": review_input["repository"], "pull_request": review_input["pull_request"], + "trace": trace, + "publisher_login": review_input.get("publisher_login"), "frozen_head": head, "base_sha": review_input["pull_request_data"]["base_sha"], @@ -2058,6 +2317,11 @@ def compile_command(args: argparse.Namespace) -> None: ) if review_input["review_scope"]["mode"] == "skip": model_output = None + emit_trace( + state_dir, + "Model output", + ["skip mode: no analysis ran, so there is nothing to compile"], + ) else: raw = os.environ.get("REVIEW_STRUCTURED_OUTPUT", "") if not raw: @@ -2072,18 +2336,38 @@ def compile_command(args: argparse.Namespace) -> None: try: model_output = json.loads(raw) except json.JSONDecodeError as error: + # Keep the unparsable bytes: without them the only evidence of + # what the model actually said dies with the runner. + (state_dir / "model-output.raw").write_text(raw, encoding="utf-8") raise PipelineError( "Claude returned malformed structured review JSON", code="MODEL_OUTPUT_INVALID", ) from error + # The model's own result, before the compiler dedupes, caps, and + # reroutes it. Comparing this against the published review is how you + # tell a model miss from a pipeline drop. + (state_dir / MODEL_OUTPUT_FILENAME).write_text( + json.dumps(model_output, indent=2, sort_keys=True) + "\n", + encoding="utf-8", + ) + emit_trace( + # Printed to the job log and kept as model-output.json; leaving it + # out of trace.log keeps that file a readable decision log. + None, + "Model output (raw, before compilation)", + json.dumps(model_output, indent=2, sort_keys=True).splitlines(), + ) + payload = compile_review(review_input, model_output) (state_dir / "review-payload.json").write_text( json.dumps(payload, indent=2, sort_keys=True) + "\n", encoding="utf-8", ) + emit_trace(state_dir, "Compilation decisions", payload.get("trace") or []) write_github_output({"verdict": payload["verdict"]}) + def existing_round_review( repository: str, pull_request: int, @@ -2724,7 +3008,40 @@ def write_job_summary(body: str) -> None: summary.write(body.rstrip() + "\n") +SUMMARY_EMBED_LIMIT = 60_000 + + +def details_block(title: str, body: str, *, language: str = "") -> str: + """Fold a long diagnostic into the step summary without flooding it.""" + text = (body or "").rstrip() + if not text.strip(): + return "" + if len(text) > SUMMARY_EMBED_LIMIT: + text = ( + text[:SUMMARY_EMBED_LIMIT] + + "\n… truncated. The complete copy is in the review-state " + "artifact attached to this run." + ) + return ( + f"\n\n
\n{title}\n\n" + f"```{language}\n{text}\n```\n\n
" + ) + + +def analysis_settings() -> list[str]: + """Report the knobs the analysis actually ran with, not the defaults.""" + rows = [ + ("Model", os.environ.get("ANALYSIS_MODEL", "")), + ("Turn budget", os.environ.get("ANALYSIS_MAX_TURNS", "")), + ("Retry turn budget", os.environ.get("ANALYSIS_RETRY_MAX_TURNS", "")), + ("Review depth", os.environ.get("ANALYSIS_REVIEW_DEPTH", "")), + ("Allowed tools", os.environ.get("ANALYSIS_ALLOWED_TOOLS", "")), + ] + return [f"- **{label}:** `{value}`" for label, value in rows if value] + + def step_outcomes() -> dict[str, str]: + return { phase: os.environ.get(f"{phase.upper()}_OUTCOME", "") for phase in ( @@ -2857,6 +3174,7 @@ def report(args: argparse.Namespace) -> None: if outcomes_text else "" ) + settings = analysis_settings() body = ( "## Claude PR review failed\n\n" f"**`{diagnostic['code']}` · phase " @@ -2864,7 +3182,11 @@ def report(args: argparse.Namespace) -> None: f"- **Cause:** {diagnostic['message']}\n" f"- **Next action:** {diagnostic['remediation']}\n" + ("\n".join(context_lines) + "\n" if context_lines else "") + + ("\n".join(settings) + "\n" if settings else "") + details + # A failure is exactly when the trace matters most: it shows how + # far the round got before it stopped. + + details_block("Pipeline trace", read_trace(state_dir)) ) write_job_summary(body) close_out_sticky( @@ -2911,17 +3233,54 @@ def report(args: argparse.Namespace) -> None: status = "✅ Review completed and publication succeeded." else: status = "✅ Review completed." + scope_reason = scope.get("reason") or "unknown" + status_summary = payload.get("status_summary") or {} + rows = [ + f"- **Verdict:** `{verdict}`", + f"- **Mode:** `{mode}` — {scope_reason}", + f"- **Head:** `{head}`", + f"- **Previous reviewed head:** " + f"`{scope.get('previous_reviewed_head') or 'none'}`", + f"- **Files in scope:** `{len(scope.get('changed_paths') or [])}`", + f"- **Model tier:** `{scope.get('model_tier') or 'unknown'}`" + f" · **high risk:** `{str(bool(scope.get('high_risk'))).lower()}`" + f" · **pre-mortem:** `{str(bool(scope.get('run_premortem'))).lower()}`", + "- **Thread resolution:** " + f"`{'enabled' if review_input.get('thread_resolution_enabled') else 'disabled'}`", + f"- **Model retry used:** `{'yes' if retry_used else 'no'}`", + ] + rows.extend(analysis_settings()) + if payload: + rows.append( + f"- **New findings:** `{status_summary.get('new_findings', 0)}`" + f" ({len(payload.get('inline_comments') or [])} inline, " + f"{len(payload.get('body_only_findings') or [])} in the review " + "body)" + ) + rows.append( + "- **New questions:** " + f"`{status_summary.get('new_questions', 0)}`" + f" · **prior findings resolved:** " + f"`{status_summary.get('resolved_findings', 0)}`" + ) body = ( "## Claude PR review\n\n" - f"{status}\n\n" - f"- **Verdict:** `{verdict}`\n" - f"- **Mode:** `{mode}`\n" - f"- **Head:** `{head}`\n" - "- **Thread resolution:** " - f"`{'enabled' if review_input.get('thread_resolution_enabled') else 'disabled'}`\n" - f"- **Model retry used:** `{'yes' if retry_used else 'no'}`\n" + f"{status}\n\n" + "\n".join(rows) + "\n" + + details_block("Pipeline trace", read_trace(state_dir)) + + details_block( + "Model output (before compilation)", + json.dumps( + read_json_file(state_dir / MODEL_OUTPUT_FILENAME) or {}, + indent=2, + sort_keys=True, + ) + if (state_dir / MODEL_OUTPUT_FILENAME).exists() + else "", + language="json", + ) ) write_job_summary(body) + print( f"Claude PR review: OK verdict={verdict} mode={mode} head={head}" ) @@ -2932,6 +3291,11 @@ def publish(args: argparse.Namespace) -> None: payload_path = state_dir / "review-payload.json" payload = json.loads(payload_path.read_text(encoding="utf-8")) if payload["mode"] == "skip": + emit_trace( + state_dir, + "Publication", + ["skip mode: nothing to publish"], + ) write_github_output({"published": "false", "stale": "false"}) return @@ -2953,6 +3317,14 @@ def require_frozen_pull() -> None: try: require_frozen_pull() except StaleReviewError: + emit_trace( + state_dir, + "Publication", + [ + "discarded before writing: the PR head moved away from " + f"{payload['frozen_head']}" + ], + ) write_github_output({"published": "false", "stale": "true"}) return @@ -3028,14 +3400,43 @@ def require_frozen_pull() -> None: before_write=require_frozen_pull, ) except StaleReviewError: + emit_trace( + state_dir, + "Publication", + [ + "discarded mid-publication: the PR head moved away from " + f"{payload['frozen_head']}" + ], + ) write_github_output({"published": "false", "stale": "true"}) return + emit_trace( + state_dir, + "Publication", + [ + f"review id: {review_id if review_id is not None else '(none)'}", + f"inline comments requested: {len(payload['inline_comments'])}" + f" · posted: {len(posted_comments)}" + f" · inline publication: " + f"{'succeeded' if inline_published else 'fell back to the review body'}", + f"body-only findings: {len(payload['body_only_findings'])}", + f"threads requested for resolution: " + f"{len(payload['resolve_thread_ids'])}" + f" · resolved: {len(resolved_threads)}", + f"question annotations attempted: " + f"{len(payload['question_annotations'])}", + + f"sticky comment id: {sticky_id}", + ], + ) + result = { "review_id": review_id, "sticky_comment_id": sticky_id, "manifest": manifest, } + (state_dir / "publish-result.json").write_text( json.dumps(result, indent=2, sort_keys=True) + "\n", encoding="utf-8", diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index b991e89..f840c99 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -107,11 +107,10 @@ def test_action_allows_structured_output(self): self.assertIn("Read,Glob,Grep,StructuredOutput,", action) self.assertIn("id: review_retry", action) self.assertIn("MUST call the\n StructuredOutput tool", action) - self.assertEqual( - action.count("show_full_output: ${{ inputs.debug_logs == 'true' }}"), - 2, - ) + self.assertEqual(action.count("show_full_output: true"), 2) self.assertIn("\n debug_logs:\n", action) + self.assertIn("Deprecated and ignored", action) + self.assertEqual(action.count("display_report: false"), 2) self.assertIn("- name: Report concise review outcome", action) self.assertIn("if: always()", action) @@ -2063,5 +2062,293 @@ def test_skip_mode_keeps_questions_open(self): self.assertEqual(payload["question_annotations"], []) +class ObservabilityTests(unittest.TestCase): + """A review that cannot be inspected cannot be improved. + + These cover the diagnostic surface: the routing explanation, the record of + every model result the compiler dropped or rerouted, and the step summary + that carries both once the runner is gone. + """ + + def test_emit_trace_prints_a_group_and_persists_it(self): + with tempfile.TemporaryDirectory() as directory: + state_dir = Path(directory) + with mock.patch("builtins.print") as printed: + pipeline.emit_trace(state_dir, "Routing", ["mode: full"]) + + printed.assert_any_call("::group::Routing", flush=True) + printed.assert_any_call("mode: full", flush=True) + printed.assert_any_call("::endgroup::", flush=True) + self.assertIn("mode: full", pipeline.read_trace(state_dir)) + + def test_emit_trace_appends_rather_than_replacing(self): + with tempfile.TemporaryDirectory() as directory: + state_dir = Path(directory) + pipeline.emit_trace(state_dir, "Routing", ["first"]) + pipeline.emit_trace(state_dir, "Publication", ["second"]) + + trace = pipeline.read_trace(state_dir) + self.assertIn("first", trace) + self.assertIn("second", trace) + + def test_emit_trace_ignores_empty_blocks(self): + with tempfile.TemporaryDirectory() as directory: + state_dir = Path(directory) + pipeline.emit_trace(state_dir, "Routing", []) + self.assertEqual(pipeline.read_trace(state_dir), "") + + def test_trace_list_marks_truncation(self): + items = [f"path/{index}.py" for index in range(pipeline.TRACE_LIST_LIMIT + 5)] + lines = pipeline.trace_list("changed paths", items) + + self.assertIn(f"changed paths ({len(items)}):", lines[0]) + self.assertIn("… 5 more", lines[-1]) + + def test_trace_list_reports_an_empty_list(self): + self.assertEqual( + pipeline.trace_list("changed paths", []), + ["changed paths: (none)"], + ) + + def test_prepare_trace_explains_the_routing_decision(self): + value = review_input(mode="incremental") + value["manifest"]["findings"]["F-open"] = { + "status": "open", + "severity": "major", + "path": "src/example.py", + "line": 11, + "title": "Retry state survives a failed attempt", + } + value["manifest"]["questions"] = { + "Q-open": { + "status": "open", + "confidence": "medium", + "question": "Can the upstream return duplicates?", + } + } + + lines = pipeline.prepare_trace( + value, + prior_state_source="sticky status comment", + version_matches=True, + comparison_status="ahead", + comparison_files=3, + full_diff_bytes=4096, + review_diff_bytes=512, + ) + text = "\n".join(lines) + + self.assertIn("MODE: incremental — test", text) + self.assertIn("prior state: sticky status comment", text) + self.assertIn("status=ahead files=3", text) + self.assertIn("review.diff: 512 bytes", text) + self.assertIn("src/example.py", text) + self.assertIn("F-open", text) + self.assertIn("Q-open", text) + + def test_prepare_trace_reports_a_truncated_conversation(self): + value = review_input() + value["conversation"].update( + { + "total_entries": 200, + "included_entries": 120, + "included_body_chars": 90_000, + "truncated": True, + } + ) + + text = "\n".join( + pipeline.prepare_trace( + value, + prior_state_source="none (first review of this PR)", + version_matches=False, + comparison_status=None, + comparison_files=0, + full_diff_bytes=10, + review_diff_bytes=10, + ) + ) + + self.assertIn("120 of 200 timeline entries", text) + self.assertIn("truncated=true", text) + + def test_compile_trace_records_accepted_and_rerouted_findings(self): + value = review_input() + output = clean_output() + output["findings"] = [ + sample_finding(), + sample_finding( + title="Stale companion config", + # Not a commentable line, so this one cannot be anchored. + line=900, + ), + ] + + payload = pipeline.compile_review(value, output) + trace = "\n".join(payload["trace"]) + + self.assertIn("finding 0 accepted", trace) + self.assertIn("(inline)", trace) + self.assertIn("line is not commentable", trace) + self.assertIn("new findings: 2", trace) + + def test_compile_trace_records_the_per_severity_cap(self): + value = review_input() + output = clean_output() + output["findings"] = [ + sample_finding(title=f"Major defect {index}", line=11) + for index in range(7) + ] + + payload = pipeline.compile_review(value, output) + trace = "\n".join(payload["trace"]) + + self.assertEqual(len(payload["inline_comments"]), 5) + self.assertIn("dropped by per-severity cap of 5", trace) + self.assertIn("Major defect 6", trace) + + def test_compile_trace_records_a_suppressed_duplicate_finding(self): + value = review_input() + first = pipeline.compile_review(value, {**clean_output(), "findings": [sample_finding()]}) + second_input = review_input() + second_input["manifest"] = first["manifest"] + + payload = pipeline.compile_review( + second_input, + {**clean_output(), "findings": [sample_finding()]}, + ) + trace = "\n".join(payload["trace"]) + + self.assertIn("same fingerprint as open finding", trace) + self.assertEqual(payload["inline_comments"], []) + + def test_compile_trace_records_question_truncation(self): + value = review_input() + output = clean_output() + output["open_questions"] = [ + { + "question": f"Question {index}?", + "confidence": "medium", + "why_it_matters": "It changes the verdict.", + "verification": "Check the upstream contract.", + } + for index in range(5) + ] + + payload = pipeline.compile_review(value, output) + trace = "\n".join(payload["trace"]) + + self.assertIn("open questions truncated to 3 of 5", trace) + self.assertIn("question 0 accepted", trace) + + def test_details_block_truncates_and_points_at_the_artifact(self): + block = pipeline.details_block( + "Pipeline trace", + "x" * (pipeline.SUMMARY_EMBED_LIMIT + 100), + ) + + self.assertIn("Pipeline trace", block) + self.assertIn("review-state artifact", block) + + def test_details_block_omits_empty_content(self): + self.assertEqual(pipeline.details_block("Pipeline trace", ""), "") + + def test_analysis_settings_reports_only_what_was_set(self): + with mock.patch.dict( + pipeline.os.environ, + { + "ANALYSIS_MODEL": "claude-opus-4-7", + "ANALYSIS_MAX_TURNS": "44", + "ANALYSIS_RETRY_MAX_TURNS": "", + "ANALYSIS_REVIEW_DEPTH": "standard", + "ANALYSIS_ALLOWED_TOOLS": "", + }, + clear=False, + ): + rows = pipeline.analysis_settings() + + text = "\n".join(rows) + self.assertIn("**Model:** `claude-opus-4-7`", text) + self.assertIn("**Turn budget:** `44`", text) + self.assertNotIn("Retry turn budget", text) + self.assertNotIn("Allowed tools", text) + + @mock.patch("builtins.print") + def test_success_report_embeds_the_trace_and_model_output(self, _print): + with tempfile.TemporaryDirectory() as directory: + state_dir = Path(directory) + summary_path = state_dir / "summary.md" + state_dir.joinpath("review-input.json").write_text( + json.dumps(review_input(mode="incremental")), + encoding="utf-8", + ) + state_dir.joinpath("review-payload.json").write_text( + json.dumps( + { + "verdict": "findings", + "mode": "incremental", + "frozen_head": "b" * 40, + "inline_comments": [{"finding_id": "F-1"}], + "body_only_findings": ["- `src/example.py:900` — …"], + "status_summary": { + "new_findings": 2, + "new_questions": 1, + "resolved_findings": 3, + }, + } + ), + encoding="utf-8", + ) + state_dir.joinpath(pipeline.MODEL_OUTPUT_FILENAME).write_text( + json.dumps({"scope_summary": "Reviewed the delta."}), + encoding="utf-8", + ) + pipeline.emit_trace( + state_dir, + "Review routing", + ["MODE: incremental — valid prior manifest"], + ) + environment = { + "GITHUB_STEP_SUMMARY": str(summary_path), + "PREPARE_OUTCOME": "success", + "COMPILE_OUTCOME": "success", + "PUBLISH_OUTCOME": "success", + "PUBLISH_PUBLISHED": "true", + "PUBLISH_STALE": "false", + "ANALYSIS_MODEL": "claude-opus-4-7", + "ANALYSIS_MAX_TURNS": "44", + } + with mock.patch.dict("os.environ", environment, clear=True): + pipeline.report(SimpleNamespace(state_dir=directory)) + summary = summary_path.read_text(encoding="utf-8") + + self.assertIn("**Mode:** `incremental` — test", summary) + self.assertIn("**Files in scope:** `1`", summary) + self.assertIn("**Model:** `claude-opus-4-7`", summary) + self.assertIn("**Turn budget:** `44`", summary) + self.assertIn( + "**New findings:** `2` (1 inline, 1 in the review body)", + summary, + ) + self.assertIn("**prior findings resolved:** `3`", summary) + self.assertIn("Pipeline trace", summary) + self.assertIn("MODE: incremental — valid prior manifest", summary) + self.assertIn("Model output (before compilation)", summary) + self.assertIn("Reviewed the delta.", summary) + + def test_action_uploads_the_review_state_artifact(self): + + action = Path(pipeline.__file__).with_name("action.yml").read_text( + encoding="utf-8" + ) + + self.assertIn("- name: Upload review state for inspection", action) + self.assertIn("actions/upload-artifact@", action) + self.assertIn("- name: Capture analysis session transcript", action) + self.assertIn("analysis-transcript.json", action) + self.assertIn("ANALYSIS_MAX_TURNS:", action) + + if __name__ == "__main__": unittest.main() +