Skip to content

Commit f226fee

Browse files
utsab345Utsab
andauthored
fix(github_action): handle synchronize event for push trigger (#2455) (#2490)
* fix: scope ticket cache per-PR and fix Dynaconf list merge leaks in tests - Cache key is now 'related_tickets_<md5(pr_url)>' instead of global 'related_tickets', preventing stale tickets leaking between unrelated PRs - Added cache_tickets=true config flag under [pr_reviewer] to allow disabling - Fixed Dynaconf merge_enabled=True list append bug in both production code (env_bridge.py uses case-insensitive pop before set) and test helpers (restore_settings now calls _remove_key before every set) - Added azure-identity dependency for Azure DevOps provider availability - 1043 tests pass, 0 failures * Revert "fix: scope ticket cache per-PR and fix Dynaconf list merge leaks in tests" This reverts commit 7f0d154. * fix(github_action): handle synchronize event for push trigger (#2455) * fix: address bot review comments for synchronize (#2455) - Check github_action_config.handle_push_trigger first (defaults true) - Remove hardcoded push_commands fallback list - Add before==after SHA guard - Add unit tests for synchronize handling * fix: address bot review comments on PR #2490 - Remove unused merge_commit_sha variable (F841) - Use is_true() to handle string values for push_trigger * fix: address review feedback for synchronize event handler - Guard before/after SHA check against None to avoid skipping when payload lacks these fields - Revert misleading test rename (test_issue_comment_from_user_is_processed) - Remove unused merge_commit_sha parameter from _write_synchronize_event - Add test coverage for pull_request_target with synchronize event * fix: address review feedback - default handle_push_trigger to false, add merge-commit/bot guards, fix push_commands fallback * fix: address Qodo comments - reorder synchronize before pr_actions, add string guard for push_commands, mirror settings in config, fix test leak * chore: remove accidentally added skpro submodule * docs: document synchronize/push trigger for GitHub Action * chore: remove accidentally added skpro submodule * chore: add skpro to gitignore * fix: address Qodo findings v2 - unblock github_app fallback, reorder synchronize to not bypass pr_actions, use set() for test setup, add return after push commands * docs: document synchronize/push trigger and guards for GitHub Action * docs: clarify synchronize is opt-in with default false --------- Co-authored-by: Utsab <utsab@pr-agent.dev>
1 parent 7d8ac32 commit f226fee

4 files changed

Lines changed: 215 additions & 3 deletions

File tree

docs/docs/usage-guide/automations_and_usage.md

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,9 +147,9 @@ pr_commands = [
147147
]
148148
```
149149

150-
#### GitHub app automatic tools for push actions (commits to an open PR)
150+
#### Automatic tools for push actions (commits to an open PR)
151151

152-
In addition to running automatic tools when a PR is opened, the GitHub app can also respond to new code that is pushed to an open PR.
152+
In addition to running automatic tools when a PR is opened, PR-Agent can also respond to new code that is pushed to an open PR. This works for both **GitHub App** and **GitHub Action** deployments.
153153

154154
The configuration toggle `handle_push_trigger` can be used to enable this feature.
155155
The configuration parameter `push_commands` defines the list of tools that will be **run automatically** when new code is pushed to the PR.
@@ -163,6 +163,8 @@ push_commands = [
163163
]
164164
```
165165

166+
For GitHub Action, settings fall back from `github_action_config.*` to `github_app.*`, so you can set either section.
167+
166168
This means that when new code is pushed to the PR, PR-Agent will run the `describe` and `review` tools, with the specified parameters.
167169

168170
### GitHub Action
@@ -186,6 +188,15 @@ If not set, the default configuration is for all three tools to run automaticall
186188

187189
`github_action_config.pr_actions` is used to configure which `pull_requests` events will trigger the enabled auto flags
188190
If not set, the default configuration is `["opened", "reopened", "ready_for_review", "review_requested"]`
191+
Adding `"synchronize"` to this list enables auto tools on new commits pushed to an open PR. You must also add `synchronize` to the workflow `pull_request: types:` list.
192+
193+
`github_action_config.handle_push_trigger` controls whether synchronize events run the push commands (default `false`). Settings fall back to `github_app.*` if not set under `github_action_config`. Since it defaults to `false`, synchronize is opt-in — you must explicitly enable it by either adding `"synchronize"` to `pr_actions` or setting `handle_push_trigger = true`.
194+
195+
`github_action_config.push_commands` defines which tools run on synchronize events when `handle_push_trigger` is enabled (fallback to `github_app.push_commands`).
196+
197+
`github_action_config.push_trigger_ignore_merge_commits` (default `true`) skips processing when the push contains a merge commit, avoiding duplicate reviews on "Update branch" clicks.
198+
199+
`github_action_config.push_trigger_ignore_bot_commits` (default `true`) skips processing when the push author is a bot, avoiding redundant runs on automated commits.
189200

190201
`github_action_config.enable_output` are used to enable/disable github actions [output parameter](https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#outputs-for-docker-container-and-javascript-actions) (default is `true`).
191202
Review result is output as JSON to `steps.{step-id}.outputs.review` property.

pr_agent/servers/github_action_runner.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,52 @@ async def run_action():
113113
# Retrieve the list of actions from the configuration
114114
pr_actions = get_settings().get("GITHUB_ACTION_CONFIG.PR_ACTIONS", ["opened", "reopened", "ready_for_review", "review_requested"])
115115

116+
# Handle synchronize first so it is not captured by pr_actions
117+
if action == "synchronize":
118+
push_trigger = get_settings().get(
119+
"github_action_config.handle_push_trigger",
120+
get_settings().get("github_app.handle_push_trigger", False),
121+
)
122+
if is_true(push_trigger):
123+
pr_url = event_payload.get("pull_request", {}).get("url")
124+
if not pr_url:
125+
return
126+
before_sha = event_payload.get("before")
127+
after_sha = event_payload.get("after")
128+
if before_sha is not None and before_sha == after_sha:
129+
return
130+
pull_request = event_payload.get("pull_request", {})
131+
merge_commit_sha = pull_request.get("merge_commit_sha")
132+
ignore_merge_commits = get_settings().get(
133+
"github_action_config.push_trigger_ignore_merge_commits",
134+
get_settings().get("github_app.push_trigger_ignore_merge_commits", True),
135+
)
136+
if is_true(ignore_merge_commits) and after_sha is not None and after_sha == merge_commit_sha:
137+
get_logger().info("Skipping synchronize: merge commit detected")
138+
return
139+
sender_type = event_payload.get("sender", {}).get("type")
140+
ignore_bot_commits = get_settings().get(
141+
"github_action_config.push_trigger_ignore_bot_commits",
142+
get_settings().get("github_app.push_trigger_ignore_bot_commits", True),
143+
)
144+
if is_true(ignore_bot_commits) and sender_type == "Bot":
145+
get_logger().info("Skipping synchronize: bot commit detected")
146+
return
147+
push_commands = get_settings().get(
148+
"github_action_config.push_commands",
149+
get_settings().get("github_app.push_commands", []),
150+
)
151+
if isinstance(push_commands, str):
152+
push_commands = [push_commands]
153+
if not push_commands:
154+
get_logger().info("No push_commands configured, skipping synchronize")
155+
return
156+
get_settings().config.is_auto_command = True
157+
get_settings().pr_description.final_update_message = False
158+
get_logger().info(f"Running push commands: {push_commands}")
159+
for command in push_commands:
160+
await PRAgent().handle_request(pr_url, command)
161+
return
116162
if action in pr_actions:
117163
pr_url = event_payload.get("pull_request", {}).get("url")
118164
if pr_url:

pr_agent/settings/configuration.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,10 @@ publish_as_check_run = false # when true, publish review/description/improve out
235235
# auto_describe = true # set as env var in .github/workflows/pr-agent.yaml
236236
# auto_improve = true # set as env var in .github/workflows/pr-agent.yaml
237237
# pr_actions = ['opened', 'reopened', 'ready_for_review', 'review_requested']
238+
# handle_push_trigger = false # when true, handle synchronize events using push_commands (falls back to github_app.handle_push_trigger)
239+
# push_trigger_ignore_bot_commits = true
240+
# push_trigger_ignore_merge_commits = true
241+
# push_commands = ['/describe', '/review'] # defaults mirror github_app.push_commands
238242

239243
[github_app]
240244
# these toggles allows running the github app from custom deployments

tests/unittest/test_github_action_runner_core.py

Lines changed: 152 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,13 +107,15 @@ class FakeSuggestions(FakeTool):
107107

108108
@pytest.fixture
109109
def restore_github_settings():
110-
"""run_action mutates global GITHUB/GITHUB_ACTION_CONFIG settings; snapshot
110+
"""run_action mutates global GITHUB/GITHUB_ACTION_CONFIG/GITHUB_APP settings; snapshot
111111
and restore them so these tests don't leak state into others."""
112112
settings = get_settings()
113113
had_github = "GITHUB" in settings
114114
original_github = copy.deepcopy(settings.get("GITHUB", None))
115115
had_cfg = "GITHUB_ACTION_CONFIG" in settings
116116
original_cfg = copy.deepcopy(settings.get("GITHUB_ACTION_CONFIG", None))
117+
had_app = "GITHUB_APP" in settings
118+
original_app = copy.deepcopy(settings.get("GITHUB_APP", None))
117119
yield
118120
if had_github:
119121
settings.set("GITHUB", original_github)
@@ -123,6 +125,28 @@ def restore_github_settings():
123125
settings.set("GITHUB_ACTION_CONFIG", original_cfg)
124126
else:
125127
settings.unset("GITHUB_ACTION_CONFIG", force=True)
128+
if had_app:
129+
settings.set("GITHUB_APP", original_app)
130+
else:
131+
settings.unset("GITHUB_APP", force=True)
132+
133+
134+
def _write_synchronize_event(tmp_path, before_sha="abc", after_sha="def", merge_commit_sha=None, sender_type="User"):
135+
payload = {
136+
"action": "synchronize",
137+
"before": before_sha,
138+
"after": after_sha,
139+
"sender": {"type": sender_type},
140+
"pull_request": {
141+
"url": "https://api.github.com/repos/org/repo/pulls/1",
142+
"html_url": "https://github.com/org/repo/pull/1",
143+
},
144+
}
145+
if merge_commit_sha is not None:
146+
payload["pull_request"]["merge_commit_sha"] = merge_commit_sha
147+
event_path = tmp_path / "event.json"
148+
event_path.write_text(json.dumps(payload))
149+
return event_path
126150

127151

128152
def _write_issue_comment_event(tmp_path, sender_type):
@@ -174,6 +198,133 @@ async def test_issue_comment_from_bot_sender_is_skipped(monkeypatch, tmp_path, r
174198
assert handled == [] # bot comment skipped; no command handled
175199

176200

201+
def _patch_synchronize_deps(monkeypatch, handled, push_commands, handle_push_trigger=True):
202+
monkeypatch.setattr(github_action_runner, "apply_repo_settings", lambda pr_url: None)
203+
settings = get_settings()
204+
monkeypatch.setitem(settings.store["github_app"], "push_commands", list(push_commands))
205+
settings.set("github_action_config", {
206+
"handle_push_trigger": handle_push_trigger,
207+
"push_trigger_ignore_merge_commits": False,
208+
"push_trigger_ignore_bot_commits": False,
209+
}, merge=False)
210+
211+
class FakeAgent:
212+
async def handle_request(self, url, body, notify=None):
213+
handled.append((url, body))
214+
215+
monkeypatch.setattr(github_action_runner, "PRAgent", FakeAgent)
216+
217+
218+
@pytest.mark.asyncio
219+
async def test_synchronize_event_triggers_push_commands(monkeypatch, tmp_path, restore_github_settings):
220+
handled = []
221+
_patch_synchronize_deps(monkeypatch, handled, ["/describe", "/improve"])
222+
monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request")
223+
monkeypatch.setenv("GITHUB_EVENT_PATH", str(_write_synchronize_event(tmp_path)))
224+
monkeypatch.setenv("GITHUB_TOKEN", "token")
225+
226+
await github_action_runner.run_action()
227+
228+
assert handled == [
229+
("https://api.github.com/repos/org/repo/pulls/1", "/describe"),
230+
("https://api.github.com/repos/org/repo/pulls/1", "/improve"),
231+
]
232+
233+
234+
@pytest.mark.asyncio
235+
async def test_synchronize_skips_when_push_trigger_disabled(monkeypatch, tmp_path, restore_github_settings):
236+
handled = []
237+
_patch_synchronize_deps(monkeypatch, handled, ["/describe"], handle_push_trigger=False)
238+
monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request")
239+
monkeypatch.setenv("GITHUB_EVENT_PATH", str(_write_synchronize_event(tmp_path)))
240+
monkeypatch.setenv("GITHUB_TOKEN", "token")
241+
242+
await github_action_runner.run_action()
243+
244+
assert handled == []
245+
246+
247+
@pytest.mark.asyncio
248+
async def test_synchronize_skips_equal_before_after_sha(monkeypatch, tmp_path, restore_github_settings):
249+
handled = []
250+
_patch_synchronize_deps(monkeypatch, handled, ["/describe"])
251+
monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request")
252+
event_path = _write_synchronize_event(tmp_path, before_sha="same", after_sha="same")
253+
monkeypatch.setenv("GITHUB_EVENT_PATH", str(event_path))
254+
monkeypatch.setenv("GITHUB_TOKEN", "token")
255+
256+
await github_action_runner.run_action()
257+
258+
assert handled == []
259+
260+
261+
@pytest.mark.asyncio
262+
async def test_synchronize_event_triggers_push_commands_on_pull_request_target(monkeypatch, tmp_path, restore_github_settings):
263+
handled = []
264+
_patch_synchronize_deps(monkeypatch, handled, ["/describe", "/improve"])
265+
monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request_target")
266+
monkeypatch.setenv("GITHUB_EVENT_PATH", str(_write_synchronize_event(tmp_path)))
267+
monkeypatch.setenv("GITHUB_TOKEN", "token")
268+
269+
await github_action_runner.run_action()
270+
271+
assert handled == [
272+
("https://api.github.com/repos/org/repo/pulls/1", "/describe"),
273+
("https://api.github.com/repos/org/repo/pulls/1", "/improve"),
274+
]
275+
276+
277+
@pytest.mark.asyncio
278+
async def test_synchronize_skips_merge_commit(monkeypatch, tmp_path, restore_github_settings):
279+
handled = []
280+
_patch_synchronize_deps(monkeypatch, handled, ["/describe"])
281+
settings = get_settings()
282+
monkeypatch.setitem(settings.store["github_action_config"], "push_trigger_ignore_merge_commits", True)
283+
monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request")
284+
monkeypatch.setenv("GITHUB_EVENT_PATH", str(_write_synchronize_event(
285+
tmp_path, before_sha="abc", after_sha="merge123", merge_commit_sha="merge123"
286+
)))
287+
monkeypatch.setenv("GITHUB_TOKEN", "token")
288+
289+
await github_action_runner.run_action()
290+
291+
assert handled == []
292+
293+
294+
@pytest.mark.asyncio
295+
async def test_synchronize_skips_bot_commit(monkeypatch, tmp_path, restore_github_settings):
296+
handled = []
297+
_patch_synchronize_deps(monkeypatch, handled, ["/describe"])
298+
settings = get_settings()
299+
monkeypatch.setitem(settings.store["github_action_config"], "push_trigger_ignore_bot_commits", True)
300+
monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request")
301+
monkeypatch.setenv("GITHUB_EVENT_PATH", str(_write_synchronize_event(
302+
tmp_path, sender_type="Bot"
303+
)))
304+
monkeypatch.setenv("GITHUB_TOKEN", "token")
305+
306+
await github_action_runner.run_action()
307+
308+
assert handled == []
309+
310+
311+
@pytest.mark.asyncio
312+
async def test_synchronize_uses_github_action_config_push_commands(monkeypatch, tmp_path, restore_github_settings):
313+
handled = []
314+
_patch_synchronize_deps(monkeypatch, handled, ["/review"], handle_push_trigger=True)
315+
settings = get_settings()
316+
monkeypatch.setitem(settings.store["github_action_config"], "push_commands", ["/describe"])
317+
monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request")
318+
monkeypatch.setenv("GITHUB_EVENT_PATH", str(_write_synchronize_event(tmp_path)))
319+
monkeypatch.setenv("GITHUB_TOKEN", "token")
320+
321+
await github_action_runner.run_action()
322+
323+
assert handled == [
324+
("https://api.github.com/repos/org/repo/pulls/1", "/describe"),
325+
]
326+
327+
177328
@pytest.mark.asyncio
178329
async def test_issue_comment_from_user_is_processed(monkeypatch, tmp_path, restore_github_settings):
179330
"""The bot guard must not over-skip: a human comment is still handled."""

0 commit comments

Comments
 (0)