Skip to content

fix(gh-action): pass workflow_run conclusion to auto tools - #2843

Closed
lb1192176991-lab wants to merge 1 commit into
The-PR-Agent:mainfrom
lb1192176991-lab:fix/workflow-run-ci-conclusion
Closed

fix(gh-action): pass workflow_run conclusion to auto tools#2843
lb1192176991-lab wants to merge 1 commit into
The-PR-Agent:mainfrom
lb1192176991-lab:fix/workflow-run-ci-conclusion

Conversation

@lb1192176991-lab

Copy link
Copy Markdown
Contributor

What

The workflow_run handler now reads workflow_run.conclusion from the payload and tells the auto tools (PR Description, PR Review, PR Code Suggestions) how the triggering workflow finished, by appending a labelled CI status block to their extra_instructions.

The append loop previously embedded in _inject_artifact_context() was extracted into a shared _append_tool_context(text) helper, and a new _inject_ci_conclusion(conclusion) uses it. The new call sits next to the existing artifact injection in the workflow_run branch.

Why

The whole point of running after CI is to review with the pipeline's findings in hand. The handler read workflow_run.event and workflow_run.pull_requests but never workflow_run.conclusion, so a post-CI review behaved identically whether the triggering workflow succeeded, failed, or was cancelled — and the model was never told which. If the conclusion is not success, the injected context instructs the tools to say so rather than implying the change is clean.

This is purely additive: no control flow changes, no prompt files touched, and it reuses the ARTIFACTS.TARGET_TOOLS settings key (no new configuration).

Testing

  • Ran PYTHONPATH=. pytest -q tests/unittest/test_github_action_runner_core.py — 19 passed.
  • Added two tests:
    • test_workflow_run_injects_ci_conclusion: a run with conclusion: "failure" results in the conclusion text appearing in get_settings().pr_reviewer.extra_instructions.
    • test_workflow_run_without_conclusion_injects_nothing: a payload without a conclusion key appends no CI context.
  • Updated _write_workflow_run_event to accept a conclusion parameter (None omits the key).

The workflow_run handler read workflow_run.event and
workflow_run.pull_requests but never workflow_run.conclusion, so the
post-CI review behaved identically whether the triggering workflow
succeeded, failed, or was cancelled.

Extract the extra_instructions append loop from
_inject_artifact_context into a shared _append_tool_context helper
and add _inject_ci_conclusion, which tells the model how the
triggering workflow run concluded via extra_instructions. Call it
from the workflow_run branch.

Fixes The-PR-Agent#2841
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Propagate workflow run conclusions to PR auto tools

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Propagates workflow completion status into configured PR auto-tool instructions.
• Warns generated output when the triggering CI workflow did not succeed.
• Adds regression coverage for failed and absent workflow conclusions.
Diagram

graph TD
  A["workflow_run event"] --> B["Action runner"] --> C{"Conclusion present?"}
  C -->|Yes| D["CI status context"] --> E["Target tool settings"] --> F["Auto tools"]
  C -->|No| F
Loading
High-Level Assessment

Reusing the existing target-tool context injection path is the best fit because CI artifacts and conclusions must reach the same configurable tools with identical deduplication and separator behavior. A separate prompt or configuration path would add unnecessary duplication for this additive metadata.

Files changed (2) +114 / -26

Bug fix (1) +37 / -19
github_action_runner.pyInject workflow conclusions into auto-tool context +37/-19

Inject workflow conclusions into auto-tool context

• Extracts shared extra-instruction appending from artifact injection and uses it to add labelled CI conclusion context. The workflow-run path now informs configured auto tools of success, failure, cancellation, or other provided conclusions while leaving missing conclusions unchanged.

pr_agent/servers/github_action_runner.py

Tests (1) +77 / -7
test_github_action_runner_core.pyCover workflow conclusion context injection +77/-7

Cover workflow conclusion context injection

• Extends the workflow-run event fixture with an optional conclusion. Adds regression tests verifying failure context reaches reviewer instructions and omitted conclusions add no CI status block.

tests/unittest/test_github_action_runner_core.py

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. CI context leaks across tests 🐞 Bug ☼ Reliability
Description
This test runs the real conclusion injector, which appends text to the process-wide pr_reviewer,
pr_description, and pr_code_suggestions settings, but restore_github_settings does not restore
those sections. Subsequent tests therefore inherit stale CI instructions (and the helper’s new
default conclusion also makes existing workflow-run tests leak success context), creating
order-dependent behavior.
Code

tests/unittest/test_github_action_runner_core.py[R460-462]

+    await github_action_runner.run_action()
+
+    assert "concluded: failure" in str(get_settings().pr_reviewer.extra_instructions)
Relevance

●●● Strong

