Fix workflow_run trigger ignoring conclusion (#2841)

[vaishaldsouza]

Fixes #2841

What

The workflow_run handler read event and pull_requests from the payload but ignored
conclusion, so post-CI reviews looked identical whether CI passed, failed, or was
cancelled.

Change

• Extracted the append loop in _inject_artifact_context() into a shared helper,
  _append_tool_context(text).
• Added _inject_ci_conclusion(conclusion), which uses it to tell the model how the
  triggering workflow concluded and to flag when the change hasn't passed CI.
• Called it right after _inject_artifact_context() in the workflow_run branch only —
  the other two call sites are untouched.

Purely additive: no new config, no prompt changes, no control-flow changes. Reuses
ARTIFACTS.TARGET_TOOLS.

Tests

• test_workflow_run_injects_ci_conclusion_failure: conclusion="failure" → CI status
  block appears in pr_reviewer.extra_instructions.
• test_workflow_run_no_conclusion_does_not_inject: no conclusion key → nothing appended.

pytest -q tests/unittest/test_github_action_runner_core.py → 19 passed. Full suite run
with 9 pre-existing, unrelated failures (Windows path handling / timing-sensitive tests).

[qodo-code-review]

PR Summary by Qodo

Propagate workflow run conclusions to PR review tools

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Adds workflow conclusions to model context for post-CI PR automation.
• Warns generated output when the triggering workflow did not pass.
• Covers failed and missing conclusions with workflow-run tests.

Diagram

sequenceDiagram
    participant GH as GitHub Actions
    participant Runner as Action Runner
    participant Context as Context Appender
    participant Settings as Tool Settings
    participant Tools as PR Tools
    GH->>Runner: workflow_run payload
    Runner->>Runner: Read conclusion
    Runner->>Context: Append CI status
    Context->>Settings: Update instructions
    Settings-->>Tools: Provide CI context

Loading

High-Level Assessment

The shared context-appending helper is the appropriate approach because artifact and CI status injection require identical target selection, separator, and deduplication behavior. Duplicating per-tool mutation in the workflow-run branch would increase drift without adding flexibility.

Files changed (2) +142 / -26

Bug fix (1) +37 / -19

github_action_runner.pyInject workflow conclusions into configured PR tool context +37/-19

Inject workflow conclusions into configured PR tool context

• Extracts shared extra-instruction appending from artifact handling and reuses it for CI conclusions. The workflow_run path now tells configured tools how the triggering workflow completed and explicitly marks non-success outcomes as not passing CI.

pr_agent/servers/github_action_runner.py

Tests (1) +105 / -7

test_github_action_runner_core.pyCover failed and absent workflow conclusions +105/-7

Cover failed and absent workflow conclusions

• Extends workflow-run event fixtures with configurable conclusions. Adds regression tests verifying failure context is injected while a missing conclusion leaves tool instructions unchanged.

tests/unittest/test_github_action_runner_core.py

[qodo-code-review]

Code Review by Qodo

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

1. Hard-coded target tool fallback 📘 Rule violation ⚙ Maintainability

Description

The malformed-value fallback embeds the repository-specific ARTIFACTS.TARGET_TOOLS list directly
in the helper instead of obtaining the default from the settings layer. This duplicates configurable
deployment behavior and can cause the fallback to diverge from configured defaults.

Code

pr_agent/servers/github_action_runner.py[R44-45]

+    elif not isinstance(target_tools, (list, set, tuple)):
+        target_tools = ["pr_reviewer", "pr_description", "pr_code_suggestions"]

Relevance

●●● Strong

Matches accepted precedent in same repo (PR #2797, #2528) removing hard-coded config defaults
duplicating settings.

PR-#2797
PR-#2528

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed fallback assigns a literal list of configured artifact target tools when the setting has
an invalid type, while the checklist requires deployment- or user-specific configuration to be
loaded through the settings layer.

Rule 2694652: Do not hard-code configuration; load it from .pr_agent.toml or pr_agent/settings
pr_agent/servers/github_action_runner.py[44-45]

Agent prompt

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

## Issue description
The malformed `ARTIFACTS.TARGET_TOOLS` fallback hard-codes a repository-specific tool list in business logic.

## Issue Context
The compliance checklist requires configuration values to come from `.pr_agent.toml` or `pr_agent/settings` when a settings mechanism exists. The fallback should remain resilient without duplicating the canonical default.

## Fix Focus Areas
- pr_agent/servers/github_action_runner.py[44-45]

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

---

2. CI injection can crash on malformed targets ✓ Resolved 🐞 Bug ☼ Reliability

Description

_inject_ci_conclusion() calls _append_tool_context() outside _inject_artifact_context()'s
exception handler, but the helper raises TypeError when ARTIFACTS.TARGET_TOOLS is None or
another non-iterable non-string value. A workflow_run with a conclusion then terminates before the
configured auto actions run, whereas the prior artifact path caught and logged this configuration
error.

Code

pr_agent/servers/github_action_runner.py[336]

+        _inject_ci_conclusion(workflow_run.get("conclusion"))

Relevance

●●● Strong

Recent accepted precedents favor defensive handling of malformed configuration in webhook and action
reliability paths.

PR-#2528
PR-#2736

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The extracted helper performs a set comprehension over target_tools after handling only string
values, so a None or scalar configuration raises TypeError. The existing catch surrounds only
_inject_artifact_context(); the newly added CI call is made afterward and outside that boundary,
and it still executes even when no artifact was loaded.

pr_agent/servers/github_action_runner.py[42-44]
pr_agent/servers/github_action_runner.py[88-97]
pr_agent/servers/github_action_runner.py[334-336]
pr_agent/algo/artifacts.py[72-89]

Agent prompt

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

## Issue description
The new `_inject_ci_conclusion(workflow_run.get("conclusion"))` call can invoke `_append_tool_context()` after `_inject_artifact_context()` has exited its `try` block. `_append_tool_context()` raises `TypeError` for a non-iterable, non-string `ARTIFACTS.TARGET_TOOLS` value, causing workflow_run processing to abort.

## Issue Context
The old artifact-target iteration was protected by `_inject_artifact_context()`'s `(OSError, ValueError, TypeError)` handler. CI conclusion injection should preserve that fault tolerance and must not prevent auto-review/description actions because of malformed artifact target configuration.

## Fix Focus Areas
- pr_agent/servers/github_action_runner.py[36-54]
- pr_agent/servers/github_action_runner.py[94-97]
- pr_agent/servers/github_action_runner.py[334-336]

Make target normalization robust for invalid values and/or protect the new CI injection call with equivalent exception handling, while preserving normal configured target behavior. Add a regression test using a non-iterable `ARTIFACTS.TARGET_TOOLS` value and a workflow_run conclusion, asserting the action continues rather than raising.

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

---

3. Trailing whitespace in test ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The added test contains whitespace-only trailing spaces on line 444, violating the repository
requirement that source files contain no trailing whitespace. This can cause lint failures and
unnecessary formatting churn.

Code

tests/unittest/test_github_action_runner_core.py[444]

+    

Relevance

●●● Strong

The team recently accepted removing trailing whitespace from newly added tests as repository
hygiene.

PR-#2545

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The diff visibly adds whitespace after an otherwise blank line in the new test, and the checklist
explicitly prohibits trailing whitespace.

Rule 2694658: Disallow trailing whitespace in source files
tests/unittest/test_github_action_runner_core.py[444-444]
tests/unittest/test_github_action_runner_core.py[493-493]

Agent prompt

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

## Issue description
Remove trailing spaces from the newly added test code.

## Issue Context
The repository compliance rule disallows trailing whitespace in source files, and the added blank line at line 444 contains spaces.

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

ⓘ 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.

Context sources

✅ Compliance rules (platform): 34 rules

Review mode: 🚀 Fast: This is a small, localized runtime change that only centralizes a default list in one helper and avoids high-risk areas; a light single-pass review is sufficient.

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 ↗

Previous reviews

Review updated until commit 4f39bd4 🚀 Fast

Results up to commit 7485a8c 🚀 Fast

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 📜 Skill insights (0)

1. CI injection can crash on malformed targets ✓ Resolved 🐞 Bug ☼ Reliability

Description

_inject_ci_conclusion() calls _append_tool_context() outside _inject_artifact_context()'s
exception handler, but the helper raises TypeError when ARTIFACTS.TARGET_TOOLS is None or
another non-iterable non-string value. A workflow_run with a conclusion then terminates before the
configured auto actions run, whereas the prior artifact path caught and logged this configuration
error.

Code

pr_agent/servers/github_action_runner.py[336]

+        _inject_ci_conclusion(workflow_run.get("conclusion"))

Relevance

●●● Strong

Recent accepted precedents favor defensive handling of malformed configuration in webhook and action
reliability paths.

PR-#2528
PR-#2736

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The extracted helper performs a set comprehension over target_tools after handling only string
values, so a None or scalar configuration raises TypeError. The existing catch surrounds only
_inject_artifact_context(); the newly added CI call is made afterward and outside that boundary,
and it still executes even when no artifact was loaded.

pr_agent/servers/github_action_runner.py[42-44]
pr_agent/servers/github_action_runner.py[88-97]
pr_agent/servers/github_action_runner.py[334-336]
pr_agent/algo/artifacts.py[72-89]

Agent prompt

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

## Issue description
The new `_inject_ci_conclusion(workflow_run.get("conclusion"))` call can invoke `_append_tool_context()` after `_inject_artifact_context()` has exited its `try` block. `_append_tool_context()` raises `TypeError` for a non-iterable, non-string `ARTIFACTS.TARGET_TOOLS` value, causing workflow_run processing to abort.

## Issue Context
The old artifact-target iteration was protected by `_inject_artifact_context()`'s `(OSError, ValueError, TypeError)` handler. CI conclusion injection should preserve that fault tolerance and must not prevent auto-review/description actions because of malformed artifact target configuration.

## Fix Focus Areas
- pr_agent/servers/github_action_runner.py[36-54]
- pr_agent/servers/github_action_runner.py[94-97]
- pr_agent/servers/github_action_runner.py[334-336]

Make target normalization robust for invalid values and/or protect the new CI injection call with equivalent exception handling, while preserving normal configured target behavior. Add a regression test using a non-iterable `ARTIFACTS.TARGET_TOOLS` value and a workflow_run conclusion, asserting the action continues rather than raising.

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

---

2. Trailing whitespace in test ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The added test contains whitespace-only trailing spaces on line 444, violating the repository
requirement that source files contain no trailing whitespace. This can cause lint failures and
unnecessary formatting churn.

Code

tests/unittest/test_github_action_runner_core.py[444]

+    

Relevance

●●● Strong

The team recently accepted removing trailing whitespace from newly added tests as repository
hygiene.

PR-#2545

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The diff visibly adds whitespace after an otherwise blank line in the new test, and the checklist
explicitly prohibits trailing whitespace.

Rule 2694658: Disallow trailing whitespace in source files
tests/unittest/test_github_action_runner_core.py[444-444]
tests/unittest/test_github_action_runner_core.py[493-493]

Agent prompt

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

## Issue description
Remove trailing spaces from the newly added test code.

## Issue Context
The repository compliance rule disallows trailing whitespace in source files, and the added blank line at line 444 contains spaces.

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

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

---

Results up to commit 9c17bc8 🚀 Fast

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 📜 Skill insights (0)

1. Hard-coded target tool fallback 📘 Rule violation ⚙ Maintainability

Description

The malformed-value fallback embeds the repository-specific ARTIFACTS.TARGET_TOOLS list directly
in the helper instead of obtaining the default from the settings layer. This duplicates configurable
deployment behavior and can cause the fallback to diverge from configured defaults.

Code

pr_agent/servers/github_action_runner.py[R44-45]

+    elif not isinstance(target_tools, (list, set, tuple)):
+        target_tools = ["pr_reviewer", "pr_description", "pr_code_suggestions"]

Relevance

●●● Strong

Matches accepted precedent in same repo (PR #2797, #2528) removing hard-coded config defaults
duplicating settings.

PR-#2797
PR-#2528

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed fallback assigns a literal list of configured artifact target tools when the setting has
an invalid type, while the checklist requires deployment- or user-specific configuration to be
loaded through the settings layer.

Rule 2694652: Do not hard-code configuration; load it from .pr_agent.toml or pr_agent/settings
pr_agent/servers/github_action_runner.py[44-45]

Agent prompt

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

## Issue description
The malformed `ARTIFACTS.TARGET_TOOLS` fallback hard-codes a repository-specific tool list in business logic.

## Issue Context
The compliance checklist requires configuration values to come from `.pr_agent.toml` or `pr_agent/settings` when a settings mechanism exists. The fallback should remain resilient without duplicating the canonical default.

## Fix Focus Areas
- pr_agent/servers/github_action_runner.py[44-45]

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

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 9c17bc8

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4f39bd4

[IsmaelMartinez]

Thanks, and your malformed-config regression test is the best single test any of the three wrote. I am taking #2848 and adding you as a co-author on it.

The hardening is correct, but it exists to cover a crash the refactor itself introduced, so it is 52 changed source lines solving a problem the issue did not have. #2848 leaves _inject_artifact_context untouched and cannot reach it. The guard idea survives, though: I suggested a version of it on #2848, so it lands either way.

One thing I measured rather than the bot: the same test-state leak Qodo raised on #2843 is present here and unflagged, since restore_github_settings does not restore the extra_instructions the run mutates.

[IsmaelMartinez]

Thanks for picking this up, 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.