From 0a4aabec180d4d2d1f2d26f851e7f2ea3acf6a1d Mon Sep 17 00:00:00 2001 From: frostebite Date: Sun, 20 Sep 2026 20:13:00 +0100 Subject: [PATCH 1/2] 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 --- src/model/docker.test.ts | 20 +++++++++--- src/model/docker.ts | 9 +++++- .../unity/unity-batchmode-failure.test.ts | 12 +++++++ src/model/unity/unity-batchmode-failure.ts | 31 +++++++++++++++---- 4 files changed, 61 insertions(+), 11 deletions(-) diff --git a/src/model/docker.test.ts b/src/model/docker.test.ts index 5d40792..429db85 100644 --- a/src/model/docker.test.ts +++ b/src/model/docker.test.ts @@ -105,8 +105,9 @@ describe("Docker", () => { }); System.run = mock(() => Promise.reject(dockerError)); - await expect( - Docker.run("game-ci/unity-editor-stub:latest", { + let rejection: Error | undefined; + try { + await Docker.run("game-ci/unity-editor-stub:latest", { hostOS: "linux", hostPlatform: "linux", currentWorkDir: "/home/runner/work/cli/cli", @@ -117,8 +118,19 @@ describe("Docker", () => { dockerWorkspacePath: "/github/workspace", engine: "unity", runTests: true, - } as any), - ).rejects.toThrow(/Scripts have compiler errors\./); + } as any); + } catch (error: any) { + rejection = error; + } + + expect(rejection?.message).toContain('Scripts have compiler errors.'); + + // And it must not be quoted back at the reader as the cause: System.run + // already streamed this stderr live, and leading with "Unable to find + // image ... locally" under an "Original error:" heading is exactly what + // made MirrorNetworking/Mirror#4128 read as an editor failing to load + // 6000.3.23f1 rather than scripts failing to compile. + expect(rejection?.message).not.toContain('Unable to find image'); }); it("still throws the original error when there is no Unity abort reason to extract", async () => { diff --git a/src/model/docker.ts b/src/model/docker.ts index 69163ef..b45f50c 100644 --- a/src/model/docker.ts +++ b/src/model/docker.ts @@ -157,7 +157,14 @@ class Docker { // failure this command hits, and error.message alone never contains // it - see UnityBatchmodeFailure's own comment for why. Checked first // since it's the highest-value case to get right. - const batchmodeFailure = UnityBatchmodeFailure.describe(error.stdout, error.message); + // No originalMessage: this command's error text is `docker run`'s own + // stderr - pull progress, led by its benign "Unable to find image ... + // locally" status line - which System.run has already streamed live + // above. Repeating it under an "Original error:" heading asserted it + // was the cause: MirrorNetworking/Mirror#4128 read as an editor failing + // to load 6000.3.23f1 when the real cause was scripts failing to + // compile, two lines up in the same message. + const batchmodeFailure = UnityBatchmodeFailure.describe(error.stdout); if (batchmodeFailure) { throw new Error(batchmodeFailure); } diff --git a/src/model/unity/unity-batchmode-failure.test.ts b/src/model/unity/unity-batchmode-failure.test.ts index ae19931..daed930 100644 --- a/src/model/unity/unity-batchmode-failure.test.ts +++ b/src/model/unity/unity-batchmode-failure.test.ts @@ -53,6 +53,18 @@ describe('UnityBatchmodeFailure', () => { expect(described).toContain('not a\ndocker/game-ci infrastructure problem'); }); + // Regression: Docker.run passes no originalMessage, because its error + // text is `docker run`'s stderr - pull progress it has already streamed + // live. Appending that under "Original error:" made + // MirrorNetworking/Mirror#4128 read as an editor failing to load a + // version when Unity had in fact aborted on script compiler errors. + it('omits the original-error section when the caller supplies nothing to add', () => { + const described = UnityBatchmodeFailure.describe(realWorldStdout); + + expect(described).toContain('Scripts have compiler errors.'); + expect(described).not.toContain('Original error:'); + }); + it('returns undefined when there is nothing to extract, leaving the caller to fall back to the original error', () => { expect(UnityBatchmodeFailure.describe('some unrelated stdout', 'Command exited with code 1')).toBeUndefined(); expect(UnityBatchmodeFailure.describe(undefined, 'Command exited with code 1')).toBeUndefined(); diff --git a/src/model/unity/unity-batchmode-failure.ts b/src/model/unity/unity-batchmode-failure.ts index be47e1a..651d967 100644 --- a/src/model/unity/unity-batchmode-failure.ts +++ b/src/model/unity/unity-batchmode-failure.ts @@ -13,6 +13,16 @@ * run failed with exit code 1" - the real reason, "Scripts have compiler * errors.", was sitting ~1400 log lines earlier and never reached the final * error message at all. + * + * Reported live once more, from the opposite direction: the same CI then + * appended that stderr under the "Original error:" heading below, burying + * the reason it was supposed to support. Docker leads its stderr with the + * benign pre-pull "Unable to find image '...' locally" line, so the message + * read as an editor failing to load 6000.3.23f1 - two maintainers went + * looking for a version problem while "Scripts have compiler errors." sat + * two lines above it. Hence `originalMessage` is optional, and the one + * caller whose error text is already in the log - Docker, via System.run's + * live stderr passthrough - omits it. */ class UnityBatchmodeFailure { static extractReason(stdout: string | undefined): string | undefined { @@ -25,21 +35,30 @@ class UnityBatchmodeFailure { return match?.[1]?.trim() || undefined; } - /** Returns a clearer error message when `stdout` contains Unity's own abort reason, or undefined otherwise. */ - static describe(stdout: string | undefined, originalMessage: string): string | undefined { + /** + * Returns a clearer error message when `stdout` contains Unity's own abort + * reason, or undefined otherwise. + * + * `originalMessage` is the caller's own error text, appended under an + * "Original error:" heading. Omit it when that text is already in the log + * above - the reason is the useful part, and repeating a stream the reader + * has already scrolled past only buries it. + */ + static describe(stdout: string | undefined, originalMessage?: string): string | undefined { const reason = UnityBatchmodeFailure.extractReason(stdout); if (!reason) return undefined; - return String.dedent` + const message = String.dedent` Unity aborted before completing: ${reason} This is a failure inside the Unity Editor itself (commonly script compiler errors, a missing package, or a crash during startup) - not a docker/game-ci infrastructure problem. The full Unity log is above. - - Original error: - ${originalMessage} `; + + if (!originalMessage) return message; + + return `${message}\n\nOriginal error:\n${originalMessage}`; } } From 5fdab58ee8aa7aecbce2fdfda5c5a92a54954c3b Mon Sep 17 00:00:00 2001 From: frostebite Date: Sun, 20 Sep 2026 20:22:11 +0100 Subject: [PATCH 2/2] 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 resolves v0.1.69, downloads, and checksum-verifies. actionlint clean. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/tests.yml | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 047d5e3..6211476 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -190,6 +190,18 @@ jobs: # lands in that window - e.g. this repo's own version-bump commit, # created right after cutting the release it's for - hit exactly # this 404 in practice, failing Tests on main itself, not just a PR. + # + # GH_TOKEN because install.sh resolves "latest" through the GitHub + # API and only authenticates when handed one (see its own comment). + # Unauthenticated is 60 requests/hour *per IP*, and Actions runners + # share IPs across unrelated repos and orgs - so the macOS job below, + # running the same install.sh against the same endpoint seconds + # later, was 403ing on that limit while this job passed beside it. + # install.sh reads GITHUB_TOKEN or GH_TOKEN and treats an empty one + # as no token, so a fork PR (no secrets, read-only github.token) + # degrades to today's unauthenticated behaviour rather than failing. + env: + GH_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN || github.token }} run: | for attempt in 1 2 3 4 5; do # install.sh writes progress to stderr and only the final binary @@ -203,7 +215,7 @@ jobs: echo "FAIL: scripts/install.sh still failing after 5 attempts" exit 1 fi - echo "install.sh failed (attempt $attempt/5) - likely the latest release's binaries are still uploading - retrying in 30s..." + echo "install.sh failed (attempt $attempt/5) - see the error above; retrying in 30s..." sleep 30 done [ -x "$binary_path" ] || { echo "FAIL: $binary_path is not executable"; exit 1; } @@ -221,6 +233,10 @@ jobs: # above has already established that the latest release is fully # uploaded. - name: Smoke-test the root install.sh wrapper + # Same GH_TOKEN reason as the step above: the wrapper defaults to + # "latest" and resolves it through the same API call. + env: + GH_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN || github.token }} run: | export GAME_CI_INSTALL=/tmp/root-install-sh-smoke-test sh ./install.sh @@ -257,8 +273,10 @@ jobs: *) echo "FAIL: expected macOS's system bash to be 3.x, got $bash_version - this job no longer covers the bash-3.2 compatibility class it exists for"; exit 1 ;; esac - name: Smoke-test scripts/install.sh against the latest release - # Same retry rationale as the ubuntu job: "latest" can resolve to a - # release whose binaries are still uploading. + # Same retry rationale, and the same GH_TOKEN reason, as the ubuntu + # job above - this is the job the token exists to stop 403ing. + env: + GH_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN || github.token }} run: | for attempt in 1 2 3 4 5; do if binary_path="$(/bin/bash scripts/install.sh latest /tmp/install-sh-smoke-test)"; then @@ -268,13 +286,17 @@ jobs: echo "FAIL: scripts/install.sh still failing after 5 attempts" exit 1 fi - echo "install.sh failed (attempt $attempt/5) - likely the latest release's binaries are still uploading - retrying in 30s..." + echo "install.sh failed (attempt $attempt/5) - see the error above; retrying in 30s..." sleep 30 done [ -x "$binary_path" ] || { echo "FAIL: $binary_path is not executable"; exit 1; } [ -d "$(dirname "$binary_path")/dist" ] || { echo "FAIL: dist/ was not extracted next to $binary_path"; exit 1; } "$binary_path" --help - name: Smoke-test the root install.sh wrapper + # Same GH_TOKEN reason as the step above: the wrapper defaults to + # "latest" and resolves it through the same API call. + env: + GH_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN || github.token }} run: | export GAME_CI_INSTALL=/tmp/root-install-sh-smoke-test /bin/sh ./install.sh