From 77c83ac1e041123c479017c044da90774cd25fd7 Mon Sep 17 00:00:00 2001 From: William Aaron Cheung Date: Fri, 31 Jul 2026 13:16:11 +0800 Subject: [PATCH] feat(pr-review): let a PR comment trigger a reconcile round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix A of the open-question workflow. Before this, the reviewer only ran on `pull_request` events, so answering an open question in a PR comment did nothing until the next push (and even then #7 was needed for the answer to register). This lets a consumer opt a comment into driving a review round. - review_pipeline.py: - `prepare` accepts `--event-name`. On an `issue_comment` event a comment never adds code to review, so it can only run a reconcile-only incremental round — and only when the manifest still has an open question or finding (something a comment could answer, justify, or invalidate). Otherwise it skips, so routine chatter does not spend a review. - `manifest_has_open_items` is the gate. - When the head has not moved the delta is empty and the round exists purely to reconcile the new discussion; a comment that races a push reviews the new delta too. The round reuses the same sticky-comment manifest (full prior context) and runs on the cheaper incremental tier; if nothing changes the publisher posts nothing. - action.yml: pass `--event-name`, and fall back to `github.event.issue.number` for the PR number on comment events. - README: document the opt-in consumer snippet (issue_comment trigger, the human-only `if:` guard, and the issue.number concurrency fallback) and the cheap gating. - tests: cover the gate helper and the action wiring. Consumers opt in per repo by adding the `issue_comment` trigger to their `claude.yml`; without it, behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01GCc8MLBBAp5jAxc2x19swa --- .github/actions/README.md | 35 ++++++++++++ .github/actions/claude-pr-review/action.yml | 3 +- .../claude-pr-review/review_pipeline.py | 57 +++++++++++++++++-- .../claude-pr-review/test_review_pipeline.py | 32 +++++++++++ 4 files changed, 121 insertions(+), 6 deletions(-) diff --git a/.github/actions/README.md b/.github/actions/README.md index a0b22b2..e587579 100644 --- a/.github/actions/README.md +++ b/.github/actions/README.md @@ -151,6 +151,41 @@ 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. +### 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: + +```yaml +on: + pull_request: + types: [opened, synchronize, ready_for_review, reopened] + issue_comment: + types: [created] + +jobs: + pr-review: + # Only human comments on a PR; ignore issue comments and bot chatter. + if: >- + github.event_name != 'issue_comment' || + (github.event.issue.pull_request != null && + github.event.comment.user.type != 'Bot') + concurrency: + # issue_comment payloads carry issue.number, not pull_request.number. + group: claude-pr-review-${{ github.event.pull_request.number || github.event.issue.number }} + cancel-in-progress: false +``` + +The action gates the round cheaply so routine chatter does not spend a review: on an +`issue_comment` event, `prepare` skips unless the PR still has an **open question or open +finding** in the manifest (something a comment could answer, justify, or invalidate). When it +does run it is an incremental round that reuses the same sticky-comment manifest — so the +reviewer keeps its full prior context, unlike a fresh `@claude` session — and it runs on the +cheaper incremental model tier. If the comment turns out not to change anything, the publisher +posts nothing (no new review, no notification). A comment with no new commit reconciles the +discussion against the existing head; a comment that races a push reviews the new delta too. + ## Per-Repo Conventions The prompt-bearing actions instruct Claude to read and respect a consumer repo's own agent diff --git a/.github/actions/claude-pr-review/action.yml b/.github/actions/claude-pr-review/action.yml index 2944cbe..0bd03a5 100644 --- a/.github/actions/claude-pr-review/action.yml +++ b/.github/actions/claude-pr-review/action.yml @@ -77,9 +77,10 @@ runs: set -euo pipefail python3 "$GITHUB_ACTION_PATH/review_pipeline.py" prepare \ --repository "$GITHUB_REPOSITORY" \ - --pull-request "${{ github.event.pull_request.number }}" \ + --pull-request "${{ github.event.pull_request.number || github.event.issue.number }}" \ --event-head "${{ github.event.pull_request.head.sha }}" \ --event-base "${{ github.event.pull_request.base.sha }}" \ + --event-name "$GITHUB_EVENT_NAME" \ --state-dir "$STATE_DIR" \ --resolve-threads "${{ inputs.github_identity_token != '' }}" \ --review-depth "$REVIEW_DEPTH" \ diff --git a/.github/actions/claude-pr-review/review_pipeline.py b/.github/actions/claude-pr-review/review_pipeline.py index 42f46bd..6db7f52 100644 --- a/.github/actions/claude-pr-review/review_pipeline.py +++ b/.github/actions/claude-pr-review/review_pipeline.py @@ -1043,6 +1043,29 @@ def write_github_output(values: dict[str, Any]) -> None: output.write(f"{key}={value}\n") +def manifest_has_open_items(manifest: dict[str, Any]) -> bool: + """True when the manifest still has an open question or open finding. + + Gates comment-triggered rounds: a PR comment only warrants a new review + round when the reviewer is still waiting on something the comment might + answer, justify, or invalidate. Findings count too, not just questions — + a comment can push back on a finding, not only answer a question. + """ + findings = manifest.get("findings") + if isinstance(findings, dict) and any( + isinstance(finding, dict) and finding.get("status") == "open" + for finding in findings.values() + ): + return True + questions = manifest.get("questions") + if isinstance(questions, dict) and any( + isinstance(question, dict) and question.get("status") == "open" + for question in questions.values() + ): + return True + return False + + def prepare(args: argparse.Namespace) -> None: action_dir = Path(__file__).resolve().parent state_dir = Path(args.state_dir) @@ -1163,16 +1186,39 @@ def prepare(args: argparse.Namespace) -> None: comparison.get("files", []) if isinstance(comparison, dict) else [] ) comparison_complete = len(comparison_files) < 300 - if previous_head == current_head and version_matches: - mode = "skip" - mode_reason = "current head already reviewed" - elif ( + ancestor_cursor = bool( previous_head and version_matches and comparison and comparison.get("status") in {"ahead", "identical"} and comparison_complete - ): + ) + if args.event_name == "issue_comment": + # A comment never adds code to review — it can only answer an open + # question or justify/invalidate an open finding. Run a reconcile-only + # incremental round when the reviewer is still waiting on something and + # the prior manifest is usable; otherwise skip, so routine chatter does + # not spend a review. When the head has not moved the delta is empty and + # the round exists purely to reconcile the new discussion. + if ( + version_matches + and manifest_has_open_items(manifest) + and (previous_head == current_head or ancestor_cursor) + ): + mode = "incremental" + mode_reason = "comment received while items are open" + incremental_paths = [ + str(file["filename"]) + for file in comparison_files + if isinstance(file, dict) and file.get("filename") + ] + else: + mode = "skip" + mode_reason = "comment received with nothing open to reconcile" + elif previous_head == current_head and version_matches: + mode = "skip" + mode_reason = "current head already reviewed" + elif ancestor_cursor: mode = "incremental" mode_reason = "valid prior manifest and ancestor review cursor" incremental_paths = [ @@ -2970,6 +3016,7 @@ def parser() -> argparse.ArgumentParser: prepare_parser.add_argument("--pull-request", type=int, required=True) prepare_parser.add_argument("--event-head") prepare_parser.add_argument("--event-base") + prepare_parser.add_argument("--event-name", default="pull_request") prepare_parser.add_argument("--state-dir", required=True) prepare_parser.add_argument( "--resolve-threads", diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index 0fb870a..4aa51d9 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -133,6 +133,38 @@ def test_action_credits_human_answers_to_open_questions(self): action, ) + def test_manifest_has_open_items_gates_comment_rounds(self): + # The gate that decides whether a PR comment is worth a review round: + # true only while a question or finding is still open. + self.assertFalse(pipeline.manifest_has_open_items({})) + self.assertFalse( + pipeline.manifest_has_open_items( + { + "findings": {"F-1": {"status": "resolved"}}, + "questions": {"Q-1": {"status": "answered"}}, + } + ) + ) + self.assertTrue( + pipeline.manifest_has_open_items( + {"findings": {"F-1": {"status": "open"}}, "questions": {}} + ) + ) + self.assertTrue( + pipeline.manifest_has_open_items( + {"findings": {}, "questions": {"Q-1": {"status": "open"}}} + ) + ) + + def test_action_wires_comment_trigger_inputs(self): + action = Path(pipeline.__file__).with_name("action.yml").read_text( + encoding="utf-8" + ) + # On an issue_comment event the PR number falls back to the issue + # number, and the event name is passed so prepare can gate the round. + self.assertIn("github.event.issue.number", action) + self.assertIn("--event-name", action) + def test_action_exposes_optional_github_identity_token(self): action = Path(pipeline.__file__).with_name("action.yml").read_text( encoding="utf-8"