fix(playwright): green playwright-summary when gate skips the pipeline#30468
Conversation
Regression from #30453: pull_request_target on a same-repo PR (or pull_request on a fork) correctly gates every upstream job to `skipped`, but the playwright-summary job then runs the render step unconditionally. render_playwright_summary.cjs counts each skipped upstream as a "CI/reporting failure" and calls core.setFailed(). The required playwright-summary check goes red on the redundant event's run, blocking merges. Seen on https://github.com/open-metadata/OpenMetadata/actions/runs/30090391086/job/89472165978 (PR #30454, a same-repo PR): pull_request_target's playwright-summary failed with "0 Playwright test failure(s); 9 CI/reporting failure(s)" even though the sibling pull_request run passed the real pipeline. Fix: - Add `gate` to playwright-summary's needs list so it can read needs.gate.outputs.should_run. - Gate the render step on `should_run == 'true'` so it doesn't fire when gate said skip. - Add a "Report gate-skipped run as green" step that runs the OTHER way (`should_run != 'true'`) and just echoes success. This keeps the job's conclusion at "success" so branch protection sees a green playwright-summary check regardless of which event fired. Behaviour matrix after this fix: Event gate render job ──────────────────────────────────────────── ──── ────── ─── merge_group / workflow_dispatch / schedule true run real result same-repo PR + pull_request true run real result same-repo PR + pull_request_target false skipped ✅ green fork PR + pull_request_target + label true run real result fork PR + pull_request_target no label false skipped ✅ green fork PR + pull_request false skipped ✅ green any draft PR false skipped ✅ green Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
@greptile-apps flagged that the previous "should_run != 'true'" condition treated an unset gate output the same as an explicit skip decision. If gate itself failed (crash, cancellation, infra error), should_run would be null, my green-report step would still fire, and the required playwright-summary check would report success — silently hiding the gate failure. Two fixes: 1. Add a "Guard against missing gate decision" step at the top of the job that fails loudly when `needs.gate.result != 'success'`. 2. Change the green-report condition from `should_run != 'true'` to the exact `should_run == 'false'` so an unset value never falls through as a synthetic green. Behaviour matrix after this fix: gate.result should_run guard green-report render job ──────────── ─────────── ───── ──────────── ────── ─── success 'true' skip skip run real success 'false' skip ✅ green skip ✅ success (unset) ❌ fail skip skip ❌ (invalid — shouldn't happen) failure any ❌ fail skip skip ❌ (visible) cancelled any ❌ fail skip skip ❌ (visible) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@greptile-apps thanks — addressed in 497d7d0b00. Two changes on
Behaviour matrix after this fix:
|
|
The fix in 497d7d0 correctly addresses the concern. The diff looks good:
The three conditions are mutually exhaustive for all valid gate states and correctly ordered so the guard always runs first. The original concern is fully resolved. |
Code Review ✅ ApprovedUpdates the Playwright E2E workflow to conditionally run the summary renderer only when the gate job outputs should_run equal to true, preventing false failures on skipped pipelines. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
* fix(playwright): rename summary check on redundant/skipped runs Followup to #30453 + #30468 that closes a real premature-green hole. Same-repo PRs fire BOTH pull_request and pull_request_target on every push. #30453's gate correctly routes them: pull_request does the real pipeline; pull_request_target gates to false and #30468's summary short-circuit reports a synthetic green. So far so good. BUT: both runs' playwright-summary jobs publish check runs with the same name — "playwright-summary" — which is exactly the required check on main's branch protection. GitHub's protection UI picks the most-recently-completed run of a given check name, and the synthetic green finishes in ~15 seconds while the real pipeline is still compiling. The PR appears mergeable before any test executes. Concrete evidence: https://github.com/open-metadata/OpenMetadata/actions/runs/30097914955/job/89496479495?pr=30388 Event: pull_request_target Branch: fix/persona-localization (same-repo PR) gate result: success (should_run=false, redundant sibling event) Summary steps: every real step skipped, "Report gate-skipped run as green" ran, job succeeded, check "playwright-summary" published green. Fix: publish the required check name "playwright-summary" ONLY when this run is going to do real work. Every other case publishes a differently-named check that branch protection ignores. Decision tree (mirrors the gate): merge_group / schedule / workflow_dispatch → 'playwright-summary' same-repo PR + pull_request (not draft, no bad label) → 'playwright-summary' fork PR + pull_request_target + 'safe to test' (not draft, no bad label) → 'playwright-summary' labeled event with non-'safe to test' label → 'playwright-summary (label ignored)' everything else (redundant events, drafts, fork PRs without label) → 'playwright-summary (skipped)' Behaviour by scenario after this fix: scenario | pull_request | pull_request_target | required check ----------------------------- | -------------------- | ----------------------- | ------------------- same-repo PR | 'playwright-summary' | '... (skipped)' | held until real run fork PR + safe-to-test label | '... (skipped)' | 'playwright-summary' | held until real run fork PR no label | '... (skipped)' | '... (skipped)' | expected → blocks draft PR | '... (skipped)' | '... (skipped)' | expected → blocks merge queue / dispatch | — | — | 'playwright-summary' Fork-PR-without-label and draft-PR cases correctly stay at "expected" so the required check blocks the PR until the maintainer applies the label (or the author marks it ready). Matches the pre-#30310 UX that the playwright-required-checks-stub.yml file used to provide. Pure name-expression change. The gate logic, the summary job's steps, and the render script are all untouched. No new secrets, permissions, or dependencies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * address greptile P1: reject synchronize on fork target in name expression @greptile-apps caught a real gap between the gate's decision and the job name expression on this PR: gate's fork-target arm rejects synchronize explicitly: [[ "$EVENT" == "pull_request_target" && "$ACTION" != "synchronize" ]] name expression's matching arm did NOT check action: github.event_name == 'pull_request_target' && head.repo != base.repo && !draft && contains(labels, 'safe to test') && !(action == 'labeled' && label != 'safe to test') Sequence that broke: 1. Maintainer applied 'safe to test' to a fork PR earlier 2. Fork author pushes a new commit → synchronize fires on pull_request_target 3. Pre-gate step strips the label from GitHub state (the label- invalidation-on-push safeguard from #30453) 4. But github.event.pull_request.labels is a snapshot from event time — it still shows the label as present in the payload 5. gate.decide: sees action == 'synchronize' → should_run=false (correct — refuses to run new unreviewed code) 6. Summary job's name expression: sees the payload's label → evaluates fork-target arm as true → picks 'playwright-summary' (WRONG — should be '(skipped)') 7. Synthetic green publishes under the required check name → branch protection satisfied without running tests on the new commit Fix: add `github.event.action != 'synchronize'` to the fork-target arm of the name expression so it matches the gate's condition exactly. Also updated the inline decision-tree comment to spell out the synchronize carve-out and its motivation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Regression from #30453 that surfaced on PR #30454 (run 30090391086):
Fix
Three-line change on the `playwright-summary` job:
Behaviour matrix after this fix
Diff scope
```
.github/workflows/playwright-postgresql-e2e.yml | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
```
Pure workflow change. No script edits, no test edits, no logic changes to the render pathway.
Test plan
🤖 Generated with Claude Code
Greptile Summary
Adds explicit handling for every Playwright gate outcome.
should_run=falsedecision as a successful intentional skip.should_run=truedecision.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failures remain.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD G[Gate job completes] --> R{Gate result is success?} R -- No --> F[Fail summary job] R -- Yes --> D{should_run decision} D -- true --> P[Render consolidated Playwright summary] D -- false --> S[Report intentional skip as green]Reviews (2): Last reviewed commit: "address greptile P1: fail if gate itself..." | Re-trigger Greptile