ci(#388): fail roll-up when tests are skipped on pull_request_target#400
Open
rh-hemartin wants to merge 1 commit into
Open
ci(#388): fail roll-up when tests are skipped on pull_request_target#400rh-hemartin wants to merge 1 commit into
rh-hemartin wants to merge 1 commit into
Conversation
On pull_request_target, if detect was skipped (gate skipped or auth denied), the roll-up now fails instead of reporting SUCCESS. Closes #388 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
PR Summary by QodoCI: fail roll-up when pull_request_target tests are skipped
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
rh-hemartin
force-pushed
the
agent/388-fix-skipped-tests-rollup
branch
from
July 23, 2026 13:22
db3284f to
fe6ef26
Compare
|
🤖 Finished Review · ✅ Success · Started 1:23 PM UTC · Completed 1:34 PM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Protected workflow file modified
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+370
to
+378
| EVENT_NAME: ${{ github.event_name }} | ||
| GATE_RESULT: ${{ needs.gate.result }} | ||
| DETECT_RESULT: ${{ needs.detect.result }} | ||
| TESTS_RESULT: ${{ needs.functional-tests.result }} | ||
| run: | | ||
| if [ "$EVENT_NAME" = "pull_request_target" ] && [ "$DETECT_RESULT" = "skipped" ]; then | ||
| echo "::error::Detect was skipped on pull_request_target — tests were not authorized to run" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
1. Protected workflow file modified 📜 Skill insight § Compliance
This PR modifies .github/workflows/functional-tests.yml, which is a protected governance/infrastructure path requiring explicit human review and must not be auto-approved. Ensure the PR receives the required human approval (e.g., CODEOWNERS) before merge.
Agent Prompt
## Issue description
Changes under `.github/` are protected governance/infrastructure modifications and must not be auto-approved.
## Issue Context
This PR updates a GitHub Actions workflow under `.github/workflows/`, which requires explicit human review/approval per policy.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[370-378]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+375
to
+378
| if [ "$EVENT_NAME" = "pull_request_target" ] && [ "$DETECT_RESULT" = "skipped" ]; then | ||
| echo "::error::Detect was skipped on pull_request_target — tests were not authorized to run" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
2. Label events fail rollup 🐞 Bug ≡ Correctness
On pull_request_target, the roll-up now hard-fails when detect is skipped; because the workflow triggers on any labeled event but gate is skipped unless the applied label is exactly ok-to-test, adding any other label will skip gate/detect and fail functional-tests-complete (even for already-authorized PRs). This can unexpectedly block merges when maintainers apply unrelated labels after tests have passed.
Agent Prompt
## Issue description
The workflow triggers on all `pull_request_target` label events, but the `gate` job is conditionally skipped unless the label being applied is `ok-to-test`. With the new roll-up rule (`pull_request_target` + `detect` skipped => fail), applying any other label causes `gate` and then `detect` to be skipped, which fails `functional-tests-complete` even when the PR is already authorized.
## Issue Context
The authorization script already supports authorizing *any* `labeled` event as long as the PR currently has `ok-to-test` (it checks the PR’s label set), but the `gate` job never runs for those events due to its `if:` filter.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[46-49]
- .github/workflows/functional-tests.yml[27-28]
### Suggested change
Adjust the `gate.if` so it runs on `pull_request_target` labeled events whenever the PR has `ok-to-test` (not only when the *newly added* label is `ok-to-test`), e.g.:
- `github.event.action != 'labeled' || contains(github.event.pull_request.labels.*.name, 'ok-to-test')`
This prevents false failures from unrelated label changes while keeping the new roll-up failure for genuinely unauthorized PRs.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+375
to
381
| if [ "$EVENT_NAME" = "pull_request_target" ] && [ "$DETECT_RESULT" = "skipped" ]; then | ||
| echo "::error::Detect was skipped on pull_request_target — tests were not authorized to run" | ||
| exit 1 | ||
| fi | ||
| if [ "$GATE_RESULT" = "failure" ] || [ "$GATE_RESULT" = "cancelled" ]; then | ||
| echo "::error::Gate job ${GATE_RESULT}" | ||
| exit 1 |
There was a problem hiding this comment.
3. Gate failure masked 🐞 Bug ◔ Observability
functional-tests-complete checks for detect==skipped before checking needs.gate.result, so if the gate job fails/cancels (causing detect to be skipped), the roll-up exits with a misleading “not authorized” message instead of reporting the gate failure. This makes diagnosing real gate errors harder.
Agent Prompt
## Issue description
The roll-up step exits early on `DETECT_RESULT=skipped` for `pull_request_target`, before it checks `GATE_RESULT`. If `gate` fails/cancels and `detect` is skipped as a consequence, the workflow reports the wrong root cause.
## Issue Context
This doesn’t change pass/fail outcome (it still fails), but it obscures the actual failure mode and slows debugging.
## Fix Focus Areas
- .github/workflows/functional-tests.yml[375-385]
### Suggested change
Reorder the checks so gate failure/cancellation is handled before the `detect==skipped` authorization guard, or make the `detect==skipped` guard conditional on `GATE_RESULT == success` (and optionally add `AUTHORIZED=${{ needs.gate.outputs.authorized }}` to emit a precise message).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| TESTS_RESULT: ${{ needs.functional-tests.result }} | ||
| run: | | ||
| if [ "$EVENT_NAME" = "pull_request_target" ] && [ "$DETECT_RESULT" = "skipped" ]; then | ||
| echo "::error::Detect was skipped on pull_request_target — tests were not authorized to run" |
There was a problem hiding this comment.
[low] error-message-format
The new error message uses a static string rather than interpolating DETECT_RESULT, unlike the existing error messages which interpolate their respective variables. Since this branch only executes when DETECT_RESULT is exactly skipped, interpolation would add redundant information. Minor stylistic inconsistency.
Suggested fix: Optionally interpolate for visual consistency: echo ::error::Detect was skipped on pull_request_target (${DETECT_RESULT}) — tests were not authorized to run
fullsend-ai-review
Bot
added
the
requires-manual-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pull_request_target, whengateis skipped or auth is denied, all downstream jobs cascade toskipped. The roll-up only checked forfailure/cancelled, so it reported SUCCESS -- letting PRs satisfy branch protection without any test running.pull_request_target, ifdetectwas skipped, fail. This covers both gate-skipped and auth-denied paths without affectingpush/merge_group/workflow_dispatch.Test plan
push/merge_groupruns are unaffected (gate is legitimately skipped, detect runs)pull_request_targetwithok-to-testlabel still passes (gate runs, auth granted, detect runs)pull_request_targetwithout authorization now fails the roll-up instead of reporting SUCCESSCloses #388
🤖 Generated with Claude Code