fix: unblock beta compile and gate releases on test success#1103
Merged
dsarno merged 4 commits intoCoplayDev:betafrom May 4, 2026
Merged
fix: unblock beta compile and gate releases on test success#1103dsarno merged 4 commits intoCoplayDev:betafrom
dsarno merged 4 commits intoCoplayDev:betafrom
Conversation
PR CoplayDev#1040 added CaptureComposited referencing the old Assets-folder names (CaptureFromCameraToAssetsFolder, AssetsRelativePath) and called PrepareCaptureResult without the now-required folderOverride argument. Renames into the Project* equivalents; same fix at the call sites in ManageScene.cs. Closes CoplayDev#1100 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Beta-release was publishing to PyPI in parallel with Unity Tests, so broken commits could ship if Unity tests failed (as happened with CoplayDev#1100 on 9.6.9-beta.5). Make publish/version-bump jobs depend on the test jobs in both beta-release.yml and release.yml. The whole release halts before any irreversible commit/tag/push if either test job fails. Also extends the test workflows to fire on PRs so failures are caught before merge: - python-tests: pull_request trigger; runs on every PR (no secrets). - unity-tests: pull_request_target [labeled] trigger gated on the safe-to-test label and on the PR being from a fork. Maintainers apply the label after reviewing the diff; the workflow then runs with UNITY_LICENSE in scope against the PR head SHA. Re-pushed commits do NOT auto-trigger; maintainer must remove and re-apply the label to re-run after additional review. In-repo PRs continue to be tested via the existing push trigger, so no labeling friction for collaborator branches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds reusable test gates and wiring so Unity/Python tests run as prerequisites for beta/release jobs, and changes screenshot capture fallbacks to produce project-relative paths (ProjectRelativePath) instead of assets-relative paths in editor and runtime flows. ChangesCI/CD Workflow Gating
Screenshot Capture Path Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (4)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
251-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the caller’s output folder in
CaptureComposited.This branch now hardcodes
folderOverride: null, so composited captures always land inAssets/Screenshotsand ignore any caller-selectedoutput_folder. The two camera fallbacks have the same problem because they also have no way to receive the override.Suggested fix
public static ScreenshotCaptureResult CaptureComposited( string fileName = null, int superSize = 1, bool ensureUniqueFileName = true, bool includeImage = false, - int maxResolution = 0) + int maxResolution = 0, + string folderOverride = null) { if (!IsScreenCaptureModuleAvailable) { var fallbackCamera = FindAvailableCamera(); if (fallbackCamera != null) - return CaptureFromCameraToProjectFolder(fallbackCamera, fileName, superSize, ensureUniqueFileName, includeImage, maxResolution); + return CaptureFromCameraToProjectFolder( + fallbackCamera, fileName, superSize, ensureUniqueFileName, + includeImage, maxResolution, folderOverride: folderOverride); throw new InvalidOperationException("ScreenCapture module is unavailable and no fallback camera found."); } - ScreenshotCaptureResult result = PrepareCaptureResult(fileName, superSize, ensureUniqueFileName, folderOverride: null, isAsync: false); + ScreenshotCaptureResult result = PrepareCaptureResult( + fileName, superSize, ensureUniqueFileName, + folderOverride: folderOverride, isAsync: false); @@ if (tex == null) { // Fallback to camera-based if ScreenCapture fails var cam = FindAvailableCamera(); if (cam != null) - return CaptureFromCameraToProjectFolder(cam, fileName, superSize, ensureUniqueFileName, includeImage, maxResolution); + return CaptureFromCameraToProjectFolder( + cam, fileName, superSize, ensureUniqueFileName, + includeImage, maxResolution, folderOverride: folderOverride); throw new InvalidOperationException("ScreenCapture.CaptureScreenshotAsTexture returned null and no fallback camera available."); }Also applies to: 260-260, 271-274
🤖 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 `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs` around lines 251 - 255, The composited capture and camera fallback branches ignore the caller's output folder because they call CaptureComposited and CaptureFromCameraToProjectFolder with folderOverride: null (hardcoding Assets/Screenshots); update the calls in CaptureComposited fallback branches and the two camera fallback sites to accept and forward the caller's output folder parameter (e.g., pass the existing output_folder or a new folderOverride variable through to CaptureComposited and CaptureFromCameraToProjectFolder instead of null) so captures respect the caller-selected output folder; locate and change calls in CaptureComposited, FindAvailableCamera fallback branches, and the two camera fallback calls to propagate the folder argument.MCPForUnity/Editor/Tools/ManageScene.cs (1)
597-607:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSpecific-camera screenshots still ignore
output_folder.When
camerais supplied, this branch bypassescmd.outputFolderand always writes to the default screenshot folder, unlike the non-play/default branches. That makes the same API request behave differently depending on camera selection.Suggested fix
bool includeImage = cmd.includeImage ?? false; int maxResolution = cmd.maxResolution ?? 0; // 0 = let ScreenshotUtility default to 640 string cameraRef = cmd.camera; + string folderOverride = ScreenshotPreferences.Resolve(cmd.outputFolder); string captureSource = string.IsNullOrWhiteSpace(cmd.captureSource) ? "game_view" : cmd.captureSource.Trim().ToLowerInvariant(); @@ ScreenshotCaptureResult result = ScreenshotUtility.CaptureFromCameraToProjectFolder( targetCamera, fileName, resolvedSuperSize, ensureUniqueFileName: true, - includeImage: includeImage, maxResolution: maxResolution); + includeImage: includeImage, maxResolution: maxResolution, + folderOverride: folderOverride); - AssetDatabase.ImportAsset(result.ProjectRelativePath, ImportAssetOptions.ForceSynchronousImport); + if (ScreenshotUtility.IsUnderAssets(result.ProjectRelativePath)) + AssetDatabase.ImportAsset(result.ProjectRelativePath, ImportAssetOptions.ForceSynchronousImport); string message = $"Screenshot captured to '{result.ProjectRelativePath}' (camera: {targetCamera.name}).";🤖 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 `@MCPForUnity/Editor/Tools/ManageScene.cs` around lines 597 - 607, The camera branch always calls ScreenshotUtility.CaptureFromCameraToProjectFolder (with no output folder) so screenshots for a supplied targetCamera ignore cmd.outputFolder; change the branch in ManageScene (the targetCamera != null block) to pass the intended output folder into the capture routine (either call a Capture... overload that takes an outputFolder or build the correct destination path from cmd.outputFolder and fileName and call the capture method that writes to that path), then use the returned result.ProjectRelativePath for AssetDatabase.ImportAsset and for the SuccessResponse as before (ensure the symbol names: targetCamera, ScreenshotUtility.CaptureFromCameraToProjectFolder / alternative overload, result.ProjectRelativePath, BuildScreenshotResponseData)..github/workflows/unity-tests.yml (2)
22-52:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
pull_request_target+ checkout of PR head SHA + exposed secrets is the canonical "pwn request" pattern."Combining
pull_request_targetworkflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise." When a workflow is triggered bypull_request_target, theGITHUB_TOKENis granted read/write repository permission even from a public fork.The specific risks here:
- Line 52 explicitly checks out
github.event.pull_request.head.sha(untrusted fork code) while running in the base-branch context with secrets.- Lines 84–111:
game-ci/unity-test-runnerreceivesUNITY_EMAIL,UNITY_PASSWORD,UNITY_SERIAL, andUNITY_LICENSEas environment variables. Unity C# test scripts have full access toSystem.Environment.GetEnvironmentVariable()and unrestricted networking, enabling secret exfiltration from within test code.- Line 49 (
actions/checkout@v4) does not setpersist-credentials: false, meaning theGITHUB_TOKENis persisted to disk by default, and any subsequent step's running code can simply read it.- Setting "require approval for all workflow runs from public fork" won't help with the
pull_request_targettrigger since workflows triggered by these events will always run, regardless of approval settings.The
safe-to-testlabel gate (lines 34–37) is the sole mitigation and relies entirely on the reviewing maintainer catching malicious test code before applying the label.Recommended hardening:
- Add
persist-credentials: falseto the checkout step to prevent GITHUB_TOKEN disk persistence.- Add an explicit
permissions:block scoping down to the minimum needed (e.g.,contents: read).- Consider documenting a mandatory diff-review checklist (specifically: any changes under
TestProjects/UnityMCPTests/) as a prerequisite before applyingsafe-to-test.- Evaluate whether
UNITY_SERIALandUNITY_PASSWORDare both required;UNITY_LICENSE(activation XML) alone may suffice for game-ci's headless activation, reducing the credential blast radius.🛡️ Minimal hardening diff
steps: - name: Checkout repository uses: actions/checkout@v4 with: lfs: true ref: ${{ inputs.ref || github.event.pull_request.head.sha || github.ref }} + persist-credentials: falseAdd a
permissionsblock to the job:testAllModes: name: Test in ${{ matrix.testMode }} runs-on: ubuntu-latest + permissions: + contents: read if: >🤖 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 22 - 52, The workflow uses pull_request_target with an explicit checkout of the PR head and exposes secrets to test runners; update the testAllModes job to harden it by (1) adding a permissions block (e.g., permissions: contents: read) at job level to limit GITHUB_TOKEN scope, (2) set persist-credentials: false on the actions/checkout@v4 step to avoid persisting GITHUB_TOKEN to disk, (3) stop checking out untrusted PR HEAD SHA by removing the explicit github.event.pull_request.head.sha ref use in the checkout input (only checkout the base repo by default and only fetch PR head after the safe-to-test label is verified), and (4) remove/limit passing UNITY_EMAIL, UNITY_PASSWORD, UNITY_SERIAL secrets into the game-ci/unity-test-runner step (pass only UNITY_LICENSE if sufficient) and gate any secret usage behind the safe-to-test label check; reference the pull_request_target trigger, testAllModes job, actions/checkout@v4, safe-to-test label, and UNITY_EMAIL/UNITY_PASSWORD/UNITY_SERIAL/UNITY_LICENSE in your changes.
132-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winArtifact upload may error with an empty path when
Run domain reload testsfails.
Run domain reload tests(line 84) has nocontinue-on-error: true. If it fails, theRun testsstep (line 98) is skipped due to the implicitsuccess()condition, leavingsteps.tests.outputs.artifactsPathunset. The upload step runs (because ofalways()) butactions/upload-artifact@v4with an emptypathwill error with "No files were found", adding noise on top of an already-failed run.🛠️ Proposed fix
- uses: actions/upload-artifact@v4 - if: always() && steps.detect.outputs.unity_ok == 'true' + if: always() && steps.detect.outputs.unity_ok == 'true' && steps.tests.outcome != 'skipped' with: name: Test results for ${{ matrix.testMode }} path: ${{ steps.tests.outputs.artifactsPath }}🤖 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 132 - 136, The upload-artifact step errors when steps.tests.outputs.artifactsPath is unset because the "Run domain reload tests" step can fail and skip the "Run tests" step; fix by either making "Run domain reload tests" continue-on-error: true or by guarding the upload step's if condition to check the artifactsPath is non-empty (e.g. require steps.tests.outputs.artifactsPath != '' && steps.detect.outputs.unity_ok == 'true') so actions/upload-artifact@v4 only runs when the tests step produced artifacts.
🧹 Nitpick comments (1)
.github/workflows/unity-tests.yml (1)
22-28: 💤 Low valueVerify that
paths+types: [labeled]combination onpull_request_targetbehaves as expected.If you define both
branches/branches-ignoreandpaths/paths-ignore, the workflow will only run when both filters are satisfied. Fortypes: [labeled], this means thelabeledevent only triggers the workflow if the PR also has changed files matching thepathsfilter. A fork PR that changes only Python/Server files but has thesafe-to-testlabel added would silently not trigger Unity tests — maintainers might not realize the tests didn't run. Confirm this is the desired gating behavior, and consider adding a comment or docs note to warn maintainers to verify tests actually ran after labeling.🤖 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 22 - 28, The pull_request_target event block uses types: [labeled] together with branches and paths filters, which causes the workflow to only run on labeled events when the PR also changes files matching the paths filter; update the workflow to avoid silently skipping tests by either removing the paths filter for the labeled trigger, adding a separate pull_request_target entry (or workflow) that listens to types: [labeled] without paths/branches filtering, or add an inline comment and documentation warning maintainers that labeled events require matching paths to run and to verify tests ran; locate the pull_request_target block and adjust the filters or add a second labeled-only trigger so the safe-to-test label reliably kicks off Unity tests.
🤖 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 22-52: The workflow uses pull_request_target with an explicit
checkout of the PR head and exposes secrets to test runners; update the
testAllModes job to harden it by (1) adding a permissions block (e.g.,
permissions: contents: read) at job level to limit GITHUB_TOKEN scope, (2) set
persist-credentials: false on the actions/checkout@v4 step to avoid persisting
GITHUB_TOKEN to disk, (3) stop checking out untrusted PR HEAD SHA by removing
the explicit github.event.pull_request.head.sha ref use in the checkout input
(only checkout the base repo by default and only fetch PR head after the
safe-to-test label is verified), and (4) remove/limit passing UNITY_EMAIL,
UNITY_PASSWORD, UNITY_SERIAL secrets into the game-ci/unity-test-runner step
(pass only UNITY_LICENSE if sufficient) and gate any secret usage behind the
safe-to-test label check; reference the pull_request_target trigger,
testAllModes job, actions/checkout@v4, safe-to-test label, and
UNITY_EMAIL/UNITY_PASSWORD/UNITY_SERIAL/UNITY_LICENSE in your changes.
- Around line 132-136: The upload-artifact step errors when
steps.tests.outputs.artifactsPath is unset because the "Run domain reload tests"
step can fail and skip the "Run tests" step; fix by either making "Run domain
reload tests" continue-on-error: true or by guarding the upload step's if
condition to check the artifactsPath is non-empty (e.g. require
steps.tests.outputs.artifactsPath != '' && steps.detect.outputs.unity_ok ==
'true') so actions/upload-artifact@v4 only runs when the tests step produced
artifacts.
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 597-607: The camera branch always calls
ScreenshotUtility.CaptureFromCameraToProjectFolder (with no output folder) so
screenshots for a supplied targetCamera ignore cmd.outputFolder; change the
branch in ManageScene (the targetCamera != null block) to pass the intended
output folder into the capture routine (either call a Capture... overload that
takes an outputFolder or build the correct destination path from
cmd.outputFolder and fileName and call the capture method that writes to that
path), then use the returned result.ProjectRelativePath for
AssetDatabase.ImportAsset and for the SuccessResponse as before (ensure the
symbol names: targetCamera, ScreenshotUtility.CaptureFromCameraToProjectFolder /
alternative overload, result.ProjectRelativePath, BuildScreenshotResponseData).
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 251-255: The composited capture and camera fallback branches
ignore the caller's output folder because they call CaptureComposited and
CaptureFromCameraToProjectFolder with folderOverride: null (hardcoding
Assets/Screenshots); update the calls in CaptureComposited fallback branches and
the two camera fallback sites to accept and forward the caller's output folder
parameter (e.g., pass the existing output_folder or a new folderOverride
variable through to CaptureComposited and CaptureFromCameraToProjectFolder
instead of null) so captures respect the caller-selected output folder; locate
and change calls in CaptureComposited, FindAvailableCamera fallback branches,
and the two camera fallback calls to propagate the folder argument.
---
Nitpick comments:
In @.github/workflows/unity-tests.yml:
- Around line 22-28: The pull_request_target event block uses types: [labeled]
together with branches and paths filters, which causes the workflow to only run
on labeled events when the PR also changes files matching the paths filter;
update the workflow to avoid silently skipping tests by either removing the
paths filter for the labeled trigger, adding a separate pull_request_target
entry (or workflow) that listens to types: [labeled] without paths/branches
filtering, or add an inline comment and documentation warning maintainers that
labeled events require matching paths to run and to verify tests ran; locate the
pull_request_target block and adjust the filters or add a second labeled-only
trigger so the safe-to-test label reliably kicks off Unity tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4846f9ce-e60f-4e82-a726-6cb445c54ac1
📒 Files selected for processing (6)
.github/workflows/beta-release.yml.github/workflows/python-tests.yml.github/workflows/release.yml.github/workflows/unity-tests.ymlMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
…camera paths Two pre-existing inconsistencies surfaced by CodeRabbit on CoplayDev#1103: 1. CaptureComposited dropped the caller's output_folder by hardcoding folderOverride: null in PrepareCaptureResult and the camera fallbacks. Adds the parameter to CaptureComposited's signature and plumbs it through both fallback paths. 2. The targetCamera and includeImage-in-play paths in ManageScene's game_view screenshot did not resolve cmd.outputFolder, so a request that selected a specific camera would always write to the default folder. Resolve via ScreenshotPreferences.Resolve as the other paths already do, and gate AssetDatabase.ImportAsset on IsUnderAssets so non-Assets folders don't trigger a futile import. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address CodeRabbit security review on CoplayDev#1103: - persist-credentials: false on the checkout step, so GITHUB_TOKEN is not written to disk and cannot be read by subsequent steps running PR-controlled code. - Explicit permissions: contents: read on the testAllModes job, scoping the workflow's token down from the default read/write set. - Skip upload-artifact when the main test step was skipped (e.g., because the preceding domain-reload step failed without continue-on-error). Avoids the noisy "No files were found" error on top of an already-failed run. The label-gated trigger plus these mitigations narrow the blast radius of the pull_request_target + checkout-PR-head pattern. The remaining trust boundary is the maintainer review before applying safe-to-test; documenting the review checklist (especially TestProjects/UnityMCPTests diffs) is a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/unity-tests.yml (2)
119-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote the
artifactsPathexpansion in thefindcommandAn unquoted
${{ steps.tests.outputs.artifactsPath }}will causefindto misparse the path if it ever contains spaces.🛡️ Proposed fix
- RESULTS_XML=$(find ${{ steps.tests.outputs.artifactsPath }} -name '*.xml' 2>/dev/null | head -1) + RESULTS_XML=$(find "${{ steps.tests.outputs.artifactsPath }}" -name '*.xml' 2>/dev/null | head -1)🤖 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 at line 119, The find command that sets RESULTS_XML uses an unquoted expansion of ${{ steps.tests.outputs.artifactsPath }}, which will break if the path contains spaces; update the command that assigns RESULTS_XML (the find invocation) to quote the expansion so find sees the full path as a single argument (i.e., use a quoted "${{ steps.tests.outputs.artifactsPath }}" in the find command and preserve the rest of the flags/redirections).
124-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty
FAILEDproduces a false-positive failureIf
grepfinds nofailed="…"attribute (e.g., format variation, NUnit2failures=vs NUnit3failed=),FAILEDis an empty string.[ "$FAILED" != "0" ]then evaluates"" != "0"as true, causing the job to exit 1 with a misleading" test(s) failed"message even when all tests passed.🛡️ Proposed fix
FAILED=$(grep -oP 'failed="\K[0-9]+' "$RESULTS_XML" | head -1) PASSED=$(grep -oP 'passed="\K[0-9]+' "$RESULTS_XML" | head -1) TOTAL=$(grep -oP 'total="\K[0-9]+' "$RESULTS_XML" | head -1) INCONCLUSIVE=$(grep -oP 'inconclusive="\K[0-9]+' "$RESULTS_XML" | head -1) SKIPPED=$(grep -oP 'skipped="\K[0-9]+' "$RESULTS_XML" | head -1) echo "Results: $PASSED passed, $FAILED failed, $INCONCLUSIVE inconclusive, $SKIPPED skipped (total: $TOTAL)" - if [ "$FAILED" != "0" ]; then - echo "::error::$FAILED test(s) failed" - exit 1 - fi + if [ -z "$FAILED" ]; then + echo "::error::Could not parse 'failed' count from test results XML — unexpected format" + exit 1 + fi + if [ "$FAILED" -gt 0 ]; then + echo "::error::$FAILED test(s) failed" + exit 1 + fi🤖 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 124 - 133, The FAILED extraction can be empty when the XML uses a different attribute name, causing the test step to treat "" != "0" as true; update the grep that sets FAILED to match both failed and failures attributes (e.g., use a regex like (failed|failures)="\K[0-9]+") and then normalize the variable after extraction with parameter expansion so it defaults to 0 when empty (FAILED=${FAILED:-0}); apply the same defensive defaulting for PASSED, TOTAL, INCONCLUSIVE, and SKIPPED variables if desired, and keep the existing conditional if [ "$FAILED" != "0" ] to fail only when FAILED is actually non‑zero.
🤖 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.
Inline comments:
In @.github/workflows/unity-tests.yml:
- Around line 54-55: Replace use of production Unity credentials by provisioning
and referencing a dedicated CI-only Unity serial (rotateable) for the job that
runs game-ci/unity-test-runner@v4; ensure the job uses that CI-only secret
instead of any production UNITY_SERIAL/UNITY_LICENSE/UNITY_EMAIL/UNITY_PASSWORD
values and document the credential name in the workflow. Also add a clear
warning comment immediately above the safe-to-test label check (the label gating
logic that permits running pull_request_target checkouts) instructing
maintainers to manually inspect all tests and assets in the PR (including C#
test files) before applying the safe-to-test label, and reference this guidance
in CONTRIBUTING.md.
---
Outside diff comments:
In @.github/workflows/unity-tests.yml:
- Line 119: The find command that sets RESULTS_XML uses an unquoted expansion of
${{ steps.tests.outputs.artifactsPath }}, which will break if the path contains
spaces; update the command that assigns RESULTS_XML (the find invocation) to
quote the expansion so find sees the full path as a single argument (i.e., use a
quoted "${{ steps.tests.outputs.artifactsPath }}" in the find command and
preserve the rest of the flags/redirections).
- Around line 124-133: The FAILED extraction can be empty when the XML uses a
different attribute name, causing the test step to treat "" != "0" as true;
update the grep that sets FAILED to match both failed and failures attributes
(e.g., use a regex like (failed|failures)="\K[0-9]+") and then normalize the
variable after extraction with parameter expansion so it defaults to 0 when
empty (FAILED=${FAILED:-0}); apply the same defensive defaulting for PASSED,
TOTAL, INCONCLUSIVE, and SKIPPED variables if desired, and keep the existing
conditional if [ "$FAILED" != "0" ] to fail only when FAILED is actually
non‑zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d738ce59-502f-4485-86fb-c512a1518d2f
📒 Files selected for processing (3)
.github/workflows/unity-tests.ymlMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
- MCPForUnity/Editor/Tools/ManageScene.cs
Comment on lines
+54
to
+55
| ref: ${{ inputs.ref || github.event.pull_request.head.sha || github.ref }} | ||
| persist-credentials: false |
Contributor
There was a problem hiding this comment.
UNITY credentials are exposed to untrusted fork code — use a dedicated CI-only serial and document the threat model
The checkout at line 54 resolves to github.event.pull_request.head.sha for pull_request_target events, placing fork-controlled C# test code on disk. That code is then executed by game-ci/unity-test-runner@v4 with UNITY_LICENSE, UNITY_EMAIL, UNITY_PASSWORD, and UNITY_SERIAL directly in the job environment. A malicious test could read those env vars and exfiltrate them (e.g., via an HTTP call in a [UnityTest]).
Combining pull_request_target with an explicit checkout of an untrusted PR is a dangerous practice. GitHub itself advises to avoid using this event if you need to build or run code from the pull request. The persist-credentials: false and contents: read correctly protect the GITHUB_TOKEN, but they offer no protection for the Unity-specific secrets passed to the test runner.
The PR explicitly acknowledges this trade-off and the label-gating is the right primary control, but two hardening steps would meaningfully reduce the blast radius:
- Use a dedicated, rotatable CI serial. Register a separate Unity serial used exclusively for CI. If it leaks it can be deactivated without impacting production licenses. This is a low-cost mitigation.
- Add a warning comment above the
safe-to-testlabel check (or inCONTRIBUTING.md) instructing maintainers to inspect all test files—not just the production code—before adding the label.
Also applies to: 86-99, 101-114
🤖 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 54 - 55, Replace use of
production Unity credentials by provisioning and referencing a dedicated CI-only
Unity serial (rotateable) for the job that runs game-ci/unity-test-runner@v4;
ensure the job uses that CI-only secret instead of any production
UNITY_SERIAL/UNITY_LICENSE/UNITY_EMAIL/UNITY_PASSWORD values and document the
credential name in the workflow. Also add a clear warning comment immediately
above the safe-to-test label check (the label gating logic that permits running
pull_request_target checkouts) instructing maintainers to manually inspect all
tests and assets in the PR (including C# test files) before applying the
safe-to-test label, and reference this guidance in CONTRIBUTING.md.
This was referenced May 4, 2026
dsarno
added a commit
that referenced
this pull request
May 5, 2026
Both the push and pull_request_target paths filters listed MCPForUnity/Editor/** but not Runtime/, so a PR (or push) that modified only files under MCPForUnity/Runtime/** would not trigger Unity tests at all -- including the safe-to-test label flow on a fork PR. CodeRabbit flagged this as a Low/💤 nitpick on #1103; PR #1106 made it a concrete recurrence: applying safe-to-test had no effect because the Runtime-only diff was filtered out. Adding Runtime/** mirrors the asmdef layout (Editor and Runtime are the two assembly roots whose code can break Unity compilation), matches the codebase's "domain symmetry" convention, and closes the silent-skip path the label gate was designed to prevent. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lint111
added a commit
to Lint111/unity-mcp
that referenced
this pull request
May 5, 2026
Pulls upstream beta progress (versions 9.6.9-beta.4 → 9.6.9-beta.7) onto our fork while preserving our local divergence (queue/gateway functionality plus today's responsiveness fixes). Substantive upstream changes pulled in: - UnityCompatShims pattern + UnityPhysicsCompat / UnityAssembliesCompat - UnityFindObjectsCompat 2022.3 compile-gap fix (CoplayDev#1106) - game_view screenshot capture of UI Toolkit overlays - Beta-release CI gating on test success (CoplayDev#1103) - unity-tests CI trigger now includes MCPForUnity/Runtime/** Conflicts resolved: - PhysicsSettingsOps.cs — took upstream's UnityPhysicsCompat shim over our #pragma warning disable suppression (matches the new shim convention). - UnityReflect.cs — kept both using directives (Editor.Services + Runtime.Helpers). - Server/uv.lock — kept our version (9.6.8, matches pyproject.toml). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
2 participants
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
Two related changes responding to the broken 9.6.9-beta.5 publish (issue #1100):
CaptureCompositedwith the renamedProject*-folder API. PR fix: include UI Toolkit overlays in game_view screenshots with include_image #1040 addedCaptureCompositedreferencing the oldCaptureFromCameraToAssetsFolder/AssetsRelativePathnames and calledPrepareCaptureResultwithout the now-requiredfolderOverridearg. Renames into theProject*equivalents inScreenshotUtility.cs, plus the matching call sites inManageScene.cs. Closes Unity 2022.3 compile errors on beta 9.6.9-beta.5 / commit 37ef016 #1100.beta-release.ymlandrelease.ymlpublish/bump jobs nowneeds: [unity_tests, python_tests]viaworkflow_call. The whole release halts before any irreversible commit/tag/push if either fails. Previously PyPI shipped 28s after push, before Unity Tests had even compiled the project.python-tests.ymlnow has apull_requesttrigger; runs on every PR (no secrets).unity-tests.ymlnow haspull_request_target: types: [labeled]gated on asafe-to-testlabel and the PR being from a fork. Maintainers review the diff, apply the label, and unity-tests runs withUNITY_LICENSEagainst the PR's head SHA. Re-pushed commits do NOT auto-trigger; maintainer must remove and re-apply the label to re-run after additional review.In-repo PRs continue to be tested via the existing
pushtrigger, so no labeling friction for collaborator branches.Maintainer setup
After this lands, create a
safe-to-testlabel in the repo (Issues/PRs → Labels → New). That's the entire one-time setup; the rest is automatic.Test plan
unity-testsandpython-testsjobs both run before any release/publish job starts.unity-testsdoes NOT run.safe-to-testto a fork PR — confirmunity-testsruns against the PR's head SHA.unity-testsdoes NOT auto-rerun.release.ymlonmainand confirm it runsunity_tests+python_testsagainstbetabeforebumpruns.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores