fix: surface the triggering workflow's conclusion in workflow_run reviews - #2848
Conversation
…iews The workflow_run handler read event and pull_requests from the payload but never conclusion, so a post-CI review reads identically whether the triggering workflow succeeded, failed, or was cancelled. conclusion is already parsed and unused elsewhere in the codebase. _inject_ci_conclusion appends a labelled note to the same extra_instructions fields _inject_artifact_context already targets, so the reviewer is told plainly when CI did not pass instead of implying a clean run by omission. Also extends the restore_github_settings test fixture to snapshot and restore the three tools' extra_instructions, since run_action now mutates that state unconditionally on every workflow_run test, not just ones that opt into artifact injection.
PR Summary by QodoSurface workflow_run CI conclusions in automated reviews
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Target-selection branches lack tests
|
Addresses automated review feedback on the two style rules this repo enforces: double-quoted string literals, and imperative-phrased docstrings for new/modified code.
|
Addressed both findings: double-quoted the new string literals, and reworded the three modified/new docstrings to imperative mood. Tests still 19 passed. |
|
Code review by qodo was updated up to the latest commit da64da4 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Taking this one of the three, and crediting all three. It is the only one that leaves _inject_artifact_context alone, and that turns out to be the whole difference: both alternatives refactor it and both pick up problems doing so.
Notes on the other two, since the comparison is what decided it. #2843 by @lb1192176991-lab arrived first and follows the issue most literally, and its ARTIFACTS.TARGET_TOOLS handling is the one thing this PR is missing, which is the inline below. #2847 by @vaishaldsouza wrote the best single test of the three, a regression test for a malformed target_tools value. When I merge I will add you both as co-authors.
One inline before that. Thanks also for extending restore_github_settings unprompted: yours is the only one of the three that does not leak CI text into global settings between tests.
The CI-status injection was checking against a fixed tuple of three tool names, so a repo that narrows ARTIFACTS.TARGET_TOOLS to fewer tools still got the CI context injected into all three anyway. Reads the configured target tools now, falling back to the same three tools when nothing is set, with a guard so a string, a malformed value, or nothing at all can't raise. Based on the approach from The-PR-Agent#2843, applied here per review feedback.
|
Code review by qodo was updated up to the latest commit 2d05fb8 |
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again manually by commenting/agentic_review on this PR.
|
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for the patience here, and for taking the ARTIFACTS.TARGET_TOOLS suggestion verbatim.
Approving and merging, with one change I pushed to your branch first as be60e784. The closing sentence asserted "the change has not passed CI", which is untrue for action_required, and that is the most common non-success conclusion on this repo: 18 of the last 300 runs against 12 outright failures. It now reports the conclusion rather than asserting failure. That wording was mine from #2841, not yours.
The coverage holds. Reverting the runner while keeping your tests puts test_workflow_run_injects_ci_conclusion_when_not_success red, and the fixture extension is load-bearing rather than padding: without it the success text leaks into the next test.
2 participants
Fixes #2841.
What
workflow_runhandling inpr_agent/servers/github_action_runner.pyreadseventandpull_requestsfrom the payload but neverconclusion, so a review triggered after another workflow finishes reads identically whether that workflow succeeded, failed, or was cancelled — the model is never told which.conclusionis already parsed in the test fixture and otherwise unused in the codebase (aside from where PR-Agent itself creates a check run).Change
Added
_inject_ci_conclusion(conclusion), which appends a short note to theextra_instructionsof the same three tools (pr_reviewer,pr_description,pr_code_suggestions) that_inject_artifact_contextalready targets, telling the model explicitly when the triggering workflow did not conclude withsuccess. Called right after the existing artifact-context injection in theworkflow_runbranch. Purely additive — no control flow changes, no new config keys, no prompt template edits.Also extended the
restore_github_settingstest fixture to snapshot/restoreextra_instructionsfor those three tool sections.run_actionnow mutates that state unconditionally on everyworkflow_runrun (previously only_inject_artifact_contextdid, which no-ops unless artifacts are explicitly enabled in config) — without this the existingtest_workflow_run_runs_auto_toolstest would leak"...concluded: success"into later tests' settings.Tests
Two new tests in
tests/unittest/test_github_action_runner_core.py:test_workflow_run_injects_ci_conclusion_when_not_success— asserts the conclusion text lands inpr_reviewer.extra_instructionsfor a"failure"conclusion.test_workflow_run_does_not_inject_ci_conclusion_when_absent— asserts nothing is appended when the payload has noconclusionkey at all.Verified both against the pre-fix code (via
git stashon just the implementation file): the first fails as expected (assert 'concluded: failure' in ''), the second still passes (there's nothing to guard against there). Fulltests/unittest/test_github_action_runner_core.py: 19 passed.