Cloud Runner Improvements - S3 Locking, Aws Local Stack (Pipelines), Testing Improvements, Rclone storage support, Provider plugin system #731
Conversation
- Implemented a primary attempt to pull LFS files using GIT_PRIVATE_TOKEN. - Added a fallback mechanism to use GITHUB_TOKEN if the initial attempt fails. - Configured git to replace SSH and HTTPS URLs with token-based authentication for the fallback. - Improved error handling to log specific failure messages for both token attempts. This change ensures more robust handling of LFS file retrieval in various authentication scenarios.
- Added permissions for packages, pull-requests, statuses, and id-token to enhance workflow capabilities. - This change improves the CI pipeline's ability to manage pull requests and access necessary resources.
…ation - Added configuration to use GIT_PRIVATE_TOKEN for git operations, replacing SSH and HTTPS URLs with token-based authentication. - Improved error handling to ensure GIT_PRIVATE_TOKEN availability before attempting to pull LFS files. - This change streamlines the process of pulling LFS files in environments requiring token authentication.
…entication - Enhanced the process of configuring git to use GIT_PRIVATE_TOKEN and GITHUB_TOKEN by clearing existing URL configurations before setting new ones. - Improved the clarity of the URL replacement commands for better readability and maintainability. - This change ensures a more robust setup for pulling LFS files in environments requiring token authentication.
… pipeline - Replaced instances of GITHUB_TOKEN with GIT_PRIVATE_TOKEN in the cloud-runner CI pipeline configuration. - This change ensures consistent use of token-based authentication across various jobs in the workflow, enhancing security and functionality.
…L unsetting - Modified the git configuration commands to append '|| true' to prevent errors if the specified URLs do not exist. - This change enhances the reliability of the URL clearing process in the RemoteClient class, ensuring smoother execution during token-based authentication setups.
…tion - Updated comments for clarity regarding the purpose of URL configuration changes. - Simplified the git configuration commands by removing redundant lines while maintaining functionality for HTTPS token-based authentication. - This change enhances the readability and maintainability of the RemoteClient class's git setup process.
# Conflicts: # dist/index.js # dist/index.js.map # jest.config.js # yarn.lock
… log lines for test assertions
…off; lint/format fixes
… cache key for retained workspace (#379)
…logs; tests: retained workspace AWS assertion (#381)
…rd local provider steps
…nd log management; update builder path logic based on provider strategy
…sed on provider strategy and credentials; update binary files
…ained markers; hooks: include AWS S3 hooks on aws provider
…t:ci script; fix(windows): skip grep-based version regex tests; logs: echo CACHE_KEY/retained markers; hooks: include AWS hooks on aws provider
… update binary files
…rintf for empty input
…I hangs; s3 steps pass again
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
src/model/cloud-runner/remote-client/index.ts (2)
282-282: Fix contradictory LFS folder assertion as previously flagged.The assertion checks that the LFS folder exists (
fs.existsSync()returns true), but the message states it "should not exist before caching." This contradiction was flagged in a previous review.Either invert the check to
!fs.existsSync(path.join('.git', 'lfs'))or update the message to match the actual assertion.
242-268: Fix unused catch bindings as previously flagged.This issue was already identified in a previous review. Remove unused catch bindings (
_error,_error2,_error3) and use parameterlesscatchblocks where the error is not used, or name the caught error appropriately when it is thrown.Based on past review comments, the recommended fix is to use
catchwithout parameters for unused errors, and preserve the error variable where it's re-thrown.src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
201-206: Non-containerized providers do not execute the build.The non-containerized code path never copies the build scripts or executes
/entrypoint.sh, so no actual Unity build occurs. The workflow only logs markers and runs post-build tasks against non-existent build outputs.This confirms the issue flagged in the previous review: "Local provider no longer executes build entrypoint... local runs will immediately log success without invoking any build logic, so local provider builds will silently do nothing."
The non-containerized path should stage and execute the build scripts similar to the containerized paths. Apply this structure:
return ` echo "game ci start" echo "game ci start" >> "$LOG_FILE" + mkdir -p ${`${CloudRunnerFolders.ToLinuxFolder(CloudRunnerFolders.projectBuildFolderAbsolute)}/build`} + cd ${CloudRunnerFolders.ToLinuxFolder(CloudRunnerFolders.projectPathAbsolute)} + cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(distFolder, 'default-build-script'))}" "/UnityBuilderAction" + cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(ubuntuPlatformsFolder, 'entrypoint.sh'))}" "/entrypoint.sh" + cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(ubuntuPlatformsFolder, 'steps'))}" "/steps" + chmod -R +x "/entrypoint.sh" + chmod -R +x "/steps" + /entrypoint.sh 2>&1 | tee -a "$LOG_FILE" & timeout 3s node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" || true node ${builderPath} -m remote-cli-post-build`;
🧹 Nitpick comments (2)
src/model/cloud-runner/remote-client/index.ts (1)
128-134: Consider consolidating logging calls.The success message is logged three separate ways (CloudRunnerLogger, stdout, console). If CloudRunnerLogger already outputs to stdout/console, this may be redundant.
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
95-106: Consider preserving git error output for troubleshooting.The multi-tier fallback strategy (checking remote branch, then falling back to known branches) is well-designed. However, redirecting stderr to
/dev/nullon line 98 may hide authentication failures or network issues that would help diagnose clone problems.Consider logging git errors while still allowing the fallback chain to proceed:
-if [ -n "$(git ls-remote --heads \"$REPO\" \"$BRANCH\" 2>/dev/null)" ]; then +if git ls-remote --heads "$REPO" "$BRANCH" 2>&1 | grep -q "refs/heads/$BRANCH"; then git clone -q -b "$BRANCH" "$REPO" "$DEST" else echo "Remote branch $BRANCH not found in $REPO; falling back to a known branch" - git clone -q -b cloud-runner-develop "$REPO" "$DEST" \ - || git clone -q -b main "$REPO" "$DEST" \ - || git clone -q "$REPO" "$DEST" + git clone -q -b cloud-runner-develop "$REPO" "$DEST" 2>&1 \ + || git clone -q -b main "$REPO" "$DEST" 2>&1 \ + || git clone -q "$REPO" "$DEST" 2>&1 \ + || { echo "ERROR: All git clone attempts failed for $REPO"; exit 1; } fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/model/cloud-runner/remote-client/index.ts(3 hunks)src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/build-automation-workflow.ts
🧬 Code graph analysis (1)
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
src/model/index.ts (1)
CloudRunner(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: WebGL on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: iOS on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: iOS on 2021.3.45f1
🔇 Additional comments (9)
src/model/cloud-runner/remote-client/index.ts (4)
66-68: LGTM: Enhanced observability for cache key.The explicit cache key logging aids debugging and test assertions.
70-89: LGTM: Defensive cache push guards.The existence and content checks prevent pushing empty or missing directories, improving robustness.
91-110: LGTM: Consistent defensive pattern.Build cache push uses the same guarded pattern as Library cache, maintaining consistency.
113-120: LGTM: Safe cleanup guard.Checking both path variants before cleanup prevents spurious errors.
src/model/cloud-runner/workflows/build-automation-workflow.ts (5)
53-60: Container-aware path resolution looks correct.The
isContainerizedflag appropriately distinguishes between containerized providers (aws,k8s,local-docker) and non-containerized ones, with thebuilderPathcorrectly resolving to absolute paths for containers andprocess.cwd()for local execution.
64-82: Conditional environment setup is well-structured.The workflow correctly skips package installation for
local-docker(where the Docker image provides tooling) and non-containerized providers, while maintaining appropriate paths for workspace and log files based on the execution context.
189-198: Containerized build execution is correct.The non-local-docker containerized path properly stages the Unity build scripts, executes
/entrypoint.sh, and streams logs through the remote CLI. The build logic is intact for these providers.
150-161: Git LFS handling is appropriately defensive.The conditional Git LFS setup correctly checks for a
.gitdirectory before attempting LFS operations and gracefully handles environments where Git LFS is unavailable or unnecessary. The error tolerance (|| true) is appropriate since LFS is optional for many repositories.
182-185: Cache mirroring supports testing workflows.The cache mirroring from
/data/cacheback into the workspace enables test assertions to validate cache behavior in local-docker runs. The error tolerance is appropriate since this is a diagnostic aid rather than a critical build step.
|
|
||
| return `export GIT_DISCOVERY_ACROSS_FILESYSTEM=1 | ||
| ${cloneBuilderCommands} | ||
| echo "log start" >> /home/job-log.txt |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=ts 'LOG_FILE|job-log\.txt' src/model/cloud-runner/workflows/build-automation-workflow.tsRepository: game-ci/unity-builder
Length of output: 1146
🏁 Script executed:
sed -n '75,90p' src/model/cloud-runner/workflows/build-automation-workflow.tsRepository: game-ci/unity-builder
Length of output: 1110
🏁 Script executed:
sed -n '108,126p' src/model/cloud-runner/workflows/build-automation-workflow.tsRepository: game-ci/unity-builder
Length of output: 923
🏁 Script executed:
sed -n '165,175p' src/model/cloud-runner/workflows/build-automation-workflow.tsRepository: game-ci/unity-builder
Length of output: 1335
🏁 Script executed:
sed -n '190,210p' src/model/cloud-runner/workflows/build-automation-workflow.tsRepository: game-ci/unity-builder
Length of output: 1354
Replace hardcoded log path with $LOG_FILE variable for consistency.
Line 119 should use $LOG_FILE instead of hardcoding /home/job-log.txt. Although the hardcoded path matches the containerized value set on line 82, the variable is already defined and should be used consistently throughout the script, as seen in lines 130, 204, and 205.
🤖 Prompt for AI Agents
In src/model/cloud-runner/workflows/build-automation-workflow.ts around line
119, the script echoes to the hardcoded path "/home/job-log.txt" instead of
using the existing $LOG_FILE variable; change that echo command to append to
"$LOG_FILE" so the script consistently uses the configured log path (matching
usage on lines 82, 130, 204, 205).
| return `export GIT_DISCOVERY_ACROSS_FILESYSTEM=1 | ||
| mkdir -p "$(dirname "$LOG_FILE")" | ||
| echo "log start" >> "$LOG_FILE" | ||
| echo "CACHE_KEY=$CACHE_KEY"`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/model/cloud-runner/workflows && wc -l build-automation-workflow.tsRepository: game-ci/unity-builder
Length of output: 97
🏁 Script executed:
cat -n src/model/cloud-runner/workflows/build-automation-workflow.ts | sed -n '80,140p'Repository: game-ci/unity-builder
Length of output: 3331
🏁 Script executed:
# Search for how builderPath is used
rg -n 'builderPath' src/model/cloud-runner/workflows/build-automation-workflow.ts -A 3 -B 1Repository: game-ci/unity-builder
Length of output: 3882
🏁 Script executed:
# Search for non-containerized provider strategy usage
rg -n 'providerStrategy.*local' src/model/cloud-runner/ -t ts --type-add='ts:*.ts' -C 3Repository: game-ci/unity-builder
Length of output: 11083
🏁 Script executed:
# Look for initialization of builder in non-containerized scenarios
rg -n 'isContainerized\s*=\s*false|!isContainerized' src/model/cloud-runner/ -t ts --type-add='ts:*.ts' -C 5Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
# Search for how "local" provider is configured and whether builder is pre-installed
rg -n "providerStrategy.*=.*['\"]local['\"]" src/model/cloud-runner/ -t ts --type-add='ts:*.ts' -B 5 -A 10Repository: game-ci/unity-builder
Length of output: 3008
🏁 Script executed:
# Look for any documentation or setup instructions for local provider
fd -e md -e txt | xargs grep -l "local provider\|local strategy" 2>/dev/null | head -5Repository: game-ci/unity-builder
Length of output: 107
🏁 Script executed:
# Search for how the builder is expected to exist before setupCommands is called
rg -n "process\.cwd\(\)" src/model/cloud-runner/ -t ts --type-add='ts:*.ts' -B 3 -A 3Repository: game-ci/unity-builder
Length of output: 6027
🏁 Script executed:
# Check if there's any initialization for non-containerized paths
rg -n "dist.*index\.js\|builderPath" src/model/cloud-runner/ -t ts --type-add='ts:*.ts' -B 2 -A 2 | head -80Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
cat -n src/model/cloud-runner/providers/README.mdRepository: game-ci/unity-builder
Length of output: 8318
🏁 Script executed:
# Look for local provider implementation and initialization
fd "local" src/model/cloud-runner/providers/ -t f | head -10Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
# Search for where the builder dist folder might be copied/prepared for local provider
rg -n "local.*provider\|LocalProvider" src/model/cloud-runner/ -t ts --type-add='ts:*.ts' -B 3 -A 10 | head -100Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
# Search for where the builder might be built/compiled before running in non-containerized mode
rg -n "npm run build\|npm run package\|dist/" src/ -t ts --type-add='ts:*.ts' | grep -E "local|setup|build" | head -40Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
# Check if there's a build step that creates dist/index.js before running workflows
rg -n "compile\|package\|build" src/index.ts src/main.ts -B 3 -A 3Repository: game-ci/unity-builder
Length of output: 116
🏁 Script executed:
# Look at how the workflow is actually executed - check the main entry point
fd "index.ts|main.ts" src/ -type f | head -5Repository: game-ci/unity-builder
Length of output: 235
🏁 Script executed:
# Find package.json to understand entry point and build setup
cat package.json | grep -A 5 -B 5 '"main"\|"bin"\|"dist"'Repository: game-ci/unity-builder
Length of output: 379
🏁 Script executed:
# Check if dist/index.js is pre-built or built at runtime
ls -la dist/ 2>/dev/null | head -20Repository: game-ci/unity-builder
Length of output: 867
🏁 Script executed:
# Look at the root index.ts or entry point
git ls-files | grep -E "^(src/)?index\.(ts|js)$|^main\.(ts|js)$"Repository: game-ci/unity-builder
Length of output: 77
🏁 Script executed:
# Check the actual structure of src directory
git ls-files src/ | head -20Repository: game-ci/unity-builder
Length of output: 666
🏁 Script executed:
# Check how local and local-system providers are actually used
rg -n "local-system\|providerStrategy.*local" src/model/cloud-runner/ -t ts --type-add='ts:*.ts' -B 2 -A 5 | head -80Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
# Verify if there's any documentation about how non-containerized mode works
fd -e md | xargs grep -l "local.*provider\|non-containerized" 2>/dev/nullRepository: game-ci/unity-builder
Length of output: 107
🏁 Script executed:
# Check the git history or comments around the setupCommands method
git log --oneline -n 20 -- src/model/cloud-runner/workflows/build-automation-workflow.ts 2>/dev/null | head -10Repository: game-ci/unity-builder
Length of output: 84
🏁 Script executed:
# Check if there are any comments or documentation about why non-containerized doesn't clone
rg -n "non-containerized\|isContainerized" src/model/cloud-runner/workflows/build-automation-workflow.ts -B 5 -A 2 | grep -E "//|/\*"Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
# Verify that node ${builderPath} commands are actually used in both paths
rg -n "node \${builderPath}" src/model/cloud-runner/workflows/build-automation-workflow.tsRepository: game-ci/unity-builder
Length of output: 173
🏁 Script executed:
# Search for node command with builderPath
rg -n 'node.*builderPath' src/model/cloud-runner/workflows/build-automation-workflow.tsRepository: game-ci/unity-builder
Length of output: 837
Non-containerized setup doesn't clone the builder repository while assuming it exists downstream.
The non-containerized code path (lines 128–131) only initializes logging but never clones the Unity builder repository. Since builderPath for non-containerized mode points to process.cwd()/dist/index.js, subsequent node ${builderPath} commands (lines 205–206) will fail unless the builder is already present in the working directory. This creates a critical inconsistency: containerized providers clone the builder (lines 108–126), while non-containerized providers do not, yet both expect the builder to exist.
Clarify whether non-containerized providers are expected to have the builder pre-installed, and if so, document this requirement. Otherwise, the non-containerized path should include clone logic similar to the containerized path.
| if ! command -v node > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/node && chmod +x /usr/local/bin/node; fi | ||
| if ! command -v npm > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/npm && chmod +x /usr/local/bin/npm; fi | ||
| if ! command -v n > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/n && chmod +x /usr/local/bin/n; fi | ||
| if ! command -v yarn > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/yarn && chmod +x /usr/local/bin/yarn; fi |
There was a problem hiding this comment.
Stub binaries may mask missing tool dependencies.
Creating no-op stubs for node, npm, n, and yarn when they're missing (lines 167-170) will cause any scripts that genuinely require these tools to silently fail. If the local-docker image is expected to provide these tools, their absence likely indicates a misconfiguration that should be diagnosed rather than hidden.
Consider at minimum logging a warning before creating stubs:
+echo "WARNING: Creating stub for missing 'node' command - build may fail if node is required"
if ! command -v node > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/node && chmod +x /usr/local/bin/node; fi
+echo "WARNING: Creating stub for missing 'npm' command - build may fail if npm is required"
if ! command -v npm > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/npm && chmod +x /usr/local/bin/npm; fi
+echo "WARNING: Creating stub for missing 'n' command - build may fail if n is required"
if ! command -v n > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/n && chmod +x /usr/local/bin/n; fi
+echo "WARNING: Creating stub for missing 'yarn' command - build may fail if yarn is required"
if ! command -v yarn > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/yarn && chmod +x /usr/local/bin/yarn; fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! command -v node > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/node && chmod +x /usr/local/bin/node; fi | |
| if ! command -v npm > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/npm && chmod +x /usr/local/bin/npm; fi | |
| if ! command -v n > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/n && chmod +x /usr/local/bin/n; fi | |
| if ! command -v yarn > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/yarn && chmod +x /usr/local/bin/yarn; fi | |
| echo "WARNING: Creating stub for missing 'node' command - build may fail if node is required" | |
| if ! command -v node > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/node && chmod +x /usr/local/bin/node; fi | |
| echo "WARNING: Creating stub for missing 'npm' command - build may fail if npm is required" | |
| if ! command -v npm > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/npm && chmod +x /usr/local/bin/npm; fi | |
| echo "WARNING: Creating stub for missing 'n' command - build may fail if n is required" | |
| if ! command -v n > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/n && chmod +x /usr/local/bin/n; fi | |
| echo "WARNING: Creating stub for missing 'yarn' command - build may fail if yarn is required" | |
| if ! command -v yarn > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/yarn && chmod +x /usr/local/bin/yarn; fi |
🤖 Prompt for AI Agents
In src/model/cloud-runner/workflows/build-automation-workflow.ts around lines
167-170, the current behavior silently creates no-op stubs for node/npm/n/yarn
when those binaries are missing; change this to avoid masking missing tool
dependencies by (1) removing the silent no-op replacements and instead emitting
a clear warning/error that the expected tool is absent (log to stderr or your
workflow logger with the binary name and suggested fix), and (2) if stubs are
still required for a specific compatibility case, only create them after logging
the warning and documenting the condition in code comments; ensure the workflow
exits with a non-zero status or fails the job when a genuine runtime dependency
is missing unless an explicit opt-in flag allows stubbing.
| tar -cf "/data/cache/$CACHE_KEY/build/build-$BUILD_GUID.tar" --files-from /dev/null || touch "/data/cache/$CACHE_KEY/build/build-$BUILD_GUID.tar" | ||
| fi | ||
| # Run post-build tasks - ensure output is captured even if command fails | ||
| node ${builderPath} -m remote-cli-post-build || echo "Post-build command completed with warnings" |
There was a problem hiding this comment.
Post-build error suppression may hide critical failures.
The || echo "Post-build command completed with warnings" pattern (line 180) converts all post-build failures into a success state with a misleading message. If remote-cli-post-build fails while uploading artifacts or persisting cache, the build will appear to succeed despite incomplete outputs.
Consider either:
- Propagating the failure if post-build tasks are critical, or
- Logging the actual error and exit code:
-node ${builderPath} -m remote-cli-post-build || echo "Post-build command completed with warnings"
+node ${builderPath} -m remote-cli-post-build || { EXIT_CODE=$?; echo "WARNING: Post-build command failed with exit code $EXIT_CODE"; exit $EXIT_CODE; }Or if post-build failures are truly non-critical:
-node ${builderPath} -m remote-cli-post-build || echo "Post-build command completed with warnings"
+node ${builderPath} -m remote-cli-post-build || { EXIT_CODE=$?; echo "WARNING: Post-build command failed with exit code $EXIT_CODE, continuing anyway"; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| node ${builderPath} -m remote-cli-post-build || echo "Post-build command completed with warnings" | |
| node ${builderPath} -m remote-cli-post-build || { EXIT_CODE=$?; echo "WARNING: Post-build command failed with exit code $EXIT_CODE"; exit $EXIT_CODE; } |
🤖 Prompt for AI Agents
In src/model/cloud-runner/workflows/build-automation-workflow.ts around line
180, the shell line `node ${builderPath} -m remote-cli-post-build || echo
"Post-build command completed with warnings"` hides failures; change it to
either let the command fail (remove the `||` fallback) if post-build steps are
critical, or capture the exit code and log the error and exit code before
exiting non-zero (e.g., run the command, check `$?`, log context and exit with
that code), or — only if truly non-critical — replace the echo with a detailed
warning that includes the captured exit code and stderr output so failures are
visible for debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/cloud-runner-integrity.yml (4)
56-59: Use region-aware S3 API for bucket creation.The past review suggested using
s3api create-bucketwithLocationConstraintfor non-us-east-1 regions, which was accepted but not applied. S3 requires this constraint outside us-east-1.- name: Create S3 bucket for tests (host LocalStack) run: | - awslocal s3 mb s3://$AWS_STACK_NAME || true + if [ "${AWS_REGION:-us-east-1}" = "us-east-1" ]; then + awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" || true + else + awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" \ + --create-bucket-configuration LocationConstraint="${AWS_REGION:-us-east-1}" || true + fi awslocal s3 ls
162-163: Stop overriding GITHUB_TOKEN with GIT_PRIVATE_TOKEN.The workflow overrides
GITHUB_TOKENwithGIT_PRIVATE_TOKENacross 13 test steps. This violates least-privilege by exposing a broader-scope PAT and breaks the intended fallback logic (try private token first, then fall back to runner token).Use the default runner-provided token and keep
GIT_PRIVATE_TOKENas a separate variable only if private repo access is truly needed:GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} - GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} + GITHUB_TOKEN: ${{ github.token }}Apply this fix to all 13 test steps: lines 162–163 (k8s), 202–203 (k8s), 242–243 (k8s), and lines 294–295, 318–319, 342–343, 366–367, 390–391, 414–415, 438–439, 462–463, 486–487, 510–511 (localstack job).
288-295: Add LocalStack-specific environment variables to all localstack job test steps.The k8s job correctly includes
AWS_S3_FORCE_PATH_STYLE: 'true'andAWS_EC2_METADATA_DISABLED: 'true'for LocalStack compatibility, but the localstack job omits these across all 10 test steps. This creates inconsistent behavior and may cause unexpected LocalStack interactions.Add these variables to every test step in the localstack job. For example, lines 288–295:
AWS_ENDPOINT: http://localhost:4566 AWS_ENDPOINT_URL: http://localhost:4566 + AWS_S3_FORCE_PATH_STYLE: 'true' + AWS_EC2_METADATA_DISABLED: 'true' GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}Apply the same additions to all other test steps: cloud-runner-end2end-caching, cloud-runner-end2end-retaining, cloud-runner-caching, cloud-runner-environment, cloud-runner-image, cloud-runner-hooks, cloud-runner-local-persistence, cloud-runner-locking-core, and cloud-runner-locking-get-locked.
40-43: Pin k3d version to avoid supply-chain risk.The code installs k3d from the main branch without version pinning, which creates a supply-chain risk. Download a specific k3d release directly with version pinning instead.
- name: Install k3d run: | - curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash + set -euo pipefail + K3D_VERSION=v5.7.3 + curl -fsSL -o /usr/local/bin/k3d "https://github.com/k3d-io/k3d/releases/download/${K3D_VERSION}/k3d-linux-amd64" + chmod +x /usr/local/bin/k3d k3d version | cat
🧹 Nitpick comments (1)
.github/workflows/cloud-runner-integrity.yml (1)
512-553: Update commented AWS job to use aws-actions/configure-aws-credentials@v4.The commented AWS job references
aws-actions/configure-aws-credentials@v1(line 529), which is deprecated and will fail on current runners. If this job is intended for future use, update to v4:- name: Configure AWS Credentials - uses: aws-actions/configure-aws-credentials@v1 + uses: aws-actions/configure-aws-credentials@v4 with: aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} aws-region: ${{ env.AWS_REGION }}If this job will remain unused, consider removing the block entirely to reduce maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cloud-runner-integrity.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: Tests
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: WebGL on 2023.2.2f1
- GitHub Check: WebGL on 6000.0.36f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: iOS on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/model/cloud-runner/remote-client/index.ts (4)
288-289: Align LFS folder assertion with its message.The assertion currently checks that
.git/lfsexists while the message says “LFS folder should not exist before caching”, which is contradictory:assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');Either invert the condition or update the message; based on the surrounding flow (clone without LFS and only then pull from cache), the intent seems to be to assert non-existence:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(!fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');
248-285: Fix unused catch bindings and improve checkout fallback clarity.The new SHA/branch/PR fetch and checkout flow is good, but the catch bindings (
_error,_error2,_error3, later_error) are either unused or only rethrown, which is what ESLint had already flagged in earlier reviews. You can simplify the try/catch structure and remove unused bindings while preserving semantics:- // Ensure refs exist (tags and PR refs) - await CloudRunnerSystem.Run(`git fetch --all --tags || true`); + // Ensure refs exist (tags and PR refs) + await CloudRunnerSystem.Run(`git fetch --all --tags || true`); ... - if (targetSha) { - try { - await CloudRunnerSystem.Run(`git checkout ${targetSha}`); - } catch (_error) { - try { - await CloudRunnerSystem.Run(`git fetch origin ${targetSha} || true`); - await CloudRunnerSystem.Run(`git checkout ${targetSha}`); - } catch (_error2) { - RemoteClientLogger.logWarning(`Falling back to branch checkout; SHA not found: ${targetSha}`); - try { - await CloudRunnerSystem.Run(`git checkout ${targetBranch}`); - } catch (_error3) { - if ((targetBranch || '').startsWith('pull/')) { - await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); - } else { - throw _error2; - } - } - } - } - } else { - try { - await CloudRunnerSystem.Run(`git checkout ${targetBranch}`); - } catch (_error) { - if ((targetBranch || '').startsWith('pull/')) { - await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); - } else { - throw _error; - } - } - RemoteClientLogger.log(`buildParameter Git Sha is empty`); - } + if (targetSha) { + try { + await CloudRunnerSystem.Run(`git checkout ${targetSha}`); + } catch { + try { + await CloudRunnerSystem.Run(`git fetch origin ${targetSha} || true`); + await CloudRunnerSystem.Run(`git checkout ${targetSha}`); + } catch (error) { + RemoteClientLogger.logWarning(`Falling back to branch checkout; SHA not found: ${targetSha}`); + try { + await CloudRunnerSystem.Run(`git checkout ${targetBranch}`); + } catch { + if ((targetBranch || '').startsWith('pull/')) { + await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); + } else { + throw error; + } + } + } + } + } else { + try { + await CloudRunnerSystem.Run(`git checkout ${targetBranch}`); + } catch (error) { + if ((targetBranch || '').startsWith('pull/')) { + await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); + } else { + throw error; + } + } + RemoteClientLogger.log(`buildParameter Git Sha is empty`); + }Also, to satisfy
lines-around-comment, consider adding a blank line before the// Ensure refs exist...comment (after the preceding log call).
309-371: LFS pull strategies never fall back due tosuppressErrorand no-op catch.In
pullLatestLFS, the multi-strategy LFS pull is conceptually great (plain pull →GIT_PRIVATE_TOKEN→GITHUB_TOKEN→ warning), but as written it won’t actually fall back:
- All
git lfs pullcalls useCloudRunnerSystem.Run(..., true), which setssuppressError = true, so non‑zero exit codes do not throw.- The first
tryblock always logs success andreturns, even ifgit lfs pullfailed.- The
catch (_error) { void 0; }is effectively unreachable for LFS failures and still trips ESLint (unused binding +void 0no‑op).This means token-based strategies and the final warning will never be exercised on LFS failures.
You can fix both correctness and style with something like:
await CloudRunnerSystem.Run(`git config --global filter.lfs.process "git-lfs filter-process"`); if (CloudRunner.buildParameters.skipLfs) { RemoteClientLogger.log(`Skipping LFS pull (skipLfs=true)`); return; } - // Best effort: try plain pull first (works for public repos or pre-configured auth) - try { - await CloudRunnerSystem.Run(`git lfs pull`, true); - await CloudRunnerSystem.Run(`git lfs checkout || true`, true); - RemoteClientLogger.log(`Pulled LFS files without explicit token configuration`); - - return; - } catch (_error) { - /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; - } + // Best effort: try plain pull first (works for public repos or pre-configured auth) + try { + await CloudRunnerSystem.Run(`git lfs pull`); + await CloudRunnerSystem.Run(`git lfs checkout || true`, true); + RemoteClientLogger.log(`Pulled LFS files without explicit token configuration`); + + return; + } catch { + // no-op here: fall through to token-based strategies + } ... - // Try with GIT_PRIVATE_TOKEN - try { + // Try with GIT_PRIVATE_TOKEN + try { const gitPrivateToken = process.env.GIT_PRIVATE_TOKEN; if (gitPrivateToken) { RemoteClientLogger.log(`Attempting to pull LFS files with GIT_PRIVATE_TOKEN...`); ... - await CloudRunnerSystem.Run(`git lfs pull`, true); - await CloudRunnerSystem.Run(`git lfs checkout || true`, true); + await CloudRunnerSystem.Run(`git lfs pull`); + await CloudRunnerSystem.Run(`git lfs checkout || true`, true); ... - // Try with GITHUB_TOKEN - try { + // Try with GITHUB_TOKEN + try { const githubToken = process.env.GITHUB_TOKEN; if (githubToken) { ... - await CloudRunnerSystem.Run(`git lfs pull`, true); - await CloudRunnerSystem.Run(`git lfs checkout || true`, true); + await CloudRunnerSystem.Run(`git lfs pull`); + await CloudRunnerSystem.Run(`git lfs checkout || true`, true);This way, failures in each strategy actually trigger the next path and, if all fail, you hit the final warning log and continue without crashing.
377-408: Retained workspace SHA/branch fallback is good; clean up catch bindings.The retained-workspace flow now:
- Logs the cache key (helpful).
- Fetches tags and PR refs.
- Does
reset --hardandcheckoutby SHA, with branch andorigin/branchfallbacks.Functionally this is solid, but the catch bindings repeat the earlier ESLint issues:
try { await CloudRunnerSystem.Run(`git reset --hard "${sha}"`); await CloudRunnerSystem.Run(`git checkout ${sha}`); } catch (_error) { RemoteClientLogger.logWarning(...); try { await CloudRunnerSystem.Run(`git checkout ${branch}`); } catch (_error2) { if ((branch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${branch}`); } else { throw _error2; } } }You can remove unused bindings and keep the rethrow behavior:
- try { - await CloudRunnerSystem.Run(`git reset --hard "${sha}"`); - await CloudRunnerSystem.Run(`git checkout ${sha}`); - } catch (_error) { + try { + await CloudRunnerSystem.Run(`git reset --hard "${sha}"`); + await CloudRunnerSystem.Run(`git checkout ${sha}`); + } catch { RemoteClientLogger.logWarning(`Retained workspace: SHA not found, falling back to branch ${branch}`); - try { - await CloudRunnerSystem.Run(`git checkout ${branch}`); - } catch (_error2) { - if ((branch || '').startsWith('pull/')) { - await CloudRunnerSystem.Run(`git checkout origin/${branch}`); - } else { - throw _error2; - } - } + try { + await CloudRunnerSystem.Run(`git checkout ${branch}`); + } catch (error) { + if ((branch || '').startsWith('pull/')) { + await CloudRunnerSystem.Run(`git checkout origin/${branch}`); + } else { + throw error; + } + } }
🧹 Nitpick comments (2)
src/model/cloud-runner/remote-client/index.ts (2)
44-52: Ensure log file writes include newlines in k8s path.In the k8s branch,
fs.appendFileSync(logFile, element);(and the lingering line write) omit a newline, so the log file can end up with concatenated lines, unlike the stdout/logging behavior.Consider appending
\nto keep the file line-oriented and consistent:- if (CloudRunnerOptions.providerStrategy === 'k8s') { - fs.appendFileSync(logFile, element); - // Write to stdout so kubectl logs can capture it - process.stdout.write(`${element}\n`); + if (CloudRunnerOptions.providerStrategy === 'k8s') { + fs.appendFileSync(logFile, `${element}\n`); + // Write to stdout so kubectl logs can capture it + process.stdout.write(`${element}\n`); ... - if (CloudRunnerOptions.providerStrategy === 'k8s') { - if (lingeringLine) { - fs.appendFileSync(logFile, lingeringLine); - process.stdout.write(`${lingeringLine}\n`); - } + if (CloudRunnerOptions.providerStrategy === 'k8s') { + if (lingeringLine) { + fs.appendFileSync(logFile, `${lingeringLine}\n`); + process.stdout.write(`${lingeringLine}\n`); + }Also applies to: 57-66
72-127: Post-build cache guards look good; minor robustness tweaks optional.The new guards for Library/Build cache pushes and the retained-workspace cleanup check are solid: they avoid throwing on missing/empty folders and log why cache writes are skipped. Logging the cache key is also helpful for debugging.
If you want to harden this further (optional), you could:
- Include
libraryFolderHost/buildFolderHostin the skip messages to aid remote debugging.- Normalize the cache destination paths via
CloudRunnerFolders.libraryCacheFolderFullrather than manual/Libraryand/buildsuffixes, to centralize path logic.No blocking issues from this block; behaviorally this is an improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (1)
src/model/cloud-runner/remote-client/index.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/model/cloud-runner/remote-client/index.ts (8)
src/model/cloud-runner/remote-client/remote-client-logger.ts (1)
RemoteClientLogger(7-93)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)src/model/cloud-runner/remote-client/caching.ts (1)
Caching(15-290)src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)src/model/input.ts (2)
gitPrivateToken(202-204)branch(56-64)src/model/github.ts (1)
sha(28-30)src/model/cloud-runner/options/cloud-runner-options.ts (1)
branch(88-98)src/model/versioning.ts (1)
branch(19-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
🔇 Additional comments (1)
src/model/cloud-runner/remote-client/index.ts (1)
134-141: Activation success signaling is clear and k8s-friendly.Writing
"Activation successful"to bothstdoutandCloudRunnerLoggeraligns with the k8s log-streaming behavior and test assertions.No changes requested here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/model/cloud-runner/workflows/build-automation-workflow.ts (5)
81-83: Use$LOG_FILEconsistently instead of hardcoded/home/job-log.txtYou set
LOG_FILEat line 82, but several places still hardcode/home/job-log.txt(log start in setup,game ci startlogs, and bothremote-cli-log-streaminvocations). This splits log writes between two paths and makes changing the log location harder. It would be cleaner and less error‑prone to route all log writes and log‑stream invocations through$LOG_FILE(and ensure its directory exists in the containerized branch as you already do in the non‑container branch).Also applies to: 117-121, 171-171, 197-199, 207-208
90-132: Non‑containerized setup assumes a prebuilt localdist/index.jswithout validationIn
setupCommands, the clone/fallback logic only runs whenisContainerizedis true; the non‑container branch just sets up logging. Combined withbuilderPath = process.cwd()/dist/index.jsfor non‑container runs, this assumes the builder has already been built and is present in the current working directory. If that invariant isn’t strictly guaranteed (e.g., forlocal/local-systemor future non‑container providers),node ${builderPath}calls will fail late and opaquely. Either reuse the clone logic (or a variant) for non‑container providers, or at least validate that${builderPath}exists and fail fast with a clear message / documented requirement.
162-170: Stubnode/npm/n/yarnbinaries can hide real dependency issuesCreating no‑op stubs for
node,npm,n, andyarnwhen they’re missing will cause any step that truly depends on these tools (includingnode ${builderPath} ...) to silently do nothing yet still “succeed”. If thelocal-dockerimage is misconfigured and lacks these tools, that’s likely something you want to surface, not mask. Prefer failing fast with a clear error, or at minimum log a loud warning and gate stub creation behind an explicit opt‑in flag intended only for specific test scenarios.
179-180: Post‑build error handling currently masksremote-cli-post-buildfailuresBoth containerized branches treat
remote-cli-post-buildas best‑effort: in the local‑docker path via|| echo "Post-build command completed with warnings"and in the standard containerized path via... || true. Ifremote-cli-post-buildis responsible for artifact upload, cache persistence, or other critical steps, these constructs will make the overall job succeed despite incomplete outputs. Consider capturing the exit code and either:
- Failing the job when post‑build is critical, or
- Logging a detailed warning with the exit code (and ideally stderr) while explicitly documenting that post‑build is optional in these modes.
This keeps behavior intentional and debuggable instead of silently swallowing real failures.
Also applies to: 198-199
205-209: Non‑containerizedBuildCommandsno longer stages or runs the build entrypointIn the non‑container branch you now only:
- Echo
"game ci start",- Run
remote-cli-log-streamfor 3 seconds, and- Invoke
remote-cli-post-build.There’s no longer any staging of the default build script or
/entrypoint.sh, so local/non‑container providers won’t execute actual build steps via this workflow and may appear to “succeed” while doing nothing. If the intention is still to drive the Unity build from here, you likely need to reintroduce the entrypoint/steps setup and execution similar to the containerized path, or otherwise clearly document that the non‑container provider is expected to handle the build entirely outside this workflow.
🧹 Nitpick comments (1)
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
53-61: MakeisContainerizedderivation future‑proof for new provider strategiesHard‑coding the containerized modes to
'aws' | 'k8s' | 'local-docker'means any new or plugin‑backed provider that still runs inside a container will silently fall into the non‑container path (differentbuilderPath, no apt/tooling, different logging). Consider centralizing this decision (e.g., via a provider capability flag or helper likeCloudRunner.isContainerizedStrategy(providerStrategy)) so new strategies don’t have to remember to update this switch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (1)
src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/build-automation-workflow.ts
🧬 Code graph analysis (1)
src/model/cloud-runner/workflows/build-automation-workflow.ts (2)
src/model/index.ts (1)
CloudRunner(27-27)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/model/cloud-runner/remote-client/index.ts (3)
265-305: Unused catch bindings and contradictory LFS assertion still present.The issues flagged in previous reviews remain unaddressed:
- Line 275:
_erroris unused — use parameterlesscatch- Line 279:
_error2should be renamed toerrorper ESLint conventions- Line 283:
_error3is unused — use parameterlesscatch- Line 305: The assertion message says "should not exist" but the code asserts the folder does exist
- } catch (_error) { + } catch { try { await CloudRunnerSystem.Run(`git fetch origin ${targetSha} || true`); await CloudRunnerSystem.Run(`git checkout ${targetSha}`); - } catch (_error2) { + } catch (error) { RemoteClientLogger.logWarning(`Falling back to branch checkout; SHA not found: ${targetSha}`); try { await CloudRunnerSystem.Run(`git checkout ${targetBranch}`); - } catch (_error3) { + } catch { if ((targetBranch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); } else { - throw _error2; + throw error; } } } }And fix the assertion:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(!fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');
339-342: Unused catch binding and unconventional no-op still present.The previous review flagged this: the catch binding
_errorat line 339 is unused, andvoid 0;is an unconventional way to express a no-op.- } catch (_error) { - /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; - } + } catch { + // no-op: best-effort git lfs pull without tokens may fail + }
411-425: Unused catch bindings in retained workspace logic still present.The previous review flagged these issues:
- Line 414:
_erroris unused — use parameterlesscatch- Line 418:
_error2should be renamed toerrorper ESLint conventionstry { await CloudRunnerSystem.Run(`git reset --hard "${sha}"`); await CloudRunnerSystem.Run(`git checkout ${sha}`); - } catch (_error) { + } catch { RemoteClientLogger.logWarning(`Retained workspace: SHA not found, falling back to branch ${branch}`); try { await CloudRunnerSystem.Run(`git checkout ${branch}`); - } catch (_error2) { + } catch (error) { if ((branch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${branch}`); } else { - throw _error2; + throw error; } } }
🧹 Nitpick comments (2)
src/model/cloud-runner/remote-client/index.ts (2)
34-38: Misleading comment:setDefaultEncodingdoesn't control buffering.The comment states this ensures stdout is "unbuffered," but
setDefaultEncodingonly sets the character encoding for write operations. Node.js stdout buffering is controlled by the stream's internal state and cannot be disabled this way. The encoding setting itself is harmless but the comment is incorrect.Consider updating the comment to reflect what the code actually does:
- // For K8s, ensure stdout is unbuffered so messages are captured immediately - if (CloudRunnerOptions.providerStrategy === 'k8s') { - process.stdout.setDefaultEncoding('utf8'); - } + // For K8s, ensure consistent UTF-8 encoding for stdout + if (CloudRunnerOptions.providerStrategy === 'k8s') { + process.stdout.setDefaultEncoding('utf8'); + }
152-155: Ineffective flush attempt.Writing an empty string to stdout does not trigger a flush in Node.js. The
isTTYcheck and empty write are essentially no-ops. If immediate flushing is critical, consider using a synchronous write approach or removing the dead code.// Write to stdout first so kubectl logs can capture it process.stdout.write(`${successMessage}\n`); - // Force flush stdout to ensure it's captured - if (process.stdout.isTTY === false) { - process.stdout.write(''); // Trigger flush - } // Also log via CloudRunnerLogger for GitHub Actions CloudRunnerLogger.log(successMessage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/model/cloud-runner/remote-client/index.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/model/cloud-runner/remote-client/index.ts (3)
src/model/cloud-runner/remote-client/remote-client-logger.ts (1)
RemoteClientLogger(7-93)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: WebGL on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: Android on 2021.3.32f1
🔇 Additional comments (3)
src/model/cloud-runner/remote-client/index.ts (3)
88-107: LGTM: Guarded cache push with proper existence and content checks.The defensive checks for folder existence and non-empty content before pushing to cache are well implemented. The error handling appropriately warns without failing the build.
109-128: LGTM: Consistent defensive pattern for build cache.Same well-structured defensive checks as the Library cache push.
131-138: LGTM: Defensive cleanup with existence check.Checking both path formats before attempting removal prevents unnecessary errors.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (23)
src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts (2)
26-52: Duplicate: Environment gating logic already flagged.A previous review already addressed stabilizing the gating logic to always probe rclone/bash availability regardless of CI status.
80-80: Duplicate: Excessive timeout already flagged.A previous review recommended reducing the ~11.6 day timeout to a sane upper bound (15–30 minutes).
src/model/cloud-runner/tests/providers/provider-git-manager.test.ts (1)
1-5: Fix import order and remove unused mocks to avoid ESLint noise
import/order: put Node built-ins before relative imports and keep all imports beforejest.mockcalls, e.g.import * as fs from 'fs';followed byimport type { GitHubUrlInfo } ...andimport { ProviderGitManager } ....no-unused-vars:mockExecAsync(Line 18) andmockFs(Line 33) are never used; either wire them into the tests (e.g., stubbingfs.existsSyncforgetProviderModulePathscenarios) or remove them.Example adjustment:
-import { GitHubUrlInfo } from '../../providers/provider-url-parser'; -import * as fs from 'fs'; +import * as fs from 'fs'; +import type { GitHubUrlInfo } from '../../providers/provider-url-parser'; @@ -const mockExecAsync = jest.fn(); @@ -const mockFs = fs as jest.Mocked<typeof fs>;This should clear the earlier import/first and no-unused-vars issues.
Also applies to: 15-20, 33-34
src/model/cloud-runner/remote-client/index.ts (4)
312-312: Fix contradictory LFS folder assertion message.The code asserts that the LFS folder EXISTS (
fs.existsSyncreturns true), but the message says "should not exist before caching". This is contradictory.Since
git lfs installwas called at line 268, the LFS folder should exist at this point. The assertion message should be corrected to match the check.Apply this diff:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder exists after git lfs install');
340-349: Remove unconventional void 0 statement.Line 348 contains an unconventional
void 0;statement in the catch block. Since this is a no-op catch, the statement is unnecessary. The comment alone is sufficient to document the intent.Apply this diff:
} catch { /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; }
49-60: Add newline when writing to log file.Line 51 appends
elementto the log file without a newline, while line 55 writes to stdout with a newline. This causes log entries in the file to be concatenated together without line breaks, making the file difficult to read.Apply this diff:
- fs.appendFileSync(logFile, element); + fs.appendFileSync(logFile, `${element}\n`);
64-75: Add newline when writing lingering line to log file.Line 66 has the same issue as line 51: it appends
lingeringLineto the log file without a newline, while line 69 writes to stdout with a newline.Apply this diff:
- fs.appendFileSync(logFile, lingeringLine); + fs.appendFileSync(logFile, `${lingeringLine}\n`);src/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts (1)
31-34: Missing-cflag for--previouslogs can break multi-container pods.The
--previousbranch omits the container name flag. For multi-container pods, this may fail or select the wrong container.- extraFlags += isRunning ? ` -f -c ${containerName} -n ${namespace}` : ` --previous -n ${namespace}`; + extraFlags += isRunning + ? ` -f -c ${containerName} -n ${namespace}` + : ` --previous -c ${containerName} -n ${namespace}`;src/model/cloud-runner/remote-client/caching.ts (2)
97-117: Proactive cleanup scope may be broader than intended.If
cacheFolderis'cache', thencacheParentbecomes'.'(current directory), andfind . -name "*.tar*"could match unrelated files. Consider restricting cleanup to the actual cache directory.
159-180: Dead code:retrySucceededcheck is unreachable.The conditional at lines 175-178 can never execute with
!retrySucceededbeing true:
- If retry succeeds,
retrySucceeded = true(line 166)- If retry throws, the catch block (lines 167-173) throws, so line 175 is never reached
This was flagged in a previous review as addressed, but the dead code remains.
// Retry the tar operation once after cleanup - let retrySucceeded = false; try { await CloudRunnerSystem.Run( `tar -cf ${cacheArtifactName}.tar${compressionSuffix} "${path.basename(sourceFolder)}"`, ); - // If retry succeeds, mark it - we'll continue normally without throwing - retrySucceeded = true; } catch (retryError: any) { throw new Error( `Failed to create cache archive after cleanup. Original error: ${errorMessage}. Retry error: ${ retryError?.message || retryError }`, ); } - // If retry succeeded, don't throw the original error - let execution continue after catch block - if (!retrySucceeded) { - throw error; - } - // If we get here, retry succeeded - execution will continue after the catch blocksrc/model/cloud-runner/providers/aws/services/task-service.ts (2)
109-111: DescribeTasksCommand has a 100-task limit.
DescribeTasksCommandaccepts a maximum of 100 task ARNs per request. If a cluster has more than 100 running tasks, this call will fail.if (taskArns.length > 0) { - const describeInput = { tasks: taskArns, cluster: element }; - const describeList = (await ecs.send(new DescribeTasksCommand(describeInput))).tasks || []; + const describeList: Task[] = []; + for (let i = 0; i < taskArns.length; i += 100) { + const batch = taskArns.slice(i, i + 100); + const response = await ecs.send(new DescribeTasksCommand({ tasks: batch, cluster: element })); + describeList.push(...(response.tasks || [])); + } if (describeList.length === 0) {
201-218: Missing pagination for ListObjectsV2Command.
ListObjectsV2Commandreturns a maximum of 1000 objects. If the bucket contains more locks, results will be silently truncated.Also, the type assertion at lines 204-206 is a workaround for
listObjectsbeing private. Consider makinglistObjectspublic inSharedWorkspaceLockingto avoid this cast.src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
17-34: Lazy S3 client initialization via factory is good.The factory pattern ensures consistent configuration (endpoints, path-style) across the codebase.
However, the
rclonehelper at lines 30-34 concatenates user-controlled values into a shell string, which is a command injection risk.The command injection concern was previously flagged. Using
execFilewith an args array instead ofexecwith string interpolation would eliminate this risk.
73-101: Missing pagination in S3 listObjects.
ListObjectsV2Commandreturns at most 1000 objects. WithoutContinuationTokenhandling, results will be truncated for large lock sets.- const result = await SharedWorkspaceLocking.s3.send( - new ListObjectsV2Command({ Bucket: bucket, Prefix: prefix, Delimiter: '/' }), - ); const entries: string[] = []; - for (const p of result.CommonPrefixes || []) { - if (p.Prefix) entries.push(p.Prefix.slice(prefix.length)); - } - for (const c of result.Contents || []) { - if (c.Key && c.Key !== prefix) entries.push(c.Key.slice(prefix.length)); - } + let token: string | undefined; + do { + const result = await SharedWorkspaceLocking.s3.send( + new ListObjectsV2Command({ Bucket: bucket, Prefix: prefix, Delimiter: '/', ContinuationToken: token }), + ); + for (const p of result.CommonPrefixes || []) { + if (p.Prefix) entries.push(p.Prefix.slice(prefix.length)); + } + for (const c of result.Contents || []) { + if (c.Key && c.Key !== prefix) entries.push(c.Key.slice(prefix.length)); + } + token = result.NextContinuationToken; + } while (token);src/model/cloud-runner/workflows/build-automation-workflow.ts (4)
62-83: Non‑container setup only initializes logging; no builder clone or pre‑build, and/home/job-log.txtis still hard‑coded.In the non‑container path,
setupCommandsnow only exportsGIT_DISCOVERY_ACROSS_FILESYSTEM, creates theLOG_FILEdirectory, and logsCACHE_KEY, but it never clones the builder repo or runsremote-cli-pre-build. Together with the non‑containerBuildCommands(see lines 205‑210), this means local/non‑container providers appear to do only log streaming/post‑build without ever staging or invoking the Unity build entrypoint, so local builds may effectively be no‑ops unless something external sets everything up.If the intent is that non‑container providers have the builder and its
dist/index.jspre‑installed and that they start the build outside this workflow, please document that expectation and/or guard this path with an explicit feature flag. Otherwise, consider aligning the non‑container setup with the containerized path (clone + pre‑build) so that it can stand alone.Also, now that
LOG_FILEis exported, you can simplify and de‑duplicate log handling by consistently using$LOG_FILEinstead of hard‑coding/home/job-log.txtin the containerized setup.Also applies to: 117-125, 128-131
162-170: Stub node/npm/n/yarn binaries can hide real dependency problems.The local‑docker branch creates no‑op stubs for
node,npm,n, andyarnthat simplyexit 0when the commands are missing. This will cause any script that actually depends on these tools to appear to succeed while silently doing nothing, making misconfigurations hard to debug.Instead, either:
- Fail fast with a clear error when required tooling is missing, or
- If stubbing is needed for a very specific compatibility case, log prominent warnings (including which binary is missing and why it’s being stubbed) and gate this behavior behind an explicit opt‑in flag.
179-180:remote-cli-post-buildfailures are being masked as warnings/success.Both containerized branches treat post‑build failures as success:
- Local‑docker:
node ${builderPath} -m remote-cli-post-build || echo "Post-build command completed with warnings".- Other containerized:
node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || true.If
remote-cli-post-buildis responsible for uploading artifacts, updating locks, or persisting cache, hiding its failures can leave the system in an inconsistent state while the job appears green.Consider either:
- Letting the command fail (removing the
||/|| true), or- Capturing the exit code and logging a clear warning that includes the exit code and context, and choosing explicitly whether to exit non‑zero.
Example (if failures should be surfaced):
-node ${builderPath} -m remote-cli-post-build || echo "Post-build command completed with warnings" +node ${builderPath} -m remote-cli-post-build || { + EXIT_CODE=$? + echo "ERROR: Post-build command failed with exit code $EXIT_CODE" + exit "$EXIT_CODE" +}Apply a similar pattern to the piped variant if post‑build is meant to be critical.
Also applies to: 199-200
205-210: Non‑container providers appear to run only log/post‑build helpers and never execute the build entrypoint.The non‑container
BuildCommandsbranch now just:
- Logs “game ci start” to
$LOG_FILE.- Runs
remote-cli-log-streamfor 3 seconds.- Runs
remote-cli-post-build.Unlike the containerized branches, it never stages the default build script, does not copy or execute
/entrypoint.sh, and has no pre‑build step. Unless some external process is starting the actual Unity build and wiring up the remote CLI, local/non‑container providers will effectively do no build work while still returning from the workflow.If local (or other non‑container) providers are meant to execute real builds through this workflow, consider restoring the entrypoint/steps invocation pattern (or adding an explicit local build path) so that this branch actually runs the build, not just the remote‑cli helpers.
src/model/cloud-runner/services/hooks/container-hook-service.ts (5)
166-184:aws-s3-pull-cache: S3 path handling and object selection are brittleThe cache pull logic works but is fragile:
BUCKET1/BUCKET2are defined withouts3://, thencpprependss3://$BUCKET...whilelsuses$BUCKETdirectly. Mixed schemes are confusing and can be error-prone.OBJECT1/OBJECT2default to'', yetaws ... cp s3://$BUCKET$OBJECTstill executes (errors are hidden by|| ''/|| true), so you silently skip cache without any guard.A more robust pattern (from the earlier suggestion) would be:
- aws $ENDPOINT_ARGS s3 ls ${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/ || true - aws $ENDPOINT_ARGS s3 ls ${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/ || true - BUCKET1="${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/Library/" - aws $ENDPOINT_ARGS s3 ls $BUCKET1 || true - OBJECT1="$(aws $ENDPOINT_ARGS s3 ls $BUCKET1 | sort | tail -n 1 | awk '{print $4}' || '')" - aws $ENDPOINT_ARGS s3 cp s3://$BUCKET1$OBJECT1 /data/cache/$CACHE_KEY/Library/ || true - BUCKET2="${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/lfs/" - aws $ENDPOINT_ARGS s3 ls $BUCKET2 || true - OBJECT2="$(aws $ENDPOINT_ARGS s3 ls $BUCKET2 | sort | tail -n 1 | awk '{print $4}' || '')" - aws $ENDPOINT_ARGS s3 cp s3://$BUCKET2$OBJECT2 /data/cache/$CACHE_KEY/lfs/ || true + aws $ENDPOINT_ARGS s3 ls s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/ || true + aws $ENDPOINT_ARGS s3 ls s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/ || true + BUCKET1="s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/Library/" + aws $ENDPOINT_ARGS s3 ls "$BUCKET1" || true + OBJECT1="$(aws $ENDPOINT_ARGS s3 ls "$BUCKET1" | sort | tail -n 1 | awk '{print $4}')" + [ -n "$OBJECT1" ] && aws $ENDPOINT_ARGS s3 cp "${BUCKET1}${OBJECT1}" "/data/cache/$CACHE_KEY/Library/" || true + BUCKET2="s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/lfs/" + aws $ENDPOINT_ARGS s3 ls "$BUCKET2" || true + OBJECT2="$(aws $ENDPOINT_ARGS s3 ls "$BUCKET2" | sort | tail -n 1 | awk '{print $4}')" + [ -n "$OBJECT2" ] && aws $ENDPOINT_ARGS s3 cp "${BUCKET2}${OBJECT2}" "/data/cache/$CACHE_KEY/lfs/" || trueThis keeps S3 URIs consistent and avoids issuing
cpcalls with empty object names.
40-56: Avoid deleting build tar when S3 upload fails (aws-s3-upload-build)The AWS CLI presence check and
AWS_S3_ENDPOINTwiring look good, but the cleanup logic is risky:
aws $ENDPOINT_ARGS s3 cp ... || trueswallows upload failures.- The subsequent
rmstill executes and is also|| true, so a failed upload still deletes/data/cache/$CACHE_KEY/build/build-*.tar[.lz4], preventing any retry using the existing artifact.To keep the behavior non-fatal while avoiding data loss, gate deletion on a successful copy, e.g.:
- aws $ENDPOINT_ARGS s3 cp /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ - CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/build/build-$BUILD_GUID.tar${ - CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } || true - rm /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ - CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } || true + aws $ENDPOINT_ARGS s3 cp /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ + CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' + } s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/build/build-$BUILD_GUID.tar${ + CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' + } && rm /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ + CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' + } || trueSame pattern applies to other upload hooks that
rmimmediately after a best-effort remote copy.Also applies to: 64-65
70-87:aws-s3-pull-build: missing$ENDPOINT_ARGSoncpand inconsistent S3 URIsThis hook is close, but two details will break LocalStack/custom endpoints and make path handling brittle:
- Both
aws $ENDPOINT_ARGS s3 lscalls use${CloudRunner.buildParameters.awsStackName}/...withouts3://, while thecpusess3://.... Consistents3://URIs are safer and match AWS CLI examples.- The
aws s3 cpline omits$ENDPOINT_ARGS, soAWS_S3_ENDPOINTis honored for listing but not for the download itself.Consider:
- aws $ENDPOINT_ARGS s3 ls ${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/ || true - aws $ENDPOINT_ARGS s3 ls ${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/build || true - aws s3 cp s3://${ - CloudRunner.buildParameters.awsStackName - }/cloud-runner-cache/$CACHE_KEY/build/build-$BUILD_GUID_TARGET.tar${ + aws $ENDPOINT_ARGS s3 ls s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/ || true + aws $ENDPOINT_ARGS s3 ls s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/build || true + aws $ENDPOINT_ARGS s3 cp s3://${ + CloudRunner.buildParameters.awsStackName + }/cloud-runner-cache/$CACHE_KEY/build/build-$BUILD_GUID_TARGET.tar${ CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' } /data/cache/$CACHE_KEY/build/build-$BUILD_GUID_TARGET.tar${ CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' } || trueThis keeps endpoint handling consistent across both list and copy operations.
Also applies to: 93-93
185-201: Gate rclone cache deletions on successful copy (rclone-upload-build/rclone-upload-cache)Both rclone upload hooks delete local data even when the remote copy fails:
rclone copy ... || truehides upload failures.rm/rm -ralways run and are also|| true, so a transient error or bad remote can wipe the only local copy of the build or cache.This is the same concern raised previously for these hooks. A safer pattern:
- rclone copy /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ - CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } ${CloudRunner.buildParameters.rcloneRemote}/cloud-runner-cache/$CACHE_KEY/build/ || true - rm /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ - CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } || true + rclone copy /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ + CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' + } ${CloudRunner.buildParameters.rcloneRemote}/cloud-runner-cache/$CACHE_KEY/build/ && rm /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ + CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' + }- rclone copy /data/cache/$CACHE_KEY/lfs ${ - CloudRunner.buildParameters.rcloneRemote - }/cloud-runner-cache/$CACHE_KEY/lfs || true - rm -r /data/cache/$CACHE_KEY/lfs || true - rclone copy /data/cache/$CACHE_KEY/Library ${ - CloudRunner.buildParameters.rcloneRemote - }/cloud-runner-cache/$CACHE_KEY/Library || true - rm -r /data/cache/$CACHE_KEY/Library || true + rclone copy /data/cache/$CACHE_KEY/lfs ${ + CloudRunner.buildParameters.rcloneRemote + }/cloud-runner-cache/$CACHE_KEY/lfs && rm -r /data/cache/$CACHE_KEY/lfs + rclone copy /data/cache/$CACHE_KEY/Library ${ + CloudRunner.buildParameters.rcloneRemote + }/cloud-runner-cache/$CACHE_KEY/Library && rm -r /data/cache/$CACHE_KEY/LibraryYou may also want to skip these hooks entirely when
CloudRunner.buildParameters.rcloneRemoteis empty to avoid rclone invoking an invalid remote.Also applies to: 221-239
203-213:rclone-pull-build: destination should be a directory (or usecopyto)The pull hook currently copies from a remote file path to what looks like a local file path:
rclone copy <remote>/build-$BUILD_GUID_TARGET.tar[.lz4] /data/cache/$CACHE_KEY/build/build-$BUILD_GUID_TARGET.tar[.lz4]
rclone copyexpects the destination to be a directory; this form typically creates nested directories named likebuild-$BUILD_GUID_TARGET.tar[.lz4]rather than the single expected file, which will confuse downstream consumers looking for/data/cache/$CACHE_KEY/build/build-*.tar[.lz4].Two options:
- Treat the destination as a directory (simpler):
- } /data/cache/$CACHE_KEY/build/build-$BUILD_GUID_TARGET.tar${ - CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } || true + } /data/cache/$CACHE_KEY/build/ || true
- Or switch to
rclone copytoand pass the full file path as the destination.Also applies to: 218-220
🧹 Nitpick comments (12)
src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts (4)
11-17: Global state mutation in test helper.
CreateParametersmutates the globalCli.optionsas a side effect, which could cause test pollution if multiple tests run in the same process without cleanup. Consider adding teardown logic to resetCli.optionsafter tests or documenting this coupling.
60-60: Redundant Unity version resolution.
UnityVersioning.determineUnityVersion(path, UnityVersioning.read(path))is redundant—read()returns the version string, anddetermineUnityVersion()returns it directly when it's not'auto'. Use one or the other:- unityVersion: UnityVersioning.determineUnityVersion('test-project', UnityVersioning.read('test-project')), + unityVersion: UnityVersioning.read('test-project'),
67-67: Double type assertion bypasses type safety.
as unknown as OptionValuescircumvents TypeScript's type checking. While acceptable in tests, consider defining a partial type or using a test utility that properly types the overrides to catch mismatches during refactoring.
75-79: Empty catch block silently swallows errors.The "best-effort" verification completely hides failures, making debugging harder if the remote is misconfigured. At minimum, log the error:
try { const lines = await CloudRunnerSystem.RunAndReadLines(`rclone lsf ${remote}`); CloudRunnerLogger.log(lines.join(',')); - } catch {} + } catch (error) { + CloudRunnerLogger.log(`Best-effort remote verification failed: ${error}`); + }src/model/cloud-runner/providers/local/index.ts (1)
70-82: Windows bash wrapping looks safe; clarify the “if available” wordingThe Windows branch now:
- Normalizes CRLF, flattens multi-line scripts into a single
;‑separated string, and- Uses
bash -lc ${quote([inline])}, which correctly shell‑escapes the script and mitigates injection viacommands.Two minor points to consider:
- The comment says “Execute via bash if available,” but the code unconditionally invokes
bashwhenprocess.platform === 'win32'. Either:
- Add a lightweight availability check (e.g., fail fast with a clear error if
bashis missing), or- Reword the comment to state that
bashis required on Windows for POSIX-style hooks.- If you expect some consumers to supply native Windows
cmdcommands, a brief note in docs about the enforced bash-on-Windows behavior would help avoid surprises.src/model/cloud-runner/tests/providers/provider-git-manager.test.ts (2)
36-154: Tests only validate manual mocks, not the realProviderGitManagerbehaviorAll the
cloneRepository,updateRepository, andgetProviderModulePathtests stub the corresponding methods onmockProviderGitManagerand then assert that the returned value matches the stub. This exercises Jest’s mocking rather than the actual implementation: the tests will still pass even ifProviderGitManager’s real logic is completely broken.Consider instead:
- Mocking dependencies (
execAsync,fs) and importing the realProviderGitManager, then asserting on:
- Correct git commands, timeouts, and working directories for
cloneRepository/updateRepository.- Cache directory behavior and branch handling.
- Entry-point resolution and fallback behavior in
getProviderModulePath.- Adding at least one test for
ensureRepositoryAvailable(existing repo, update success/failure, fresh clone) and optionallycleanupOldRepositorieswith a fake cache directory structure.That will give you regression protection for the provider loading flow instead of just type/shape checks.
36-155: Add coverage for high‑level flows likeensureRepositoryAvailable
ProviderGitManager.ensureRepositoryAvailableandcleanupOldRepositoriesare key to the provider plugin system and cache behavior, but they are not exercised here. A few targeted tests using mockedfsandexecAsyncwould catch regressions in:
- Update-then-clone fallback when
updateRepositoryfails.- Behavior when the repo is missing vs already cloned.
- Old cache eviction logic.
This can be incremental, but adding at least one “happy path” and one “error path” test for
ensureRepositoryAvailablewould materially increase confidence in the new plugin loader.src/model/cloud-runner/remote-client/index.ts (1)
300-308: Rename catch binding to follow convention.Line 302 uses
_errorbut the error is referenced at line 306 when rethrown. The naming convention for catch bindings that are used should beerror, not_error.Apply this diff:
- } catch (_error) { + } catch (error) { if ((targetBranch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); } else { - throw _error; + throw error; } }src/model/cloud-runner/services/core/shared-workspace-locking.ts (1)
51-72: Bucket existence check and creation logic is correct.The region handling for
LocationConstraint(only for non-us-east-1) follows AWS requirements.Consider using
CreateBucketCommandInputtype instead ofanyat line 66:- const createParameters: any = { Bucket: bucket }; + const createParameters: { Bucket: string; CreateBucketConfiguration?: { LocationConstraint: string } } = { Bucket: bucket };src/model/cloud-runner/providers/aws/aws-task-runner.ts (1)
229-254: Consider restoring specific types instead ofany.Multiple parameters and variables have been changed to
any, losing type safety. Consider using SDK response types:import type { DescribeStreamCommandOutput, GetRecordsCommandOutput } from '@aws-sdk/client-kinesis'; import type { Task } from '@aws-sdk/client-ecs'; // Line 229 private static checkStreamingShouldContinue(taskData: Task | undefined, ...) // Line 249 private static logRecords(records: GetRecordsCommandOutput, ...) // Line 280 private static async getLogIterator(stream: DescribeStreamCommandOutput)src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
53-61: Make containerization a provider capability, not a string whitelist.
isContainerizedis derived from hard‑codedproviderStrategyvalues. This works foraws/k8s/local-dockertoday but will drift as new strategies or plugin providers are introduced. Consider surfacing anisContainerized(or similar) flag from the provider itself and derivingbuilderPathfrom that instead of string comparisons.src/model/cloud-runner/workflows/custom-workflow.ts (1)
35-64: Per-stepallowFailurehandling looks correct; consider refining logging and catch scopeThe step-level try/catch and
allowFailuregating are wired sensibly and align with the new default set inContainerHookService.ParseContainerHooks. Two minor refinements you might consider:
- Use
CloudRunnerLogger.error(or.error+ message) in the non‑allowFailurebranch so hard failures surface clearly in logs/annotations rather than as plainlog.- The outer
try/catchthat immediately rethrows (Lines 27–70) is redundant; letting errors bubble up naturally will preserve behavior while simplifying the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (16)
src/model/cloud-runner/providers/aws/aws-task-runner.ts(10 hunks)src/model/cloud-runner/providers/aws/services/task-service.ts(6 hunks)src/model/cloud-runner/providers/k8s/kubernetes-job-spec-factory.ts(5 hunks)src/model/cloud-runner/providers/k8s/kubernetes-pods.ts(1 hunks)src/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts(2 hunks)src/model/cloud-runner/providers/local/index.ts(2 hunks)src/model/cloud-runner/remote-client/caching.ts(1 hunks)src/model/cloud-runner/remote-client/index.ts(5 hunks)src/model/cloud-runner/services/core/shared-workspace-locking.ts(8 hunks)src/model/cloud-runner/services/core/task-parameter-serializer.ts(2 hunks)src/model/cloud-runner/services/hooks/container-hook-service.ts(3 hunks)src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts(1 hunks)src/model/cloud-runner/tests/e2e/cloud-runner-end2end-retaining.test.ts(4 hunks)src/model/cloud-runner/tests/providers/provider-git-manager.test.ts(1 hunks)src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)src/model/cloud-runner/workflows/custom-workflow.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/model/cloud-runner/providers/k8s/kubernetes-job-spec-factory.ts
- src/model/cloud-runner/services/core/task-parameter-serializer.ts
- src/model/cloud-runner/providers/k8s/kubernetes-pods.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/custom-workflow.tssrc/model/cloud-runner/workflows/build-automation-workflow.ts
🧬 Code graph analysis (9)
src/model/cloud-runner/workflows/custom-workflow.ts (2)
src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)src/model/cloud-runner/services/core/cloud-runner-logger.ts (1)
error(24-26)
src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts (3)
src/model/cli/cli.ts (1)
Cli(16-175)src/model/unity-versioning.ts (1)
UnityVersioning(4-32)src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)
src/model/cloud-runner/providers/local/index.ts (1)
src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)
src/model/cloud-runner/providers/aws/aws-task-runner.ts (2)
src/model/cloud-runner/providers/aws/aws-client-factory.ts (1)
AwsClientFactory(9-71)src/model/cloud-runner/services/core/cloud-runner-logger.ts (1)
error(24-26)
src/model/cloud-runner/remote-client/caching.ts (3)
src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
cacheFolder(87-89)src/model/cloud-runner/services/core/cloud-runner-logger.ts (1)
error(24-26)
src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
src/model/cloud-runner/providers/aws/aws-client-factory.ts (1)
AwsClientFactory(9-71)src/model/input.ts (1)
region(48-50)
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)
src/model/cloud-runner/providers/aws/services/task-service.ts (1)
src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
SharedWorkspaceLocking(17-400)s3(19-26)
src/model/cloud-runner/tests/providers/provider-git-manager.test.ts (2)
src/model/cloud-runner/providers/provider-git-manager.ts (1)
ProviderGitManager(25-278)src/model/cloud-runner/providers/provider-url-parser.ts (1)
GitHubUrlInfo(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: iOS on 2022.3.13f1
🔇 Additional comments (30)
src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts (3)
1-9: LGTM!Imports are appropriate for the test's needs—CloudRunner, BuildParameters, ImageTag for build execution, UnityVersioning for version resolution, uuid for unique cache keys, and CloudRunnerSystem for remote verification.
20-24: LGTM!Placeholder tests and
setups()invocation provide basic smoke testing to ensure the file loads correctly.
81-85: LGTM!Skipped test with informative logging is appropriate when rclone is not configured.
src/model/cloud-runner/providers/local/index.ts (1)
9-9: Good choice usingshell-quotefor argument escapingImporting
quotefromshell-quoteis a solid fix for the previous manual-escaping approach and should address the code scanning warning about incomplete escaping (including backslashes).src/model/cloud-runner/tests/e2e/cloud-runner-end2end-retaining.test.ts (4)
27-27: Explicit debug flag is good for clarity.Adding
cloudRunnerDebug: trueto the test parameters ensures the flag is explicitly passed to the build, even though the test is already guarded byCloudRunnerOptions.cloudRunnerDebug. This provides clear intent and proper configuration propagation.
38-40: Improved test assertions with boolean BuildSucceeded property.The update from string-based assertions to the explicit
BuildSucceededboolean property makes the test more robust and less brittle. The comment on line 40 provides helpful context for future maintenance. The consistent use across both test runs (lines 38 and 75) ensures uniform verification.Also applies to: 75-75
89-98: Good defensive cleanup with error handling.The enhanced cleanup properly handles potential failures during workspace removal. The combination of
|| truein the command and try-catch wrapping provides robust error handling, ensuring cleanup failures don't break the test suite. The use ofpath.basename()adds safety by preventing directory traversal in the constructed path.
100-109: Cache cleanup is safe for CI environments due to sequential test execution.The cache directory cleanup is good test hygiene and will prevent disk space accumulation. The concern about parallel test execution is mitigated by the Jest CI configuration (
jest.ci.config.js), which explicitly setsmaxWorkers: 1to enforce sequential test execution in CI pipelines.Multiple tests reference the
cloud-runner-cachedirectory, but since they execute sequentially in CI and this cleanup runs at the end of the test suite, there is no conflict risk. The wildcard deletion (rm -rf ${cachePath}/*) removes all cache contents safely when tests complete.Note: In local development with the default Jest configuration, tests could theoretically run in parallel; the cleanup happening at test completion limits any practical impact.
src/model/cloud-runner/remote-client/index.ts (8)
35-38: LGTM!Setting stdout encoding for K8s ensures consistent output capture by kubectl logs.
82-110: LGTM!The defensive guards prevent unnecessary cache operations and provide clear logging when skipping. The error handling ensures the build continues even if cache push fails.
112-136: LGTM!Consistent defensive pattern for Build cache push, matching the Library cache logic.
139-146: LGTM!The existence check prevents errors when attempting to clean up a non-existent folder.
154-164: LGTM!Writing to stdout before CloudRunnerLogger ensures kubectl logs can capture the success marker for test assertions.
272-298: LGTM!The multi-layered checkout fallback strategy properly handles SHA misses, PR refs, and origin refs. The catch block patterns are now correct, using parameterless catch where errors aren't referenced and naming the error when it's rethrown.
351-391: LGTM!The fallback strategy for LFS pull tries multiple authentication methods (plain, GIT_PRIVATE_TOKEN, GITHUB_TOKEN) with proper error handling and logging at each step.
396-432: LGTM!The retained workspace handling properly fetches refs (including PR refs), pulls LFS files, and uses the same robust SHA/branch checkout fallback strategy as the initial clone path.
src/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts (2)
54-95: Fallback log retrieval logic looks reasonable.The fallback from
--previousto current container logs handles edge cases well. The retry logic increments correctly in both success and failure paths. The nested try-catch adds complexity but is justified for resilience.
104-114: Good error discrimination after retry exhaustion.Rethrowing non-"previous container" errors while gracefully breaking on expected container lifecycle issues is the right approach.
src/model/cloud-runner/remote-client/caching.ts (2)
82-96: Pre-flight disk space check is a good addition.The disk usage parsing is best-effort and gracefully handles failures. This proactive check helps prevent tar operations from failing mid-write.
201-207: Good defensive check: recreate cache folder if deleted.Cleanup operations might delete the cache folder if it becomes empty. Recreating it before
mvprevents failures.src/model/cloud-runner/providers/aws/services/task-service.ts (2)
6-18: Good use of type-only imports and factory pattern.Using
import typefor SDK types and centralizing client creation viaAwsClientFactoryimproves type safety and consistency with the codebase's approach.
88-108: Pagination for ListClusters and ListTasks is well implemented.The do-while pattern with
nextTokencorrectly handles pagination for both cluster and task enumeration.src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
297-321: CreateWorkspace correctly handles both backends.The key generation and conditional backend selection (rclone touch vs S3 PutObject) is implemented consistently.
392-399: ReadLines provides backward compatibility.Parsing legacy command strings (
aws s3 ls,rclone lsf) and routing tolistObjectsmaintains compatibility with existing code paths.src/model/cloud-runner/providers/aws/aws-task-runner.ts (4)
1-14: Good migration to factory-based AWS clients.Centralizing client creation via
AwsClientFactoryensures consistent configuration across the codebase.
135-162: Well-implemented retry with exponential backoff and jitter.The throttle detection, exponential backoff (doubling with 60s cap), and jitter follow AWS SDK best practices for handling rate limiting.
201-216: GetRecords throttle handling is correct.Returning early with the same iterator preserves position while respecting rate limits. The outer loop will retry on the next iteration.
86-94: Good defensive handling for missing container data.Checking for empty/undefined containers array before accessing prevents undefined access errors during task startup.
src/model/cloud-runner/services/hooks/container-hook-service.ts (2)
263-265: Debug-cache hook: safer apt usage and consistent secretsThe updated debug-cache hook is a nice improvement:
apt-get updateand the optionaltreeinstallation/listing are now guarded with|| true, so missingaptortreeno longer breaks the build for a purely diagnostic step.- Adding
AWS_S3_ENDPOINTto the secrets keeps the secret schema aligned with other hooks even if this particular command block doesn’t currently use it.No changes requested here.
Also applies to: 272-274
338-341: Good defaulting ofallowFailureinParseContainerHooksDefaulting
step.allowFailuretofalsewhen it’s undefined ensures:
- Existing hooks without an explicit flag remain strict and will still fail the build on errors.
- The new per-step handling in
CustomWorkflow.runContainerJobcan safely rely on a boolean and treat only explicittrueas “soft-fail”.This is a straightforward, low-risk change that complements the workflow error handling.
| // In local provider mode (non-container) or when AWS credentials are not present, skip AWS S3 hooks | ||
| const provider = CloudRunner.buildParameters?.providerStrategy; | ||
| const isContainerized = provider === 'aws' || provider === 'k8s' || provider === 'local-docker'; | ||
| const hasAwsCreds = | ||
| (process.env.AWS_ACCESS_KEY_ID && process.env.AWS_SECRET_ACCESS_KEY) || | ||
| (process.env.awsAccessKeyId && process.env.awsSecretAccessKey); | ||
|
|
||
| // Always include AWS hooks on the AWS provider (task role provides creds), | ||
| // otherwise require explicit creds for other containerized providers. | ||
| const shouldIncludeAwsHooks = | ||
| isContainerized && !CloudRunner.buildParameters?.skipCache && (provider === 'aws' || Boolean(hasAwsCreds)); | ||
| const filteredBuiltIns = shouldIncludeAwsHooks | ||
| ? builtInContainerHooks | ||
| : builtInContainerHooks.filter((x) => x.image !== 'amazon/aws-cli'); | ||
|
|
||
| if (filteredBuiltIns.length > 0) { | ||
| results.push(...filteredBuiltIns); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "container-hook-service.ts" | head -5Repository: game-ci/unity-builder
Length of output: 130
🏁 Script executed:
wc -l ./src/model/cloud-runner/services/hooks/container-hook-service.tsRepository: game-ci/unity-builder
Length of output: 134
🏁 Script executed:
cat -n ./src/model/cloud-runner/services/hooks/container-hook-service.tsRepository: game-ci/unity-builder
Length of output: 18940
🏁 Script executed:
rg "containerHookFiles" --type ts -A 3 -B 3Repository: game-ci/unity-builder
Length of output: 7951
🏁 Script executed:
rg "skipCache" --type ts -B 3 -A 3Repository: game-ci/unity-builder
Length of output: 3524
🏁 Script executed:
rg "skipCache.*true" --type ts -B 5 -A 5Repository: game-ci/unity-builder
Length of output: 2024
🏁 Script executed:
rg "rcloneRemote" --type ts -B 2 -A 2 | head -40Repository: game-ci/unity-builder
Length of output: 3538
🏁 Script executed:
rg "skipCache" --type ts | grep -i rcloneRepository: game-ci/unity-builder
Length of output: 47
Hook filtering: skipCache does not disable rclone caching
When skipCache is true, AWS hooks are correctly excluded. However, rclone hooks (rclone-pull-cache, rclone-upload-cache, rclone-pull-build, rclone-upload-build) are not gated by this flag. If these are included in containerHookFiles with skipCache=true, they will execute but fail silently (due to || true in their commands).
For consistency with skipCache's intent to disable all caching, consider extending the filtering logic to exclude rclone hooks when cache is disabled:
const shouldIncludeAwsHooks =
isContainerized && !CloudRunner.buildParameters?.skipCache && (provider === 'aws' || Boolean(hasAwsCreds));
const shouldIncludeRcloneHooks =
!CloudRunner.buildParameters?.skipCache && Boolean(CloudRunner.buildParameters?.rcloneRemote);
const filteredBuiltIns = builtInContainerHooks.filter((x) => {
if (x.image === 'amazon/aws-cli') return shouldIncludeAwsHooks;
if (x.image === 'rclone/rclone') return shouldIncludeRcloneHooks;
return true;
});🤖 Prompt for AI Agents
In src/model/cloud-runner/services/hooks/container-hook-service.ts around lines
277 to 293, the current filter only removes AWS CLI hooks when skipCache is true
but still allows rclone hooks to run; update the filtering so rclone hooks are
also excluded when CloudRunner.buildParameters?.skipCache is true or when
rcloneRemote is not configured. Specifically, compute a shouldIncludeRcloneHooks
boolean (false if skipCache is true or rcloneRemote is missing) and change the
builtInContainerHooks filtering to return false for entries where x.image ===
'rclone/rclone' unless shouldIncludeRcloneHooks is true, while keeping the
existing logic for the AWS image; push the filtered results into results as
before.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/model/cloud-runner/workflows/build-automation-workflow.ts (5)
119-119: Hardcoded log path should use$LOG_FILEvariable.Line 119 hardcodes
/home/job-log.txtinstead of using the$LOG_FILEvariable defined on line 82, creating an inconsistency with lines 130, 172, 181, 183, 184, 201, 203, and 212 which correctly use the variable.Based on learnings, this issue was already flagged in a previous review.
Apply this diff to use the variable consistently:
-echo "log start" >> /home/job-log.txt +echo "log start" >> "$LOG_FILE"
128-131: Non-containerized setup omits builder repository clone.The non-containerized code path only initializes logging but doesn't clone the Unity builder repository, while the containerized path (lines 109-115) does. Since
builderPathfor non-containerized mode points toprocess.cwd()/dist/index.js, downstreamnode ${builderPath}commands (lines 212-213) will fail unless the builder is already present.Based on learnings, this critical issue was already flagged in a previous review.
Clarify whether non-containerized providers require the builder to be pre-installed in the working directory, and document this requirement. If the builder should be present, add clone logic similar to the containerized path.
167-170: Stub binaries may silently mask missing tool dependencies.Creating no-op stubs for
node,npm,n, andyarnwhen they're missing will cause scripts requiring these tools to fail silently. If local-docker images should provide these tools, their absence indicates a misconfiguration.Based on learnings, this major issue was already flagged in a previous review.
Consider logging warnings before creating stubs or removing stubs entirely and letting builds fail explicitly when required tools are missing.
181-181: Post-build error suppression may hide critical failures.The
|| echo "Post-build command completed with warnings"pattern converts all post-build failures into success, potentially hiding critical issues like failed artifact uploads or cache persistence.Based on learnings, this major issue was already flagged in a previous review.
Consider either propagating the failure or capturing and logging the actual exit code before continuing:
-node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "Post-build command completed with warnings" +node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || { EXIT_CODE=$?; echo "WARNING: Post-build failed with exit code $EXIT_CODE"; exit $EXIT_CODE; }
208-214: Non-containerized path omits build execution logic.The non-containerized code path (local/local-system providers) only logs markers and runs post-build commands but never copies build scripts or executes
/entrypoint.sh. In contrast, both containerized paths (lines 172 and 201) execute the entrypoint which performs the actual Unity build. Without this execution, non-containerized builds will log success without performing any build operations.Based on learnings, this critical issue was already flagged in a previous review.
Add the entrypoint execution to the non-containerized path:
return ` echo "game ci start" echo "game ci start" >> "$LOG_FILE" + cd ${CloudRunnerFolders.ToLinuxFolder(CloudRunnerFolders.projectPathAbsolute)} + cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(distFolder, 'default-build-script'))}" "/UnityBuilderAction" + cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(ubuntuPlatformsFolder, 'entrypoint.sh'))}" "/entrypoint.sh" + cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(ubuntuPlatformsFolder, 'steps'))}" "/steps" + chmod -R +x "/entrypoint.sh" + chmod -R +x "/steps" + /entrypoint.sh | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" - timeout 3s node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" || true node ${builderPath} -m remote-cli-post-build`;src/model/cloud-runner/remote-client/index.ts (2)
275-301: Fix LFS folder assertion inconsistency and clean up catch naming.The expanded fetch/checkout logic (tags + PR refs, SHA‑first with branch/PR fallbacks) is a good improvement, but there are two issues:
- Line 315 currently asserts that the LFS folder exists while the message says it “should not exist before caching”:
assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');If the intent is to verify that no LFS objects are present before the cache pull (which matches the message and surrounding flow), the predicate should be inverted. As written, the assertion enforces the opposite of what the message says, which is misleading for anyone debugging this path.
- In the
targetShaempty path, the catch binding_erroris used but still non‑idiomatic and will typically tripunicorn/catch-error-namelint rules. Usingerror(without the underscore) matches the convention used elsewhere in this file and prior review guidance. Based on learnings / past comments, this was previously flagged.Suggested fix:
- } else { - try { - await CloudRunnerSystem.Run(`git checkout ${targetBranch}`); - } catch (_error) { - if ((targetBranch || '').startsWith('pull/')) { - await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); - } else { - throw _error; - } - } + } else { + try { + await CloudRunnerSystem.Run(`git checkout ${targetBranch}`); + } catch (error) { + if ((targetBranch || '').startsWith('pull/')) { + await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); + } else { + throw error; + } + } @@ - assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(!fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');Also applies to: 303-315
336-398:pullLatestLFSfallbacks are effectively disabled due tosuppressError=true.The multi‑strategy LFS pull logic (plain, then
GIT_PRIVATE_TOKEN, thenGITHUB_TOKEN, finally warn and continue) is a good design, but with the current use ofCloudRunnerSystem.Runit doesn’t behave as intended:
In the “plain pull” block you call:
await CloudRunnerSystem.Run(`git lfs pull`, true); await CloudRunnerSystem.Run(`git lfs checkout || true`, true);With
suppressError = true,CloudRunnerSystem.Runnever rejects even whengit lfs pullfails (seecloud-runner-system.ts: non‑zero exit only rejects whensuppressErroris false). As a result, thetryblock always “succeeds”, you log:RemoteClientLogger.log(`Pulled LFS files without explicit token configuration`);and
return, skipping all token‑based fallbacks and the final warning.The
GIT_PRIVATE_TOKENandGITHUB_TOKENblocks similarly callgit lfs pullandgit lfs checkoutwithsuppressError = true, so theircatch (error)blocks won’t run for normal command failures either, and the “Successfully pulled…” logs may be emitted even when the commands fail.The final
RemoteClientLogger.logWarning('Proceeding without LFS files...')will only execute if something other than thegitcommands throws, which defeats the safety net you’re aiming for.This is a functional bug: builds can silently proceed without LFS content while logs claim success, and the fallback token strategies are practically unreachable. Based on learnings and the provided
CloudRunnerSystem.Runimplementation, error handling hinges onsuppressError.A safer implementation is to let
git lfs pullthrow (nosuppressError) and catch at the TS level, while still keeping the overall operation best‑effort:private static async pullLatestLFS() { @@ - if (CloudRunner.buildParameters.skipLfs) { - RemoteClientLogger.log(`Skipping LFS pull (skipLfs=true)`); - - return; - } - - // Best effort: try plain pull first (works for public repos or pre-configured auth) - try { - await CloudRunnerSystem.Run(`git lfs pull`, true); - await CloudRunnerSystem.Run(`git lfs checkout || true`, true); - RemoteClientLogger.log(`Pulled LFS files without explicit token configuration`); - - return; - } catch { - /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; - } + if (CloudRunner.buildParameters.skipLfs) { + RemoteClientLogger.log(`Skipping LFS pull (skipLfs=true)`); + return; + } + + // Best effort: try plain pull first (works for public repos or pre-configured auth) + try { + await CloudRunnerSystem.Run(`git lfs pull`); // allow failure to throw + await CloudRunnerSystem.Run(`git lfs checkout || true`, true); + RemoteClientLogger.log(`Pulled LFS files without explicit token configuration`); + return; + } catch (error: any) { + RemoteClientLogger.logCliDiagnostic( + `git lfs pull without explicit token configuration failed: ${error?.message ?? String(error)}`, + ); + } @@ - // Try with GIT_PRIVATE_TOKEN - try { + // Try with GIT_PRIVATE_TOKEN + try { @@ - await CloudRunnerSystem.Run(`git lfs pull`, true); - await CloudRunnerSystem.Run(`git lfs checkout || true`, true); + await CloudRunnerSystem.Run(`git lfs pull`); + await CloudRunnerSystem.Run(`git lfs checkout || true`, true); RemoteClientLogger.log(`Successfully pulled LFS files with GIT_PRIVATE_TOKEN`); @@ - } catch (error: any) { - RemoteClientLogger.logCliError(`Failed with GIT_PRIVATE_TOKEN: ${error.message}`); + } catch (error: any) { + RemoteClientLogger.logCliError(`Failed with GIT_PRIVATE_TOKEN: ${error?.message ?? String(error)}`); } @@ - // Try with GITHUB_TOKEN - try { + // Try with GITHUB_TOKEN + try { @@ - await CloudRunnerSystem.Run(`git lfs pull`, true); - await CloudRunnerSystem.Run(`git lfs checkout || true`, true); + await CloudRunnerSystem.Run(`git lfs pull`); + await CloudRunnerSystem.Run(`git lfs checkout || true`, true); RemoteClientLogger.log(`Successfully pulled LFS files with GITHUB_TOKEN`); @@ - } catch (error: any) { - RemoteClientLogger.logCliError(`Failed with GITHUB_TOKEN: ${error.message}`); + } catch (error: any) { + RemoteClientLogger.logCliError(`Failed with GITHUB_TOKEN: ${error?.message ?? String(error)}`); } @@ - // If we get here, all strategies failed; continue without failing the build - RemoteClientLogger.logWarning(`Proceeding without LFS files (no tokens or pull failed)`); + // If we get here, all strategies failed; continue without failing the build + RemoteClientLogger.logWarning(`Proceeding without LFS files (no tokens or pull failed)`);This preserves “best effort” semantics while actually exercising the token fallbacks and the final warning. It also removes the now‑redundant
void 0;no‑op that was previously called out.
🧹 Nitpick comments (4)
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
172-184: Consider using$LOG_FILEvariable consistently throughout containerized paths.Lines 172, 181, 183, 184, 201, and 203 hardcode
/home/job-log.txt, which matches the containerized value set on line 82. While functionally correct, using the$LOG_FILEvariable would improve consistency and make the code more maintainable if the log path ever needs to change.Example for line 172:
-{ echo "game ci start"; echo "game ci start" >> /home/job-log.txt; echo "CACHE_KEY=$CACHE_KEY"; echo "$CACHE_KEY"; if [ -n "$LOCKED_WORKSPACE" ]; then echo "Retained Workspace: true"; fi; if [ -n "$LOCKED_WORKSPACE" ] && [ -d "$GITHUB_WORKSPACE/.git" ]; then echo "Retained Workspace Already Exists!"; fi; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt +{ echo "game ci start"; echo "game ci start" >> "$LOG_FILE"; echo "CACHE_KEY=$CACHE_KEY"; echo "$CACHE_KEY"; if [ -n "$LOCKED_WORKSPACE" ]; then echo "Retained Workspace: true"; fi; if [ -n "$LOCKED_WORKSPACE" ] && [ -d "$GITHUB_WORKSPACE/.git" ]; then echo "Retained Workspace Already Exists!"; fi; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE"Apply similar changes to lines 181, 183, 184, 201, and 203.
Also applies to: 201-203
src/model/cloud-runner/remote-client/index.ts (3)
35-78: K8s log streaming changes look solid; only minor whitespace/logging nits.The real‑time streaming behavior and separation of concerns (file, stdout for k8s, CloudRunnerLogger) look good.
Two minor consistency tweaks you might consider:
- The log file skips lines where
element.trim()is falsy, butCloudRunnerLogger.log(element)and the k8sstdoutpath still emit them. If you ever assert on exact line counts between file/stdout, this divergence could surprise you.- On
stdinend,CloudRunnerLogger.log(lingeringLine)runs even whenlingeringLineis empty; guarding that call would avoid an extra blank client log entry.Example tweak if you want to align behavior:
- for (const element of lines) { - // Always write to log file so output can be collected by providers - if (element.trim()) { - fs.appendFileSync(logFile, `${element}\n`); - } + for (const element of lines) { + const trimmed = element.trimEnd(); + // Always write to log file so output can be collected by providers + if (trimmed) { + fs.appendFileSync(logFile, `${trimmed}\n`); + } @@ - if (lingeringLine) { + if (lingeringLine) { // Always write to log file so output can be collected by providers fs.appendFileSync(logFile, `${lingeringLine}\n`); @@ - CloudRunnerLogger.log(lingeringLine); + if (lingeringLine) { + CloudRunnerLogger.log(lingeringLine); + }
85-139: Post‑build cache guards and activation logging are good; consider small DRY/error‑logging improvements.The guarded
Library/Buildcache pushes and best‑effort error handling are clear and match the intent to avoid failing builds on cache issues. The activation success marker to both stdout andCloudRunnerLoggeris also a nice touch for tests and diagnostics.Two non‑blocking suggestions:
- The
LibraryandBuildblocks are almost identical; extracting a small helper likepushFolderToCacheIfNonEmpty({source, target, key, label})would reduce duplication and keep future behavior changes in sync.- In both cache
catchblocks you useerror: anyand logerror.message. If a non‑Error is thrown,messagewill be undefined. UsingString(error)as a fallback is a bit more robust.Illustrative change for the logging part:
- } catch (error: any) { - RemoteClientLogger.logWarning(`Library cache push skipped with error: ${error.message}`); + } catch (error: any) { + const msg = error?.message ?? String(error); + RemoteClientLogger.logWarning(`Library cache push skipped with error: ${msg}`); @@ - } catch (error: any) { - RemoteClientLogger.logWarning(`Build cache push skipped with error: ${error.message}`); + } catch (error: any) { + const msg = error?.message ?? String(error); + RemoteClientLogger.logWarning(`Build cache push skipped with error: ${msg}`);Also applies to: 142-149, 157-166
404-435: Retained workspace fetch/checkout flow is solid; consider reusingpullLatestLFSand guarding empty SHA/branch.The retained workspace path now:
- Logs the cache key,
- Fetches all refs + tags (and PR refs when needed),
- Updates LFS,
- Then does a SHA‑first checkout with branch/PR fallbacks.
That’s a good, predictable resolution strategy.
Two follow‑ups to consider:
- Once
pullLatestLFSis fixed, you can likely reuse it here instead ofgit lfs pull+git lfs checkout || trueto keep LFS behavior (incl. token fallbacks and “proceed without LFS” warning) consistent between fresh clones and retained workspaces.shaandbranchare read without guards; if either is unexpectedly empty, you’ll end up runninggit reset --hard ""orgit checkout undefined. Even though upstream usually supplies them, adding a small guard would make this path more defensive.Illustrative change:
- await CloudRunnerSystem.Run(`git lfs pull`); - await CloudRunnerSystem.Run(`git lfs checkout || true`); - const sha = CloudRunner.buildParameters.gitSha; - const branch = CloudRunner.buildParameters.branch; + await RemoteClient.pullLatestLFS(); + const sha = CloudRunner.buildParameters.gitSha; + const branch = CloudRunner.buildParameters.branch; + if (!sha && !branch) { + RemoteClientLogger.logWarning( + 'Retained workspace: no gitSha or branch provided; skipping reset/checkout step', + ); + return true; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/model/cloud-runner/remote-client/index.ts(4 hunks)src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/build-automation-workflow.ts
🧬 Code graph analysis (2)
src/model/cloud-runner/remote-client/index.ts (2)
src/model/cloud-runner/remote-client/remote-client-logger.ts (1)
RemoteClientLogger(7-93)src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)
src/model/cloud-runner/workflows/build-automation-workflow.ts (2)
src/model/index.ts (1)
CloudRunner(27-27)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
🔇 Additional comments (9)
src/model/cloud-runner/workflows/build-automation-workflow.ts (9)
53-60: Container-awareness logic is well-structured.The
isContainerizedflag correctly distinguishes between containerized providers (aws, k8s, local-docker) and non-containerized providers, and thebuilderPathselection appropriately reflects where the builder will be located for each scenario.
64-73: Conditional toolchain setup appropriately handles provider differences.Skipping apt-get and toolchain setup for local-docker and non-containerized providers is correct, as local-docker uses pre-built images and non-containerized environments are expected to have tools pre-installed.
75-82: Workspace and log file path handling is environment-aware.The conditional
GITHUB_WORKSPACEsetup correctly handles local-docker's special workspace requirements, and theLOG_FILEpath appropriately differentiates between containerized and non-containerized environments.
95-106: Robust builder repository clone with branch fallback.The enhanced clone logic correctly checks for the specified branch using
git ls-remotebefore cloning, and provides a sensible fallback chain (cloud-runner-develop → main → default branch). This prevents failures when the requested branch doesn't exist.
150-161: Defensive Git LFS handling for local-docker environments.The Git LFS pull logic correctly checks for the
.gitdirectory before attempting operations and uses|| truefor graceful degradation. This ensures LFS content is available when possible without failing builds in non-git environments.
162-166: CRLF normalization prevents script execution failures.Converting potential CRLF line endings to LF in shell scripts is essential for local-docker scenarios where scripts may originate from Windows environments. The use of
|| trueensures the build continues even ifsedis unavailable.
173-179: Empty tar creation prevents downstream cache errors.Creating empty tar files when cache doesn't exist is a good defensive practice that prevents errors in subsequent operations expecting these files to be present.
185-189: Cache mirroring enables test assertions in local-docker mode.Copying cache contents back to the workspace allows test workflows to verify cache behavior. The
|| trueensures build success even if cache directories are empty or inaccessible.
192-206: General containerized path executes build correctly.The AWS/K8s containerized path appropriately copies build scripts, executes the entrypoint, and streams logs. The structure is simpler than local-docker since these providers run in controlled environments without Windows compatibility concerns.
Note: Line 203 uses
|| trueto suppress post-build errors, which is more transparent than the misleading message in line 181 but still prevents error propagation.
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 (1)
src/model/cloud-runner/remote-client/index.ts (1)
320-320: CRITICAL: Fix contradictory LFS folder assertion (past review not addressed).Line 320 was flagged in a previous review but remains unfixed. The assertion checks that the LFS folder exists (truthy) but the message says it "should not exist." At this point in the flow (after cloning with LFS skip, before LFS pull), the folder should not exist yet.
Apply this diff to invert the check:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(!fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');
♻️ Duplicate comments (7)
src/model/cloud-runner/workflows/build-automation-workflow.ts (6)
119-119: Replace hardcoded log path with$LOG_FILEfor consistency.Line 119 still hardcodes
/home/job-log.txtinstead of using the$LOG_FILEvariable defined on line 82. This creates an inconsistency because line 130 (non-containerized path) correctly uses"$LOG_FILE". All log operations should reference the same variable to ensure proper log file routing.Apply this diff to use the variable consistently:
-echo "log start" >> /home/job-log.txt +echo "log start" >> "$LOG_FILE"
128-131: Non-containerized path omits builder repository clone, breaking downstream commands.The non-containerized branch (lines 128-131) only initializes logging but never clones the Unity builder repository, while the containerized branch (lines 108-126) includes comprehensive clone logic. Since
builderPathfor non-containerized mode points toprocess.cwd()/dist/index.js(line 60), subsequent invocations likenode ${builderPath} -m remote-cli-post-build(line 214) will fail with "Cannot find module" unless the builder is already present.Clarify the deployment model: if non-containerized providers are expected to have the builder pre-installed in
process.cwd(), document this requirement explicitly. Otherwise, add clone logic similar to the containerized path.Run the following script to check if non-containerized providers are documented to require pre-installed builder:
#!/bin/bash # Search for documentation about non-containerized provider setup requirements rg -i 'non-containerized|local-system|pre-installed.*builder|builder.*pre-install' -g '*.md' -C 3
167-170: Stub binaries silently mask missing tool dependencies.Creating no-op stubs for
node,npm,n, andyarnwhen they're missing will cause any scripts that genuinely require these tools to silently fail. If the local-docker image is expected to provide these tools, their absence indicates a misconfiguration that should be diagnosed, not hidden.Consider logging a warning before creating stubs, or fail the build if these tools are actually required.
Apply this diff to add warnings:
+ echo "WARNING: node not found, creating stub - build may fail if node is required" >&2 if ! command -v node > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/node && chmod +x /usr/local/bin/node; fi + echo "WARNING: npm not found, creating stub - build may fail if npm is required" >&2 if ! command -v npm > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/npm && chmod +x /usr/local/bin/npm; fi + echo "WARNING: n not found, creating stub - build may fail if n is required" >&2 if ! command -v n > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/n && chmod +x /usr/local/bin/n; fi + echo "WARNING: yarn not found, creating stub - build may fail if yarn is required" >&2 if ! command -v yarn > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/yarn && chmod +x /usr/local/bin/yarn; fi
181-181: Post-build error suppression hides critical failures.The
|| echo "Post-build command completed with warnings"pattern converts all post-build failures into a success state with a misleading message. Ifremote-cli-post-buildfails while uploading artifacts or persisting cache, the build will appear to succeed despite incomplete outputs.Either propagate the failure if post-build tasks are critical, or log the actual error and exit code.
Apply this diff to capture and propagate the error:
- node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "Post-build command completed with warnings" + node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || { EXIT_CODE=$?; echo "ERROR: Post-build command failed with exit code $EXIT_CODE" >&2; exit $EXIT_CODE; }
210-214: Non-containerized build path omits Unity build execution entirely.The non-containerized code path (lines 210-214) only logs markers and runs
remote-cli-log-streamandremote-cli-post-build, but never executes the actual Unity build. There's no invocation of/entrypoint.shor any build scripts that would produce build artifacts. This means non-containerized provider builds will silently complete without performing any build work.Verify whether this is intentional (e.g., non-containerized providers are deprecated) or if build execution logic is missing. If non-containerized builds are supported, add the necessary commands to clone build scripts and execute the Unity build.
Run the following script to check if non-containerized providers are actively used or tested:
#!/bin/bash # Search for test cases or documentation using non-containerized providers rg -i 'providerStrategy.*local-system|local-system.*provider|non-containerized.*test' -g '*.ts' -g '*.md' -C 3
203-203: Post-build error suppression may hide failures in non-local-docker containerized builds.Similar to line 181, using
|| trueon line 203 suppresses all post-build failures. While less misleading than the error message on line 181, it still prevents detection of critical issues during artifact upload or cache persistence.Consider capturing and logging the exit code to maintain visibility into post-build failures.
Apply this diff to log failures while continuing:
- node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || true + node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || { EXIT_CODE=$?; echo "WARNING: Post-build command failed with exit code $EXIT_CODE, continuing anyway" >&2; }src/model/cloud-runner/remote-client/index.ts (1)
341-403: Multi-strategy LFS pull is comprehensive, but remove unconventionalvoid 0;statement.The cascading fallback strategy (plain → GIT_PRIVATE_TOKEN → GITHUB_TOKEN → warn and continue) is excellent for resilience across different authentication scenarios.
However, line 356 contains
void 0;which was flagged in a previous review comment as unconventional and marked as addressed in commit a99defa, but it remains in the code. The comment alone is sufficient for a no-op catch block.Apply this diff to remove the statement:
} catch { /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/model/cloud-runner/remote-client/index.ts(4 hunks)src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/build-automation-workflow.ts
🧬 Code graph analysis (1)
src/model/cloud-runner/workflows/build-automation-workflow.ts (2)
src/model/index.ts (1)
CloudRunner(27-27)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: WebGL on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: WebGL on 6000.0.36f1 (via Build Profile)
- GitHub Check: WebGL on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: WebGL on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: WebGL on 2022.3.13f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
🔇 Additional comments (8)
src/model/cloud-runner/workflows/build-automation-workflow.ts (2)
53-60: LGTM: Container-awareness logic is clear and well-structured.The derivation of
isContainerizedfromproviderStrategyand the corresponding builder path selection correctly distinguishes containerized environments from non-containerized ones.
62-88: LGTM: Conditional command execution properly handles different provider strategies.The workflow correctly adapts toolchain setup, workspace configuration, and logging paths based on whether the provider is containerized and whether it's local-docker. The parameter passing to
setupCommandsandBuildCommandsis consistent.src/model/cloud-runner/remote-client/index.ts (6)
35-78: LGTM! Log streaming improvements address past review concerns.The K8s-aware stdout configuration, consistent newline handling, and removal of the non-existent
flushcheck resolve the issues flagged in previous reviews. The comments clearly explain why stdout writes are conditional and why explicit flushing isn't needed.
85-139: LGTM! Defensive cache push guards improve robustness.The existence and content checks before cache operations, combined with non-throwing error handling, prevent spurious failures while maintaining observability through explicit logging. This is well-suited for resilient CI/CD pipelines.
141-150: LGTM! Consistent defensive pattern for cleanup.The existence checks and skip logging align with the defensive cache-push strategy and prevent cleanup failures from affecting build success.
157-172: LGTM! Multi-target activation logging strategy is well-explained.Writing the success marker to stdout, file loggers, and CloudRunnerLogger ensures the message is captured across all provider strategies (K8s, local, etc.). The detailed comments effectively document the rationale.
280-306: LGTM! Enhanced checkout with comprehensive fallbacks.The pre-fetch of tags and PR refs followed by multi-level fallback logic (direct SHA → explicit fetch → branch → origin/branch) significantly improves resilience for missing refs. The catch blocks are properly structured—parameterless when errors are discarded, named when rethrown.
408-440: LGTM! Retained workspace enhancements mirror bootstrap improvements.The consistent application of enhanced ref fetching, SHA-based checkout with fallbacks, and proper catch block structure across both bootstrap and retained workspace paths improves maintainability. The explicit cache key logging aids debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (13)
src/model/cloud-runner/services/hooks/container-hook-service.ts (7)
40-63: Avoid deleting local artifacts when remote upload fails.The
rmat line 57-59 executes regardless of whether theaws s3 cpat lines 52-56 succeeded, risking cache loss on upload failure. Chain thermwith&&so it only runs after successful upload.Apply this diff:
- } || true - rm /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ + } && rm /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } || true + }
86-96: aws-s3-pull-build: missing endpoint flag on cp + inconsistent s3:// usage.The
aws s3 cpat line 90 lacks$ENDPOINT_ARGS, and thelscommands at lines 88-89 omit thes3://prefix. This breaks LocalStack/alternate endpoints and can misinterpret paths.Apply this diff:
- aws $ENDPOINT_ARGS s3 ls ${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/ || true - aws $ENDPOINT_ARGS s3 ls ${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/build || true - aws s3 cp s3://${ + aws $ENDPOINT_ARGS s3 ls s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/ || true + aws $ENDPOINT_ARGS s3 ls s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/build || true + aws $ENDPOINT_ARGS s3 cp s3://${ CloudRunner.buildParameters.awsStackName }/cloud-runner-cache/$CACHE_KEY/build/build-$BUILD_GUID_TARGET.tar${
194-208: aws-s3-pull-cache: ls without s3://, brittle object selection.The
lscommands lacks3://prefix, variables are unquoted, and object selection doesn't guard against empty results before attemptingcp.Apply this diff:
- aws $ENDPOINT_ARGS s3 ls ${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/ || true - aws $ENDPOINT_ARGS s3 ls ${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/ || true - BUCKET1="${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/Library/" - aws $ENDPOINT_ARGS s3 ls $BUCKET1 || true - OBJECT1="$(aws $ENDPOINT_ARGS s3 ls $BUCKET1 | sort | tail -n 1 | awk '{print $4}' || '')" - aws $ENDPOINT_ARGS s3 cp s3://$BUCKET1$OBJECT1 /data/cache/$CACHE_KEY/Library/ || true + aws $ENDPOINT_ARGS s3 ls s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/ || true + aws $ENDPOINT_ARGS s3 ls s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/ || true + BUCKET1="s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/Library/" + aws $ENDPOINT_ARGS s3 ls "$BUCKET1" || true + OBJECT1="$(aws $ENDPOINT_ARGS s3 ls "$BUCKET1" | sort | tail -n 1 | awk '{print $4}')" + [ -n "$OBJECT1" ] && aws $ENDPOINT_ARGS s3 cp "${BUCKET1}${OBJECT1}" "/data/cache/$CACHE_KEY/Library/" || true - BUCKET2="${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/lfs/" - aws $ENDPOINT_ARGS s3 ls $BUCKET2 || true - OBJECT2="$(aws $ENDPOINT_ARGS s3 ls $BUCKET2 | sort | tail -n 1 | awk '{print $4}' || '')" - aws $ENDPOINT_ARGS s3 cp s3://$BUCKET2$OBJECT2 /data/cache/$CACHE_KEY/lfs/ || true + BUCKET2="s3://${CloudRunner.buildParameters.awsStackName}/cloud-runner-cache/$CACHE_KEY/lfs/" + aws $ENDPOINT_ARGS s3 ls "$BUCKET2" || true + OBJECT2="$(aws $ENDPOINT_ARGS s3 ls "$BUCKET2" | sort | tail -n 1 | awk '{print $4}')" + [ -n "$OBJECT2" ] && aws $ENDPOINT_ARGS s3 cp "${BUCKET2}${OBJECT2}" "/data/cache/$CACHE_KEY/lfs/" || true
213-222: rclone-upload-build: rm unconditionally deletes local file after copy.The
rmat lines 217-219 executes regardless of whetherrclone copyat lines 214-216 succeeded. Gate the deletion on copy success.Apply this diff:
rclone copy /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } ${CloudRunner.buildParameters.rcloneRemote}/cloud-runner-cache/$CACHE_KEY/build/ || true - rm /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ + } ${CloudRunner.buildParameters.rcloneRemote}/cloud-runner-cache/$CACHE_KEY/build/ && rm /data/cache/$CACHE_KEY/build/build-${CloudRunner.buildParameters.buildGuid}.tar${ CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } || true + }
231-237: rclone copy destination is incorrect.
rclone copyexpects the destination to be a directory, not a file path. The current syntax at lines 231-237 will create a nested directory structure.Apply this diff:
rclone copy ${ CloudRunner.buildParameters.rcloneRemote }/cloud-runner-cache/$CACHE_KEY/build/build-$BUILD_GUID_TARGET.tar${ CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } /data/cache/$CACHE_KEY/build/build-$BUILD_GUID_TARGET.tar${ - CloudRunner.buildParameters.useCompressionStrategy ? '.lz4' : '' - } || true + } /data/cache/$CACHE_KEY/build/ || true
249-260: rclone-upload-cache: rm unconditionally deletes local cache after copy.The
rm -rcommands at lines 253 and 258 execute regardless of whether the precedingrclone copycommands succeeded.Apply the same fix pattern: chain
rmwith&&after successful copy.
301-318: Hook filtering:skipCachedoes not disable rclone caching.When
skipCacheis true, AWS hooks are correctly excluded. However, rclone hooks are not gated by this flag. For consistency, extend the filtering logic to exclude rclone hooks when cache is disabled or whenrcloneRemoteis not configured:const shouldIncludeAwsHooks = isContainerized && !CloudRunner.buildParameters?.skipCache && (provider === 'aws' || Boolean(hasAwsCreds)); - const filteredBuiltIns = shouldIncludeAwsHooks - ? builtInContainerHooks - : builtInContainerHooks.filter((x) => x.image !== 'amazon/aws-cli'); + const shouldIncludeRcloneHooks = + !CloudRunner.buildParameters?.skipCache && Boolean(CloudRunner.buildParameters?.rcloneRemote); + const filteredBuiltIns = builtInContainerHooks.filter((x) => { + if (x.image === 'amazon/aws-cli') return shouldIncludeAwsHooks; + if (x.image === 'rclone/rclone') return shouldIncludeRcloneHooks; + return true; + });src/model/cloud-runner/workflows/build-automation-workflow.ts (4)
82-83: Use$LOG_FILEconsistently instead of hard‑coding/home/job-log.txt.You export
LOG_FILE(line 82) but still write and stream logs via the literal/home/job-log.txtin multiple places (lines 119, 171–187, 204–209). That splits log path configuration between an env var and hard‑coded string, making future changes error‑prone.Consider switching those call sites to use
"$LOG_FILE"so the log destination is controlled in exactly one place. For example:-echo "log start" >> /home/job-log.txt +echo "log start" >> "$LOG_FILE" ... - { echo "game ci start"; echo "game ci start" >> /home/job-log.txt; ...; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt + { echo "game ci start"; echo "game ci start" >> "$LOG_FILE"; ...; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE"This also keeps containerized and non‑containerized paths symmetrical.
Also applies to: 117-121, 171-187, 204-209
167-170: No‑op stubs for node/npm/n/yarn hide real configuration errors.These branches create dummy
node,npm,n, andyarnbinaries that simplyexit 0if the tools are missing. In that case, every subsequent Node‑based step (includingremote-cli-log-stream/remote-cli-post-build) silently does nothing while the script continues as if successful.Instead of masking the problem, either fail fast or at least emit a loud warning and only stub behind an explicit opt‑in:
- if ! command -v node > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/node && chmod +x /usr/local/bin/node; fi + if ! command -v node > /dev/null 2>&1; then + echo "ERROR: 'node' binary is missing in local-docker image; remote-cli cannot run." >&2 + exit 1 + fi(and similarly for
npm,n, andyarn, or guard the stubbing behind a dedicated env flag).
181-184: Post‑build failures are fully swallowed, potentially hiding critical issues.Both containerized branches treat
remote-cli-post-buildfailures as success:
- Local‑docker:
... || echo "Post-build command completed with warnings".- Generic container:
... || true.If post‑build handles artifact upload or cache persistence, a non‑zero exit will still yield a “successful” job, which is very hard to diagnose.
At minimum, capture and log the exit code; if post‑build is critical, propagate failure:
- set +e - node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "Post-build command completed with warnings" - set -e + set +e + node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt + POST_BUILD_EXIT_CODE=$? + if [ "$POST_BUILD_EXIT_CODE" -ne 0 ]; then + echo "WARNING: remote-cli-post-build failed with exit code $POST_BUILD_EXIT_CODE" >&2 + # If post-build is critical, uncomment to fail the job: + # exit "$POST_BUILD_EXIT_CODE" + fi + set -eand similarly replace
|| truein the non‑local‑docker branch with explicit exit‑code handling.Also applies to: 205-206
212-217: Non‑container providers never execute the Unity build entrypoint.The non‑container branch of
BuildCommandsjust:
- writes a “game ci start” marker to
$LOG_FILE,- runs
remote-cli-log-streamfor 3 seconds, and- calls
remote-cli-post-build.It never stages the default build script or runs
/entrypoint.sh, solocal/local-systemproviders will effectively perform no Unity build but still run post‑build and can report success.Either:
- mirror the containerized flow here (stage the default build script and call
/entrypoint.sh, piping throughremote-cli-log-stream), or- clearly document that non‑container providers must execute the actual build outside this workflow before invoking
BuildAutomationWorkflow.As it stands, this is a silent no‑op for non‑container providers.
src/model/cloud-runner/remote-client/index.ts (2)
286-327: Fix inverted LFS assertion and misleading catch binding name.Two issues here:
- The LFS assertion contradicts its message.
The message says “should not exist before caching” but the condition asserts existence:assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');To make the check and message consistent, invert the condition:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(!fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');
- The catch binding
_erroris used but the underscore suggests “unused” and conflicts with lint rules:} catch (_error) { if ((targetBranch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); } else { throw _error; } }Rename it to
errorand update the throw:- } catch (_error) { + } catch (error) { if ((targetBranch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); } else { - throw _error; + throw error; } }
347-409: Avoid persisting tokens in global git config and remove the no-opvoid 0statement.The LFS pull strategy fallback is a good robustness improvement, but two changes are needed:
Security: tokens persisting in global git config.
The code usesgit config --global url."https://${token}@github.com/".insteadOf ...to store tokens. This embeds secrets into~/.gitconfig, which persists beyond the job—especially problematic in Retained Workspace mode where the same environment is reused. Use--local(repo-scoped) orgit -c(command-only, no persistence) instead:- await CloudRunnerSystem.Run(`git config --global url."https://${gitPrivateToken}@github.com/".insteadOf "https://github.com/"`) + await CloudRunnerSystem.Run(`git config --local url."https://${gitPrivateToken}@github.com/".insteadOf "https://github.com/"`)Or pass the config inline and avoid writing it at all:
- await CloudRunnerSystem.Run(`git lfs pull`, true) + await CloudRunnerSystem.Run(`git -c url."https://${gitPrivateToken}@github.com/".insteadOf="https://github.com/" lfs pull`, true)Remove redundant
void 0in the best-effort catch block.
The first catch statement has an unnecessaryvoid 0;statement; the comment is sufficient. Remove it.
🧹 Nitpick comments (2)
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
53-61: Hard‑coded containerization check is brittle with new providers.
isContainerizedcurrently depends on a fixed list of strategies (aws,k8s,local-docker). With the new plugin/provider system, any additional containerized provider will default into the non‑container path unless this list is updated, changing builder path, setup, and logging behavior.Consider centralizing this decision (e.g., via a
Provider.isContainerizedflag or a helper in the options layer) so that new providers don’t have to remember to update this switch as well.Also applies to: 64-83
src/model/cloud-runner/remote-client/index.ts (1)
415-447: Consider reusingpullLatestLFS(andskipLfs) logic in retained workspace path.The retained workspace flow now correctly fetches all refs/tags and has improved SHA/branch fallback, which is good. However, it still calls:
await CloudRunnerSystem.Run(`git lfs pull`); await CloudRunnerSystem.Run(`git lfs checkout || true`);directly, ignoring the new
skipLfsflag and the best‑effort/token‑aware logic you added inpullLatestLFS. To keep behavior consistent and avoid retained‑workspace builds failing when LFS auth is missing (while fresh clones would just warn and continue), consider:
- calling
await RemoteClient.pullLatestLFS();here instead of rawgit lfs pull, or- at least honoring
CloudRunner.buildParameters.skipLfsand making this pull best‑effort in the same way.This would align retained‑workspace behavior with the main checkout path and reduce surprising differences between modes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (3)
src/model/cloud-runner/remote-client/index.ts(4 hunks)src/model/cloud-runner/services/hooks/container-hook-service.ts(3 hunks)src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/build-automation-workflow.ts
🧬 Code graph analysis (2)
src/model/cloud-runner/services/hooks/container-hook-service.ts (1)
src/model/index.ts (1)
CloudRunner(27-27)
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (K8s)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (LocalStack)
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: WebGL on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: iOS on 2021.3.32f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: WebGL on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
🔇 Additional comments (3)
src/model/cloud-runner/services/hooks/container-hook-service.ts (1)
361-365: LGTM: sensible default for allowFailure.Setting
allowFailuretofalsewhen undefined ensures consistent hook behavior and prevents steps from silently failing.src/model/cloud-runner/remote-client/index.ts (2)
35-77: K8s log streaming behavior looks good; verify stdout duplication is acceptable.The per-line log handling, newline fixes, and K8s‐specific stdout writes are sensible and align with the stated goals. One thing to double‑check: if
CloudRunnerLogger.logalso writes to stdout, K8s runs will now see each line twice on stdout (direct write + logger). If that’s not desired, you may want to gate one of them behind the provider check or adjustCloudRunnerLogger.logbehavior for this path.
83-177: Post‑build guards and success marker are solid and align with “best‑effort” semantics.The added existence/contents checks for Library/Build caches, guarded cleanup of the unique job folder, and wrapping all post‑build work in a non‑failing try/catch are all good improvements. Emitting
Activation successfulvia stdout plus both loggers ensures the marker is reliably present for tests and log consumers.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/model/cloud-runner/services/core/shared-workspace-locking.ts (1)
259-279: FixGetWorkspaceTimestamplogic: current implementation returnsNaNand breaks workspace retention ordering.Keys are created as
${timestamp}_${workspace}_workspace(seeCreateWorkspace), butGetWorkspaceTimestampcurrently:
- Returns
Number(workspace.split('_')[1])whenworkspacecontains_, which is the workspace name (extracted byGetAllWorkspaces), not the timestamp.- The condition
workspace.split('_').length > 0is always true for non-empty strings, making the fallback unreachable.- This means the function always returns
Number(undefined)=NaN.In
IsWorkspaceBelowMax, theseNaNtimestamps are used for sorting workspaces by age, breaking retention logic and making workspace ordering unreliable.A safer implementation extracts the timestamp correctly:
- public static async GetWorkspaceTimestamp( - workspace: string, - buildParametersContext: BuildParameters, - ): Promise<Number> { - if (workspace.split(`_`).length > 0) { - return Number(workspace.split(`_`)[1]); - } + public static async GetWorkspaceTimestamp( + workspace: string, + buildParametersContext: BuildParameters, + ): Promise<number> { + const parts = workspace.split(`_`); + // Case 1: caller passed the full key, e.g. "timestamp_workspace_workspace" + if (parts.length >= 3 && /^\d+$/.test(parts[0])) { + return Number(parts[0]); + } @@ - return ( - await SharedWorkspaceLocking.listObjects( - `${SharedWorkspaceLocking.workspacePrefix}${buildParametersContext.cacheKey}/`, - ) - ) - .map((x) => x.replace(`/`, ``)) - .filter((x) => x.includes(workspace) && x.endsWith(`_workspace`)) - .map((x) => Number(x))[0]; + const matches = ( + await SharedWorkspaceLocking.listObjects( + `${SharedWorkspaceLocking.workspacePrefix}${buildParametersContext.cacheKey}/`, + ) + ) + .map((x) => x.replace(`/`, ``)) + .filter((x) => x.includes(workspace) && x.endsWith(`_workspace`)); + + if (matches.length === 0) { + throw new Error(`Workspace marker not found for ${workspace}`); + } + + const [timestampString] = matches[0].split(`_`); + return Number(timestampString); }This ensures
IsWorkspaceBelowMaxsorts workspaces by actual creation time for deterministic retention.
♻️ Duplicate comments (12)
src/model/cloud-runner/providers/aws/services/task-service.ts (2)
212-218: ListObjectsV2Command missing pagination for >1000 objects.Despite being flagged in previous reviews, the S3 path still lacks pagination.
ListObjectsV2Commandreturns a maximum of 1000 objects per request, silently truncating results when more locks exist. This causes incorrect locking behavior when the bucket contains many locks.Apply this diff to add pagination:
const s3 = AwsClientFactory.getS3(); - const listRequest = { - Bucket: CloudRunner.buildParameters.awsStackName, - }; - - const results = await s3.send(new ListObjectsV2Command(listRequest)); - - return (results.Contents || []).map((object) => ({ Key: object.Key || '' })); + const allObjects: Array<{ Key: string }> = []; + let continuationToken: string | undefined; + do { + const results = await s3.send( + new ListObjectsV2Command({ + Bucket: CloudRunner.buildParameters.awsStackName, + ContinuationToken: continuationToken, + }), + ); + allObjects.push(...(results.Contents || []).map((obj) => ({ Key: obj.Key || '' }))); + continuationToken = results.IsTruncated ? results.NextContinuationToken : undefined; + } while (continuationToken); + + return allObjects;
110-112: DescribeTasksCommand still exceeds 100-task limit.Despite being marked as addressed in previous reviews, the code still passes all
taskArnsto a singleDescribeTasksCommandcall. AWS enforces a maximum of 100 task ARNs per request, causing validation errors when a cluster has more than 100 tasks.Apply this diff to batch the requests:
if (taskArns.length > 0) { - const describeInput = { tasks: taskArns, cluster: element }; - const describeList = (await ecs.send(new DescribeTasksCommand(describeInput))).tasks || []; + const describeList: Task[] = []; + for (let i = 0; i < taskArns.length; i += 100) { + const batch = taskArns.slice(i, i + 100); + const describeInput = { tasks: batch, cluster: element }; + const response = await ecs.send(new DescribeTasksCommand(describeInput)); + describeList.push(...(response.tasks || [])); + } if (describeList.length === 0) {src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts (2)
26-56: Stabilize rclone/bash gating: always probe tools and gate purely on availability.Right now:
execSync('rclone version')/bash --versiononly run when not in CI.- In CI,
shouldRunRclonedepends onGITHUB_ACTIONS+ presence of a remote env var, even though the test later falls back to a local remote (local:./temp/rclone-remote).This leads to:
- No validation that rclone/bash are actually present on CI.
- Skipping the test on CI when env remotes are absent, even though the local fallback remote would work.
You can make the gating more robust and simpler by:
- Always probing for rclone (and bash on Windows), regardless of CI.
- Computing the remote once.
- Gating purely on tool availability:
- const isCI = process.env.GITHUB_ACTIONS === 'true'; const isWindows = process.platform === 'win32'; let rcloneAvailable = false; let bashAvailable = !isWindows; // assume available on non-Windows - if (!isCI) { - try { - const { execSync } = require('child_process'); - execSync('rclone version', { stdio: 'ignore' }); - rcloneAvailable = true; - } catch { - rcloneAvailable = false; - } - if (isWindows) { - try { - const { execSync } = require('child_process'); - execSync('bash --version', { stdio: 'ignore' }); - bashAvailable = true; - } catch { - bashAvailable = false; - } - } - } - - const hasRcloneRemote = Boolean(process.env.RCLONE_REMOTE || process.env.rcloneRemote); - const shouldRunRclone = (isCI && hasRcloneRemote) || (rcloneAvailable && (!isWindows || bashAvailable)); + try { + const { execSync } = require('node:child_process'); + execSync('rclone version', { stdio: 'ignore' }); + rcloneAvailable = true; + } catch { + rcloneAvailable = false; + } + if (isWindows) { + try { + const { execSync } = require('node:child_process'); + execSync('bash --version', { stdio: 'ignore' }); + bashAvailable = true; + } catch { + bashAvailable = false; + } + } + + const hasRcloneRemote = Boolean(process.env.RCLONE_REMOTE || process.env.rcloneRemote); + const remote = hasRcloneRemote ? process.env.RCLONE_REMOTE || process.env.rcloneRemote : 'local:./temp/rclone-remote'; + const shouldRunRclone = rcloneAvailable && (!isWindows || bashAvailable); @@ - const remote = process.env.RCLONE_REMOTE || process.env.rcloneRemote || 'local:./temp/rclone-remote'; + // reuse computed `remote`This keeps
hasRcloneRemoteavailable if you ever want different behavior when a real remote is configured, while making the decision to run the test depend solely on whether the required tools are present.
55-83: Reduce the Jest timeout from ~11.6 days to a reasonable upper bound.The test timeout of
1_000_000_000ms (~11.6 days) can easily mask hung tests and slow failures in CI. This is far beyond what’s needed even for a heavy integration build.Consider capping this around 15–30 minutes, e.g.:
- }, 1_000_000_000); + }, 30 * 60 * 1000);(or
15 * 60 * 1000if you’re comfortable with a tighter bound).src/model/cloud-runner/providers/k8s/kubernetes-pods.ts (3)
13-19: Guard event fetching so diagnostics don’t mask real pod failures
kubeClient.listNamespacedEvent(namespace)is awaited directly; if it throws (RBAC/timeout/API error),IsPodRunningwill fail before reporting the actual pod/container failure, even though events are only diagnostic.Consider wrapping the fetch in a try/catch, logging a warning and falling back to an empty array so pod failure analysis still completes:
- const events = (await kubeClient.listNamespacedEvent(namespace)).body.items - .filter((x) => x.involvedObject?.name === podName) - .map((x) => ({ - message: x.message || '', - reason: x.reason || '', - type: x.type || '', - })); + let events: Array<{ message: string; reason: string; type: string }> = []; + try { + events = (await kubeClient.listNamespacedEvent(namespace)).body.items + .filter((x) => x.involvedObject?.name === podName) + .map((x) => ({ + message: x.message || '', + reason: x.reason || '', + type: x.type || '', + })); + } catch (eventError) { + CloudRunnerLogger.logWarning(`Failed to fetch pod events for ${podName}: ${eventError}`); + }
34-57: Multi‑container pods: exit‑code handling only reflects the last container
containerExitCodeandcontainerSucceededare reassigned on each iteration overcontainerStatuses, so after the loop they only describe the last container. In multi‑container pods, this can misclassify:
- Earlier failed container + later successful container ⇒ treated as overall success (non‑fatal path at lines 68–85).
- Earlier successful container + later failed container ⇒ classified correctly, but behavior now depends on array order.
Instead, aggregate across all containers and only treat the pod as “succeeded” if all terminated containers have exit code 0. For example:
- let containerExitCode: number | undefined; - let containerSucceeded = false; + let containerExitCode: number | undefined; + let containerSucceeded = false; + let anyTerminated = false; + let allContainersSucceeded = true; @@ - if (containerStatuses.length > 0) { - for (const [index, cs] of containerStatuses.entries()) { + if (containerStatuses.length > 0) { + for (const [index, cs] of containerStatuses.entries()) { @@ - if (cs.state?.terminated) { - const exitCode = cs.state.terminated.exitCode; - containerExitCode = exitCode; - if (exitCode === 0) { - containerSucceeded = true; - } + if (cs.state?.terminated) { + anyTerminated = true; + const exitCode = cs.state.terminated.exitCode; + if (exitCode !== 0) { + allContainersSucceeded = false; + containerExitCode = exitCode; // keep a non-zero exit code for diagnostics + } errorDetails.push( `Container ${index} (${cs.name}) terminated: ${cs.state.terminated.reason} - ${ cs.state.terminated.message || '' } (exit code: ${exitCode})`, ); } }); } + + if (anyTerminated && allContainersSucceeded) { + containerExitCode = 0; + containerSucceeded = true; + }This keeps later checks (
containerSucceeded && containerExitCode === 0, exit‑code‑137 handling, etc.) meaningful at the pod level.
95-121: Retry loop still assumes single-container pods (only inspects index 0)Inside the wait‑and‑retry loop you only check
updatedPod.status.containerStatuses[0]. For multi‑container pods this can again misclassify:
- First container succeeds (exit 0), another container fails ⇒ you’ll treat the pod as non‑fatal and return
false.- First container fails, another succeeds ⇒ behavior depends on which container happens to be index 0.
To align with the multi‑container handling above, consider aggregating across all containers here as well:
- if (updatedPod?.status?.containerStatuses && updatedPod.status.containerStatuses.length > 0) { - const updatedContainerStatus = updatedPod.status.containerStatuses[0]; - if (updatedContainerStatus.state?.terminated) { - const updatedExitCode = updatedContainerStatus.state.terminated.exitCode; - if (updatedExitCode === 0) { - CloudRunnerLogger.logWarning( - `Pod ${podName} container succeeded (exit code 0) after waiting. PreStopHook failure is non-fatal.`, - ); - - return false; // Pod is not running, but container succeeded - } else { - CloudRunnerLogger.log( - `Pod ${podName} container failed with exit code ${updatedExitCode} after waiting.`, - ); - errorDetails.push(`Container terminated after wait: exit code ${updatedExitCode}`); - containerExitCode = updatedExitCode; - containerSucceeded = false; - break; - } - } - } + if (updatedPod?.status?.containerStatuses && updatedPod.status.containerStatuses.length > 0) { + let allUpdatedSucceeded = true; + let anyUpdatedTerminated = false; + + for (const cs of updatedPod.status.containerStatuses) { + if (cs.state?.terminated) { + anyUpdatedTerminated = true; + const updatedExitCode = cs.state.terminated.exitCode; + if (updatedExitCode !== 0) { + allUpdatedSucceeded = false; + errorDetails.push( + `Container (${cs.name}) terminated after wait: exit code ${updatedExitCode}`, + ); + containerExitCode = updatedExitCode; + containerSucceeded = false; + } + } + } + + if (anyUpdatedTerminated) { + if (allUpdatedSucceeded) { + CloudRunnerLogger.logWarning( + `Pod ${podName} all containers succeeded (exit code 0) after waiting. PreStopHook failure is non-fatal.`, + ); + return false; // Pod is not running, but all containers succeeded + } + break; // At least one container failed after waiting + } + }This keeps the retry behavior consistent with the main status evaluation and safer for multi‑container workloads.
src/model/cloud-runner/remote-client/index.ts (3)
319-325: Remove underscore prefix from catch binding that is used.The catch binding on line 319 is named
_error(suggesting it's unused), but it's thrown on line 323. Rename toerrorfor consistency with the convention used elsewhere in the file.- } catch (_error) { + } catch (error) { if ((targetBranch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); } else { - throw _error; + throw error; } }
329-329: Fix contradictory LFS folder assertion.The assertion message says "LFS folder should not exist before caching" but
assert(fs.existsSync(...))asserts that it does exist. Either the message is wrong, or the check should be inverted.Based on the context (this is before caching LFS files), the folder likely should exist (created by
git lfs install). Update the message to accurately reflect the intent:- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder must exist after git lfs install');
363-366: Remove unconventionalvoid 0statement.The
void 0;statement is an unconventional no-op. The comment alone is sufficient to indicate intentional empty catch block.} catch { /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; }src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
73-101: Handle S3 pagination inlistObjectsto avoid silent truncation beyond 1,000 keys.The S3 branch of
listObjectsissues a singleListObjectsV2CommandwithoutContinuationToken. For prefixes with more than 1,000 objects,result.Contents/CommonPrefixeswill be truncated, which will break workspace discovery, lock detection, cleanup, and timestamp ordering.You can loop over
NextContinuationTokenand accumulate results, e.g.:- const result = await SharedWorkspaceLocking.s3.send( - new ListObjectsV2Command({ Bucket: bucket, Prefix: prefix, Delimiter: '/' }), - ); - const entries: string[] = []; - for (const p of result.CommonPrefixes || []) { - if (p.Prefix) entries.push(p.Prefix.slice(prefix.length)); - } - for (const c of result.Contents || []) { - if (c.Key && c.Key !== prefix) entries.push(c.Key.slice(prefix.length)); - } - return entries; + const entries: string[] = []; + let token: string | undefined; + do { + const result = await SharedWorkspaceLocking.s3.send( + new ListObjectsV2Command({ + Bucket: bucket, + Prefix: prefix, + Delimiter: '/', + ContinuationToken: token, + }), + ); + for (const p of result.CommonPrefixes || []) { + if (p.Prefix) entries.push(p.Prefix.slice(prefix.length)); + } + for (const c of result.Contents || []) { + if (c.Key && c.Key !== prefix) entries.push(c.Key.slice(prefix.length)); + } + token = result.NextContinuationToken; + } while (token); + return entries;This keeps
listObjectssafe even when caches grow large.
14-16: Avoid shell stringexec('rclone …'): switch toexecFilewith arg array to prevent injection and quoting issues.
rclone()builds a shell command string (exec(\rclone ${command}`)) wherebucket, prefixes, workspace names, etc. are interpolated. If any of these contain spaces or shell metacharacters, you risk command injection or broken calls; this affects all rclone usages (bucket existence checks,lsjson,touch,delete`).Recommend switching to
execFileand passing arguments as an array withshell: false, e.g.:-import { promisify } from 'node:util'; -import { exec as execCallback } from 'node:child_process'; -const exec = promisify(execCallback); +import { promisify } from 'node:util'; +import { execFile } from 'node:child_process'; +const execFileAsync = promisify(execFile); @@ - private static async rclone(command: string): Promise<string> { - const { stdout } = await exec(`rclone ${command}`); - return stdout.toString(); - } + private static async rclone(...args: string[]): Promise<string> { + const { stdout } = await execFileAsync('rclone', args, { + shell: false, + windowsHide: true, + }); + return stdout.toString(); + } @@ - await SharedWorkspaceLocking.rclone(`lsf ${bucket}`); + await SharedWorkspaceLocking.rclone('lsf', bucket); @@ - await SharedWorkspaceLocking.rclone(`mkdir ${bucket}`); + await SharedWorkspaceLocking.rclone('mkdir', bucket); @@ - const output = await SharedWorkspaceLocking.rclone(`lsjson ${path}`); + const output = await SharedWorkspaceLocking.rclone('lsjson', path); @@ - await (SharedWorkspaceLocking.useRclone - ? SharedWorkspaceLocking.rclone(`touch ${SharedWorkspaceLocking.bucket}/${key}`) + await (SharedWorkspaceLocking.useRclone + ? SharedWorkspaceLocking.rclone('touch', `${SharedWorkspaceLocking.bucket}/${key}`) @@ - await (SharedWorkspaceLocking.useRclone - ? SharedWorkspaceLocking.rclone(`touch ${SharedWorkspaceLocking.bucket}/${key}`) + await (SharedWorkspaceLocking.useRclone + ? SharedWorkspaceLocking.rclone('touch', `${SharedWorkspaceLocking.bucket}/${key}`) @@ - await (SharedWorkspaceLocking.useRclone - ? SharedWorkspaceLocking.rclone(`delete ${SharedWorkspaceLocking.bucket}/${key}`) + await (SharedWorkspaceLocking.useRclone + ? SharedWorkspaceLocking.rclone('delete', `${SharedWorkspaceLocking.bucket}/${key}`) @@ - await (SharedWorkspaceLocking.useRclone - ? SharedWorkspaceLocking.rclone( - `delete ${SharedWorkspaceLocking.bucket}/${SharedWorkspaceLocking.workspacePrefix}${buildParametersContext.cacheKey}/${file}`, - ) + await (SharedWorkspaceLocking.useRclone + ? SharedWorkspaceLocking.rclone( + 'delete', + `${SharedWorkspaceLocking.bucket}/${SharedWorkspaceLocking.workspacePrefix}${buildParametersContext.cacheKey}/${file}`, + ) @@ - for (const file of files.filter((x) => x.includes(`_${workspace}_`))) { - await (SharedWorkspaceLocking.useRclone - ? SharedWorkspaceLocking.rclone(`delete ${SharedWorkspaceLocking.bucket}/${prefix}${file}`) + for (const file of files.filter((x) => x.includes(`_${workspace}_`))) { + await (SharedWorkspaceLocking.useRclone + ? SharedWorkspaceLocking.rclone('delete', `${SharedWorkspaceLocking.bucket}/${prefix}${file}`)This keeps behavior the same while eliminating shell injection and quoting problems.
Also applies to: 30-34, 55-58, 81-82, 302-308, 334-338, 345-347, 366-368, 384-388
🧹 Nitpick comments (10)
src/model/cloud-runner/providers/aws/aws-job-stack.ts (1)
1-5: Inline ESLint disable works, but consider a type‑only import insteadThe
// eslint-disable-next-line import/namedsolves the immediate lint issue but adds inline suppression noise. Consider switching to a type‑only import (and ensuring the TS resolver is configured) so the rule can stay enabled without suppression, e.g.:import { CloudFormation, CreateStackCommand, DescribeStackResourcesCommand, DescribeStacksCommand, ListStacksCommand, waitUntilStackCreateComplete, } from '@aws-sdk/client-cloudformation'; import type { CreateStackCommandInput } from '@aws-sdk/client-cloudformation';If your ESLint setup still flags this even with
import type, keeping the suppression is acceptable but should remain rare and intentional.src/model/cloud-runner/workflows/async-workflow.ts (1)
40-40: Consider applying the same robustness pattern to the target repo clone.The builder repo clone now has branch verification and fallbacks (lines 30-39), but the target repo clone on line 40 remains a simple single-line command. For consistency and resilience, consider adding similar branch existence checks and error handling.
Example implementation:
TARGET_BRANCH="${CloudRunner.buildParameters.branch}" TARGET_REPO="${CloudRunnerFolders.targetBuildRepoUrl}" if [ -n "$(git ls-remote --heads "$TARGET_REPO" "$TARGET_BRANCH" 2>/dev/null)" ]; then git clone -q -b "$TARGET_BRANCH" "$TARGET_REPO" /repo || { echo "ERROR: Failed to clone target repository"; exit 1; } else echo "ERROR: Target branch $TARGET_BRANCH not found in repository" exit 1 fisrc/model/cloud-runner/providers/aws/services/task-service.ts (1)
32-83: Consider adding pagination for ListStacksCommand.The method now correctly uses the factory and proper types. However, ListStacksCommand supports pagination via
NextTokenand can return a maximum of 100 stacks per request. While most accounts won't hit this limit, consider adding pagination for robustness.Example pattern (optional):
let nextToken: string | undefined; do { const response = await CF.send(new ListStacksCommand({ NextToken: nextToken })); stacks.push(...(response.StackSummaries?.filter(...) || [])); nextToken = response.NextToken; } while (nextToken);src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts (2)
11-17: Avoid leakingCli.optionsacross tests inCreateParameters.
CreateParametersoverwrites the globalCli.optionsbut never restores it, which can cause subtle cross-test interference if other tests rely on different CLI options or unset state.Consider capturing and restoring the previous options around the call to
BuildParameters.create():async function CreateParameters(overrides: OptionValues | undefined) { - if (overrides) { - Cli.options = overrides; - } - - return await BuildParameters.create(); + const previousOptions = Cli.options; + if (overrides) { + Cli.options = overrides; + } + + try { + return await BuildParameters.create(); + } finally { + Cli.options = previousOptions; + } }
58-61: Simplify Unity version resolution in test overrides.Given
UnityVersioning.determineUnityVersion(projectPath, unityVersion)simply returnsunityVersionwhen it’s not'auto'(seesrc/model/unity-versioning.ts), the current call:unityVersion: UnityVersioning.determineUnityVersion('test-project', UnityVersioning.read('test-project')),reads
ProjectVersion.txtonce and then feeds that value straight back through a no-op branch. For clarity, you could either:
- Exercise the
'auto'path:unityVersion: UnityVersioning.determineUnityVersion('test-project', 'auto'),or
- Just read directly:
unityVersion: UnityVersioning.read('test-project'),Either makes the intent clearer without changing behavior. Based on learnings from
src/model/unity-versioning.ts.src/model/cloud-runner/tests/providers/provider-git-manager.test.ts (1)
1-1: Consider using a type-only import forGitHubUrlInfo.
GitHubUrlInfois used only as a type annotation (line 33), so you can use a type-only import to make the intent clearer and potentially improve build times.Apply this diff:
-import { GitHubUrlInfo } from '../../providers/provider-url-parser'; +import type { GitHubUrlInfo } from '../../providers/provider-url-parser';src/model/cloud-runner/providers/k8s/kubernetes-pods.ts (1)
150-162: Re‑evaluate treating exit code 137 as non‑fatal in CI contextsExit code 137 (SIGKILL/OOM) combined with PreStopHook/grace‑period issues is currently treated as non‑fatal (
return false). In many CI/build scenarios, an OOM or hard kill is exactly what you want to surface as a hard failure so resource issues don’t get silently ignored.If this leniency is intentional, consider at least making it:
- Configurable (e.g., “treat OOM as non‑fatal” flag), or
- Narrowed to very specific known‑benign cases (e.g., only certain pod labels/containers),
so that unexpected OOMs in regular builds still fail loudly.
src/model/cloud-runner/remote-client/index.ts (1)
65-78: Consider guarding the final log call against empty strings.Line 77 calls
CloudRunnerLogger.log(lingeringLine)unconditionally, butlingeringLinecould be an empty string (initialized as''on line 40). This could produce empty log entries. Consider moving it inside theif (lingeringLine)block or adding a guard.- CloudRunnerLogger.log(lingeringLine); + if (lingeringLine) { + CloudRunnerLogger.log(lingeringLine); + }src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
51-71: Align bucket creation region with the S3 client configuration.
ensureBucketExistsderives the bucket region fromInput.region || AWS_REGION || AWS_DEFAULT_REGION || 'us-east-1', but the S3 client itself is configured viaAwsClientFactory.getS3()(which usesInput.region). If action inputs and env vars diverge, you could create the bucket in a region different from the client’s region, causing confusing S3 errors.Consider deriving the region for
CreateBucketCommandfrom the same source asAwsClientFactory.getS3()(or from the client config itself) and avoid mixing in env-based fallbacks here, so that client and bucket regions stay in sync.
392-398:ReadLinesparsing is brittle; consider hardening or bypassing the CLI string format entirely.
ReadLinesnow assumes commands are exactly of the form'aws s3 ls …'or'rclone lsf …'and strips those substrings before inferringbucketandprefix. Any change in call sites (extra flags, reordered tokens, different subcommand) will break this parsing.Given you now have a proper
listObjects(prefix, bucket)abstraction, consider:
- Either moving callers to pass
bucket/prefixdirectly instead of a shell command string, or- At least adding tests for every
ReadLinescall pattern to ensure the simplereplace(...).trim().split('/')logic continues to work.This reduces the chance of subtle regressions if someone tweaks the CLI-style strings later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (10)
src/model/cloud-runner/providers/aws/aws-base-stack.ts(1 hunks)src/model/cloud-runner/providers/aws/aws-job-stack.ts(1 hunks)src/model/cloud-runner/providers/aws/cloud-runner-aws-task-def.ts(1 hunks)src/model/cloud-runner/providers/aws/services/task-service.ts(6 hunks)src/model/cloud-runner/providers/k8s/kubernetes-pods.ts(1 hunks)src/model/cloud-runner/remote-client/index.ts(4 hunks)src/model/cloud-runner/services/core/shared-workspace-locking.ts(8 hunks)src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts(1 hunks)src/model/cloud-runner/tests/providers/provider-git-manager.test.ts(1 hunks)src/model/cloud-runner/workflows/async-workflow.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/model/cloud-runner/providers/aws/aws-base-stack.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/async-workflow.ts
🧬 Code graph analysis (6)
src/model/cloud-runner/workflows/async-workflow.ts (1)
src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)
src/model/cloud-runner/tests/providers/provider-git-manager.test.ts (2)
src/model/cloud-runner/providers/provider-git-manager.ts (1)
ProviderGitManager(25-278)src/model/cloud-runner/providers/provider-url-parser.ts (1)
GitHubUrlInfo(3-10)
src/model/cloud-runner/providers/aws/services/task-service.ts (1)
src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
SharedWorkspaceLocking(17-400)s3(19-26)
src/model/cloud-runner/tests/cloud-runner-rclone-steps.test.ts (4)
src/model/cloud-runner/tests/create-test-parameter.ts (1)
CreateParameters(4-8)src/model/cli/cli.ts (1)
Cli(16-175)src/model/unity-versioning.ts (1)
UnityVersioning(4-32)src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)
src/model/cloud-runner/services/core/shared-workspace-locking.ts (3)
src/model/cloud-runner/providers/aws/aws-client-factory.ts (1)
AwsClientFactory(9-71)src/model/cloud-runner/options/cloud-runner-options.ts (1)
region(53-55)src/model/input.ts (1)
region(48-50)
src/model/cloud-runner/remote-client/index.ts (3)
src/model/cloud-runner/remote-client/remote-client-logger.ts (1)
RemoteClientLogger(7-93)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (K8s)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (LocalStack)
- GitHub Check: Tests
- GitHub Check: WebGL on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: WebGL on 6000.0.36f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: WebGL on 2022.3.13f1
- GitHub Check: WebGL on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
🔇 Additional comments (8)
src/model/cloud-runner/providers/aws/services/task-service.ts (3)
1-19: LGTM! Imports properly restructured.The type-only imports for AWS SDK types and the addition of AwsClientFactory successfully address previous review feedback. The switch to ListObjectsV2Command is correct for pagination support.
134-165: LGTM! Factory integration successful.The method correctly uses
AwsClientFactory.getCloudFormation()and maintains proper error handling.
166-201: LGTM! Complete with proper typing and pagination.The method correctly implements pagination for CloudWatch Logs, uses the factory, and has proper TypeScript types throughout.
src/model/cloud-runner/remote-client/index.ts (4)
35-38: LGTM - K8s stdout encoding setup.Setting the default encoding to UTF-8 is appropriate for consistent character handling. The comment about "unbuffered" is slightly misleading since
setDefaultEncodingcontrols character encoding rather than buffering behavior, but Node.js stdout is already line-buffered on newlines which is sufficient for kubectl log capture.
48-62: LGTM - Log streaming data handler improvements.Good changes: empty lines are now filtered before writing to the log file, newlines are properly appended, and the dead
process.stdout.flushcode has been removed. The K8s-specific stdout writing ensures kubectl logs can capture output.
83-164: Good defensive post-build implementation.The cache push operations are now properly guarded with existence and non-empty checks, preventing errors when folders are missing. The outer try/catch ensures post-build tasks don't fail the entire build. The explicit warnings when skipping operations improve debuggability.
417-449: LGTM - Retained workspace handling improvements.Good enhancements: explicit cache key logging aids debugging, comprehensive ref fetching (including PR refs), and robust SHA/branch fallback logic. The catch binding issues from past reviews have been addressed - line 438 uses parameterless catch and line 442 properly names and throws the error.
src/model/cloud-runner/services/core/shared-workspace-locking.ts (1)
18-29: Overall S3/rclone abstraction and workspace/lock flow look consistent.The refactor to centralize S3 access via
AwsClientFactory.getS3(), abstract paths throughworkspaceBucketRoot/workspacePrefix, and route workspace/lock operations throughlistObjectsplusPutObjectCommand/DeleteObjectCommand(or the rclone equivalents) is coherent and keeps the public API ofSharedWorkspaceLockingintact. The new logic forCreateWorkspace,LockWorkspace,ReleaseWorkspace,CleanupWorkspace, andIsWorkspaceLockedall appear consistent with the{timestamp}_{workspace}_workspace/ lock key format.Apart from the specific issues already called out (rclone exec, pagination, timestamp parsing), the flow and usage of the helpers look good.
Also applies to: 35-50, 102-116, 136-152, 216-257, 281-288, 297-321, 323-351, 353-378, 380-390
| // eslint-disable-next-line import/named | ||
| import { StackResource } from '@aws-sdk/client-cloudformation'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check package.json for @aws-sdk/client-cloudformation version and dependencies
echo "=== Checking package.json for AWS SDK version ==="
if [ -f "package.json" ]; then
cat package.json | jq '.dependencies["@aws-sdk/client-cloudformation"], .devDependencies["@aws-sdk/client-cloudformation"]' 2>/dev/null || echo "jq not available, trying grep"
else
echo "package.json not found"
fi
# Search for other files importing StackResource
echo -e "\n=== Checking other imports of StackResource ==="
rg "import.*StackResource" --type ts --type tsx -n
# Check the actual file in question
echo -e "\n=== Checking the file in question ==="
head -20 src/model/cloud-runner/providers/aws/cloud-runner-aws-task-def.ts 2>/dev/null || echo "File not found"Repository: game-ci/unity-builder
Length of output: 625
🌐 Web query:
AWS SDK v3 client-cloudformation StackResource export documentation
💡 Result:
Short answer — in AWS SDK for JavaScript v3 (package @aws-sdk/client-cloudformation) CloudFormation exports are exposed via the ListExports/ListImports APIs and the Export shape. Use the CloudFormationClient plus ListExportsCommand (and ListImportsCommand) to list exported output names/values; to see a stack's Outputs (including an Output's ExportName) call DescribeStacks/DescribeStackResources. [1][2][3]
References
- @aws-sdk/client-cloudformation (v3) package and usage (ListExportsCommand example). [1]
- CloudFormation API: ListExports (returns Export objects). [2]
- Export structure / Outputs (ExportName, Value, ExportingStackId). [3]
🏁 Script executed:
#!/bin/bash
# Try to find type definitions or source related to StackResource
echo "=== Searching for StackResource references in codebase ==="
rg "StackResource" --type ts -A 2 -B 2
# Check if there's a types directory or definitions file
echo -e "\n=== Looking for AWS SDK type definitions ==="
find . -type f -name "*.d.ts" | grep -i "cloudformation" | head -10
# Check imports more broadly in the file
echo -e "\n=== Full file content ==="
cat src/model/cloud-runner/providers/aws/cloud-runner-aws-task-def.tsRepository: game-ci/unity-builder
Length of output: 5148
🌐 Web query:
AWS SDK JavaScript v3 StackResource type CloudFormation API reference
💡 Result:
The CloudFormation StackResource data type (fields and types) is defined in the AWS CloudFormation API reference. Key members include: LogicalResourceId, ResourceStatus, ResourceType, Timestamp, Description, DriftInformation, ModuleInfo, PhysicalResourceId, ResourceStatusReason, StackId, StackName. See the full API reference for field details and valid values. [1][2]
Sources:
[1] AWS CloudFormation API — StackResource (API Reference).
[2] AWS SDK for JavaScript (CloudFormation) — DescribeStackResources / StackResource structure.
🏁 Script executed:
#!/bin/bash
# Check the exact import patterns in related files
echo "=== Import patterns from @aws-sdk/client-cloudformation ==="
rg "from '@aws-sdk/client-cloudformation'" --type ts -A 1 -B 1 | head -50
# Check if there are any eslint config files
echo -e "\n=== ESLint configuration files ==="
find . -maxdepth 3 -name ".eslintrc*" -o -name "eslint.config.*" | head -5
# Look at package.json eslint config
echo -e "\n=== ESLint config in package.json ==="
cat package.json | jq '.eslintConfig' 2>/dev/null || echo "No eslintConfig in package.json"Repository: game-ci/unity-builder
Length of output: 2761
🌐 Web query:
@aws-sdk/client-cloudformation StackResource named export correct import syntax v3
💡 Result:
There is no named export called "StackResource" in the AWS SDK for JavaScript v3. Use CloudFormationClient and the relevant command (e.g., DescribeStackResourceCommand). Example:
import { CloudFormationClient, DescribeStackResourceCommand } from "@aws-sdk/client-cloudformation";
const client = new CloudFormationClient({});
const cmd = new DescribeStackResourceCommand({ StackName: "my-stack", LogicalResourceId: "MyResource" });
const resp = await client.send(cmd);
(CloudFormation SDK docs and code examples). [1][2]
🏁 Script executed:
#!/bin/bash
# Let's check what type is actually returned by DescribeStackResourcesCommand
echo "=== Checking actual usage in aws-job-stack.ts ==="
cat -n src/model/cloud-runner/providers/aws/aws-job-stack.ts | sed -n '1,100p' | grep -A 5 "StackResources"
# Check the .eslintrc.json to see the eslint config
echo -e "\n=== ESLint configuration ==="
cat .eslintrc.json | head -50Repository: game-ci/unity-builder
Length of output: 1738
🌐 Web query:
AWS SDK v3 DescribeStackResourcesCommand response type StackResources property TypeScript
💡 Result:
Short answer: In AWS SDK for JavaScript v3 (@aws-sdk/client-cloudformation) the DescribeStackResourcesCommand output includes
StackResources?: StackResource[]
where StackResource is an object with properties such as:
- StackName?: string
- StackId?: string
- LogicalResourceId?: string
- PhysicalResourceId?: string
- ResourceType?: string
- Timestamp?: Date
- ResourceStatus?: string
- ResourceStatusReason?: string
- Description?: string
- (plus optional DriftInformation and ModuleInfo structures)
So the TypeScript shape is effectively:
type DescribeStackResourcesCommandOutput = {
StackResources?: Array<{
StackName?: string;
StackId?: string;
LogicalResourceId?: string;
PhysicalResourceId?: string;
ResourceType?: string;
Timestamp?: Date;
ResourceStatus?: string;
ResourceStatusReason?: string;
Description?: string;
DriftInformation?: any;
ModuleInfo?: any;
}>;
// ...other response metadata
};
Sources: AWS DescribeStackResources API (response elements & StackResource structure). [1][2]
References
[1] AWS CloudFormation DescribeStackResources — Response Elements.
[2] DescribeStackResources response structure (fields like ResourceType, Timestamp, ResourceStatus, etc.).
Remove the invalid StackResource import and define the type locally or use the correct import path.
The import { StackResource } from '@aws-sdk/client-cloudformation' is invalid—StackResource is not a named export from the AWS SDK. The eslint rule correctly identifies this problem.
Based on AWS SDK v3 documentation, StackResource is an inline type definition within the DescribeStackResourcesCommand response, not an exported named type. You have two options:
- Define the type locally in this file matching the StackResource structure (LogicalResourceId, PhysicalResourceId, ResourceType, Timestamp, ResourceStatus, etc.)
- Use the response type from
DescribeStackResourcesCommandand extract the array type
Remove the // eslint-disable-next-line import/named directive along with the invalid import, and update the type definitions accordingly. The disable suppresses a legitimate error rather than addressing the underlying issue.
🤖 Prompt for AI Agents
In src/model/cloud-runner/providers/aws/cloud-runner-aws-task-def.ts lines 1-2,
remove the invalid import and the `// eslint-disable-next-line import/named`
line that silences it; `StackResource` is not a named export from
@aws-sdk/client-cloudformation. Replace it by either (A) declaring a local
interface/type (e.g. StackResourceLocal) matching the StackResource shape you
need (LogicalResourceId, PhysicalResourceId, ResourceType, Timestamp,
ResourceStatus, etc.) and use that type wherever StackResource was used, or (B)
import the correct SDK output type (e.g. DescribeStackResourcesCommandOutput
from @aws-sdk/client-cloudformation) and derive the array element type from its
StackResources property (use that derived type in place of StackResource).
Update all type references accordingly and remove the eslint disable comment.
| await CloudRunnerSystem.Run( | ||
| `git config --global url."https://${gitPrivateToken}@github.com/".insteadOf "https://github.com/"`, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find CloudRunnerSystem.Run definition
find . -name "*.ts" -type f | head -20Repository: game-ci/unity-builder
Length of output: 765
🏁 Script executed:
#!/bin/bash
# Search for CloudRunnerSystem class and Run method
rg "class CloudRunnerSystem" -A 20Repository: game-ci/unity-builder
Length of output: 50380
🏁 Script executed:
#!/bin/bash
# Search for the Run method signature
rg "static.*Run\(" -A 10 -B 2Repository: game-ci/unity-builder
Length of output: 50380
🏁 Script executed:
#!/bin/bash
# Check lines 397-399 in the remote-client index.ts file
sed -n '390,405p' src/model/cloud-runner/remote-client/index.tsRepository: game-ci/unity-builder
Length of output: 982
Add suppressLogs parameter to prevent token exposure in git config commands.
The git config commands on lines 376-378 and 398 embed sensitive tokens (gitPrivateToken and GITHUB_TOKEN) directly in URLs. CloudRunnerSystem.Run logs all commands by default, which would expose these tokens in plaintext logs.
Use the suppressLogs parameter (3rd argument) to prevent logging:
await CloudRunnerSystem.Run(
`git config --global url."https://${gitPrivateToken}@github.com/".insteadOf "https://github.com/"`,
+ false,
+ true, // suppressLogs
);Apply the same fix to the GITHUB_TOKEN configuration at line 398.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/model/cloud-runner/remote-client/index.ts around lines 376-378 and also
at line 398, the git config commands embed sensitive tokens (gitPrivateToken and
GITHUB_TOKEN) and are passed to CloudRunnerSystem.Run which logs commands by
default; change those calls to pass the suppressLogs third parameter (e.g., {
suppressLogs: true }) so the commands still execute but are not written to logs,
keeping the token values from appearing in plaintext logs.
| jest.mock('../../providers/provider-git-manager', () => { | ||
| const originalModule = jest.requireActual('../../providers/provider-git-manager'); | ||
|
|
||
| return { | ||
| ...originalModule, | ||
| ProviderGitManager: { | ||
| ...originalModule.ProviderGitManager, | ||
| cloneRepository: jest.fn(), | ||
| updateRepository: jest.fn(), | ||
| getProviderModulePath: jest.fn(), | ||
| }, | ||
| }; | ||
| }); | ||
| const mockProviderGitManager = ProviderGitManager as jest.Mocked<typeof ProviderGitManager>; | ||
|
|
||
| describe('ProviderGitManager', () => { | ||
| const mockUrlInfo: GitHubUrlInfo = { | ||
| type: 'github', | ||
| owner: 'test-user', | ||
| repo: 'test-repo', | ||
| branch: 'main', | ||
| url: 'https://github.com/test-user/test-repo', | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe('cloneRepository', () => { | ||
| it('successfully clones a repository', async () => { | ||
| const expectedResult = { | ||
| success: true, | ||
| localPath: '/path/to/cloned/repo', | ||
| }; | ||
| mockProviderGitManager.cloneRepository.mockResolvedValue(expectedResult); | ||
|
|
||
| const result = await mockProviderGitManager.cloneRepository(mockUrlInfo); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.localPath).toBe('/path/to/cloned/repo'); | ||
| }); | ||
|
|
||
| it('handles clone errors', async () => { | ||
| const expectedResult = { | ||
| success: false, | ||
| localPath: '/path/to/cloned/repo', | ||
| error: 'Clone failed', | ||
| }; | ||
| mockProviderGitManager.cloneRepository.mockResolvedValue(expectedResult); | ||
|
|
||
| const result = await mockProviderGitManager.cloneRepository(mockUrlInfo); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.error).toContain('Clone failed'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('updateRepository', () => { | ||
| it('successfully updates a repository when updates are available', async () => { | ||
| const expectedResult = { | ||
| success: true, | ||
| updated: true, | ||
| }; | ||
| mockProviderGitManager.updateRepository.mockResolvedValue(expectedResult); | ||
|
|
||
| const result = await mockProviderGitManager.updateRepository(mockUrlInfo); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.updated).toBe(true); | ||
| }); | ||
|
|
||
| it('reports no updates when repository is up to date', async () => { | ||
| const expectedResult = { | ||
| success: true, | ||
| updated: false, | ||
| }; | ||
| mockProviderGitManager.updateRepository.mockResolvedValue(expectedResult); | ||
|
|
||
| const result = await mockProviderGitManager.updateRepository(mockUrlInfo); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.updated).toBe(false); | ||
| }); | ||
|
|
||
| it('handles update errors', async () => { | ||
| const expectedResult = { | ||
| success: false, | ||
| updated: false, | ||
| error: 'Update failed', | ||
| }; | ||
| mockProviderGitManager.updateRepository.mockResolvedValue(expectedResult); | ||
|
|
||
| const result = await mockProviderGitManager.updateRepository(mockUrlInfo); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.updated).toBe(false); | ||
| expect(result.error).toContain('Update failed'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getProviderModulePath', () => { | ||
| it('returns the specified path when provided', () => { | ||
| const urlInfoWithPath = { ...mockUrlInfo, path: 'src/providers' }; | ||
| const localPath = '/path/to/repo'; | ||
| const expectedPath = '/path/to/repo/src/providers'; | ||
|
|
||
| mockProviderGitManager.getProviderModulePath.mockReturnValue(expectedPath); | ||
|
|
||
| const result = mockProviderGitManager.getProviderModulePath(urlInfoWithPath, localPath); | ||
|
|
||
| expect(result).toBe(expectedPath); | ||
| }); | ||
|
|
||
| it('finds common entry points when no path specified', () => { | ||
| const localPath = '/path/to/repo'; | ||
| const expectedPath = '/path/to/repo/index.js'; | ||
|
|
||
| mockProviderGitManager.getProviderModulePath.mockReturnValue(expectedPath); | ||
|
|
||
| const result = mockProviderGitManager.getProviderModulePath(mockUrlInfo, localPath); | ||
|
|
||
| expect(result).toBe(expectedPath); | ||
| }); | ||
|
|
||
| it('returns repository root when no entry point found', () => { | ||
| const localPath = '/path/to/repo'; | ||
|
|
||
| mockProviderGitManager.getProviderModulePath.mockReturnValue(localPath); | ||
|
|
||
| const result = mockProviderGitManager.getProviderModulePath(mockUrlInfo, localPath); | ||
|
|
||
| expect(result).toBe(localPath); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Tests mock the class under test instead of exercising real implementation.
This test suite mocks ProviderGitManager's static methods (lines 17-29) and then verifies that the mocks return pre-configured values. For example, lines 46-57 configure the mock to return { success: true, localPath: '...' }, call the mock, and assert it returns exactly that. This doesn't test any actual implementation logic.
The real ProviderGitManager (per relevant code snippets) performs git operations via execAsync, manages the file system, handles timeouts, and implements error recovery. None of this logic is validated here.
Proper unit testing approach:
- Don't mock the SUT: Import the real
ProviderGitManagerwithout mocking it. - Mock dependencies: Mock
execAsync(fromutil.promisify) andfsoperations thatProviderGitManagercalls. - Test real behavior: Call real methods like
cloneRepository(...)and verify they correctly handle successful git output, error scenarios, directory cleanup, etc.
Example structure for a real test:
// Mock dependencies, not ProviderGitManager
jest.mock('util', () => ({
promisify: jest.fn(() => mockExecAsync),
}));
jest.mock('fs');
// Import real ProviderGitManager after mocks
import { ProviderGitManager } from '../../providers/provider-git-manager';
describe('ProviderGitManager', () => {
it('successfully clones a repository', async () => {
// Arrange: configure mocked execAsync response
mockExecAsync.mockResolvedValue({ stdout: '', stderr: '' });
// Act: call real method
const result = await ProviderGitManager.cloneRepository(mockUrlInfo);
// Assert: verify real implementation behavior
expect(mockExecAsync).toHaveBeenCalledWith(
expect.stringContaining('git clone'),
expect.any(Object)
);
expect(result.success).toBe(true);
expect(result.localPath).toContain('.provider-cache');
});
});This approach validates that cloneRepository constructs the correct git command, handles the response, and returns the expected result structure.
🤖 Prompt for AI Agents
In src/model/cloud-runner/tests/providers/provider-git-manager.test.ts around
lines 17-151, the test suite currently mocks the ProviderGitManager itself which
prevents exercising its real logic; remove the jest.mock that replaces
ProviderGitManager, instead mock its external dependencies (e.g.,
util.promisify/execAsync, fs operations, and any timeout or temp-dir helpers)
before importing the real ProviderGitManager, then call the real static methods
(cloneRepository, updateRepository, getProviderModulePath) with controlled
mocked dependency responses to assert constructed git commands, filesystem
interactions, error handling, cleanup behavior, and the returned result shapes
for success and failure cases.
| BRANCH="${CloudRunner.buildParameters.cloudRunnerBranch}" | ||
| REPO="${CloudRunnerFolders.unityBuilderRepoUrl}" | ||
| if [ -n "$(git ls-remote --heads "$REPO" "$BRANCH" 2>/dev/null)" ]; then | ||
| git clone -q -b "$BRANCH" "$REPO" /builder | ||
| else | ||
| echo "Remote branch $BRANCH not found in $REPO; falling back to a known branch" | ||
| git clone -q -b cloud-runner-develop "$REPO" /builder \ | ||
| || git clone -q -b main "$REPO" /builder \ | ||
| || git clone -q "$REPO" /builder | ||
| fi |
There was a problem hiding this comment.
Critical: Token exposure in fallback log message.
Line 35 echoes $REPO when the branch is not found. Since CloudRunnerFolders.unityBuilderRepoUrl embeds gitPrivateToken in the URL (per the relevant code snippets), this will leak the token in build logs.
Apply this diff to redact the sensitive URL:
else
- echo "Remote branch $BRANCH not found in $REPO; falling back to a known branch"
+ echo "Remote branch $BRANCH not found; falling back to a known branch"
git clone -q -b cloud-runner-develop "$REPO" /builder \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BRANCH="${CloudRunner.buildParameters.cloudRunnerBranch}" | |
| REPO="${CloudRunnerFolders.unityBuilderRepoUrl}" | |
| if [ -n "$(git ls-remote --heads "$REPO" "$BRANCH" 2>/dev/null)" ]; then | |
| git clone -q -b "$BRANCH" "$REPO" /builder | |
| else | |
| echo "Remote branch $BRANCH not found in $REPO; falling back to a known branch" | |
| git clone -q -b cloud-runner-develop "$REPO" /builder \ | |
| || git clone -q -b main "$REPO" /builder \ | |
| || git clone -q "$REPO" /builder | |
| fi | |
| BRANCH="${CloudRunner.buildParameters.cloudRunnerBranch}" | |
| REPO="${CloudRunnerFolders.unityBuilderRepoUrl}" | |
| if [ -n "$(git ls-remote --heads "$REPO" "$BRANCH" 2>/dev/null)" ]; then | |
| git clone -q -b "$BRANCH" "$REPO" /builder | |
| else | |
| echo "Remote branch $BRANCH not found; falling back to a known branch" | |
| git clone -q -b cloud-runner-develop "$REPO" /builder \ | |
| || git clone -q -b main "$REPO" /builder \ | |
| || git clone -q "$REPO" /builder | |
| fi |
🤖 Prompt for AI Agents
In src/model/cloud-runner/workflows/async-workflow.ts around lines 30 to 39, the
fallback echo prints $REPO which contains an embedded gitPrivateToken and will
leak secrets in logs; change the code to never print the raw $REPO value —
instead sanitize it before logging (e.g., remove the credential portion from the
URL using a shell-safe transform or extract only the host/path) or log a fixed
placeholder like "<redacted-repo-url>"; ensure any sanitization handles URLs
with and without credentials and update the echo to use the sanitized variable
or placeholder.
| git clone -q -b cloud-runner-develop "$REPO" /builder \ | ||
| || git clone -q -b main "$REPO" /builder \ | ||
| || git clone -q "$REPO" /builder | ||
| fi |
There was a problem hiding this comment.
Add error handling for failed clone fallbacks.
If all three fallback clones fail, the script continues without /builder being set up. Line 46 will then fail with an unclear error when attempting to execute node /builder/dist/index.js.
Apply this diff to add explicit error handling:
git clone -q -b cloud-runner-develop "$REPO" /builder \
|| git clone -q -b main "$REPO" /builder \
- || git clone -q "$REPO" /builder
+ || git clone -q "$REPO" /builder \
+ || { echo "ERROR: Failed to clone builder repository"; exit 1; }
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| git clone -q -b cloud-runner-develop "$REPO" /builder \ | |
| || git clone -q -b main "$REPO" /builder \ | |
| || git clone -q "$REPO" /builder | |
| fi | |
| git clone -q -b cloud-runner-develop "$REPO" /builder \ | |
| || git clone -q -b main "$REPO" /builder \ | |
| || git clone -q "$REPO" /builder \ | |
| || { echo "ERROR: Failed to clone builder repository"; exit 1; } | |
| fi |
🤖 Prompt for AI Agents
In src/model/cloud-runner/workflows/async-workflow.ts around lines 36 to 39, the
three git clone fallbacks can all fail and the script will continue without
/builder, causing a later cryptic failure; update the script to detect when none
of the clone attempts succeeded (e.g., test for the presence of /builder or
check the exit status), log a clear error message including the attempted branch
names and repository URL, and exit with a non-zero status (exit 1) immediately
so downstream commands like node /builder/dist/index.js are not run.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/model/cloud-runner/remote-client/index.ts (5)
326-335: Remove underscore prefix from catch binding that is used.Line 328 declares
_errorwith an underscore prefix suggesting it's unused, but line 332 throws it. Remove the underscore to match convention.- } catch (_error) { + } catch (error) { if ((targetBranch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); } else { - throw _error; + throw error; } }
338-338: Fix contradictory LFS folder assertion.The assertion message says "LFS folder should not exist before caching" but the code asserts that the folder exists. Either the check or the message is incorrect.
If the folder should NOT exist before caching:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(!fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');If the folder SHOULD exist (assertion is correct):
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder must exist after git lfs install');
372-375: Remove unconventionalvoid 0;no-op.The
void 0;statement is unconventional for a no-op. Use an empty block or just a comment.} catch { /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; }
385-387: Suppress logs to prevent token exposure.The git config command embeds
gitPrivateTokenin the URL. SinceCloudRunnerSystem.Runlogs commands by default, this will expose the token in plaintext logs. PasssuppressLogs: true(3rd parameter) to prevent logging sensitive commands.await CloudRunnerSystem.Run( `git config --global url."https://${gitPrivateToken}@github.com/".insteadOf "https://github.com/"`, + false, + true, // suppressLogs - prevent token from appearing in logs );
406-408: Suppress logs for GITHUB_TOKEN configuration as well.Same issue as with
GIT_PRIVATE_TOKEN- the token will be logged in plaintext.await CloudRunnerSystem.Run( `git config --global url."https://${githubToken}@github.com/".insteadOf "https://github.com/"`, + false, + true, // suppressLogs - prevent token from appearing in logs );
🧹 Nitpick comments (1)
src/model/cloud-runner/remote-client/index.ts (1)
175-182: Consider using write callback instead of setTimeout for flush reliability.The 100ms setTimeout is a heuristic that may not guarantee the write has completed. For more reliable flushing, use the callback from
write()or wait for thedrainevent.// Use synchronous write and ensure newline is included for proper flushing - process.stdout.write(`${successMessage}\n`, 'utf8'); - - // Ensure stdout is flushed before process exits (critical for K8s where process might exit quickly) - // For non-TTY streams, we need to explicitly ensure the write completes - if (!process.stdout.isTTY) { - // Give the pipe a moment to process the write - await new Promise((resolve) => setTimeout(resolve, 100)); - } + await new Promise<void>((resolve) => { + const flushed = process.stdout.write(`${successMessage}\n`, 'utf8'); + if (flushed) { + resolve(); + } else { + process.stdout.once('drain', resolve); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (1)
src/model/cloud-runner/remote-client/index.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/model/cloud-runner/remote-client/index.ts (5)
src/model/cloud-runner/remote-client/remote-client-logger.ts (1)
RemoteClientLogger(7-93)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)src/model/cloud-runner/remote-client/caching.ts (1)
Caching(15-300)src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)src/model/input.ts (1)
gitPrivateToken(202-204)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: iOS on 2022.3.13f1
🔇 Additional comments (4)
src/model/cloud-runner/remote-client/index.ts (4)
35-62: LGTM - Log streaming improvements look good.The changes correctly handle K8s stdout encoding and remove the dead
process.stdout.flushcode from previous versions. The conditional write to log file (only non-empty lines) while unconditionally writing to stdout preserves stdout formatting for K8s while keeping log files cleaner.
65-78: LGTM - End handler properly handles lingering lines.The lingering line handling is consistent and correctly includes newlines for both log file and stdout writes.
83-164: Well-structured defensive error handling for post-build tasks.The nested try/catch blocks with folder existence and content checks provide good resilience. Making post-build tasks best-effort (logging warnings instead of failing) is a sensible approach for non-critical operations.
422-458: LGTM - Retained workspace handling improvements.The enhanced fetch with tags and PR refs, combined with the SHA-to-branch fallback logic, improves reliability for various checkout scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/model/cloud-runner/remote-client/index.ts (1)
347-347: Fix contradictory LFS folder assertion.The assertion message says the LFS folder "should not exist before caching", but the code asserts that it DOES exist (
fs.existsSync(...)). At this point in the flow (aftergit lfs installbut before LFS cache operations), the.git/lfsfolder should not yet contain cached content.Apply this diff to make the assertion consistent with the message:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(!fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching');Or if the folder should exist at this point, update the message instead:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder exists after git lfs install');
♻️ Duplicate comments (5)
src/model/cloud-runner/remote-client/index.ts (1)
337-343: Remove underscore prefix from catch binding that is rethrown.The catch parameter
_errorhas an underscore prefix suggesting it's unused, but line 341 rethrows it. Remove the underscore to match the project's naming convention.Apply this diff:
- } catch (_error) { + } catch (error) { if ((targetBranch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); } else { - throw _error; + throw error; } }src/model/cloud-runner/workflows/build-automation-workflow.ts (4)
62-83: Use$LOG_FILEconsistently instead of hardcoding/home/job-log.txt.You export
LOG_FILE(containerized:/home/job-log.txt, non-container:$(pwd)/temp/job-log.txt), but several places still append directly to/home/job-log.txtor pass that literal to--logFile. Swapping those for$LOG_FILEkeeps the log sink configurable and avoids subtle drift if the path changes in one place but not the others.Also applies to: 117-120, 171-195, 213-218
128-131: Non-container providers never execute the Unity build entrypoint.In the non-container branch (
isContainerized === false):
setupCommandsonly setsGIT_DISCOVERY_ACROSS_FILESYSTEMand initializes$LOG_FILE.BuildCommandsjust logs “game ci start”, briefly runsremote-cli-log-streamwith a 3s timeout, then callsnode ${builderPath} -m remote-cli-post-build.There is no staging of
default-build-script, no/entrypoint.sh, and no invocation of any build script, solocal/local-systemstyle providers will report success without actually running a Unity build. This is a behavior regression compared to the containerized branches, which copydefault-build-script/entrypoint.shand execute/entrypoint.shpiped throughremote-cli-log-stream.I’d expect the non-container path either to mirror that flow (using the local filesystem and
$LOG_FILE) or to explicitly trigger a build via the remote client before callingremote-cli-post-build.Also applies to: 221-226
167-170: No-op stubs fornode/npm/n/yarncan hide real misconfigurations.When these binaries are missing, the script quietly creates
exit 0wrappers in/usr/local/bin. Any subsequent command that genuinely depends on Node/npm (including your ownremote-cli-*calls) will appear to succeed while doing nothing, making failures hard to debug and potentially masking broken images.At minimum, log a loud warning before creating each stub, and consider failing the job by default when a required tool is missing, with an explicit opt-in for environments/tests that expect stubbing. For example, for
node:- if ! command -v node > /dev/null 2>&1; then printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/node && chmod +x /usr/local/bin/node; fi + if ! command -v node > /dev/null 2>&1; then + echo "WARNING: 'node' not found; creating no-op stub. Builds that depend on Node may silently do nothing." >&2 + printf '#!/bin/sh\nexit 0\n' > /usr/local/bin/node + chmod +x /usr/local/bin/node + fi(and similarly for
npm,n, andyarnif you still need stubs).
180-184: Post-build failures are still effectively swallowed in containerized flows.Both containerized branches treat
remote-cli-post-buildas “best effort”:
- Local-docker:
set +ethen... || echo "Post-build command completed with warnings", thenset -e.- Other containerized:
... || true.Any non-zero exit (artifact upload/cache persistence/cleanup failures) is converted into success, and the only signal is a generic message (or nothing at all), which can hide critical issues.
If post-build is important, prefer capturing and surfacing the real exit code, and optionally failing the job. For example in the local-docker branch:
- set +e - node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "Post-build command completed with warnings" - set -e + set +e + node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" + EXIT_CODE=$? + if [ "$EXIT_CODE" -ne 0 ]; then + echo "WARNING: remote-cli-post-build failed with exit code $EXIT_CODE" >> "$LOG_FILE" + # Optionally: exit "$EXIT_CODE" if failures should fail the job + fi + set -eand mirror the same pattern (using
$LOG_FILE) in the other containerized branch.Also applies to: 215-215
🧹 Nitpick comments (2)
src/model/cloud-runner/remote-client/index.ts (2)
166-200: Consider consolidating redundant stdout writes for K8s.The success message is written to stdout twice for K8s (lines 175 and 183). While the comments suggest this is intentional (once for pipe capture, once directly for kubectl logs), the duplication adds complexity.
Consider verifying whether both writes are necessary or if a single write suffices for all capture mechanisms.
383-383: Replace unconventionalvoid 0;statement.The
void 0;statement is an unconventional no-op. Consider removing it or using an empty catch block for clarity.Apply this diff:
} catch { /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/model/cloud-runner/remote-client/index.ts(4 hunks)src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/build-automation-workflow.ts
🧬 Code graph analysis (1)
src/model/cloud-runner/workflows/build-automation-workflow.ts (2)
src/model/index.ts (1)
CloudRunner(27-27)src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
🔇 Additional comments (9)
src/model/cloud-runner/remote-client/index.ts (6)
35-38: LGTM: Proper stdout configuration for K8s.Setting unbuffered UTF-8 encoding ensures kubectl logs can capture messages immediately.
49-62: LGTM: Log streaming correctly includes newlines.The file writes and stdout writes properly include trailing newlines, ensuring log entries are line-separated and kubectl logs can capture complete lines.
66-78: LGTM: End handler properly includes newlines.The lingering line handler correctly appends newlines to both file and stdout writes.
83-164: LGTM: Robust post-build task handling with proper guards.The explicit cache key logging and guarded cache operations (checking for folder existence and non-empty contents) improve resilience and prevent failures when folders are missing or empty.
436-467: LGTM: Retained workspace handling with proper fetch semantics.The expanded fetch coverage (all refs, tags, and PR refs) and explicit cache key logging improve robustness and debuggability. The catch blocks properly use parameterless syntax or named bindings when errors are rethrown.
394-396: Prevent token exposure in git config command logs.The git config commands on lines 395 and 416 embed sensitive tokens (
gitPrivateTokenandGITHUB_TOKEN) directly in command URLs.CloudRunnerSystem.Runlogs all commands by default, which exposes these tokens in plaintext logs.Add the
suppressLogsparameter to prevent logging. The exact parameter position requires verification of the CloudRunnerSystem.Run method signature:await CloudRunnerSystem.Run( `git config --global url."https://${gitPrivateToken}@github.com/".insteadOf "https://github.com/"`, + suppressLogs: true, );Apply the same fix to the GITHUB_TOKEN configuration around line 416.
src/model/cloud-runner/workflows/build-automation-workflow.ts (3)
90-126: Builder clone + branch-fallback logic for containerized runs looks good.The
git ls-remotecheck with fallbacks (cloud-runner-develop → main → default) plus the “builder already exists” guard should keep container jobs idempotent and avoid redundant clones while still recovering from missing branches. The conditionalremote-cli-pre-buildfor non-local-dockerproviders also matches the intended containerized flow.
134-162: Containerized/local-docker build script wiring generally looks correct.The local-docker branch correctly:
- Creates project/build and cache directories.
- Uses
$GITHUB_WORKSPACE/${projectPath}and/data/cache/$CACHE_KEY/...consistently.- Copies
default-build-script,entrypoint.sh, andstepsfrom the builder dist into the container filesystem and marks them executable.- Warms Git LFS content and normalizes potential CRLF endings.
- Primes empty
Library/buildtarballs and mirrors cache back into$GITHUB_WORKSPACE/cloud-runner-cache.Aside from the separate concerns about stubs and post-build error handling, this wiring aligns with the expected builder and cache layout.
Also applies to: 173-179, 197-203
53-60: EnsureisContainerizedremains aligned with all container-backed providers.Hard-coding
aws | k8s | local-dockerworks for the current set of strategies, but any new container-backed provider (including plugin-based ones) will silently fall into the non-container path (no/datausage, no toolchain setup, differentbuilderPath). Consider centralizing this flag on the provider (e.g.,provider.isContainerized) or at least documenting that any new containerized strategy must be added to this check to prevent silent failures.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/model/cloud-runner/remote-client/index.ts (1)
357-357: Fix contradictory assertion message.The assertion checks that the LFS folder EXISTS (
fs.existsSync(...)), but the message claims it "should not exist before caching." Aftergit lfs installon line 313, the.git/lfsfolder is expected to exist.Apply this diff to make the message consistent with the check:
- assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder should not exist before caching'); + assert(fs.existsSync(path.join(`.git`, `lfs`)), 'LFS folder exists after git lfs install');
♻️ Duplicate comments (9)
src/model/cloud-runner/remote-client/index.ts (3)
347-353: Remove underscore prefix from used catch binding.Line 347 declares
_errorwith an underscore prefix suggesting it's unused, but line 351 throws it. Remove the underscore to match convention.Apply this diff:
- } catch (_error) { + } catch (error) { if ((targetBranch || '').startsWith('pull/')) { await CloudRunnerSystem.Run(`git checkout origin/${targetBranch}`); } else { - throw _error; + throw error; } }
384-394: Remove unconventionalvoid 0;no-op statement.Line 393 uses
void 0;as a no-op in the catch block, which is unconventional. The catch block can be empty or contain a brief comment.Apply this diff:
} catch { /* no-op: best-effort git lfs pull without tokens may fail */ - void 0; }
404-406: Prevent token exposure in git config logs.The git config command embeds
gitPrivateTokenin the URL. SinceCloudRunnerSystem.Runlogs commands by default, this would expose the token in plaintext logs.Add the
suppressLogsparameter to prevent logging:await CloudRunnerSystem.Run( `git config --global url."https://${gitPrivateToken}@github.com/".insteadOf "https://github.com/"`, + false, + true, // suppressLogs );src/model/cloud-runner/workflows/build-automation-workflow.ts (6)
119-119: Replace hardcoded log path with$LOG_FILEvariable.Line 119 hardcodes
/home/job-log.txtinstead of using the$LOG_FILEvariable defined on line 82. While the hardcoded path matches the containerized value, using the variable ensures consistency and maintainability throughout the script (as seen in lines 130, 227 which correctly use"$LOG_FILE").Apply this diff:
-echo "log start" >> /home/job-log.txt +echo "log start" >> "$LOG_FILE"
128-131: Non-containerized setup assumes pre-installed builder.The non-containerized code path only initializes logging but never clones the Unity builder repository. Since
builderPathfor non-containerized mode points toprocess.cwd()/dist/index.js(line 60), subsequentnode ${builderPath}commands (lines 227-228) will fail unless the builder is already present in the working directory. This differs from the containerized path (lines 108-115) which clones the builder when missing.If non-containerized providers require the builder to be pre-installed, document this requirement. Otherwise, add clone logic similar to the containerized path.
167-170: Stub binaries mask missing tool dependencies.Creating no-op stubs for
node,npm,n, andyarnwhen they're missing will cause scripts that genuinely require these tools to silently fail. If the local-docker image is expected to provide these tools, their absence likely indicates a misconfiguration that should be diagnosed rather than hidden.At minimum, log warnings before creating stubs to alert users that expected tools are missing.
183-183: Post-build error suppression may hide critical failures.Line 183 uses
|| echo "Post-build command completed with warnings"to convert all post-build failures into success. Ifremote-cli-post-buildfails while uploading artifacts or persisting cache, the build will appear to succeed despite incomplete outputs. The surroundingset +e(line 182) further weakens error propagation.Consider propagating the failure if post-build tasks are critical, or at minimum logging the actual error and exit code.
Apply this diff to capture and log the exit code:
- node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "Post-build command completed with warnings" + node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" || { EXIT_CODE=$?; echo "WARNING: Post-build command failed with exit code $EXIT_CODE"; }
223-229: Non-containerized provider doesn't execute the build entrypoint.The non-containerized code path only runs logging and post-build commands but never stages Unity build scripts or executes
/entrypoint.sh. Unlike the containerized paths (lines 142-148, 210-214) which copy build scripts and execute the entrypoint, the non-containerized path immediately logs success without invoking any build logic. This means local provider builds will silently skip the actual Unity build step.If non-containerized builds are expected to work differently (e.g., Unity is invoked externally), document this behavior. Otherwise, add the necessary script staging and entrypoint execution similar to the containerized paths.
217-217: Post-build error suppression may hide critical failures.Line 217 uses
|| trueto suppress all post-build failures. Ifremote-cli-post-buildfails during artifact upload or cache persistence, the build will appear successful despite incomplete outputs.Consider either propagating the failure if post-build tasks are critical, or logging the actual error and exit code.
Apply this diff to capture and log the exit code:
- node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || true + node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" || { EXIT_CODE=$?; echo "WARNING: Post-build command failed with exit code $EXIT_CODE"; }
🧹 Nitpick comments (5)
src/model/cloud-runner/remote-client/index.ts (1)
166-209: Consider consolidating success marker outputs if possible.The success marker is written to five different outputs (direct log file, stdout, stderr for K8s, RemoteClientLogger, CloudRunnerLogger) with a 100ms delay for non-TTY streams. While this defensive approach may be necessary for reliability across providers, the complexity and setTimeout workaround suggest potential fragility.
If some outputs prove redundant after testing across all providers, consolidating to fewer streams would reduce complexity and the need for timing workarounds.
src/model/cloud-runner/workflows/build-automation-workflow.ts (4)
172-172: Use$LOG_FILEvariable instead of hardcoded path.Line 172 hardcodes
/home/job-log.txtinstead of using the$LOG_FILEvariable defined on line 82. For consistency and to support non-containerized paths correctly, reference the variable.Apply this diff:
- { echo "game ci start"; echo "game ci start" >> /home/job-log.txt; echo "CACHE_KEY=$CACHE_KEY"; echo "$CACHE_KEY"; if [ -n "$LOCKED_WORKSPACE" ]; then echo "Retained Workspace: true"; fi; if [ -n "$LOCKED_WORKSPACE" ] && [ -d "$GITHUB_WORKSPACE/.git" ]; then echo "Retained Workspace Already Exists!"; fi; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt + { echo "game ci start"; echo "game ci start" >> "$LOG_FILE"; echo "CACHE_KEY=$CACHE_KEY"; echo "$CACHE_KEY"; if [ -n "$LOCKED_WORKSPACE" ]; then echo "Retained Workspace: true"; fi; if [ -n "$LOCKED_WORKSPACE" ] && [ -d "$GITHUB_WORKSPACE/.git" ]; then echo "Retained Workspace Already Exists!"; fi; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE"
190-195: Use$LOG_FILEvariable instead of hardcoded paths.Lines 190-191 and 194-195 hardcode
/home/job-log.txtinstead of using the$LOG_FILEvariable. For consistency, reference the variable throughout.Apply this diff:
- echo "end of cloud runner job" | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "end of cloud runner job" >> /home/job-log.txt - echo "---${CloudRunner.buildParameters.logId}" | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "---${CloudRunner.buildParameters.logId}" >> /home/job-log.txt + echo "end of cloud runner job" | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" || echo "end of cloud runner job" >> "$LOG_FILE" + echo "---${CloudRunner.buildParameters.logId}" | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" || echo "---${CloudRunner.buildParameters.logId}" >> "$LOG_FILE" else # Builder path doesn't exist, write directly to log file - echo "end of cloud runner job" >> /home/job-log.txt - echo "---${CloudRunner.buildParameters.logId}" >> /home/job-log.txt + echo "end of cloud runner job" >> "$LOG_FILE" + echo "---${CloudRunner.buildParameters.logId}" >> "$LOG_FILE"
215-215: Use$LOG_FILEvariable instead of hardcoded path.Line 215 hardcodes
/home/job-log.txtinstead of using the$LOG_FILEvariable. For consistency, reference the variable.Apply this diff:
- { echo "game ci start"; echo "game ci start" >> /home/job-log.txt; echo "CACHE_KEY=$CACHE_KEY"; echo "$CACHE_KEY"; if [ -n "$LOCKED_WORKSPACE" ]; then echo "Retained Workspace: true"; fi; if [ -n "$LOCKED_WORKSPACE" ] && [ -d "$GITHUB_WORKSPACE/.git" ]; then echo "Retained Workspace Already Exists!"; fi; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt + { echo "game ci start"; echo "game ci start" >> "$LOG_FILE"; echo "CACHE_KEY=$CACHE_KEY"; echo "$CACHE_KEY"; if [ -n "$LOCKED_WORKSPACE" ]; then echo "Retained Workspace: true"; fi; if [ -n "$LOCKED_WORKSPACE" ] && [ -d "$GITHUB_WORKSPACE/.git" ]; then echo "Retained Workspace Already Exists!"; fi; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE"
219-220: Use$LOG_FILEvariable instead of hardcoded paths.Lines 219-220 hardcode
/home/job-log.txtinstead of using the$LOG_FILEvariable. For consistency, reference the variable.Apply this diff:
- echo "end of cloud runner job" | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt - echo "---${CloudRunner.buildParameters.logId}" | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt`; + echo "end of cloud runner job" | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" + echo "---${CloudRunner.buildParameters.logId}" | node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE"`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/model/cloud-runner/remote-client/index.ts(4 hunks)src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/build-automation-workflow.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: WebGL on 6000.0.36f1 (via Build Profile)
- GitHub Check: WebGL on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (K8s)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (LocalStack)
- GitHub Check: Tests
🔇 Additional comments (8)
src/model/cloud-runner/remote-client/index.ts (6)
35-38: LGTM!Setting stdout encoding explicitly for K8s ensures kubectl logs properly capture the output.
49-62: LGTM!The log streaming now correctly appends newlines to both file and stdout, and the dead flush code has been removed as previously requested.
66-78: LGTM!Consistent with the data handler, this properly handles the lingering line with newlines.
83-164: Well-guarded post-build logic.The defensive checks for folder existence and contents before cache operations prevent unnecessary failures and improve observability with explicit logging. The best-effort error handling is appropriate for post-build tasks.
317-343: Robust git fetch and checkout with comprehensive fallbacks.The multi-level fallback strategy (SHA → fetch SHA → branch → origin/branch for PRs) properly handles various checkout scenarios.
455-477: Consistent git fetch and checkout for retained workspace.The fetch strategy (all refs/tags plus PR refs) and fallback logic (SHA → branch → origin/branch) mirror the clone scenario, ensuring consistent behavior across both paths.
src/model/cloud-runner/workflows/build-automation-workflow.ts (2)
53-60: LGTM! isContainerized flag and builderPath resolution.The conditional logic correctly determines the containerization mode and sets the appropriate builder path for each scenario.
62-88: LGTM! Template logic correctly branches by provider strategy.The conditional setup (apt-get, toolchain, workspace paths, LOG_FILE) appropriately distinguishes between containerized, local-docker, and non-containerized execution contexts.
| await CloudRunnerSystem.Run( | ||
| `git config --global url."https://${githubToken}@github.com/".insteadOf "https://github.com/"`, | ||
| ); |
There was a problem hiding this comment.
Prevent token exposure in git config logs.
Same issue as with GIT_PRIVATE_TOKEN above - the GITHUB_TOKEN would be exposed in logs.
Add the suppressLogs parameter:
await CloudRunnerSystem.Run(
`git config --global url."https://${githubToken}@github.com/".insteadOf "https://github.com/"`,
+ false,
+ true, // suppressLogs
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await CloudRunnerSystem.Run( | |
| `git config --global url."https://${githubToken}@github.com/".insteadOf "https://github.com/"`, | |
| ); | |
| await CloudRunnerSystem.Run( | |
| `git config --global url."https://${githubToken}@github.com/".insteadOf "https://github.com/"`, | |
| false, | |
| true, // suppressLogs | |
| ); |
🤖 Prompt for AI Agents
In src/model/cloud-runner/remote-client/index.ts around lines 425-427, the git
config command is currently executed with the GITHUB_TOKEN embedded in the
command string which will be captured in logs; update the CloudRunnerSystem.Run
invocation to pass the suppressLogs option (e.g. call
CloudRunnerSystem.Run(command, { suppressLogs: true })) so the command output
and the token are not logged, keeping the same command string but adding the
suppressLogs parameter.
| set +e | ||
| node ${builderPath} -m remote-cli-post-build | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "Post-build command completed with warnings" | ||
| set -e | ||
| # Write end marker and pipe through log stream | ||
| # Use set +e to prevent failure if builder path doesn't exist (builder might have been cleaned up) | ||
| # Keep set +e for the rest of the script to prevent exit on error | ||
| set +e | ||
| if [ -f "${builderPath}" ]; then | ||
| echo "end of cloud runner job" | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "end of cloud runner job" >> /home/job-log.txt | ||
| echo "---${CloudRunner.buildParameters.logId}" | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt || echo "---${CloudRunner.buildParameters.logId}" >> /home/job-log.txt | ||
| else | ||
| # Builder path doesn't exist, write directly to log file | ||
| echo "end of cloud runner job" >> /home/job-log.txt | ||
| echo "---${CloudRunner.buildParameters.logId}" >> /home/job-log.txt | ||
| fi | ||
| # Don't restore set -e - keep set +e to prevent script from exiting on error | ||
| # This ensures the script completes successfully even if some operations fail |
There was a problem hiding this comment.
set +e behavior may hide legitimate failures.
Lines 182-198 use set +e to prevent the script from exiting on error, with comments indicating this ensures the script "completes successfully even if some operations fail." While this may be intentional for cache mirroring and cleanup operations, it means genuine failures in post-build steps or log finalization will be silently ignored.
If these operations are truly optional cleanup tasks, the current approach is acceptable. Otherwise, consider limiting set +e to only the operations that are expected to fail gracefully (e.g., cache copy operations on lines 202-203), and restore set -e before critical operations.
🤖 Prompt for AI Agents
In src/model/cloud-runner/workflows/build-automation-workflow.ts around lines
182 to 198, the script globally enables set +e which can suppress real failures
in post-build and log-finalization steps; change it so set +e is only applied to
the specific commands that are expected to fail (wrap those commands
individually or use command-level fallbacks), restore set -e immediately after
those optional commands, and ensure critical operations (final log writes and
post-build success/failure markers) run under set -e so failures are surfaced;
additionally, keep the existing fallback writes to /home/job-log.txt but also
capture and log any unexpected errors before continuing.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
.github/workflows/cloud-runner-integrity.yml (2)
374-392: Add LocalStack-specific environment variables to first localstack test step.The first test step in the localstack job omits
AWS_S3_FORCE_PATH_STYLEandAWS_EC2_METADATA_DISABLED, which are present in the k8s job (lines 159–160) and some other localstack test steps (e.g., lines 418–419). This creates inconsistent LocalStack behavior.Apply this diff to align with other test steps:
AWS_ENDPOINT: http://localhost:4566 AWS_ENDPOINT_URL: http://localhost:4566 + AWS_S3_FORCE_PATH_STYLE: 'true' + AWS_EC2_METADATA_DISABLED: 'true' GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
40-43: Replacecurl | bashk3d installation with pinned release download.This reinstates a past suggestion that was marked addressed but not actually applied. The unpinned
curl | bashpattern carries supply-chain and reproducibility risks.Apply this diff to pin k3d to a specific release:
- - name: Install k3d + - name: Install k3d (pinned) if: ${{ matrix.provider == 'k8s' }} run: | - curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash + set -euo pipefail + K3D_VERSION=v5.7.3 + curl -fsSL -o /usr/local/bin/k3d "https://github.com/k3d-io/k3d/releases/download/${K3D_VERSION}/k3d-linux-amd64" + chmod +x /usr/local/bin/k3d k3d version | cat
🧹 Nitpick comments (3)
src/model/cloud-runner/tests/e2e/cloud-runner-end2end-caching.test.ts (3)
25-25: Consider using a more specific type for test overrides.The
anytype reduces type safety. Consider defining an interface for test overrides or usingPartial<BuildParameters>to improve compile-time validation of test configuration.
38-41: Enhance LocalStack detection to handle all localhost representations.The current check only matches endpoints containing the literal string
'localhost'. LocalStack endpoints might also use127.0.0.1,0.0.0.0, orhost.docker.internal, which this check would miss.Consider a more robust check:
-if (process.env.AWS_S3_ENDPOINT && process.env.AWS_S3_ENDPOINT.includes('localhost')) { +if (process.env.AWS_S3_ENDPOINT && + (process.env.AWS_S3_ENDPOINT.includes('localhost') || + process.env.AWS_S3_ENDPOINT.includes('127.0.0.1') || + process.env.AWS_S3_ENDPOINT.includes('host.docker.internal'))) { overrides.providerStrategy = 'aws'; overrides.containerHookFiles += `,aws-s3-pull-cache,aws-s3-upload-cache`; }
96-109: Good addition of cleanup logic to prevent disk space issues.The afterAll cleanup appropriately handles cache removal for local-docker and aws strategies. The error handling is correct for cleanup code.
Consider using
CloudRunnerLogger.error()instead ofCloudRunnerLogger.log()for error messages at line 105:- CloudRunnerLogger.log(`Failed to cleanup cache: ${error.message}`); + CloudRunnerLogger.error(`Failed to cleanup cache: ${error.message}`);This would provide better log level semantics for error conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/cloud-runner-integrity.yml(1 hunks)src/model/cloud-runner/tests/e2e/cloud-runner-end2end-caching.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/model/cloud-runner/tests/e2e/cloud-runner-end2end-caching.test.ts (2)
src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)src/model/cloud-runner/services/core/cloud-runner-logger.ts (1)
error(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: Android on 2021.3.32f1
🔇 Additional comments (1)
src/model/cloud-runner/tests/e2e/cloud-runner-end2end-caching.test.ts (1)
54-54: No changes needed.The
BuildSucceededproperty is correctly typed asbooleaninCloudRunnerResultclass, so the assertions using.toBe(true)are appropriate.
| - name: Create S3 bucket for tests (host LocalStack) | ||
| run: | | ||
| awslocal s3 mb s3://$AWS_STACK_NAME || true | ||
| awslocal s3 ls |
There was a problem hiding this comment.
Make S3 bucket creation region-aware for non-us-east-1 compatibility.
This reinstates a past suggestion accepted by a maintainer but not applied to the current code. S3 requires LocationConstraint outside us-east-1; LocalStack mirrors this.
Apply this diff to use the region-aware S3 API:
- name: Create S3 bucket for tests (host LocalStack)
run: |
- awslocal s3 mb s3://$AWS_STACK_NAME || true
+ if [ "${AWS_REGION:-us-east-1}" = "us-east-1" ]; then
+ awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" || true
+ else
+ awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" \
+ --create-bucket-configuration LocationConstraint="${AWS_REGION:-us-east-1}" || true
+ fi
awslocal s3 ls
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
.github/workflows/cloud-runner-integrity.yml (2)
57-59: Make S3 bucket creation region-aware for non-us-east-1 compatibility.S3 requires
LocationConstraintoutsideus-east-1; LocalStack mirrors this behavior. The current bucket creation will fail ifAWS_REGIONis set to anything other thanus-east-1.Apply this diff:
- name: Create S3 bucket for tests (host LocalStack) run: | - awslocal s3 mb s3://$AWS_STACK_NAME || true + if [ "${AWS_REGION:-us-east-1}" = "us-east-1" ]; then + awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" || true + else + awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" \ + --create-bucket-configuration LocationConstraint="${AWS_REGION:-us-east-1}" || true + fi awslocal s3 ls
40-43: Pin k3d version and avoid unpinned main branch.This step uses the main branch of the k3d install script, which fetches whatever the latest version is. Pin a specific release version using the
TAGparameter for reproducibility and to avoid unexpected version changes.Apply this change:
- name: Install k3d run: | - curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash + curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | TAG=v5.7.3 bash k3d version | catThis uses the official k3d installation method with a pinned release version, which aligns with k3d's recommended practice for reproducible deployments.
🧹 Nitpick comments (2)
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
151-161: Consider extending Git LFS handling to all containerized providers.Git LFS setup is currently only performed in the
local-dockerpath but not in the standard containerized path (aws/k8s, lines 209-230). If Unity projects use Git LFS assets, regular containerized builds may fail to resolve LFS pointers, leading to missing or corrupted assets.Consider adding similar Git LFS handling to the standard containerized path:
cp -r "${CloudRunnerFolders.ToLinuxFolder(path.join(ubuntuPlatformsFolder, 'steps'))}" "/steps" chmod -R +x "/entrypoint.sh" chmod -R +x "/steps" + # Ensure Git LFS files are available + if [ -d "${CloudRunnerFolders.ToLinuxFolder(CloudRunnerFolders.repoPathAbsolute)}/.git" ]; then + echo "Ensuring Git LFS content is pulled" + (cd "${CloudRunnerFolders.ToLinuxFolder(CloudRunnerFolders.repoPathAbsolute)}" \ + && git lfs install || true \ + && git config --global filter.lfs.smudge "git-lfs smudge -- %f" \ + && git config --global filter.lfs.process "git-lfs filter-process" \ + && git lfs pull || true \ + && git lfs checkout || true) + fi { echo "game ci start"; echo "game ci start" >> /home/job-log.txt; echo "CACHE_KEY=$CACHE_KEY"; echo "$CACHE_KEY"; if [ -n "$LOCKED_WORKSPACE" ]; then echo "Retained Workspace: true"; fi; if [ -n "$LOCKED_WORKSPACE" ] && [ -d "$GITHUB_WORKSPACE/.git" ]; then echo "Retained Workspace Already Exists!"; fi; /entrypoint.sh; } | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txtsrc/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts (1)
60-101: Normalize error message handling and centralize the "previous terminated container" checkThe new fallback for
"previous terminated container"errors is a nice resilience improvement, but the condition is currently evaluated twice viaerror?.message?.includes(...)(Lines 61 and 111). This has two drawbacks:
- If
CloudRunnerSystem.Runever rejects with a string instead of anError,error?.messagewill beundefined, so the special-case handling for"previous terminated container"won’t trigger even if the text is present.- Duplicating the string check increases the chance that future edits will accidentally change one branch but not the other.
You can improve robustness and readability by normalizing the error into a string once and reusing a single boolean flag:
} catch (error: any) { await new Promise((resolve) => setTimeout(resolve, 3000)); const continueStreaming = await KubernetesPods.IsPodRunning(podName, namespace, kubeClient); CloudRunnerLogger.log(`K8s logging error ${error} ${continueStreaming}`); - // If pod is not running and we tried --previous but it failed, try without --previous - if (!isRunning && !continueStreaming && error?.message?.includes('previous terminated container')) { + const errorMessage = + typeof error === 'string' ? error : error?.message ?? String(error ?? ''); + const isPreviousContainerError = errorMessage.includes('previous terminated container'); + + // If pod is not running and we tried --previous but it failed, try without --previous + if (!isRunning && !continueStreaming && isPreviousContainerError) { CloudRunnerLogger.log(`Previous container not found, trying current container logs...`); try { @@ - // If we've exhausted retries and it's not a previous container issue, throw - if (!error?.message?.includes('previous terminated container')) { + // If we've exhausted retries and it's not a previous container issue, throw + if (!isPreviousContainerError) { throw error; }This keeps the behavior intent the same while making the error handling more future‑proof and easier to maintain.
Also applies to: 110-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (4)
.github/workflows/cloud-runner-integrity.yml(1 hunks)src/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts(2 hunks)src/model/cloud-runner/remote-client/remote-client-logger.ts(3 hunks)src/model/cloud-runner/workflows/build-automation-workflow.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/model/cloud-runner/remote-client/remote-client-logger.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T18:01:50.339Z
Learnt from: MichaelBuhler
Repo: game-ci/unity-builder PR: 685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
src/model/cloud-runner/workflows/build-automation-workflow.ts
🧬 Code graph analysis (2)
src/model/cloud-runner/workflows/build-automation-workflow.ts (1)
src/model/cloud-runner/options/cloud-runner-folders.ts (1)
CloudRunnerFolders(6-90)
src/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts (2)
src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)src/model/cloud-runner/services/core/follow-log-stream-service.ts (1)
FollowLogStreamService(7-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 6000.0.36f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: iOS on 2023.2.2f1
🔇 Additional comments (5)
src/model/cloud-runner/workflows/build-automation-workflow.ts (2)
53-60: Container-aware builder path resolution looks correct.The
isContainerizedflag correctly identifies provider strategies that require containerized execution paths, andbuilderPathappropriately switches between absolute container paths and local CWD-relative paths.
62-88: Container-aware workflow branching is well-structured.The conditional logic correctly adapts toolchain setup, workspace paths, and logging configuration based on the provider strategy and containerization model. The exclusion of apt-get and toolchain setup for local-docker is appropriate since the image should provide those dependencies.
src/model/cloud-runner/providers/k8s/kubernetes-task-runner.ts (1)
31-54: Explicit container selection and-f/--previoussplit looks goodAlways passing
-c ${containerName}and switching between-f(whenisRunning) and--previous(when not) in thekubectl logscall (around Line 50) is a solid improvement and resolves the earlier multi‑container pod ambiguity. The singleisRunningcheck per loop is also clear and easy to reason about..github/workflows/cloud-runner-integrity.yml (2)
12-26: LGTM on permissions and global environment.The permissions block correctly uses least-privilege (contents: read, checks: write, statuses: write, id-token: write), and the global environment is appropriately configured for LocalStack testing.
37-39: Verify k8s v1.34.1 compatibility with cloud-runner.The workflow pins kubectl to v1.34.1. While the version is recent and fixed (good for reproducibility), ensure cloud-runner tests work correctly with this Kubernetes version. If tests fail due to API changes, consider pinning an earlier stable version.
| AWS_S3_FORCE_PATH_STYLE: 'true' | ||
| AWS_EC2_METADATA_DISABLED: 'true' | ||
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} |
There was a problem hiding this comment.
Stop overriding GITHUB_TOKEN with GIT_PRIVATE_TOKEN in k8s job test steps.
All five test steps in the k8s job (lines 162, 203, 244, 285, 325) override GITHUB_TOKEN with GIT_PRIVATE_TOKEN, exposing a broader-scope PAT unnecessarily and breaking fallback logic. Use the default runner-provided token instead.
Apply this diff pattern to all five occurrences:
GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
- GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+ GITHUB_TOKEN: ${{ github.token }}Also applies to: 203-203, 244-244, 285-285, 325-325
| - name: Run cloud-runner-end2end-locking test | ||
| timeout-minutes: 60 | ||
| run: yarn run test "cloud-runner-end2end-locking" --detectOpenHandles --forceExit --runInBand | ||
| env: | ||
| UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} | ||
| UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} | ||
| UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} | ||
| PROJECT_PATH: test-project | ||
| TARGET_PLATFORM: StandaloneWindows64 | ||
| cloudRunnerTests: true | ||
| versioning: None | ||
| KUBE_STORAGE_CLASS: local-path | ||
| PROVIDER_STRATEGY: aws | ||
| AWS_ACCESS_KEY_ID: test | ||
| AWS_SECRET_ACCESS_KEY: test | ||
| AWS_ENDPOINT: http://localhost:4566 | ||
| AWS_ENDPOINT_URL: http://localhost:4566 | ||
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} |
There was a problem hiding this comment.
Add missing LocalStack environment variables to cloud-runner-end2end-locking test.
This test step is missing AWS_S3_FORCE_PATH_STYLE and AWS_EC2_METADATA_DISABLED, which are required for consistent LocalStack behavior. Other test steps have these variables; ensure all LocalStack tests are consistent.
Apply this diff:
AWS_ENDPOINT: http://localhost:4566
AWS_ENDPOINT_URL: http://localhost:4566
+ AWS_S3_FORCE_PATH_STYLE: 'true'
+ AWS_EC2_METADATA_DISABLED: 'true'
GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}🤖 Prompt for AI Agents
.github/workflows/cloud-runner-integrity.yml around lines 374 to 392: the
cloud-runner-end2end-locking test step is missing LocalStack-specific
environment variables; add AWS_S3_FORCE_PATH_STYLE: "true" and
AWS_EC2_METADATA_DISABLED: "true" to the env block for this step so its
LocalStack behavior matches the other test steps, ensuring consistent S3
path-style usage and disabling EC2 metadata lookups.
| AWS_ENDPOINT: http://localhost:4566 | ||
| AWS_ENDPOINT_URL: http://localhost:4566 | ||
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} |
There was a problem hiding this comment.
Stop overriding GITHUB_TOKEN with GIT_PRIVATE_TOKEN in localstack job test steps.
All 14 test steps in the localstack job override GITHUB_TOKEN with GIT_PRIVATE_TOKEN, violating least-privilege and breaking fallback logic. Use the default runner-provided token instead.
Apply this diff pattern to each of the 14 occurrences (lines 392, 421, 450, 480, 509, 538, 562, 586, 610, 634, 658, 682, 706, 730):
GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
- GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+ GITHUB_TOKEN: ${{ github.token }}Also applies to: 421-421, 450-450, 480-480, 509-509, 538-538, 562-562, 586-586, 610-610, 634-634, 658-658, 682-682, 706-706, 730-730
🤖 Prompt for AI Agents
.github/workflows/cloud-runner-integrity.yml lines 392, 421, 450, 480, 509, 538,
562, 586, 610, 634, 658, 682, 706, 730: the test steps in the localstack job are
overriding the runner-provided GITHUB_TOKEN by setting GITHUB_TOKEN: ${{
secrets.GIT_PRIVATE_TOKEN }} which violates least-privilege and breaks fallback
logic; remove the explicit GITHUB_TOKEN mapping at each listed line so the
default runner GITHUB_TOKEN is used (if a private token is required for a
specific step, set it under a different input name or a step-specific env like
GIT_PRIVATE_TOKEN and reference that only where strictly necessary), then verify
the workflow runs and that any steps that truly need elevated token read from
the separate secret without replacing GITHUB_TOKEN.
| - name: Run cloud-runner-end2end-retaining test | ||
| timeout-minutes: 60 | ||
| run: yarn run test "cloud-runner-end2end-retaining" --detectOpenHandles --forceExit --runInBand | ||
| env: | ||
| UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} | ||
| UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} | ||
| UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} | ||
| PROJECT_PATH: test-project | ||
| TARGET_PLATFORM: StandaloneWindows64 | ||
| cloudRunnerTests: true | ||
| versioning: None | ||
| KUBE_STORAGE_CLASS: local-path | ||
| PROVIDER_STRATEGY: aws | ||
| AWS_ACCESS_KEY_ID: test | ||
| AWS_SECRET_ACCESS_KEY: test | ||
| AWS_ENDPOINT: http://localhost:4566 | ||
| AWS_ENDPOINT_URL: http://localhost:4566 | ||
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} |
There was a problem hiding this comment.
Add missing LocalStack environment variables to remaining localstack job test steps.
Eight test steps in the localstack job (cloud-runner-end2end-retaining, cloud-runner-caching, cloud-runner-environment, cloud-runner-image, cloud-runner-hooks, cloud-runner-local-persistence, cloud-runner-locking-core, cloud-runner-locking-get-locked) are missing AWS_S3_FORCE_PATH_STYLE: 'true' and AWS_EC2_METADATA_DISABLED: 'true', creating inconsistent behavior across the test suite.
Add these two lines to the env: block of each test step. For example, lines 559–560 should become:
AWS_ENDPOINT: http://localhost:4566
AWS_ENDPOINT_URL: http://localhost:4566
+ AWS_S3_FORCE_PATH_STYLE: 'true'
+ AWS_EC2_METADATA_DISABLED: 'true'
GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}Repeat this addition for all test steps at the listed line ranges.
Also applies to: 568-586, 592-610, 616-634, 640-658, 664-682, 688-706, 712-730
🤖 Prompt for AI Agents
.github/workflows/cloud-runner-integrity.yml around lines 544-562, 568-586,
592-610, 616-634, 640-658, 664-682, 688-706, and 712-730: several localstack
test steps are missing LocalStack-specific environment variables causing
inconsistent behavior; for each of the listed job steps add
AWS_S3_FORCE_PATH_STYLE: 'true' and AWS_EC2_METADATA_DISABLED: 'true' into the
env: block (use the same quoting style as other env vars) so every localstack
test step includes these two variables.
| - name: Create S3 bucket for tests | ||
| run: | | ||
| awslocal s3 mb s3://$AWS_STACK_NAME || true | ||
| awslocal s3 ls |
There was a problem hiding this comment.
Make S3 bucket creation region-aware in localDocker job.
This step uses the same region-unaware pattern as the k8s job. For consistency and to support non-us-east-1 regions, apply the region-aware approach:
- name: Create S3 bucket for tests
run: |
- awslocal s3 mb s3://$AWS_STACK_NAME || true
+ if [ "${AWS_REGION:-us-east-1}" = "us-east-1" ]; then
+ awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" || true
+ else
+ awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" \
+ --create-bucket-configuration LocationConstraint="${AWS_REGION:-us-east-1}" || true
+ fi
awslocal s3 ls🤖 Prompt for AI Agents
.github/workflows/cloud-runner-integrity.yml around lines 761-764: the S3 bucket
creation is region-unaware; update the awslocal commands to pass the region flag
so non-us-east-1 runs work (e.g., awslocal s3 mb --region "$AWS_REGION"
s3://$AWS_STACK_NAME and awslocal s3 ls --region "$AWS_REGION"), and ensure
AWS_REGION is exported/set earlier in the job (or default to us-east-1) so the
variable is available.
| versioning: None | ||
| PROVIDER_STRATEGY: local-docker | ||
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} |
There was a problem hiding this comment.
Stop overriding GITHUB_TOKEN with GIT_PRIVATE_TOKEN in localDocker job test steps.
All nine test steps in the localDocker job override GITHUB_TOKEN with GIT_PRIVATE_TOKEN, violating least-privilege and breaking fallback logic. Use the default runner-provided token instead.
Apply this diff pattern to each of the nine occurrences (lines 784, 803, 822, 841, 860, 879, 898, 926, 954):
GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
- GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}
+ GITHUB_TOKEN: ${{ github.token }}Also applies to: 803-803, 822-822, 841-841, 860-860, 879-879, 898-898, 926-926, 954-954
🤖 Prompt for AI Agents
.github/workflows/cloud-runner-integrity.yml around line 784 (and also at 803,
822, 841, 860, 879, 898, 926, 954): the localDocker job’s test steps are
incorrectly overriding the runner-provided GITHUB_TOKEN by setting GITHUB_TOKEN:
${{ secrets.GIT_PRIVATE_TOKEN }}, which violates least privilege and breaks
fallback logic; remove the explicit GITHUB_TOKEN environment assignment in each
of the nine test steps so they use the default runner-provided token (or if a
secret is absolutely required, set a different env var name like
GIT_PRIVATE_TOKEN and do not assign it to GITHUB_TOKEN).
Changes
Related Issues
Related PRs
Successful Workflow Run Link
PRs don't have access to secrets so you will need to provide a link to a successful run of the workflows from your own
repo.
Checklist
code of conduct
in the documentation repo)
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.