From 7545370a6b8b8d36c9726d0f9d48ccf750970949 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Tue, 15 Sep 2026 11:41:34 -0500 Subject: [PATCH 1/4] feat(code-review): record the reviewed checkout and commit in review output (ISS-9137) --- .../code-review/.claude-plugin/plugin.json | 2 +- plugins/code-review/SCHEMA.md | 5 +- plugins/code-review/commands/start.md | 8 +- plugins/code-review/prompts/github-review.md | 12 +- .../tools/python/code_review_helpers.py | 143 +++++++++++++++++- .../tools/python/code_review_schema.py | 4 + .../code-review/tools/python/config/cli.json | 14 ++ .../expected/review_result.json | 3 + .../expected/review_result.json | 3 + .../expected/review_result.json | 3 + .../expected/review_result.json | 3 + .../cli_parser_resolved.json | 14 ++ .../tools/python/test_code_review_helpers.py | 141 ++++++++++++++++- 13 files changed, 346 insertions(+), 9 deletions(-) diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 463f0cfd..a2c3e3be 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "description": "Code review plugin", - "version": "3.9.0", + "version": "3.10.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index 596f8a75..c335cbe5 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -143,6 +143,9 @@ The terminal artifact of every review run. "pr_number": , "head_sha": "", "diff_tip": "", + "review_root": "", // checkout the review read (ISS-9137) + "review_root_sha": "", // commit that checkout held at resolve-scope + "review_root_tree": "", // staged scope only: the pinned index tree "review_branch": "", "base_ref": "", "diff_scope": "", @@ -492,7 +495,7 @@ finding — best-effort omissions are budget-driven, not coverage gaps. | 27 | review-state-write | `review-state-write` | Review state | | 28 | verdict | `verdict` | `verdict.json` | | 29 | present | (present) | Local or GitHub output | -| 30 | footer | `footer` | Footer line | +| 30 | footer | `footer` | Footer line + `reviewed_line` (checkout and commit read) | Stages from plans 01/03/05/06 are present in `run_plan.json` but marked `enabled: false` until those plans land. diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index 9dc03221..31bbfe61 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -486,15 +486,19 @@ Follow Steps 6 and 8 in `github-review.md` (loaded in stage 0c for GitHub mode). ## Review Footer (stage_30_footer) -The footer prints elapsed time, cache stats, and token usage. Stage 30's helper is `footer`; the walker calls it with the args declared in the run plan. The plan includes `--cache-result /cache_result.json` unconditionally — when cache was inactive or fast-path bypassed it, the file simply does not exist and the helper falls back to `"Cache: disabled"` via its existing OSError handling. +The footer prints elapsed time, cache stats, and token usage, then names the checkout and commit the review read (ISS-9137). Stage 30's helper is `footer`; the walker calls it with the args declared in the run plan. The plan includes `--cache-result /cache_result.json` unconditionally — when cache was inactive or fast-path bypassed it, the file simply does not exist and the helper falls back to `"Cache: disabled"` via its existing OSError handling. -Read `/footer.json` for `footer_line` and print: +Read `/footer.json` for `footer_line` and `reviewed_line` and print both, verbatim: ```markdown --- **Review complete** — 8m 59s | Cache: 5/10 files (50%) | Full review | Tokens: ~281K effective (613 in, 5.6K out, 225K cache-write, 2.5M cache-read) + +**Reviewed:** `/Users/me/src/repo` @ `1a2b3c4d5e6f` ``` +Never hand-author or drop `reviewed_line`. It names `PR #N head` instead of a path when the PR head was isolated into a worktree (this stage removes that worktree), appends the pinned index tree for a staged review, and reads `checkout not recorded` when `scope.json` carried no valid `review_root_sha`. Print that line too: it means nothing vouches for which tree was read. + --- ## PR Verdict (stage_28_verdict — printed last) diff --git a/plugins/code-review/prompts/github-review.md b/plugins/code-review/prompts/github-review.md index 4c85f519..6aba4c9c 100644 --- a/plugins/code-review/prompts/github-review.md +++ b/plugins/code-review/prompts/github-review.md @@ -354,9 +354,19 @@ SUMMARY_EOF ## Code Review Summary **Status:** [Approved | Changes Requested | Needs Attention] + +**Reviewed commit:** [render-reviewed-commit output] +``` + +**Reviewed commit line (ISS-9137).** Do NOT hand-author it. Run the renderer and embed its output verbatim, on its own line, immediately after the **Status** line: + +```bash +python "${CLAUDE_PLUGIN_ROOT}/tools/python/code_review_helpers.py" render-reviewed-commit --cr-dir ``` -**Reviewer Fleet block (PLN-725 Phase 9 / v2.23.0).** Do NOT hand-author the Reviewers / Model Routing lines. Run the canonical renderer and embed its output verbatim immediately after the **Status** line: +It names the commit the review read, and also the PR head when the runner checked out a different commit (GitHub's merge ref). It never prints a filesystem path: the runner's path means nothing on a PR. If it reads `not recorded`, embed that line as-is. + +**Reviewer Fleet block (PLN-725 Phase 9 / v2.23.0).** Do NOT hand-author the Reviewers / Model Routing lines. Run the canonical renderer and embed its output verbatim immediately after the **Reviewed commit** line: ```bash python "${CLAUDE_PLUGIN_ROOT}/tools/python/code_review_helpers.py" render-fleet-summary --cr-dir diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 915ceadd..49e79b78 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -6151,8 +6151,123 @@ def _format_elapsed(seconds: int) -> str: return " ".join(parts) +# --------------------------------------------------------------------------- +# Review provenance (ISS-9137): which checkout and commit a review read +# --------------------------------------------------------------------------- + +_PROVENANCE_SHA_LEN = 12 + + +def _review_provenance( + cr_dir: str | Path, scope_meta: object, setup_meta: object, +) -> dict[str, Any]: + """Validated record of the checkout and commit a review read. + + ``scope.json`` and ``setup.json`` are operator-writable, and these values + are printed into the footer, the GitHub summary, and ``review_result.json``, + so each is re-validated: a root that is not an absolute path free of + control bytes, ``<``, ``>`` and backticks, or a SHA that is not hex, + becomes ``None``. ``isolated`` is true only when ``worktree_path`` is this + run's canonical PR-head worktree. ``pr_head_sha`` prefers ``setup.json``, + which carries the PR head in github mode, where ``scope.json``'s + ``head_sha`` is always empty. + """ + scope = scope_meta if isinstance(scope_meta, dict) else {} + setup = setup_meta if isinstance(setup_meta, dict) else {} + raw_root = scope.get("review_root") + root = ( + raw_root + if isinstance(raw_root, str) + and os.path.isabs(raw_root) + and not _REVIEW_ROOT_FORBIDDEN.search(raw_root) + else None + ) + raw_pr = str(scope.get("pr_number") or "") + return { + "review_root": root, + "review_root_sha": _validated_head_sha(scope.get("review_root_sha")) or None, + "review_root_tree": _validated_head_sha(scope.get("review_root_tree")) or None, + "pr_head_sha": ( + _validated_head_sha(setup.get("head_sha")) + or _validated_head_sha(scope.get("head_sha")) + or None + ), + "pr_number": raw_pr if re.fullmatch(r"[0-9]+", raw_pr) else None, + "isolated": bool(_validated_worktree_path(cr_dir, scope.get("worktree_path"))), + } + + +def _same_commit(a: str, b: str) -> bool: + """Whether two validated hex SHAs, either possibly abbreviated, name one commit.""" + a, b = a.lower(), b.lower() + return a.startswith(b) or b.startswith(a) + + +def _render_reviewed_line(provenance: dict[str, Any]) -> str: + """The footer line naming the checkout and commit a review read. + + A PR head isolated into ``/pr_head_worktree`` is named as the PR + head rather than by path, because the footer removes that worktree in the + same call. A staged review also names the index tree it pinned: the + content under review is the index, not the commit. + """ + sha = provenance["review_root_sha"] + if not sha: + return "**Reviewed:** checkout not recorded (scope.json has no valid review_root_sha)" + short = f"`{sha[:_PROVENANCE_SHA_LEN]}`" + if provenance["isolated"]: + pr_number = provenance["pr_number"] + subject = f"PR #{pr_number} head" if pr_number else "PR head" + return f"**Reviewed:** {subject} @ {short} (isolated worktree, removed after review)" + root = provenance["review_root"] + subject = f"`{root}`" if root else "checkout (path not recorded)" + line = f"**Reviewed:** {subject} @ {short}" + tree = provenance["review_root_tree"] + if tree: + line += f" + staged index (tree `{tree[:_PROVENANCE_SHA_LEN]}`)" + return line + + +def _render_reviewed_commit_line(provenance: dict[str, Any]) -> str: + """The GitHub summary line naming the reviewed commit, never a path. + + In github mode ``review_root`` is the runner's filesystem path, which means + nothing on a PR. When the runner checked out a commit other than the PR + head (GitHub's merge ref), both commits are named so a reader can tell + which one was read. + """ + sha = provenance["review_root_sha"] + if not sha: + return "**Reviewed commit:** not recorded (scope.json has no valid review_root_sha)" + line = f"**Reviewed commit:** `{sha[:_PROVENANCE_SHA_LEN]}`" + head = provenance["pr_head_sha"] + if head and _same_commit(sha, head): + return f"{line} (PR head)" + if head: + return f"{line} (PR head is `{head[:_PROVENANCE_SHA_LEN]}`)" + return line + + +def cmd_render_reviewed_commit(args: argparse.Namespace) -> int: + """Print the GitHub summary's ``**Reviewed commit:**`` line (ISS-9137). + + Reads ``/scope.json`` and ``/setup.json``. Always exits 0: + a run with no recorded commit prints a line that says so, which the summary + shows rather than drops. + """ + cr_dir = Path(args.cr_dir) + provenance = _review_provenance( + cr_dir, + _read_optional_json(cr_dir / "scope.json", {}), + _read_optional_json(cr_dir / "setup.json", {}), + ) + print(_render_reviewed_commit_line(provenance)) + return 0 + + def cmd_footer(args: argparse.Namespace) -> int: - """Compute review footer with timing, cache stats, and token usage.""" + """Compute the review footer (timing, cache stats, token usage) and the + ``reviewed_line`` naming the checkout and commit the review read.""" import time start_time: float = args.start_time @@ -6210,6 +6325,17 @@ def cmd_footer(args: argparse.Namespace) -> int: footer_line = f"**Review complete** — {elapsed_str} | {cache_str} | {mode_str} | {token_str}" + scope_meta: object = {} + setup_meta: object = {} + if cr_dir: + scope_meta = _read_optional_json(Path(cr_dir) / "scope.json", {}) + setup_meta = _read_optional_json(Path(cr_dir) / "setup.json", {}) + # ISS-9137: name the checkout and commit the review read. Resolved before + # the teardown below removes an isolated PR-head worktree. + reviewed_line = _render_reviewed_line( + _review_provenance(cr_dir or "", scope_meta, setup_meta), + ) + # Teardown: remove any PR-head worktree created during scope resolution # (local PR review isolation). The path is validated against the # canonical ``/pr_head_worktree`` before this destructive @@ -6219,14 +6345,17 @@ def cmd_footer(args: argparse.Namespace) -> int: # gitignored CR_DIR for the next run's startup GC, and never affects # footer output. if cr_dir: - scope_meta = _read_optional_json(Path(cr_dir) / "scope.json", {}) wt = _validated_worktree_path( cr_dir, scope_meta.get("worktree_path") if isinstance(scope_meta, dict) else None, ) if wt: _remove_pr_head_worktree(wt) - json.dump({"footer_line": footer_line}, sys.stdout, indent=2) + json.dump( + {"footer_line": footer_line, "reviewed_line": reviewed_line}, + sys.stdout, + indent=2, + ) sys.stdout.write("\n") return 0 @@ -14217,12 +14346,20 @@ def _normalize_bucket( or "unknown" ) + # ISS-9137: which checkout and commit this review read, re-validated from + # scope.json (null when absent or malformed). ``diff_tip`` cannot say this: + # for a branch review it is the literal ``HEAD``. + provenance = _review_provenance(cr_dir, scope_data, setup_data) + envelope: dict[str, Any] = { "schema_version": SCHEMA_VERSION, "review_id": str(uuid.uuid4()), "pr_number": args.pr_number if args.pr_number else scope_data.get("pr_number"), "head_sha": setup_data.get("head_sha") or scope_data.get("head_sha"), "diff_tip": str(diff_tip), + "review_root": provenance["review_root"], + "review_root_sha": provenance["review_root_sha"], + "review_root_tree": provenance["review_root_tree"], "review_branch": setup_data.get("current_branch") or scope_data.get("review_branch"), "base_ref": scope_data.get("base_ref"), "diff_scope": scope_data.get("diff_scope"), diff --git a/plugins/code-review/tools/python/code_review_schema.py b/plugins/code-review/tools/python/code_review_schema.py index 0772e593..408c19a1 100644 --- a/plugins/code-review/tools/python/code_review_schema.py +++ b/plugins/code-review/tools/python/code_review_schema.py @@ -1080,6 +1080,10 @@ def _validate_envelope_scalars(envelope: dict[str, Any]) -> list[str]: verdict = envelope.get("verdict") if verdict not in VERDICTS: out.append(f"verdict {verdict!r} not in {sorted(VERDICTS)}") + for key in ("review_root", "review_root_sha", "review_root_tree"): + value = envelope.get(key) + if value is not None and not isinstance(value, str): + out.append(f"{key} must be a string or null") return out diff --git a/plugins/code-review/tools/python/config/cli.json b/plugins/code-review/tools/python/config/cli.json index db7bf3e7..68f414d9 100644 --- a/plugins/code-review/tools/python/config/cli.json +++ b/plugins/code-review/tools/python/config/cli.json @@ -1516,6 +1516,20 @@ } ] }, + { + "name": "render-reviewed-commit", + "help": "Print the GitHub summary's Reviewed commit line from scope.json and setup.json (ISS-9137)", + "func": "cmd_render_reviewed_commit", + "args": [ + { + "flags": [ + "--cr-dir" + ], + "required": true, + "help": "Session CR_DIR path" + } + ] + }, { "name": "prepare-run", "help": "Emit run_plan.json describing the full review pipeline", diff --git a/plugins/code-review/tools/python/fixtures/golden_all_categories/expected/review_result.json b/plugins/code-review/tools/python/fixtures/golden_all_categories/expected/review_result.json index d5873aec..cb756e32 100644 --- a/plugins/code-review/tools/python/fixtures/golden_all_categories/expected/review_result.json +++ b/plugins/code-review/tools/python/fixtures/golden_all_categories/expected/review_result.json @@ -24,6 +24,9 @@ "rejected": [], "review_branch": "feature/x", "review_id": "00000000-0000-4000-8000-000000000000", + "review_root": null, + "review_root_sha": null, + "review_root_tree": null, "schema_version": 2, "stats": { "agent_failures": [], diff --git a/plugins/code-review/tools/python/fixtures/golden_injection_quarantine/expected/review_result.json b/plugins/code-review/tools/python/fixtures/golden_injection_quarantine/expected/review_result.json index 6761a5ef..ffd74dc6 100644 --- a/plugins/code-review/tools/python/fixtures/golden_injection_quarantine/expected/review_result.json +++ b/plugins/code-review/tools/python/fixtures/golden_injection_quarantine/expected/review_result.json @@ -24,6 +24,9 @@ "rejected": [], "review_branch": "feature/x", "review_id": "00000000-0000-4000-8000-000000000000", + "review_root": null, + "review_root_sha": null, + "review_root_tree": null, "schema_version": 2, "stats": { "agent_failures": [], diff --git a/plugins/code-review/tools/python/fixtures/golden_minimal_correctness/expected/review_result.json b/plugins/code-review/tools/python/fixtures/golden_minimal_correctness/expected/review_result.json index d5733f75..9f2344eb 100644 --- a/plugins/code-review/tools/python/fixtures/golden_minimal_correctness/expected/review_result.json +++ b/plugins/code-review/tools/python/fixtures/golden_minimal_correctness/expected/review_result.json @@ -24,6 +24,9 @@ "rejected": [], "review_branch": "feature/x", "review_id": "00000000-0000-4000-8000-000000000000", + "review_root": null, + "review_root_sha": null, + "review_root_tree": null, "schema_version": 2, "stats": { "agent_failures": [], diff --git a/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/expected/review_result.json b/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/expected/review_result.json index 2ab9d8ba..b40629f7 100644 --- a/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/expected/review_result.json +++ b/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/expected/review_result.json @@ -24,6 +24,9 @@ "rejected": [], "review_branch": "feature/x", "review_id": "00000000-0000-4000-8000-000000000000", + "review_root": null, + "review_root_sha": null, + "review_root_tree": null, "schema_version": 2, "stats": { "agent_failures": [], diff --git a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json index a3c5669f..ced07a82 100644 --- a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json +++ b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json @@ -1636,6 +1636,20 @@ } ] }, + { + "name": "render-reviewed-commit", + "func": "cmd_render_reviewed_commit", + "args": [ + { + "flags": [ + "--cr-dir" + ], + "dest": "cr_dir", + "default": null, + "required": true + } + ] + }, { "name": "prepare-run", "func": "cmd_prepare_run", diff --git a/plugins/code-review/tools/python/test_code_review_helpers.py b/plugins/code-review/tools/python/test_code_review_helpers.py index a2d5200c..338e5ce0 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -10563,6 +10563,145 @@ def test_footer_ignores_forged_worktree_path( assert removed == [] +class TestReviewProvenance: + """ISS-9137: the footer, GitHub summary, and envelope name what was read.""" + + SHA = "a" * 40 + HEAD = "b" * 40 + TREE = "c" * 40 + + def _footer( + self, tmp_path: Path, cr_dir: Path, scope: dict[str, Any], + monkeypatch: Any, capsys: Any, + ) -> str: + from code_review_helpers import cmd_footer + + cr_dir.mkdir(parents=True, exist_ok=True) + (cr_dir / "scope.json").write_text(json.dumps(scope)) + monkeypatch.setattr("code_review_helpers._remove_pr_head_worktree", lambda p: None) + ns = argparse.Namespace( + start_time=0.0, cache_result=None, review_mode_line="Full review", + cr_dir=str(cr_dir), project_dir=str(tmp_path), + ) + assert cmd_footer(ns) == 0 + return json.loads(capsys.readouterr().out)["reviewed_line"] + + def test_footer_names_checkout_and_commit( + self, tmp_path: Path, monkeypatch: Any, capsys: Any, + ) -> None: + line = self._footer( + tmp_path, tmp_path / "cr", + {"review_root": "/repo/wt", "review_root_sha": self.SHA, "worktree_path": ""}, + monkeypatch, capsys, + ) + assert line == "**Reviewed:** `/repo/wt` @ `aaaaaaaaaaaa`" + + def test_footer_names_isolated_pr_head_not_its_removed_path( + self, tmp_path: Path, monkeypatch: Any, capsys: Any, + ) -> None: + cr_dir = tmp_path / "cr" + worktree = str(cr_dir / "pr_head_worktree") + line = self._footer( + tmp_path, cr_dir, + { + "review_root": worktree, "review_root_sha": self.SHA, + "worktree_path": worktree, "pr_number": 42, + }, + monkeypatch, capsys, + ) + assert line == ( + "**Reviewed:** PR #42 head @ `aaaaaaaaaaaa` (isolated worktree, removed after review)" + ) + + def test_footer_names_staged_index_tree( + self, tmp_path: Path, monkeypatch: Any, capsys: Any, + ) -> None: + line = self._footer( + tmp_path, tmp_path / "cr", + {"review_root": "/repo", "review_root_sha": self.SHA, "review_root_tree": self.TREE}, + monkeypatch, capsys, + ) + assert line == "**Reviewed:** `/repo` @ `aaaaaaaaaaaa` + staged index (tree `cccccccccccc`)" + + def test_footer_never_prints_malformed_values( + self, tmp_path: Path, monkeypatch: Any, capsys: Any, + ) -> None: + cr_dir = tmp_path / "cr" + forged_root = self._footer( + tmp_path, cr_dir, + {"review_root": "/repo`\n**Approved**", "review_root_sha": self.SHA}, + monkeypatch, capsys, + ) + assert forged_root == "**Reviewed:** checkout (path not recorded) @ `aaaaaaaaaaaa`" + ref_not_sha = self._footer( + tmp_path, cr_dir, {"review_root": "/repo", "review_root_sha": "HEAD"}, + monkeypatch, capsys, + ) + assert ref_not_sha == ( + "**Reviewed:** checkout not recorded (scope.json has no valid review_root_sha)" + ) + + @pytest.mark.parametrize( + ("setup_head", "expected"), + [ + ("a" * 40, "**Reviewed commit:** `aaaaaaaaaaaa` (PR head)"), + ("b" * 40, "**Reviewed commit:** `aaaaaaaaaaaa` (PR head is `bbbbbbbbbbbb`)"), + ("", "**Reviewed commit:** `aaaaaaaaaaaa`"), + ], + ) + def test_github_summary_line_names_commits_never_the_runner_path( + self, tmp_path: Path, capsys: Any, setup_head: str, expected: str, + ) -> None: + from code_review_helpers import cmd_render_reviewed_commit + + (tmp_path / "scope.json").write_text(json.dumps({ + "review_root": "/home/runner/work/repo/repo", + "review_root_sha": self.SHA, + "head_sha": "", + })) + (tmp_path / "setup.json").write_text(json.dumps({"head_sha": setup_head})) + assert cmd_render_reviewed_commit(argparse.Namespace(cr_dir=str(tmp_path))) == 0 + assert capsys.readouterr().out.strip() == expected + + def _finalize(self, cr_dir: Path, scope: dict[str, Any], capsys: Any) -> dict[str, Any]: + from code_review_helpers import cmd_finalize_result + + validated = cr_dir / "findings_validated.json" + validated.write_text(json.dumps({"validated": [], "discarded": [], "stats": {}})) + (cr_dir / "scope.json").write_text(json.dumps(scope)) + ns = argparse.Namespace( + cr_dir=str(cr_dir), findings_validated=str(validated), + mode="local", diff_tip="HEAD", pr_number=None, + ) + assert cmd_finalize_result(ns) == 0 + capsys.readouterr() + return json.loads((cr_dir / "review_result.json").read_text()) + + def test_envelope_records_reviewed_checkout(self, tmp_path: Path, capsys: Any) -> None: + envelope = self._finalize( + tmp_path, + {"review_root": "/repo", "review_root_sha": self.SHA, "review_root_tree": self.TREE}, + capsys, + ) + assert ( + envelope["review_root"], envelope["review_root_sha"], envelope["review_root_tree"], + ) == ("/repo", self.SHA, self.TREE) + + def test_envelope_nulls_malformed_checkout_and_validator_rejects_non_strings( + self, tmp_path: Path, capsys: Any, + ) -> None: + from code_review_schema import validate_result_envelope + + envelope = self._finalize( + tmp_path, {"review_root": "relative/path", "review_root_sha": "--output=x"}, capsys, + ) + assert ( + envelope["review_root"], envelope["review_root_sha"], envelope["review_root_tree"], + ) == (None, None, None) + envelope["review_root"] = 5 + assert "review_root must be a string or null" in validate_result_envelope(envelope) + + class TestRemovePrHeadWorktree: """``_remove_pr_head_worktree`` issues the git teardown commands.""" @@ -21204,7 +21343,7 @@ def test_resolved_parser_spec_matches_snapshot(self) -> None: Catches: cli.json type/default/choices/action edits, $$ constant misroutes (the original false positive was --max-files = 20 vs BUDGET_TOTAL_CAP_DEFAULT = 20), missing required flags, mutex routing - drift, and func name drift across all 46 subparsers. + drift, and func name drift across all 47 subparsers. """ expected = json.loads( (self._snapshot_dir() / "cli_parser_resolved.json").read_text(), From 4d9cb97896cb152dc923872c3039443625d68e5a Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Tue, 15 Sep 2026 11:44:27 -0500 Subject: [PATCH 2/4] docs(code-review): changelog and README for ISS-9137 --- CHANGELOG.md | 17 +++++++++++++++++ plugins/code-review/README.md | 5 +++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 568e0560..136df6bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,23 @@ All notable changes to the claude-plugins project will be documented in this fil The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`. +### code-review v3.10.0 + +#### Added +- `review_result.json` records which checkout and commit a review read: + - `review_root`, `review_root_sha`, and, for a staged review, `review_root_tree`. + - Values are copied from `scope.json` and re-validated first. A root must be an absolute path free of control characters, `<`, `>` and backticks, and a SHA must be hex. Anything absent or malformed is written as `null`. + - The fields are additive, so `schema_version` stays at 2. The envelope validator rejects any value that is neither a string nor `null`. + - `diff_tip` could not carry this, because for a branch review it is the literal `HEAD`. +- The review footer prints a second line, `reviewed_line` from `footer.json`, in the form ``**Reviewed:** `` @ `<12-char sha>` ``: + - A PR head isolated into a worktree is named `PR #N head` rather than by path, because the footer stage removes that worktree in the same call. + - A staged review appends the pinned index tree. + - A run whose `scope.json` has no valid `review_root_sha` prints `checkout not recorded`. +- New `render-reviewed-commit` helper subcommand prints the GitHub summary's `**Reviewed commit:**` line, which `github-review.md` places right after **Status**: + - It names the commit the review read, and marks it `(PR head)` when it matches the PR head in `setup.json`. + - When the runner checked out a different commit, such as a merge ref, it names the PR head as well. + - It never prints a filesystem path. + ### code-review v3.9.0 #### Fixed diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index 851f8f04..f4196987 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -217,7 +217,7 @@ The orchestrator executes these steps in order: 11. **Cache update** (if caching is active) — writes validated findings to the cache for future incremental runs 12. **Present results** — local mode: prints findings by severity in the terminal; GitHub mode: writes `.closedloop-ai/code-review-findings.json`, `.closedloop-ai/code-review-threads.json`, and `.closedloop-ai/code-review-summary.md` for the CI workflow to post 13. **Review state write** — persists the current diff tip so future `--since-last-review` runs can narrow the scope -14. **Footer** — prints elapsed time, token usage stats, and writes the deterministic verdict JSON to `/verdict.json` (consumed by the `code` plugin's `run-loop.sh`) +14. **Footer** — prints elapsed time, token usage stats, and the checkout and commit the review read (`reviewed_line`), and writes the deterministic verdict JSON to `/verdict.json` (consumed by the `code` plugin's `run-loop.sh`) (Step numbers in this list are illustrative; the canonical 30-stage ordering lives in `prepare-run`'s `run_plan.json`. Steps 2–8 — the deterministic prefix through routing and partitioning — run in a single process via the `run-prefix` helper; the orchestrator walks the reviewer/validation/presentation tail from step 9 onward.) @@ -243,7 +243,8 @@ The helper script is a multi-subcommand Python CLI. The orchestrator invokes it | `post-comments` | Posts validated findings as inline GitHub PR comments (GitHub mode) | | `resolve-threads` | Resolves outdated bot review threads on a PR (GitHub mode) | | `session-tokens` | Collects token usage stats from the session | -| `footer` | Computes the formatted review footer string | +| `footer` | Computes the formatted review footer string and the `reviewed_line` naming the checkout and commit the review read (ISS-9137) | +| `render-reviewed-commit` | Prints the GitHub summary's `Reviewed commit` line from `scope.json` and `setup.json`: the commit the review read, plus the PR head when they differ; never a filesystem path (ISS-9137) | | `resolve-scope` | Resolves diff scope (branch, PR number, base ref, path filter) from CLI arguments and git context | | `fetch-intent` | Fetches context (PR description, recent commits) used to classify the diff intent | | `classify-intent` | Classifies the diff intent (feature, bugfix, refactor, etc.) for model routing | From 35a435a63659f85a9754f4515b06a52f2f960f80 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Tue, 15 Sep 2026 12:24:13 -0500 Subject: [PATCH 3/4] fix(code-review): record PR head for github provenance, declare envelope fields, use resolved helper path --- plugins/code-review/SCHEMA.md | 2 +- plugins/code-review/commands/start.md | 2 +- plugins/code-review/prompts/github-review.md | 4 +- .../code-review/skills/present-local/SKILL.md | 2 +- .../tools/python/code_review_helpers.py | 50 +++++++++---------- .../tools/python/code_review_schema.py | 3 ++ .../code-review/tools/python/config/cli.json | 2 +- .../tools/python/test_code_review_helpers.py | 36 +++++++++++-- .../tools/python/test_code_review_schema.py | 2 + 9 files changed, 66 insertions(+), 37 deletions(-) diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index c335cbe5..aaa839d5 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -136,7 +136,7 @@ The terminal artifact of every review run. ```jsonc { - "schema_version": 1, + "schema_version": 2, "review_id": "", // ── Run context ─────────────────────────────────────────── diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index 31bbfe61..cd4a59a1 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -375,7 +375,7 @@ These notes annotate the run-plan stages with anything not obvious from the plan - **stage_01_setup**: already executed in stage 0b (which captured stdout and wrote `setup.json` itself). The walker treats this as a no-op; the run plan's `stdout` field is `None` for this stage because no shell redirect is correct here. - **stage_02_prep_assets**: copies `shared_prompt.txt` and `bha_suffix.txt` from `/tools/prompts/` to ``. Both cache and non-cache paths use these assets. -- **stage_03_resolve_scope**: writes `/scope.json` with `diff_scope`, `base_ref`, `head_ref`, `review_branch`, `diff_tip`, `pr_number`, `path_filter`, `scope_kind`, `pr_auto_detected`, `head_sha`, `review_root`, `review_root_sha`, `review_root_tree` (staged scope only), `worktree_path`. **Review root:** `review_root` is the absolute root of the checkout the diff was computed from — the PR-head worktree when one was created, otherwise the invoking checkout's `git rev-parse --show-toplevel` — and it is **always** populated; resolve-scope returns non-zero rather than emit an empty one. Spawned reviewer/verifier agents inherit the invoking SESSION's working directory, so on any worktree-based run "read the working tree" means a different checkout than the diff, and a clean report from there is indistinguishable from a real pass. Every dispatch stage (`derive-spawn-spec`, `derive-static-spec`, `verify-prepare`, `review-dismissed-prepare`) re-proves the root against `scope.json` and `diff_data.json` — including that no file the diff changes differs in the working tree from `review_root_sha` (a commit, reset, or edit after resolution); for `staged` scope, whose diff is index-vs-HEAD, the baseline is `review_root_tree` — the index tree `resolve-scope` records with `git write-tree` — so an unstaged edit to a staged file or a re-stage after resolution is caught, and a staged scope without a valid `review_root_tree` is refused — and exits `3` when it cannot. The walker aborts on exit `3` regardless of the stage's `on_failure`, because those stages are `on_failure: continue` and every path a continue degrades to — the static reviewer table, "no verifier this run" — dispatches the same agents against the same wrong tree. A positional scope argument that is a git ref or revision range (e.g. `origin/main...HEAD`) is also rejected here rather than folded into a pathspec that matches nothing. After this stage, run `finalize-cache` to populate `/cache_config.json`. The walker uses these for token resolution downstream. **PR-head worktree (local PR review):** for `MODE=local` PR reviews (`scope_kind == "pr"`, not hygiene-only) resolve-scope isolates source reads so reviewer/verifier agents read the PR head, not the operator's working tree. The diff is computed from the fetched remote refs (`origin/...origin/`); reading the working tree is safe **only** when it already IS the PR head with a clean tree. Otherwise resolve-scope materializes a detached git worktree at the PR head SHA under `/pr_head_worktree` and records its absolute path in `review_root`/`worktree_path`; reviewer prompts and verifier inputs then read **source** under ``. **Fail-closed:** if isolation is required but cannot be established (PR head unresolvable, or `git worktree add` fails), resolve-scope returns non-zero and the run aborts (`on_failure: abort`) rather than silently review the wrong branch — the operator is told to check out the PR branch or fix the git error. `worktree_path` is empty (no isolation needed) for the already-at-head-and-clean case, staged/file/branch scope, hygiene-only, and GitHub mode; `review_root` is still populated in all of those, from the invoking checkout. **GitHub mode verifies instead of isolating (ISS-8769):** for `MODE=github` PR reviews (`scope_kind == "pr"`, not hygiene-only) resolve-scope first requires the PR head to come from the PR's own metadata — when `gh pr view` fails, the only head left is a guess (the checked-out branch), and a tree matching a guess proves nothing, so it refuses — then checks that the checked-out tree actually holds that head: either HEAD is that commit, or HEAD is the PR's merge ref (`refs/pull/N/merge`, the default `actions/checkout` for a `pull_request` event), a merge with exactly two parents whose second parent is that commit and whose first parent is on the PR's base branch (an ancestor of `origin/` from the PR metadata, not a `--base` override, which tolerates the base advancing after GitHub computed the merge) — with no uncommitted changes to tracked files. Anything else **refuses** with a non-zero exit before any scope is emitted, so a refused run prints nothing to stdout. `--github` is a supported invocation from a developer machine (it selects file-based handoff output; it does not declare "I am inside GitHub Actions"), and before this check such a run silently reviewed the operator's branch against the PR's diff. It refuses rather than isolating because a worktree would move the agent fleet to a different tree than `github-review.md`'s own steps read; the remedy is to check out the PR branch, or drop `--github` and use the local flow, which isolates for you. On an accepted run no worktree is created and `head_sha` stays empty (populating it would re-route `_file_content_hash` and the inline-comment `commit_id`); `review_root` is the invoking checkout's toplevel — the same tree the check just verified — and `review_root_sha` is that checkout's HEAD: the PR head for a head checkout, the merge commit for a merge-ref checkout, so the dispatch drift check compares against what is actually checked out. **Worktree lifecycle:** resolve-scope runs a startup GC (`_gc_stale_pr_head_worktrees`) that reclaims orphaned `cr-*/pr_head_worktree` checkouts from prior runs that aborted before teardown; `stage_30_footer` tears down the current run's worktree (validating the path equals the canonical `/pr_head_worktree` before the destructive removal). Because the walker can abort before the footer, the next run's startup GC is the backstop — a leaked worktree is never silently reused. Code intelligence stays on for branch, staged, and file-path review run from the primary checkout AND from a separate git worktree, for local PR review already at the head, and in GitHub mode; it is off (`CODE_INTEL_ALLOWED=false`) only for the per-run PR-head worktree (`worktree_path` set), which is created and torn down on every review, so any index for it would be cold. When `review_root` is not the git toplevel of the SESSION's primary working directory — the directory the Claude Code session was started in, not the directory a helper process happened to run in — or that cannot be determined, `CODE_INTEL_REQUIRE_ROOT_ARG=true` restricts reviewers to code-intelligence calls scoped to `review_root` through a root argument, because a tool called without one answers for the session's checkout. +- **stage_03_resolve_scope**: writes `/scope.json` with `diff_scope`, `base_ref`, `head_ref`, `review_branch`, `diff_tip`, `pr_number`, `path_filter`, `scope_kind`, `pr_auto_detected`, `head_sha`, `pr_head_sha` (PR scope only; github mode records it while leaving `head_sha` empty), `review_root`, `review_root_sha`, `review_root_tree` (staged scope only), `worktree_path`. **Review root:** `review_root` is the absolute root of the checkout the diff was computed from — the PR-head worktree when one was created, otherwise the invoking checkout's `git rev-parse --show-toplevel` — and it is **always** populated; resolve-scope returns non-zero rather than emit an empty one. Spawned reviewer/verifier agents inherit the invoking SESSION's working directory, so on any worktree-based run "read the working tree" means a different checkout than the diff, and a clean report from there is indistinguishable from a real pass. Every dispatch stage (`derive-spawn-spec`, `derive-static-spec`, `verify-prepare`, `review-dismissed-prepare`) re-proves the root against `scope.json` and `diff_data.json` — including that no file the diff changes differs in the working tree from `review_root_sha` (a commit, reset, or edit after resolution); for `staged` scope, whose diff is index-vs-HEAD, the baseline is `review_root_tree` — the index tree `resolve-scope` records with `git write-tree` — so an unstaged edit to a staged file or a re-stage after resolution is caught, and a staged scope without a valid `review_root_tree` is refused — and exits `3` when it cannot. The walker aborts on exit `3` regardless of the stage's `on_failure`, because those stages are `on_failure: continue` and every path a continue degrades to — the static reviewer table, "no verifier this run" — dispatches the same agents against the same wrong tree. A positional scope argument that is a git ref or revision range (e.g. `origin/main...HEAD`) is also rejected here rather than folded into a pathspec that matches nothing. After this stage, run `finalize-cache` to populate `/cache_config.json`. The walker uses these for token resolution downstream. **PR-head worktree (local PR review):** for `MODE=local` PR reviews (`scope_kind == "pr"`, not hygiene-only) resolve-scope isolates source reads so reviewer/verifier agents read the PR head, not the operator's working tree. The diff is computed from the fetched remote refs (`origin/...origin/`); reading the working tree is safe **only** when it already IS the PR head with a clean tree. Otherwise resolve-scope materializes a detached git worktree at the PR head SHA under `/pr_head_worktree` and records its absolute path in `review_root`/`worktree_path`; reviewer prompts and verifier inputs then read **source** under ``. **Fail-closed:** if isolation is required but cannot be established (PR head unresolvable, or `git worktree add` fails), resolve-scope returns non-zero and the run aborts (`on_failure: abort`) rather than silently review the wrong branch — the operator is told to check out the PR branch or fix the git error. `worktree_path` is empty (no isolation needed) for the already-at-head-and-clean case, staged/file/branch scope, hygiene-only, and GitHub mode; `review_root` is still populated in all of those, from the invoking checkout. **GitHub mode verifies instead of isolating (ISS-8769):** for `MODE=github` PR reviews (`scope_kind == "pr"`, not hygiene-only) resolve-scope first requires the PR head to come from the PR's own metadata — when `gh pr view` fails, the only head left is a guess (the checked-out branch), and a tree matching a guess proves nothing, so it refuses — then checks that the checked-out tree actually holds that head: either HEAD is that commit, or HEAD is the PR's merge ref (`refs/pull/N/merge`, the default `actions/checkout` for a `pull_request` event), a merge with exactly two parents whose second parent is that commit and whose first parent is on the PR's base branch (an ancestor of `origin/` from the PR metadata, not a `--base` override, which tolerates the base advancing after GitHub computed the merge) — with no uncommitted changes to tracked files. Anything else **refuses** with a non-zero exit before any scope is emitted, so a refused run prints nothing to stdout. `--github` is a supported invocation from a developer machine (it selects file-based handoff output; it does not declare "I am inside GitHub Actions"), and before this check such a run silently reviewed the operator's branch against the PR's diff. It refuses rather than isolating because a worktree would move the agent fleet to a different tree than `github-review.md`'s own steps read; the remedy is to check out the PR branch, or drop `--github` and use the local flow, which isolates for you. On an accepted run no worktree is created and `head_sha` stays empty (populating it would re-route `_file_content_hash` and the inline-comment `commit_id`); `review_root` is the invoking checkout's toplevel — the same tree the check just verified — and `review_root_sha` is that checkout's HEAD: the PR head for a head checkout, the merge commit for a merge-ref checkout, so the dispatch drift check compares against what is actually checked out. **Worktree lifecycle:** resolve-scope runs a startup GC (`_gc_stale_pr_head_worktrees`) that reclaims orphaned `cr-*/pr_head_worktree` checkouts from prior runs that aborted before teardown; `stage_30_footer` tears down the current run's worktree (validating the path equals the canonical `/pr_head_worktree` before the destructive removal). Because the walker can abort before the footer, the next run's startup GC is the backstop — a leaked worktree is never silently reused. Code intelligence stays on for branch, staged, and file-path review run from the primary checkout AND from a separate git worktree, for local PR review already at the head, and in GitHub mode; it is off (`CODE_INTEL_ALLOWED=false`) only for the per-run PR-head worktree (`worktree_path` set), which is created and torn down on every review, so any index for it would be cold. When `review_root` is not the git toplevel of the SESSION's primary working directory — the directory the Claude Code session was started in, not the directory a helper process happened to run in — or that cannot be determined, `CODE_INTEL_REQUIRE_ROOT_ARG=true` restricts reviewers to code-intelligence calls scoped to `review_root` through a root argument, because a tool called without one answers for the session's checkout. - **stage_07_auto_incremental**: runs **before** `stage_05_parse_diff` (its array position is between `stage_04_finalize_cache` and `stage_05_parse_diff`). This ordering matters: any `diff_scope` override must be applied to the cached `` token BEFORE parse-diff and extract-patches materialize `diff_data.json` and `patches_all.txt`, otherwise downstream stages see full-PR diff data alongside a narrowed token. The stage retains its `_07_` id as a stable label; execution order follows array position. Writes `/auto_incremental.json` with optional `diff_scope` (override) and `review_mode_line`. If `diff_scope` is non-null, update the cached `` token. Print `review_mode_line` (always) and, if `pr_auto_detected` was true in `scope.json`, print `"Auto-detected PR # for branch ."`. - **stage_08_fetch_intent**: the helper writes `intent_context.json` into `cr_dir` itself; its stdout is a small `{path, source}` summary that the walker discards. The run plan's `stdout` field is `None` here because redirecting stdout to `intent_context.json` would corrupt the file by overwriting the helper's structured payload with the summary. - **stage_09_detect_injection** (PLN-720): scores PR title/body/commits against the canonical 9-pattern catalogue and writes `/injection_report.json`. On severity ≥ Medium (score ≥ 30), rewrites `/intent_context.json` in place with `quarantine: true` and redacted fields. On severity ≥ High (score ≥ 70), also writes `/agent_injection-detector.json` containing a canonical `InjectionAttempt` finding — the `agent_*.json` naming makes `cmd_collect_findings` pick it up via the standard glob with no extra wiring. Always appends one JSONL entry to `.closedloop-ai/injection-log.jsonl` (90-day TTL, swept on read). `on_failure: continue` is intentional — a detector crash must never abort the pipeline. diff --git a/plugins/code-review/prompts/github-review.md b/plugins/code-review/prompts/github-review.md index 6aba4c9c..bafa7d78 100644 --- a/plugins/code-review/prompts/github-review.md +++ b/plugins/code-review/prompts/github-review.md @@ -361,7 +361,7 @@ SUMMARY_EOF **Reviewed commit line (ISS-9137).** Do NOT hand-author it. Run the renderer and embed its output verbatim, on its own line, immediately after the **Status** line: ```bash -python "${CLAUDE_PLUGIN_ROOT}/tools/python/code_review_helpers.py" render-reviewed-commit --cr-dir +python render-reviewed-commit --cr-dir ``` It names the commit the review read, and also the PR head when the runner checked out a different commit (GitHub's merge ref). It never prints a filesystem path: the runner's path means nothing on a PR. If it reads `not recorded`, embed that line as-is. @@ -369,7 +369,7 @@ It names the commit the review read, and also the PR head when the runner checke **Reviewer Fleet block (PLN-725 Phase 9 / v2.23.0).** Do NOT hand-author the Reviewers / Model Routing lines. Run the canonical renderer and embed its output verbatim immediately after the **Reviewed commit** line: ```bash -python "${CLAUDE_PLUGIN_ROOT}/tools/python/code_review_helpers.py" render-fleet-summary --cr-dir +python render-fleet-summary --cr-dir ``` The renderer reads `/spawn.json` (sections: `spec` — intended fleet from stage_19b; `verification` — runtime tally from stage_20b; `route` — model assignments from Gate B) and emits the **Reviewers**, **Model Routing**, **Fleet** (`N intended | N ran | N required missing`), and a conditional **Notes** block. The notes surface non-default outcomes (BLOCKING sanitization, runtime missing required, BHA budget cap, PLN-723 deferral, malformed-plan required skips) — operators reading the summary comment see these without having to dig into `coverage_gaps.json` or `spawn.json.verification`. diff --git a/plugins/code-review/skills/present-local/SKILL.md b/plugins/code-review/skills/present-local/SKILL.md index 010018a1..c5133d86 100644 --- a/plugins/code-review/skills/present-local/SKILL.md +++ b/plugins/code-review/skills/present-local/SKILL.md @@ -33,7 +33,7 @@ Output in this format: **Reviewer Fleet block (PLN-725 Phase 9 / v2.23.0).** Do NOT write the Reviewers / Model Routing / Fleet lines from scratch. Run the canonical renderer and embed its output verbatim: ```bash -python "${CLAUDE_PLUGIN_ROOT}/tools/python/code_review_helpers.py" render-fleet-summary --cr-dir +python render-fleet-summary --cr-dir ``` The renderer consumes `/spawn.json` (sections: `spec` — intended fleet from stage_19b; `verification` — runtime tally from stage_20b; `route` — model assignments from Gate B). The output is a deterministic markdown block of 2–9 lines — 2–4 for the core **Reviewers** / **Model Routing** / **Fleet** section, plus up to 5 conditional note bullets — covering: diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 49e79b78..ddd6eeb1 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -5515,6 +5515,7 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int: # Hygiene-only runs read no source (and Gate A exits before the footer # teardown), so they skip it. head_sha = "" + pr_head = "" worktree_path = "" if mode == "local" and scope_kind == "pr" and not hygiene_only: cr_dir = os.path.dirname(os.path.abspath(setup_json_path)) @@ -5653,6 +5654,13 @@ def cmd_resolve_scope(args: argparse.Namespace) -> int: } if review_root_tree is not None: result_out["review_root_tree"] = review_root_tree + # ISS-9137: the PR head this scope reviews, for provenance. Kept apart from + # ``head_sha``, which github mode must leave empty (it re-routes + # ``_file_content_hash`` and the inline-comment ``commit_id``) even though + # the verification block above resolved the head. + pr_head_sha = head_sha or pr_head + if pr_head_sha: + result_out["pr_head_sha"] = pr_head_sha json.dump(result_out, sys.stdout, indent=2) sys.stdout.write("\n") return 0 @@ -6158,22 +6166,18 @@ def _format_elapsed(seconds: int) -> str: _PROVENANCE_SHA_LEN = 12 -def _review_provenance( - cr_dir: str | Path, scope_meta: object, setup_meta: object, -) -> dict[str, Any]: - """Validated record of the checkout and commit a review read. +def _review_provenance(cr_dir: str | Path, scope_meta: object) -> dict[str, Any]: + """Validated record, from ``scope.json``, of the checkout and commit a review read. - ``scope.json`` and ``setup.json`` are operator-writable, and these values - are printed into the footer, the GitHub summary, and ``review_result.json``, - so each is re-validated: a root that is not an absolute path free of - control bytes, ``<``, ``>`` and backticks, or a SHA that is not hex, - becomes ``None``. ``isolated`` is true only when ``worktree_path`` is this - run's canonical PR-head worktree. ``pr_head_sha`` prefers ``setup.json``, - which carries the PR head in github mode, where ``scope.json``'s - ``head_sha`` is always empty. + ``scope.json`` is operator-writable, and these values are printed into the + footer, the GitHub summary, and ``review_result.json``, so each is + re-validated: a root that is not an absolute path free of control bytes, + ``<``, ``>`` and backticks, or a SHA that is not hex, becomes ``None``. + ``isolated`` is true only when ``worktree_path`` is this run's canonical + PR-head worktree. ``pr_head_sha`` reads ``resolve-scope``'s ``pr_head_sha``, + which github mode records while leaving ``head_sha`` empty. """ scope = scope_meta if isinstance(scope_meta, dict) else {} - setup = setup_meta if isinstance(setup_meta, dict) else {} raw_root = scope.get("review_root") root = ( raw_root @@ -6188,7 +6192,7 @@ def _review_provenance( "review_root_sha": _validated_head_sha(scope.get("review_root_sha")) or None, "review_root_tree": _validated_head_sha(scope.get("review_root_tree")) or None, "pr_head_sha": ( - _validated_head_sha(setup.get("head_sha")) + _validated_head_sha(scope.get("pr_head_sha")) or _validated_head_sha(scope.get("head_sha")) or None ), @@ -6251,15 +6255,13 @@ def _render_reviewed_commit_line(provenance: dict[str, Any]) -> str: def cmd_render_reviewed_commit(args: argparse.Namespace) -> int: """Print the GitHub summary's ``**Reviewed commit:**`` line (ISS-9137). - Reads ``/scope.json`` and ``/setup.json``. Always exits 0: - a run with no recorded commit prints a line that says so, which the summary - shows rather than drops. + Reads ``/scope.json``. Always exits 0: a run with no recorded + commit prints a line that says so, which the summary shows rather than + drops. """ cr_dir = Path(args.cr_dir) provenance = _review_provenance( - cr_dir, - _read_optional_json(cr_dir / "scope.json", {}), - _read_optional_json(cr_dir / "setup.json", {}), + cr_dir, _read_optional_json(cr_dir / "scope.json", {}), ) print(_render_reviewed_commit_line(provenance)) return 0 @@ -6326,15 +6328,11 @@ def cmd_footer(args: argparse.Namespace) -> int: footer_line = f"**Review complete** — {elapsed_str} | {cache_str} | {mode_str} | {token_str}" scope_meta: object = {} - setup_meta: object = {} if cr_dir: scope_meta = _read_optional_json(Path(cr_dir) / "scope.json", {}) - setup_meta = _read_optional_json(Path(cr_dir) / "setup.json", {}) # ISS-9137: name the checkout and commit the review read. Resolved before # the teardown below removes an isolated PR-head worktree. - reviewed_line = _render_reviewed_line( - _review_provenance(cr_dir or "", scope_meta, setup_meta), - ) + reviewed_line = _render_reviewed_line(_review_provenance(cr_dir or "", scope_meta)) # Teardown: remove any PR-head worktree created during scope resolution # (local PR review isolation). The path is validated against the @@ -14349,7 +14347,7 @@ def _normalize_bucket( # ISS-9137: which checkout and commit this review read, re-validated from # scope.json (null when absent or malformed). ``diff_tip`` cannot say this: # for a branch review it is the literal ``HEAD``. - provenance = _review_provenance(cr_dir, scope_data, setup_data) + provenance = _review_provenance(cr_dir, scope_data) envelope: dict[str, Any] = { "schema_version": SCHEMA_VERSION, diff --git a/plugins/code-review/tools/python/code_review_schema.py b/plugins/code-review/tools/python/code_review_schema.py index 408c19a1..0ef1a397 100644 --- a/plugins/code-review/tools/python/code_review_schema.py +++ b/plugins/code-review/tools/python/code_review_schema.py @@ -1290,6 +1290,9 @@ def result_envelope_json_schema() -> dict[str, Any]: "pr_number": {"type": ["integer", "null"]}, "head_sha": {"type": ["string", "null"]}, "diff_tip": {"type": "string"}, + "review_root": {"type": ["string", "null"]}, + "review_root_sha": {"type": ["string", "null"]}, + "review_root_tree": {"type": ["string", "null"]}, "review_branch": {"type": ["string", "null"]}, "base_ref": {"type": ["string", "null"]}, "diff_scope": {"type": ["string", "null"]}, diff --git a/plugins/code-review/tools/python/config/cli.json b/plugins/code-review/tools/python/config/cli.json index 68f414d9..f5560731 100644 --- a/plugins/code-review/tools/python/config/cli.json +++ b/plugins/code-review/tools/python/config/cli.json @@ -1518,7 +1518,7 @@ }, { "name": "render-reviewed-commit", - "help": "Print the GitHub summary's Reviewed commit line from scope.json and setup.json (ISS-9137)", + "help": "Print the GitHub summary's Reviewed commit line from scope.json (ISS-9137)", "func": "cmd_render_reviewed_commit", "args": [ { diff --git a/plugins/code-review/tools/python/test_code_review_helpers.py b/plugins/code-review/tools/python/test_code_review_helpers.py index 338e5ce0..61646df2 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -7568,6 +7568,29 @@ def test_github_mode_accepts_the_prs_merge_ref(self, tmp_path: Path) -> None: assert json.loads(out)["review_root"] == _MOCK_TOPLEVEL assert not [c for c in calls if c[:3] == ["git", "worktree", "add"]] + def test_github_merge_ref_records_the_pr_head_for_provenance( + self, tmp_path: Path, + ) -> None: + # ISS-9137: github mode leaves ``head_sha`` empty, so the PR head the + # verification block resolved is recorded as ``pr_head_sha``, and the + # summary line built from this resolved scope names both commits. + from code_review_helpers import _render_reviewed_commit_line, _review_provenance + + head, merge = "a" * 40, "e" * 40 + with patch("code_review_helpers._git_head_at", return_value=merge): + rc, out, _calls = self._invoke( + head_sha=head, work_head=merge, merge_parent=head, + mode="github", pr_number=42, tmp_path=tmp_path, + ) + assert rc == 0 + scope = json.loads(out) + assert (scope["head_sha"], scope["pr_head_sha"], scope["review_root_sha"]) == ( + "", head, merge, + ) + assert _render_reviewed_commit_line(_review_provenance(tmp_path, scope)) == ( + "**Reviewed commit:** `eeeeeeeeeeee` (PR head is `aaaaaaaaaaaa`)" + ) + def test_github_mode_refuses_a_merge_ref_for_a_different_head( self, tmp_path: Path, ) -> None: @@ -10642,7 +10665,7 @@ def test_footer_never_prints_malformed_values( ) @pytest.mark.parametrize( - ("setup_head", "expected"), + ("pr_head", "expected"), [ ("a" * 40, "**Reviewed commit:** `aaaaaaaaaaaa` (PR head)"), ("b" * 40, "**Reviewed commit:** `aaaaaaaaaaaa` (PR head is `bbbbbbbbbbbb`)"), @@ -10650,16 +10673,19 @@ def test_footer_never_prints_malformed_values( ], ) def test_github_summary_line_names_commits_never_the_runner_path( - self, tmp_path: Path, capsys: Any, setup_head: str, expected: str, + self, tmp_path: Path, capsys: Any, pr_head: str, expected: str, ) -> None: from code_review_helpers import cmd_render_reviewed_commit - (tmp_path / "scope.json").write_text(json.dumps({ + # resolve-scope's github-mode shape: head_sha empty, pr_head_sha set. + scope: dict[str, Any] = { "review_root": "/home/runner/work/repo/repo", "review_root_sha": self.SHA, "head_sha": "", - })) - (tmp_path / "setup.json").write_text(json.dumps({"head_sha": setup_head})) + } + if pr_head: + scope["pr_head_sha"] = pr_head + (tmp_path / "scope.json").write_text(json.dumps(scope)) assert cmd_render_reviewed_commit(argparse.Namespace(cr_dir=str(tmp_path))) == 0 assert capsys.readouterr().out.strip() == expected diff --git a/plugins/code-review/tools/python/test_code_review_schema.py b/plugins/code-review/tools/python/test_code_review_schema.py index e98fd379..56964c65 100644 --- a/plugins/code-review/tools/python/test_code_review_schema.py +++ b/plugins/code-review/tools/python/test_code_review_schema.py @@ -523,6 +523,8 @@ def test_result_envelope_json_schema_well_formed(): schema = result_envelope_json_schema() assert schema["title"] == "CodeReviewResultEnvelope" assert "verdict" in schema["required"] + for key in ("review_root", "review_root_sha", "review_root_tree"): + assert schema["properties"][key] == {"type": ["string", "null"]} assert json.loads(json.dumps(schema)) == schema From 634cd7e3230313d6bbc56c28ff26f6ddd8a25445 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Tue, 15 Sep 2026 12:26:30 -0500 Subject: [PATCH 4/4] docs(code-review): update changelog and README for PR head provenance --- CHANGELOG.md | 8 ++++++-- plugins/code-review/README.md | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 136df6bc..ab793713 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,16 +10,20 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `review_result.json` records which checkout and commit a review read: - `review_root`, `review_root_sha`, and, for a staged review, `review_root_tree`. - Values are copied from `scope.json` and re-validated first. A root must be an absolute path free of control characters, `<`, `>` and backticks, and a SHA must be hex. Anything absent or malformed is written as `null`. - - The fields are additive, so `schema_version` stays at 2. The envelope validator rejects any value that is neither a string nor `null`. + - The fields are additive, so `schema_version` stays at 2. The envelope validator rejects any value that is neither a string nor `null`, and `result_envelope_json_schema()` declares all three as nullable strings. - `diff_tip` could not carry this, because for a branch review it is the literal `HEAD`. - The review footer prints a second line, `reviewed_line` from `footer.json`, in the form ``**Reviewed:** `` @ `<12-char sha>` ``: - A PR head isolated into a worktree is named `PR #N head` rather than by path, because the footer stage removes that worktree in the same call. - A staged review appends the pinned index tree. - A run whose `scope.json` has no valid `review_root_sha` prints `checkout not recorded`. - New `render-reviewed-commit` helper subcommand prints the GitHub summary's `**Reviewed commit:**` line, which `github-review.md` places right after **Status**: - - It names the commit the review read, and marks it `(PR head)` when it matches the PR head in `setup.json`. + - It names the commit the review read, and marks it `(PR head)` when it matches the PR head. - When the runner checked out a different commit, such as a merge ref, it names the PR head as well. - It never prints a filesystem path. + - The PR head comes from a new `scope.json` key, `pr_head_sha`, which `resolve-scope` writes for PR scope. Github mode records it there while still leaving `head_sha` empty, because `head_sha` redirects `_file_content_hash` and the inline-comment `commit_id` away from the verified working tree. + +#### Fixed +- `github-review.md` and the `present-local` skill now run `render-reviewed-commit` and `render-fleet-summary` through the resolved `` path instead of `${CLAUDE_PLUGIN_ROOT}`. When `CLAUDE_PLUGIN_ROOT` is empty and `start.md` resolved the helpers from the in-repo tree or the marketplace cache, the old form pointed at `/tools/python/code_review_helpers.py`. ### code-review v3.9.0 diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index f4196987..23ae517e 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -244,7 +244,7 @@ The helper script is a multi-subcommand Python CLI. The orchestrator invokes it | `resolve-threads` | Resolves outdated bot review threads on a PR (GitHub mode) | | `session-tokens` | Collects token usage stats from the session | | `footer` | Computes the formatted review footer string and the `reviewed_line` naming the checkout and commit the review read (ISS-9137) | -| `render-reviewed-commit` | Prints the GitHub summary's `Reviewed commit` line from `scope.json` and `setup.json`: the commit the review read, plus the PR head when they differ; never a filesystem path (ISS-9137) | +| `render-reviewed-commit` | Prints the GitHub summary's `Reviewed commit` line from `scope.json`: the commit the review read, plus the PR head (`pr_head_sha`) when they differ; never a filesystem path (ISS-9137) | | `resolve-scope` | Resolves diff scope (branch, PR number, base ref, path filter) from CLI arguments and git context | | `fetch-intent` | Fetches context (PR description, recent commits) used to classify the diff intent | | `classify-intent` | Classifies the diff intent (feature, bugfix, refactor, etc.) for model routing |