ci: retire safe-to-test gate, make skipped Unity checks visible - #1308
Conversation
Fork PRs touching MCPForUnity/** get green Unity checks that verified nothing. GitHub withholds secrets from pull_request runs originating in a fork, so the detect step writes unity_ok=false and every real step is gated off. Step-level `if:` produces step-conclusion `skipped`, which contributes nothing to the job conclusion, so the job reports success having compiled and tested nothing. Make the skip unmissable: both workflows now emit ::warning:: and a $GITHUB_STEP_SUMMARY block stating the check is not a pass. Retire the safe-to-test label gate. It was the intended escape hatch but never worked in practice -- actions/checkout's floating v4 tag has since rolled forward to v4.4.0, which refuses to check out fork code under pull_request_target without allow-unsafe-pr-checkout: true. Repairing it would mean running fork-authored C# through game-ci/unity-test-runner with UNITY_* secrets in scope, which is the classic pwn-request shape. Removing the trigger makes both job-level `if:` gates dead code (each began with `github.event_name != 'pull_request_target' ||`), so they go too. To test a fork PR, review the diff and push its branch into this repo; the push trigger runs the full suite in a trusted context. Known tradeoff: the full-matrix label now takes effect on the next push rather than on application, since nothing re-triggers on `labeled`. This does not give fork PRs real signal -- it stops the absence of signal from looking like success. A license-free compile job is the follow-up.
📝 WalkthroughWalkthroughThe workflows remove ChangesWorkflow safety and skip reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the Unity-related GitHub Actions workflows to prevent fork PR runs (which lack Unity license secrets) from presenting “green” checks that can be misread as having compiled/tested code. It removes the broken safe-to-test/pull_request_target escape hatch and instead makes the “skipped due to missing secrets” outcome highly visible via warnings and run-page summaries.
Changes:
- Remove the
pull_request_target+safe-to-testgate fromunity-tests.yml(and the now-dead job-level gating logic). - Add explicit
::warning::output plus$GITHUB_STEP_SUMMARYblocks to clearly state when Unity tests/E2E smoke were skipped and did not compile/test/boot anything. - Adjust the full-matrix label logic comment/behavior expectations (label is read only at workflow trigger time; applying it requires a subsequent push).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| .github/workflows/unity-tests.yml | Removes pull_request_target safe-to-test gate and adds prominent skip messaging so “green” doesn’t imply compiled/tested for fork PRs. |
| .github/workflows/e2e-bridge.yml | Adds run-page step summary + warning when license secrets are absent so the no-op green check is clearly labeled as skipped. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/unity-tests.yml (1)
200-234: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winProtect raw failure details from workflow-command injection.
esc_data()protects the annotation fields, butmsgandstackare printed unescaped during raw XML failure output. A value containing a line starting with::can still create unwanted workflow commands inside the group. Output a unique::stop-commands::token before printing these details, and reset command parsing after::endgroup::.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/unity-tests.yml around lines 200 - 234, Update the failure-detail loop around msg and stack to emit a unique ::stop-commands:: token before printing raw XML content, preventing lines beginning with :: from being interpreted as workflow commands. After closing the group with ::endgroup::, emit the matching token to resume command parsing, while preserving the existing annotation and full message/stack output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/unity-tests.yml:
- Around line 200-234: Update the failure-detail loop around msg and stack to
emit a unique ::stop-commands:: token before printing raw XML content,
preventing lines beginning with :: from being interpreted as workflow commands.
After closing the group with ::endgroup::, emit the matching token to resume
command parsing, while preserving the existing annotation and full message/stack
output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 121ba45f-5e7f-4351-bc90-d3f0d7cd71f6
📒 Files selected for processing (2)
.github/workflows/e2e-bridge.yml.github/workflows/unity-tests.yml
Problem
Fork PRs touching
MCPForUnity/**get green Unity checks that verified nothing.Observed on #1250 (a one-file C# change). Three green checks totalling 22 seconds, none of which compiled a line of the changed code:
Mechanism: GitHub withholds repository secrets from
pull_requestruns originating in a fork, soDetect Unity license secretswritesunity_ok=falseand every real step is gated off. Step-levelif:produces a step conclusion ofskipped, which contributes nothing to the job conclusion — the job reportssuccesshaving done nothing. There is no path to failure, because the only step that canexit 1(Check test results) is itself gated.e2e-bridgeis worse: even the checkout is gated, so it concludes green in ~4s having run exactly one step.A reviewer reading
gh pr checkssees green and reasonably assumes the code compiled. It did not.The
safe-to-testescape hatch is brokenunity-tests.ymlhad apull_request_target: types: [labeled]trigger so a maintainer could applysafe-to-testand get a real run. Applying it now fails:Nothing in this repo changed.
actions/checkout@v4is a floating tag that has since rolled forward to v4.4.0, which added this guardrail. Both checkouts passgithub.event.pull_request.head.sha, so both are blocked.What this PR does
1. Makes the skip unmissable. Both workflows now emit
::warning::plus a$GITHUB_STEP_SUMMARYblock stating plainly that no C# was compiled and no test ran, and that the green check is not a pass.unity-testspreviously logged only a plainecho;e2e-bridgehad a warning but nothing on the run page.2. Retires
safe-to-testrather than repairing it. Repairing meansallow-unsafe-pr-checkout: true, and the warning is accurate for this workflow specifically: after checkout, the job runsgame-ci/unity-test-runner, which executes arbitrary fork-authored C# — any[InitializeOnLoad]editor script is enough — in a process withUNITY_LICENSE,UNITY_EMAIL,UNITY_PASSWORDandUNITY_SERIALin its environment.persist-credentials: falseandpermissions: contents: readalready limitGITHUB_TOKENexposure, so the prize for an attacker is the Unity credentials, and a label was the only thing in the way. The gate was never used in practice.Removing the trigger makes both job-level
if:gates dead code — each began withgithub.event_name != 'pull_request_target' ||, unconditionally true once the trigger is gone — so they go too. Net +39 / −50.To test a fork PR going forward: review the diff, then push its branch into this repo. The existing
pushtrigger (branches-ignore: [beta, main]) runs the full licensed suite in a genuinely trusted context, against a repo-owned ref, with no opt-in flag.Known tradeoff
The
full-matrixlabel used to re-trigger a run when applied to an open PR — that wastypes: [labeled]doing the work. It now takes effect on the next push instead. The label logic itself is unchanged and still works. If on-demand re-runs are wanted back, addinglabeledto thepull_requesttrigger'stypesgets it safely (no secrets for forks, secrets for same-repo) and is a one-line follow-up.Deliberately not included
.github/dependabot.yml, so a lone pinned SHA would go stale with no update path while every other action stayed floating.v4resolves tov4.4.0today, so pinning to it changes nothing functionally.UnityEngine.*/UnityEditor.*DLLs at CI time from the publicunityci/editorimage (the pull needs no Unity credentials), cache the ~100 MB subset populated from abetapush, compile Runtime then Editor withcsc. Fork PRs can read base-branch caches but not write them. Cost is real — the Editor asmdef resolves to 219 references and 110DefineConstants, and that define list must be captured from a licensed run and re-captured whendefaultVersionbumps.This PR does not give fork PRs real signal. It stops the absence of signal from looking like success.
Verification
Both workflows parse (
yaml.safe_load).unity-tests.ymltriggers are nowworkflow_dispatch, workflow_call, push, pull_request; jobsmatrixandtestAllModesare unchanged apart from the removed dead gates. The only surviving mention ofpull_request_targetis the comment explaining why there deliberately isn't one.Since this PR modifies
.github/workflows/unity-tests.yml, it matches that workflow's own paths filter — so its checks should exercise the new skip messaging directly.Summary by CodeRabbit
New Features
Documentation