Skip to content

ci(#388): fail roll-up when tests are skipped on pull_request_target #400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rh-hemartin wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −0
Open

ci(#388): fail roll-up when tests are skipped on pull_request_target #400

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/functional-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,10 +367,15 @@ jobs:
steps:
- name: Check results
env:
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"

@fullsend-ai-review fullsend-ai-review Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

exit 1
fi
Comment on lines +370 to +378

@qodo-code-review qodo-code-review Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

@qodo-code-review qodo-code-review Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

if [ "$GATE_RESULT" = "failure" ] || [ "$GATE_RESULT" = "cancelled" ]; then
echo "::error::Gate job ${GATE_RESULT}"
exit 1
Comment on lines +375 to 381

@qodo-code-review qodo-code-review Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

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

Expand Down
Loading