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 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}`; } }