fix: stop quoting docker's pull noise as the cause of a Unity abort - #293
Merged
Merged
Conversation
When Unity aborts batchmode, Docker.run passes the reason from stdout and then appended `docker run`'s own stderr under an "Original error:" heading. That stderr opens with the benign pre-pull "Unable to find image ... locally" status line, so the message read as an editor failing to load a version and buried the actual cause two lines above it: Unity aborted before completing: Scripts have compiler errors. ... Original error: Unable to find image 'unityci/editor:...' locally Two Mirror maintainers read it exactly that way on Mirror#4128 - they went looking for a 6000.3.23f1 problem while "Scripts have compiler errors." sat in the same message. System.run already streams that stderr live, so repeating it loses nothing. describe() now takes originalMessage as optional and omits the section when the caller has nothing to add. HostRunner keeps passing it: on its --silent path the streamed copy does not exist, so the quoted text is the only one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change makes the original error message optional for Unity batchmode failures. Docker no longer appends streamed stderr to the failure description. Regression tests verify that Unity's compiler-error reason remains visible without Docker pull-progress noise. ChangesUnity batchmode reporting
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Two defects in the release-download smoke tests, both hit live on PR #293: The macOS job 403'd on api.github.com while the ubuntu job passed beside it, running the same install.sh against the same endpoint in the same minute. install.sh resolves "latest" through the API and authenticates only when handed a token - unauthenticated is 60 requests/hour per IP, and Actions runners share IPs across unrelated repos and orgs. The script was written to be given one, and says so in its own comment; the workflow never passed it. Both smoke steps, and the root-wrapper steps that default to "latest" and hit the same call, now pass GH_TOKEN - the pattern already used throughout orchestrator-integrity.yml. install.sh treats an empty token as no token, so a fork PR (no secrets, read-only github.token) degrades to today's behaviour rather than failing. The retry loop reported every failure the same way - "likely the latest release's binaries are still uploading". A 403 rate limit is not that, and re-running the identical unauthenticated request 30 seconds later cannot clear it, so it spent all five attempts and 2.5 minutes asserting a cause it had no way to know. That is the same misdiagnosis this branch's Unity abort message fix exists to remove: point at the error that actually occurred instead of substituting a guess for it. Verified end-to-end: GH_TOKEN=... bash scripts/install.sh latest <dir> resolves v0.1.69, downloads, and checksum-verifies. actionlint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 tasks
frostebite
added a commit
that referenced
this pull request
Sep 21, 2026
* fix: stop quoting docker's pull noise as the cause of a Unity abort When Unity aborts batchmode, Docker.run passes the reason from stdout and then appended `docker run`'s own stderr under an "Original error:" heading. That stderr opens with the benign pre-pull "Unable to find image ... locally" status line, so the message read as an editor failing to load a version and buried the actual cause two lines above it: Unity aborted before completing: Scripts have compiler errors. ... Original error: Unable to find image 'unityci/editor:...' locally Two Mirror maintainers read it exactly that way on Mirror#4128 - they went looking for a 6000.3.23f1 problem while "Scripts have compiler errors." sat in the same message. System.run already streams that stderr live, so repeating it loses nothing. describe() now takes originalMessage as optional and omits the section when the caller has nothing to add. HostRunner keeps passing it: on its --silent path the streamed copy does not exist, so the quoted text is the only one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: stop the install smoke tests failing for the wrong reason Two defects in the release-download smoke tests, both hit live on PR #293: The macOS job 403'd on api.github.com while the ubuntu job passed beside it, running the same install.sh against the same endpoint in the same minute. install.sh resolves "latest" through the API and authenticates only when handed a token - unauthenticated is 60 requests/hour per IP, and Actions runners share IPs across unrelated repos and orgs. The script was written to be given one, and says so in its own comment; the workflow never passed it. Both smoke steps, and the root-wrapper steps that default to "latest" and hit the same call, now pass GH_TOKEN - the pattern already used throughout orchestrator-integrity.yml. install.sh treats an empty token as no token, so a fork PR (no secrets, read-only github.token) degrades to today's behaviour rather than failing. The retry loop reported every failure the same way - "likely the latest release's binaries are still uploading". A 403 rate limit is not that, and re-running the identical unauthenticated request 30 seconds later cannot clear it, so it spent all five attempts and 2.5 minutes asserting a cause it had no way to know. That is the same misdiagnosis this branch's Unity abort message fix exists to remove: point at the error that actually occurred instead of substituting a guess for it. Verified end-to-end: GH_TOKEN=... bash scripts/install.sh latest <dir> resolves v0.1.69, downloads, and checksum-verifies. actionlint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: reproduce the Mirror#4128 Unity abort message end to end A container aborts Unity, docker leads its own stderr with the benign "Unable to find image '...' locally" pre-pull notice, and the CLI's final error used to append that stderr under an "Original error:" heading. The message then read as an editor failing to load a Unity version, while Unity's real reason - a genuine script compile error - sat two lines above it (#288). #293 fixed the message and src/model/docker.test.ts asserts it, but that test hard-codes BOTH streams onto its fake error, so it stays green if Docker.run reads the wrong stream or System.run folds them together. That plumbing is precisely what broke, and nothing in CI ran a real `docker run` through it. This adds that, and grades what the user is actually shown. - New unity-abort-message job: an aborting-editor stub (no Unity, no licence, no multi-GB pull), driven through both CLI routes because they reach stdout differently - build.sh passes `-logfile /dev/stdout`, while test.sh writes the log to a file and cats it after capturing the exit code. Both graded by scripts/assert-unity-abort-message.sh. - Wired into smoke-gate.needs, so a failure there actually fails the gate. - scripts/test-assert-unity-abort-message.sh drives the grader in both directions against hand-written logs - including the pre-#293 message it exists to reject - because its two "must not contain" assertions can only ever pass vacuously on a run that died early. - A stream-split case in system.integration.test.ts, against a real child process rather than the mocked System.run. - A comment pinning the `silent` coupling Docker.run's catch block rests on ("System.run has already streamed it live above"), which holds only while options.silent stays falsy. No behaviour change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: address review on the abort-message reproduction Declare UNITY_VERSION through each step's env rather than interpolating the workflow_dispatch input into the script body, where ${{ }} expands into shell source before bash sees it. Clean the grader's temp file from an EXIT trap so an interrupt cannot leave it behind. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <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
1 participant
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.
Changes
The Unity abort message (
0a4aabe)UnityBatchmodeFailure.describe()now takesoriginalMessageas optional, and omits theOriginal error:section entirely when the caller has nothing to add.Docker.runstops passing its stderr.System.runhas already streamed that stream live above, so repeating it lost nothing and only asserted it was the cause.HostRunneris deliberately unchanged: it still passes its message, because on its--silentpath the streamed copy does not exist and the quoted text is the only one.docker.test.tsnow asserts the message does not containUnable to find image; a second test covers theoriginalMessage-omitted shape directly.When Unity aborts batchmode, the reason lives in stdout and the surrounding log. What got appended underneath it was
docker run's own stderr, which opens with the benign pre-pull status line:The actual cause is the first line. The last three read as a version-pull failure, and that is exactly how two Mirror maintainers read it on MirrorNetworking/Mirror#4128 — they went hunting a
6000.3.23f1problem whileScripts have compiler errors.sat in the same message, two lines above. Surfacing the abort reason (#288) was doing its job; the appended stderr was burying it.No fix is needed for the failure itself — that is a compile error in the PR's own code (
Assembly.GetLoadedAssemblyPath, a Unity 6.4+ API called under aUNITY_6000_0_OR_NEWERguard). This is about the message not pointing people the wrong way.The install smoke tests (
5fdab58)Added because this PR's own CI hit both problems, and they are the same shape as the one above — a check failing for a reason its message never names.
api.github.comwhile the ubuntu job passed beside it, running the sameinstall.shagainst the same endpoint in the same minute.install.shresolveslatestthrough the API and authenticates only when handed a token; unauthenticated is 60 requests/hour per IP, and Actions runners share IPs across unrelated repos and orgs. The script was written to be given one and says so in its own comment — the workflow never passed it. Both smoke steps, plus the root-wrapper steps that default tolatestand make the same call, now passGH_TOKEN(the pattern already used throughoutorchestrator-integrity.yml)."likely the latest release's binaries are still uploading". A 403 rate limit is not that, and re-running the identical unauthenticated request 30 seconds later cannot clear it — it spent all five attempts and 2.5 minutes asserting a cause it had no way to know. It now points at the error that actually occurred.install.shtreats an empty token as no token, so a fork PR (no secrets, read-onlygithub.token) degrades to today's unauthenticated behaviour rather than failing. Verified end-to-end locally:GH_TOKEN=… bash scripts/install.sh latest <dir>resolvesv0.1.69, downloads, and checksum-verifies.actionlintclean.Checklist
🤖 Generated with Claude Code