Recent precedent explicitly accepts restoring every mutated global settings section to prevent
cross-test leakage.

PR-#2406

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
_append_tool_context directly assigns each matching tool section's extra_instructions, while the
fixture restores only GITHUB, GITHUB_ACTION_CONFIG, GITHUB_APP, config.is_auto_command, and
pr_description.final_update_message. The new test invokes that path without restoring the three
mutated instruction values; this matches the previously accepted global-Dynaconf test pollution
pattern.

pr_agent/servers/github_action_runner.py[66-83]
tests/unittest/test_github_action_runner_core.py[108-137]
tests/unittest/test_github_action_runner_core.py[438-462]
PR-#2406

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Workflow-run tests invoke `_inject_ci_conclusion`, which mutates global Dynaconf tool sections, but the shared fixture does not restore those mutations. Snapshot and restore each affected tool's `extra_instructions` around every workflow-run test.

## Issue Context
`get_settings()` is a process-wide singleton, and `_append_tool_context` updates all configured target tools. Ensure restoration occurs even when a test fails; consolidating this into `restore_github_settings` avoids bespoke cleanup and covers both existing and newly added workflow-run tests.

## Fix Focus Areas
- tests/unittest/test_github_action_runner_core.py[108-137]
- tests/unittest/test_github_action_runner_core.py[438-503]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Injection test docstring non-imperative 📘 Rule violation ⚙ Maintainability
Description
The new test docstring starts with the declarative phrase The workflow_run handler must pass
rather than an imperative verb. This violates the required imperative phrasing for newly added
function docstrings.
Code

tests/unittest/test_github_action_runner_core.py[440]

+    """The workflow_run handler must pass the triggering workflow's conclusion to the tools."""
Relevance

●● Moderate

Recent docstring precedents conflict: similar imperative rewrites were both accepted and rejected.

PR-#2807
PR-#2703
PR-#2774

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires imperative phrasing in newly added docstrings, while the cited PR line uses a
declarative subject followed by must pass.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_github_action_runner_core.py[440-440]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Rewrite the test docstring so its first sentence begins with an imperative verb.

## Issue Context
Compliance rule 2694688 requires newly added function docstrings to use imperative phrasing, such as `Verify` or `Ensure`.

## Fix Focus Areas
- tests/unittest/test_github_action_runner_core.py[440-440]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Missing-conclusion docstring non-imperative 📘 Rule violation ⚙ Maintainability
Description
The new test docstring starts with the declarative phrase A payload ... must not append rather
than an imperative verb. This violates the required imperative phrasing for newly added function
docstrings.
Code

tests/unittest/test_github_action_runner_core.py[467]

+    """A payload without a conclusion key must not append any CI context."""
Relevance

●● Moderate

Recent docstring-style precedents conflict, with comparable imperative rewrites both accepted and
rejected.

PR-#2807
PR-#2703
PR-#2774

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires imperative phrasing in newly added docstrings, while the cited PR line uses a
declarative subject followed by must not append.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_github_action_runner_core.py[467-467]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Rewrite the test docstring so its first sentence begins with an imperative verb.

## Issue Context
Compliance rule 2694688 requires newly added function docstrings to use imperative phrasing, such as `Verify` or `Ensure`.

## Fix Focus Areas
- tests/unittest/test_github_action_runner_core.py[467-467]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Grey Divider

Context sources
✅ Compliance rules (platform): 34 rules
Review mode: ⚖️ Balanced: This is a behavioral change in a GitHub Actions workflow handler that modifies shared tool prompt context, with several logic and test edits; it is not clearly trivial or dense enough to warrant extended review.

Grey Divider

Tip of the day
💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@IsmaelMartinez

Copy link
Copy Markdown
Collaborator

Thanks for this, and for getting there first. I am taking #2848, and adding you as a co-author on it, because its ARTIFACTS.TARGET_TOOLS handling is yours: I have suggested your approach there almost verbatim.

The reason it is not this one is measured rather than stylistic. Lifting the target-tool loop out of _inject_artifact_context moved it outside that function's except (OSError, ValueError, TypeError), and _inject_ci_conclusion calls it unguarded, so a scalar or null target_tools raises and no auto tool runs at all, where main runs all three:

scalar int 5  ->  TypeError: 'int' object is not iterable
None          ->  TypeError: 'NoneType' object is not iterable

Qodo's test-pollution finding here is also still live. Under default config all three PRs inject byte-identical text, so this came down to regression surface rather than quality.

afonsojanu added a commit to afonsojanu/pr-agent that referenced this pull request Aug 28, 2026
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.
@IsmaelMartinez

Copy link
Copy Markdown
Collaborator

Thanks for taking this on so quickly, and sorry it did not land as your own PR. #2841 went to #2848, and your work is credited as a co-author on ceae34b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants