Skip to content

fix: stop Build Gate from passing when no build actually ran - #8861

Merged
aixaCode merged 2 commits into
devfrom
fix/build-gate-skip-loophole
May 22, 2026
Merged

fix: stop Build Gate from passing when no build actually ran#8861
aixaCode merged 2 commits into
devfrom
fix/build-gate-skip-loophole

Conversation

@aixaCode

Copy link
Copy Markdown
Contributor

Summary

PRs have been showing a green Build Gate (Windows + macOS) check without any build artifact being produced — most recently visible on #8858. Two bugs introduced in #8822 combine to cause this:

  1. concurrency.cancel-in-progress evaluated true for almost every pull_request event (the expression was "cancel unless it's a non-override label change"). An incidental PR event — e.g. a transient draft toggle, an auto-labeler firing — would cancel the real in-flight Windows/macOS builds.
  2. The Build Gate auto-passed whenever prebuild.result != 'success', which is exactly what happens after the cancellation above. The skipped/cancelled prebuild produced "Gate not applicable → exit 0", and the PR went green on a commit nothing was ever built for.

Repro on #8858

  • 11:30:05Z — PR marked ready_for_review → workflow run 26285219799 fires, Prebuild succeeds, Build matrix starts.
  • ~11:31:27Z — second pull_request event lands on the same SHA (the second Auto-Assign-Reviewers log on that run says "Skips ... since PR type is draft", so the PR briefly went back to draft).
  • Old cancel-in-progress evaluates true → cancels the in-flight Build (windows64) and Build (macos) of run 1.
  • The new run skips Prebuild (the prebuild if: rejects the action), skips Build, and the gate posts ✅ via the "Gate not applicable" branch.

Changes

  • Invert concurrency.cancel-in-progress to only cancel for events that will actually produce a new build (push, workflow_dispatch, merge_group, or PR actions opened/reopened/synchronize/ready_for_review, or labeled/unlabeled for force-build/clean-build/windows-only/macos-only).
  • Make build-gate fail closed. Only the legitimate skip path (prebuild.result == 'success' && should_build != 'true', i.e. no Explorer/ changes — already covered by Prebuild's per-target commit-status step) auto-passes. Any other prebuild outcome now fails with an actionable error pointing at re-trigger paths.

Behavior change to flag

Draft PRs without a force-build/clean-build override will now show a red Build Gate instead of a green one. That's arguably more correct (drafts aren't built), but it's user-visible. If we'd rather hide the gate on those, we can gate it behind a job-level if: mirroring prebuild's whitelist — happy to follow up.

Test plan

  • After merge, mark a fresh PR ready_for_review and confirm one Unity Cloud Build run executes end-to-end on the head SHA (no spurious second run).
  • Toggle a PR draft → ready_for_review → draft → ready_for_review quickly and confirm the in-flight build is not cancelled by a non-build-producing event, and the gate reflects the actual build outcome.
  • Add and remove a non-override label on an open PR and confirm the in-flight build is not cancelled.
  • PR with no `Explorer/` changes: gate still passes green via the "Gate not applicable" branch.
  • Apply windows-only / macos-only labels and confirm the gate correctly fails (only one target built) — unchanged behavior from feat: windows-only and macos-only ci flags #8822.

Closes the regression introduced in #8822. cc @NickKhalow

Two issues introduced in #8822 caused PRs to show a green Build Gate
without producing any build artifact:

1. concurrency.cancel-in-progress evaluated true for almost every
   pull_request event, so an incidental event (e.g. a transient
   draft toggle) cancelled the real in-flight Windows/macOS builds.

2. The Build Gate auto-passed whenever prebuild was skipped/cancelled
   (only required prebuild.result != success), which is exactly what
   happens after the cancellation above — leaving a green gate on a
   commit that was never built.

Invert the cancel-in-progress logic to only cancel for events that
will actually produce a new build, and make the gate fail closed for
any prebuild outcome other than a successful "no Explorer/ changes"
decision.
@aixaCode
aixaCode requested a review from a team as a code owner May 22, 2026 14:30
@github-actions

github-actions Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

badge

Build skipped — no changes detected under Explorer/.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

Comment thread .github/workflows/build-unitycloud.yml
Comment thread .github/workflows/build-unitycloud.yml
@claude

claude Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @aixaCode's task in 2m 25s —— View job


Code Review

  • Read project documentation
  • Read the PR diff
  • Analyze workflow changes
  • Post review findings

Root-cause check

PASS. This PR fixes both root causes directly:

  1. concurrency.cancel-in-progress now uses an explicit positive allowlist (push, workflow_dispatch, merge_group, and the specific pull_request actions that will actually trigger a new build). The previous negative allowlist let converted_to_draft and any other incidental action cancel in-flight builds.
  2. The Build Gate now fails closed — only passes when prebuild.result == 'success' && should_build != 'true' (prebuild ran and decided no build was needed). Any non-success prebuild outcome produces an actionable error. No symptom-masking.

The three blocking issues raised in the prior review round (undocumented removal of script_debugging, enable-sentry label, and Print S3 upload URL) are confirmed resolved — all three are present in the current file. The synchronize commit addressed those concerns.


Blocking issues

None.


Non-blocking notes

perf_test gate behavior change — Prebuild's job-level if: skips the job when a PR has the perf_test label (line 207). With the new fail-closed logic, prebuild.result == 'skipped' now causes the gate to emit "Prebuild did not succeed (result: skipped)" and exit 1. Previously these PRs passed green. Not a regression for the main use-case, but worth calling out in the PR description if the intent is that perf_test PRs should show a red Build Gate going forward.

cancel-in-progress / prebuild if: draft drift — The new cancel list fires for synchronize unconditionally, but prebuild's if: filters out draft PRs on opened/synchronize (line 219, !github.event.pull_request.draft). If you push to a draft PR (no force-build), cancel-in-progress = true but prebuild is skipped — any in-flight build is cancelled without a replacement. Gate fails closed ("skipped"). The PR description acknowledges this draft behavior. Could be eliminated by mirroring prebuild's draft guard in the cancel expression, but it's a narrow edge case and not a regression.


REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Changes only CI/CD workflow YAML — concurrency.cancel-in-progress expression and build-gate bash logic.
QA_REQUIRED: NO

@claude

claude Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @aixaCode's task in 2m 15s —— View job


Security Review

This PR modifies one workflow file: .github/workflows/build-unitycloud.yml (+37 / -9 against origin/dev). The two changes — inverting concurrency.cancel-in-progress and making build-gate fail closed — are net-positive for both correctness and security.

  • Read changed workflow file
  • Compare against origin/dev base
  • Run workflow security review (Step W)
  • Post findings

Workflow review (Step W)

Check Result
W.1 Prompt loaded from PR checkout n/a — no LLM in this workflow
W.2 Heredoc delimiter on $GITHUB_OUTPUT n/a — change does not touch $GITHUB_OUTPUT
W.3 TOCTOU between trigger and run n/a — pull_request trigger only
W.4 Untrusted event text in prompt n/a — no LLM
W.5 Wildcard write/exfil tools n/a
W.6 Actor check before LLM n/a
W.7 Mutable action refs No actions added or changed
W.8 pull_request_target w/ PR execution Workflow uses pull_request, not _target; no secret exposure to forks
W.9 Long-retention artifact upload Unchanged
W.10 Permissions Unchanged

Untrusted-input handling in the modified lines

  • cancel-in-progress expression (lines 168–198) only reads github.event_name, github.event.action, and github.event.label.name. Each is compared against a closed set of literals via Actions expression syntax — no path to widen the cancellation set or alter control flow from PR-author content.
  • build-gate bash (lines 954–970) interpolates ${{ needs.prebuild.outputs.should_build }}, ${{ needs.prebuild.result }}, and (line 972) ${{ needs.prebuild.outputs.targets }}. All three are workflow-internal:
    • prebuild.result is GitHub-internal (success | failure | cancelled | skipped).
    • should_build is produced as literal true / false by the prebuild decide step (lines 268–311).
    • targets is built from a fixed list inside prebuild and parsed via jq.
      No PR-controlled content reaches a shell-interpolated ${{ ... }} in single quotes here.

Correctness notes (non-blocking)

1. The earlier review's "Blocking issues" are false positives. A prior automated review (2026-05-22T14:31:11Z) flagged removals of script_debugging input, enable-sentry PR label support, and Print S3 upload URL step. None of those are in this PR. The git diff origin/dev...HEAD is +37/−9, restricted to concurrency.cancel-in-progress and build-gate. The flagged features are still present at:

  • script_debugging input: lines 56 and 120
  • enable-sentry label fallback: line 371
  • script-debugging label fallback: lines 460–466
  • Print S3 upload URL step: line 870

Looks like that review compared against the wrong base — safe to disregard those three "blocking" items.

2. Allowlist duplication — cancel-in-progress vs prebuild.if. The new positive allowlist (lines 168–198) now mirrors the prebuild job's if: whitelist (lines 207–236). They must stay in sync; if a future change adds a new trigger to one without the other, the original race reappears. Worth either a comment cross-referencing both, or factoring the predicate into a reusable check. Non-blocking.

3. perf_test PRs now show a red gate. Prebuild's if: (line 207) rejects perf_test-labeled PRs → prebuild.result == 'skipped' → falls into the new "fail closed" branch. The PR description flags the draft case but not this one. If the team wants perf_test PRs green, either add || contains(...labels..., 'perf_test') to the legit-no-op branch, or add a job-level if: to build-gate mirroring prebuild's whitelist. Non-blocking — just decide which behavior you want.

4. Empty targets edge case. When prebuild_result == 'success' and should_build == 'true', control falls through to the jq checks on targets. The jq 'any(. == "windows64")' filter on an empty string would fail with a parse error and exit non-zero (because of set -euo pipefail). That's still fail-closed (good), but it would surface as a confusing jq error rather than the intended "missing target" message. Worth a defensive if [ -z "$targets" ] check above the jq calls. Non-blocking.


Verdict

Net improvement. cancel-in-progress becomes restrictive-by-default; the gate fails closed. Both close real loopholes from #8822 and neither introduces a new attack surface.

DEPENDENCY_REVIEW: PASS


Branch fix/build-gate-skip-loophole

@github-actions

Copy link
Copy Markdown
Contributor

🔒 Claude reviewed this PR — sensitive paths modified (.github/prompts, .github/workflows, or CODEOWNERS). Claude will not auto-approve these PRs — human DEV review is required. No QA needed (Claude reported QA_REQUIRED: NO).

@github-actions github-actions Bot added the no QA needed Used to tag pull requests that does not require QA validation label May 22, 2026
@aixaCode
aixaCode enabled auto-merge (squash) May 22, 2026 15:26
@aixaCode
aixaCode merged commit bac52c8 into dev May 22, 2026
19 of 20 checks passed
@aixaCode
aixaCode deleted the fix/build-gate-skip-loophole branch May 22, 2026 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-dependency no QA needed Used to tag pull requests that does not require QA validation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants