Skip to content

feat(code-review): record the reviewed checkout and commit in review output (ISS-9137) - #200

Merged
wongk merged 4 commits into
mainfrom
fix/iss-9137-review-provenance
Sep 15, 2026
Merged

wongk merged 4 commits into
mainfrom
fix/iss-9137-review-provenance

Conversation

@wongk

@wongk wongk commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

With #191 and #198 merged, the pipeline knows which checkout it reviewed, but nothing a reader sees says so. A review of the wrong tree still looks ordinary. diff_tip can't fill the gap, because for a branch review it's the literal HEAD. This PR records the checkout and resolved commit in three places:

  • review_result.json gains review_root, review_root_sha and review_root_tree (staged reviews only).
    • Values are re-validated from scope.json, and anything absent or malformed is written as null.
    • The fields are additive, so there is no SCHEMA_VERSION bump (SCHEMA.md §10). The validator rejects any value that is neither a string nor null.
  • Footer: footer.json gains reviewed_line, which start.md prints under the footer line.
    • Normal review: **Reviewed:** `<root>` @ `<sha12>`
    • PR head isolated into a worktree: PR #N head @ <sha12> (isolated worktree, removed after review). The same stage deletes that worktree, so it isn't named by path.
    • Staged review: appends + staged index (tree <tree12>).
    • No valid SHA recorded: checkout not recorded.
  • GitHub summary: a new render-reviewed-commit subcommand prints a **Reviewed commit:** line, which github-review.md embeds right after Status.
    • It names the reviewed commit, marked (PR head) when it matches, or followed by (PR head is <sha12>) when the runner checked out a merge ref.
    • It never prints the runner's path.

Every value is re-validated before it is printed. A root must be an absolute path free of control characters, <, > and backticks. A SHA must be hex.

The optional --review-root override from the issue was dropped, because campaign already runs /code-review:deep from inside the chunk worktree. ISS-9265 (cwd-relative refs and paths in subagent prompts) stays separate: this change makes that failure visible but does not fix it.

Test plan

  • pytest plugins/code-review: 1396 passed, 3 skipped
  • uv run ruff check . and uv run pyright: clean. git diff --check: clean
  • Mutation checks, each caught by the new tests:
    • dropping the root character guard
    • dropping review_root from the envelope
    • disabling isolated-worktree detection
    • breaking the PR-head comparison
  • End-to-end CLI probe against a real git repo, running resolve-scope, then finalize-result, footer and render-reviewed-commit:
    • branch scope: the path and commit match git rev-parse HEAD
    • staged scope: the tree matches git write-tree
    • missing scope.json: prints not recorded
  • Regenerated fixtures: 4 golden envelopes (3 new null keys each) and the CLI parser snapshot (1 new parser)

Closes ISS-9137.

@shafty023

Copy link
Copy Markdown
Collaborator

Beginning a code-review. Please allow time for me to finish before merging the PR

@mikeangstadt mikeangstadt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the thing that would have made this decorative: resolve-scope really does write review_root, review_root_sha and review_root_tree, so the new envelope fields carry real values instead of always-null. Ran the suite locally, 1396 pass, ruff and pyright clean. One stale doc line inline, not blocking.

Comment thread plugins/code-review/SCHEMA.md
Comment thread plugins/code-review/tools/python/code_review_helpers.py Outdated
Comment thread plugins/code-review/tools/python/code_review_schema.py
Comment thread plugins/code-review/prompts/github-review.md Outdated
Comment thread plugins/code-review/tools/python/code_review_helpers.py
@wongk
wongk merged commit 4d60e59 into main Sep 15, 2026
5 checks passed
@wongk
wongk deleted the fix/iss-9137-review-provenance branch September 15, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants