Skip to content

Commit 702e951

Browse files
igerberclaude
andcommitted
Mirror non-opened pull_request branch into prompt-build IS_RERUN
R1 review on PR #418 caught the sibling-surface gap: the post-comment step's JS `isRerun` was updated to treat synchronize/reopened as reruns, but the prompt-build step's YAML `IS_RERUN` env was not. Effect: on synchronize/reopened, a new comment was posted (correct) but the prompt omitted the <previous-ai-review-output> block and re-review framing (incorrect), so the model could not focus on whether prior findings were addressed. Fix mirrors the JS predicate into the YAML predicate: IS_RERUN: ${{ github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment' || (github.event_name == 'pull_request' && github.event.action != 'opened') }} Also expands TestWorkflowCommentPosting to pin BOTH gates and the parity between them, so future edits to one cannot silently diverge from the other. New tests: - test_yaml_isrerun_includes_non_opened_pull_request - test_yaml_isrerun_still_includes_comment_events - test_both_gates_enumerate_same_triggers 175 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e1918c1 commit 702e951

2 files changed

Lines changed: 76 additions & 29 deletions

File tree

.github/workflows/ai_pr_review.yml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,12 @@ jobs:
113113
PR_BODY: ${{ steps.pr.outputs.body }}
114114
BASE_SHA: ${{ steps.pr.outputs.base_sha }}
115115
HEAD_SHA: ${{ steps.pr.outputs.head_sha }}
116-
IS_RERUN: ${{ github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment' }}
116+
# IS_RERUN must agree with the post-comment step's `isRerun` JS
117+
# expression below: any non-`opened` pull_request event is also a
118+
# rerun. Otherwise synchronize/reopened pushes get a new comment
119+
# but the prompt omits <previous-ai-review-output>, and the model
120+
# cannot focus on whether prior findings were addressed.
121+
IS_RERUN: ${{ github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment' || (github.event_name == 'pull_request' && github.event.action != 'opened') }}
117122
PREV_REVIEW: ${{ steps.prev_review.outputs.body }}
118123
PREV_REVIEW_FOUND: ${{ steps.prev_review.outputs.found }}
119124
run: |

tests/test_openai_review.py

Lines changed: 70 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1756,41 +1756,83 @@ def test_workflow_sanitizes_pr_body_closing_tag(self):
17561756

17571757

17581758
class TestWorkflowCommentPosting:
1759-
"""The post-comment step must distinguish initial `opened` (update canonical
1760-
comment) from every other trigger (create new comment), so re-reviews on
1761-
push / reopen / /ai-review preserve prior review history.
1762-
1763-
Without this, `pull_request: synchronize` from a push would overwrite the
1764-
original review comment via GitHub's update-comment API, losing the prior
1765-
content (the API does not expose comment edit history)."""
1766-
1767-
def test_isrerun_includes_non_opened_pull_request(self):
1768-
"""Non-opened pull_request events (synchronize, reopened) must be
1769-
treated as reruns so they create a new comment, not update."""
1759+
"""The workflow has TWO rerun-detection gates that must agree:
1760+
1. YAML `IS_RERUN` env in the prompt-build step — controls whether
1761+
the prompt includes the <previous-ai-review-output> block and
1762+
re-review framing.
1763+
2. JS `isRerun` in the post-comment step — controls whether the
1764+
comment is created fresh or updates the canonical comment.
1765+
1766+
If they disagree, you get nonsense states like "new comment posted but
1767+
prompt didn't see prior review" (synchronize bug pre-fix) or "canonical
1768+
comment updated but prompt was framed as rerun" (the inverse).
1769+
1770+
The contract: `pull_request.opened` is non-rerun; everything else
1771+
(`pull_request.synchronize`, `pull_request.reopened`, `issue_comment`,
1772+
`pull_request_review_comment`) is a rerun."""
1773+
1774+
@pytest.fixture
1775+
def workflow_text(self):
17701776
assert _SCRIPT_PATH is not None
17711777
repo_root = _SCRIPT_PATH.parent.parent.parent
17721778
wf = repo_root / ".github" / "workflows" / "ai_pr_review.yml"
17731779
if not wf.exists():
17741780
pytest.skip("workflow not found")
1775-
text = wf.read_text()
1776-
# The isRerun JS expression must gate on action !== "opened" for
1777-
# pull_request events.
1778-
assert 'context.payload.action !== "opened"' in text, (
1779-
"Workflow comment-posting must treat pull_request events other "
1780-
"than 'opened' as reruns; otherwise synchronize/reopened will "
1781-
"overwrite the canonical review comment and lose prior content."
1781+
return wf.read_text()
1782+
1783+
# --- Post-comment JS gate ---
1784+
1785+
def test_js_isrerun_includes_non_opened_pull_request(self, workflow_text):
1786+
"""JS gate: non-opened pull_request events create a new comment."""
1787+
assert 'context.payload.action !== "opened"' in workflow_text, (
1788+
"post-comment isRerun must treat pull_request events other than "
1789+
"'opened' as reruns; otherwise synchronize/reopened overwrite the "
1790+
"canonical review comment and lose prior content."
17821791
)
17831792

1784-
def test_isrerun_still_includes_comment_events(self):
1785-
"""Issue-comment and review-comment triggers must remain reruns."""
1786-
assert _SCRIPT_PATH is not None
1787-
repo_root = _SCRIPT_PATH.parent.parent.parent
1788-
wf = repo_root / ".github" / "workflows" / "ai_pr_review.yml"
1789-
if not wf.exists():
1790-
pytest.skip("workflow not found")
1791-
text = wf.read_text()
1792-
assert 'context.eventName === "issue_comment"' in text
1793-
assert 'context.eventName === "pull_request_review_comment"' in text
1793+
def test_js_isrerun_still_includes_comment_events(self, workflow_text):
1794+
assert 'context.eventName === "issue_comment"' in workflow_text
1795+
assert 'context.eventName === "pull_request_review_comment"' in workflow_text
1796+
1797+
# --- YAML IS_RERUN gate ---
1798+
1799+
def test_yaml_isrerun_includes_non_opened_pull_request(self, workflow_text):
1800+
"""YAML gate: non-opened pull_request events make the prompt include
1801+
the previous-review block. Must agree with the JS gate above."""
1802+
assert (
1803+
"github.event_name == 'pull_request' && github.event.action != 'opened'"
1804+
in workflow_text
1805+
), (
1806+
"prompt-build IS_RERUN must include non-opened pull_request events "
1807+
"alongside comment triggers; otherwise synchronize/reopened pushes "
1808+
"create a new comment but the prompt omits <previous-ai-review-output>."
1809+
)
1810+
1811+
def test_yaml_isrerun_still_includes_comment_events(self, workflow_text):
1812+
assert "github.event_name == 'issue_comment'" in workflow_text
1813+
assert "github.event_name == 'pull_request_review_comment'" in workflow_text
1814+
1815+
# --- Parity contract: both gates must enumerate the same trigger set ---
1816+
1817+
def test_both_gates_enumerate_same_triggers(self, workflow_text):
1818+
"""Whatever the gates use to express the rerun set, both must mention
1819+
each of the four rerun-trigger names so they cannot silently disagree.
1820+
1821+
This is a string-presence check (not a true semantic equality), but it
1822+
catches the realistic regression: someone editing one gate and
1823+
forgetting the other."""
1824+
rerun_signals = [
1825+
"issue_comment",
1826+
"pull_request_review_comment",
1827+
# The synchronize/reopened branch is expressed via action != opened
1828+
# in both gates, so we anchor on the action-comparison strings:
1829+
"github.event.action != 'opened'", # YAML
1830+
'context.payload.action !== "opened"', # JS
1831+
]
1832+
for signal in rerun_signals:
1833+
assert signal in workflow_text, (
1834+
f"Expected rerun-set signal {signal!r} not found in workflow YAML"
1835+
)
17941836

17951837

17961838
class TestExtractResponseText:

0 commit comments

Comments
 (0)