Add GitHub Action wrapper for GameCI CLI#43
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces the repository as a reusable GitHub Action for installing and running the GameCI CLI, with accompanying workflow tests validating Unity version detection and Docker stub builds. It also refactors Docker and environment variable command generation from string templates to array-based construction for improved maintainability. ChangesGitHub Action Implementation
Docker Command Generation Refactoring
Sequence DiagramsequenceDiagram
participant Workflow as GitHub Workflow
participant Action as GameCI Action
participant Installer as Installation Logic
participant CLI as GameCI CLI
participant Docker as Docker Engine
Workflow->>Action: Trigger with args & version
Action->>Installer: Resolve version (tag/API)
Installer->>Installer: Download/verify binary
Installer->>CLI: Install & verify CLI
Action->>CLI: Execute with arguments
CLI->>Docker: Build with --engineVersion & --customImage
Docker->>Docker: Run container & build
Docker-->>Workflow: Output artifact
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
action.yml (3)
80-80: 💤 Low value
-matchtreats the asset name as a regex.
$assetName = 'game-ci-windows-x64.exe'is passed directly to-match, where the dots and dashes are regex tokens. In practice this is benign becausechecksums.txtis unlikely to contain near-misses, but a literal-match (-likewith wildcards or escape via[regex]::Escape) would be more robust and clearer about intent.- $expectedLine = $checksums -split "`n" | Where-Object { $_ -match $assetName } | Select-Object -First 1 + $expectedLine = $checksums -split "`n" | Where-Object { $_ -match [regex]::Escape($assetName) } | Select-Object -First 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@action.yml` at line 80, The current search uses -match which treats $assetName as a regex; change the lookup to perform a literal match by either escaping $assetName with [regex]::Escape before using -match or by switching to a wildcard literal comparison such as using -like with "*$assetName*" when building $expectedLine (refer to the expression that assigns $expectedLine from $checksums -split and the $assetName variable) so dots/dashes are matched literally.
63-66: ⚡ Quick winUnauthenticated GitHub API call may hit rate limits.
Invoke-RestMethodagainstapi.github.comwithout an auth token shares the runner IP's 60 req/hour anonymous quota, which can be exhausted on busy org runners and cause the installer to fail before download. Passing${{ github.token }}via anAuthorization: Bearer ...header raises the limit substantially and is a no-op for the consumer.Sketch
- name: Install GameCI CLI if: runner.os == 'Windows' shell: pwsh env: GH_TOKEN: ${{ github.token }} run: | ... if (-not $version) { $headers = @{ 'Authorization' = "Bearer $env:GH_TOKEN"; 'X-GitHub-Api-Version' = '2022-11-28' } $release = Invoke-RestMethod "https://api.github.com/repos/$repo/releases/latest" -Headers $headers $version = $release.tag_name }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@action.yml` around lines 63 - 66, The Invoke-RestMethod call that fetches releases when $version is empty is unauthenticated and can hit anonymous GitHub rate limits; modify the install step to accept a token via environment (e.g. GH_TOKEN from ${{ github.token }}), build a headers hashtable including 'Authorization' = "Bearer $env:GH_TOKEN" and 'X-GitHub-Api-Version' = '2022-11-28', and pass that hashtable to Invoke-RestMethod when calling "https://api.github.com/repos/$repo/releases/latest" so $release and subsequently $version are retrieved using an authenticated request.
98-108: 💤 Low valueMap
inputs.argsthroughenv:instead of direct template substitution.
game-ci ${{ inputs.args }}injects user input directly into the shell script at YAML parse time. While this is conventional for a thin CLI wrapper, it creates a potential injection vector if a caller wiresargsto${{ github.event.* }}(PR title, issue body, etc.). Mitigation is straightforward: define the arguments viaenv:and reference as$INPUT_ARGSin the script, following GitHub's documented composite action pattern. This avoids YAML/template-time interpretation of shell metacharacters while still expanding the flags correctly at runtime.Example:
- name: Run GameCI CLI if: inputs.args != '' shell: bash working-directory: ${{ inputs.working-directory }} env: INPUT_ARGS: ${{ inputs.args }} run: game-ci $INPUT_ARGSLow effort; currently the README only uses static flags, but this improves the pattern for future use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@action.yml` around lines 98 - 108, Replace direct template substitution of inputs.args in the Run GameCI CLI steps with an env-bound variable to avoid YAML/template-time injection: for both steps that reference "if: inputs.args != ''" and the run lines "run: game-ci ${{ inputs.args }}" set env: INPUT_ARGS: ${{ inputs.args }} and change the run command to reference the env var (game-ci $INPUT_ARGS or game-ci $env:INPUT_ARGS for pwsh) so the arguments are expanded at runtime rather than at YAML parse time.src/model/docker.ts (2)
61-61: ⚡ Quick winBrittle coupling between consumer regex and producer formatting.
replace(/ \\\n/g, ' ')assumesgetEnvVarString(Linux) emits exactly' \\\n'between entries. That assumption holds today and is covered byimage-environment-factory.test.ts, but it's not guaranteed by the producer's contract — any change to the join separator (e.g., dropping the leading space, or aligning Windows/Linux producers) silently leaves stray backslash-newlines in the final command. Consider exposing a helper fromImageEnvironmentFactorythat returns either a continuation-joined string or a single-line string, so the consumer doesn't string-match the producer's internal format.Sketch
// in ImageEnvironmentFactory public static getEnvVarArgs(options: Options): string { // produce single-line flat string for direct CLI use } public static getEnvVarStringMultiline(options: Options): string { // produce continuation-joined string for human-readable scripts }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/model/docker.ts` at line 61, The current code in src/model/docker.ts relies on brittle string-munging of ImageEnvironmentFactory.getEnvVarString by doing .replace(/ \\\n/g, ' '); instead, add a clear, stable API on ImageEnvironmentFactory (e.g., getEnvVarSingleLine or getEnvVarArgs) that returns the env vars formatted as a single-line/CLI-safe string (or an array of args) and update the use site in docker.ts to call that new method (replace references to ImageEnvironmentFactory.getEnvVarString and the .replace(...) logic with the new helper) so consumers don’t rely on the producer’s internal continuation-formatting.
87-112: ⚖️ Poor tradeoffConsider mirroring the Linux array-based builder for
getWindowsCommand.
getLinuxCommandis now a clean filter/join over an args array, whilegetWindowsCommandcontinues to use aString.dedenttemplate with hard-coded continuations. The asymmetry makes future edits (adding/removing flags conditionally) error-prone on the Windows path — e.g., conditionalgitPrivateToken/sshAgentinjections would need to be retrofitted as inline ternaries inside the template. Aligning the two builders would also letgetEnvVarString's windows output be normalized symmetrically (no need for divergent post-processing later).Not blocking; safe to defer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/model/docker.ts` around lines 87 - 112, The Windows command builder getWindowsCommand should be refactored to mirror getLinuxCommand: build an args array (instead of a String.dedent template) and push each flag/volume/env entry into that array, adding conditional entries (e.g., GIT_PRIVATE_TOKEN, UNITY_SERIAL, sshAgent-related items) only when present; use ImageEnvironmentFactory.getEnvVarString(options) as one array element and then join the array into the final string with the proper PowerShell continuations/backticks so behavior is identical to the Linux path while keeping the same values for currentWorkDir, cliDistPath, cliStoragePath, dockerWorkspacePath, homeDir, and image and preserving the final "powershell c:/steps/entrypoint.ps1" execution item.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@action.yml`:
- Around line 77-93: Change the checksum logic from "fail-open" to
"fail-closed": make fetching and verifying checksums using
Invoke-WebRequest/$checksumUrl mandatory unless a new opt-in input (e.g.
inputs.skip_checksum) is set; if Invoke-WebRequest throws or no matching
$expectedLine is found, throw an error instead of only Write-Warning, and only
bypass this error when inputs.skip_checksum=true. Locate the block that sets
$expectedHash, the catch that currently Write-Warning, and the verification that
compares $expectedHash to (Get-FileHash -Path $binaryPath -Algorithm
SHA256).Hash; replace the silent fallback (empty $expectedHash) with an explicit
throw on fetch/missing checksum unless the new skip flag is enabled, and surface
a clear error message explaining how to opt out via the new input.
- Around line 28-47: The verify_checksum() in install.sh currently treats
missing sha256sum, failed fetch of checksums.txt, or missing matching checksum
as soft-success; change it to be strict: in verify_checksum() detect absence of
sha256sum, HTTP/fetch failures for checksums.txt, or no matching checksum and
log an explicit error via stderr and return non-zero so the script exits
non-zero; keep the existing mismatch comparison behavior but ensure any of these
earlier failures cause install.sh to fail (so the non-Windows path enforces
integrity comparable to the Windows path).
---
Nitpick comments:
In `@action.yml`:
- Line 80: The current search uses -match which treats $assetName as a regex;
change the lookup to perform a literal match by either escaping $assetName with
[regex]::Escape before using -match or by switching to a wildcard literal
comparison such as using -like with "*$assetName*" when building $expectedLine
(refer to the expression that assigns $expectedLine from $checksums -split and
the $assetName variable) so dots/dashes are matched literally.
- Around line 63-66: The Invoke-RestMethod call that fetches releases when
$version is empty is unauthenticated and can hit anonymous GitHub rate limits;
modify the install step to accept a token via environment (e.g. GH_TOKEN from
${{ github.token }}), build a headers hashtable including 'Authorization' =
"Bearer $env:GH_TOKEN" and 'X-GitHub-Api-Version' = '2022-11-28', and pass that
hashtable to Invoke-RestMethod when calling
"https://api.github.com/repos/$repo/releases/latest" so $release and
subsequently $version are retrieved using an authenticated request.
- Around line 98-108: Replace direct template substitution of inputs.args in the
Run GameCI CLI steps with an env-bound variable to avoid YAML/template-time
injection: for both steps that reference "if: inputs.args != ''" and the run
lines "run: game-ci ${{ inputs.args }}" set env: INPUT_ARGS: ${{ inputs.args }}
and change the run command to reference the env var (game-ci $INPUT_ARGS or
game-ci $env:INPUT_ARGS for pwsh) so the arguments are expanded at runtime
rather than at YAML parse time.
In `@src/model/docker.ts`:
- Line 61: The current code in src/model/docker.ts relies on brittle
string-munging of ImageEnvironmentFactory.getEnvVarString by doing .replace(/
\\\n/g, ' '); instead, add a clear, stable API on ImageEnvironmentFactory (e.g.,
getEnvVarSingleLine or getEnvVarArgs) that returns the env vars formatted as a
single-line/CLI-safe string (or an array of args) and update the use site in
docker.ts to call that new method (replace references to
ImageEnvironmentFactory.getEnvVarString and the .replace(...) logic with the new
helper) so consumers don’t rely on the producer’s internal
continuation-formatting.
- Around line 87-112: The Windows command builder getWindowsCommand should be
refactored to mirror getLinuxCommand: build an args array (instead of a
String.dedent template) and push each flag/volume/env entry into that array,
adding conditional entries (e.g., GIT_PRIVATE_TOKEN, UNITY_SERIAL,
sshAgent-related items) only when present; use
ImageEnvironmentFactory.getEnvVarString(options) as one array element and then
join the array into the final string with the proper PowerShell
continuations/backticks so behavior is identical to the Linux path while keeping
the same values for currentWorkDir, cliDistPath, cliStoragePath,
dockerWorkspacePath, homeDir, and image and preserving the final "powershell
c:/steps/entrypoint.ps1" execution item.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ef28687-8bd6-4965-824b-1d53be773fcd
📒 Files selected for processing (7)
.github/workflows/engine-smoke-test.ymlREADME.mdaction.ymlsrc/model/docker.test.tssrc/model/docker.tssrc/model/image-environment-factory.test.tssrc/model/image-environment-factory.ts
| - name: Install GameCI CLI | ||
| if: runner.os != 'Windows' | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| version="${{ inputs.version }}" | ||
| action_ref="${{ github.action_ref }}" | ||
|
|
||
| if [ -z "$version" ] && printf '%s' "$action_ref" | grep -Eq '^v[0-9]'; then | ||
| version="$action_ref" | ||
| fi | ||
|
|
||
| export GAME_CI_INSTALL="${RUNNER_TEMP}/game-ci/bin" | ||
| if [ -n "$version" ]; then | ||
| export GAME_CI_VERSION="$version" | ||
| fi | ||
|
|
||
| "${GITHUB_ACTION_PATH}/install.sh" | ||
| echo "$GAME_CI_INSTALL" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect install.sh for checksum verification
fd -t f 'install\.sh$'
fd -t f 'install\.sh$' --exec cat {}Repository: game-ci/cli
Length of output: 5240
Checksum verification is present in install.sh, but uses soft-failure logic that succeeds when tools/files are unavailable.
The install.sh script includes a verify_checksum() function that executes after the binary is installed. However, it returns success (exit 0) in several scenarios without actually verifying the checksum:
- If
sha256sumcommand is unavailable - If
checksums.txtcannot be fetched from releases - If no matching checksum is found for the asset
The function only errors if a checksum is found and the comparison fails. This is more lenient than strict enforcement; while checksums are attempted, the verification is not guaranteed on every install. Confirm whether this soft-failure behavior is intentional or should be made more strict for consistency with the Windows path's integrity guarantees.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@action.yml` around lines 28 - 47, The verify_checksum() in install.sh
currently treats missing sha256sum, failed fetch of checksums.txt, or missing
matching checksum as soft-success; change it to be strict: in verify_checksum()
detect absence of sha256sum, HTTP/fetch failures for checksums.txt, or no
matching checksum and log an explicit error via stderr and return non-zero so
the script exits non-zero; keep the existing mismatch comparison behavior but
ensure any of these earlier failures cause install.sh to fail (so the
non-Windows path enforces integrity comparable to the Windows path).
| $expectedHash = '' | ||
| try { | ||
| $checksums = Invoke-WebRequest -Uri $checksumUrl -UseBasicParsing | Select-Object -ExpandProperty Content | ||
| $expectedLine = $checksums -split "`n" | Where-Object { $_ -match $assetName } | Select-Object -First 1 | ||
| if ($expectedLine) { | ||
| $expectedHash = ($expectedLine -split '\s+')[0].ToLower() | ||
| } | ||
| } catch { | ||
| Write-Warning "Checksum verification skipped because checksum metadata could not be fetched: $_" | ||
| } | ||
|
|
||
| if ($expectedHash) { | ||
| $actualHash = (Get-FileHash -Path $binaryPath -Algorithm SHA256).Hash.ToLower() | ||
| if ($expectedHash -ne $actualHash) { | ||
| throw "Checksum verification failed. Expected $expectedHash, got $actualHash" | ||
| } | ||
| } |
There was a problem hiding this comment.
Failing open on checksum verification weakens supply-chain integrity guarantees.
If Invoke-WebRequest to checksums.txt throws for any reason (transient 404, network blip, GitHub outage, asset not yet uploaded for a fresh release), $expectedHash stays empty, the if ($expectedHash) guard at line 88 is false, and the binary is installed and added to PATH with no integrity check — only a Write-Warning that is easy to miss in workflow logs. This is the opposite of fail-closed behavior expected from an installer.
Suggest making the failure mode explicit: fail by default, allow opt-in skip via input.
Proposed fix
$expectedHash = ''
try {
$checksums = Invoke-WebRequest -Uri $checksumUrl -UseBasicParsing | Select-Object -ExpandProperty Content
$expectedLine = $checksums -split "`n" | Where-Object { $_ -match $assetName } | Select-Object -First 1
if ($expectedLine) {
$expectedHash = ($expectedLine -split '\s+')[0].ToLower()
}
} catch {
- Write-Warning "Checksum verification skipped because checksum metadata could not be fetched: $_"
+ throw "Checksum verification failed: could not fetch $checksumUrl ($_)"
}
- if ($expectedHash) {
- $actualHash = (Get-FileHash -Path $binaryPath -Algorithm SHA256).Hash.ToLower()
- if ($expectedHash -ne $actualHash) {
- throw "Checksum verification failed. Expected $expectedHash, got $actualHash"
- }
+ if (-not $expectedHash) {
+ throw "Checksum verification failed: no entry for $assetName in $checksumUrl"
+ }
+ $actualHash = (Get-FileHash -Path $binaryPath -Algorithm SHA256).Hash.ToLower()
+ if ($expectedHash -ne $actualHash) {
+ throw "Checksum verification failed. Expected $expectedHash, got $actualHash"
}📝 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.
| $expectedHash = '' | |
| try { | |
| $checksums = Invoke-WebRequest -Uri $checksumUrl -UseBasicParsing | Select-Object -ExpandProperty Content | |
| $expectedLine = $checksums -split "`n" | Where-Object { $_ -match $assetName } | Select-Object -First 1 | |
| if ($expectedLine) { | |
| $expectedHash = ($expectedLine -split '\s+')[0].ToLower() | |
| } | |
| } catch { | |
| Write-Warning "Checksum verification skipped because checksum metadata could not be fetched: $_" | |
| } | |
| if ($expectedHash) { | |
| $actualHash = (Get-FileHash -Path $binaryPath -Algorithm SHA256).Hash.ToLower() | |
| if ($expectedHash -ne $actualHash) { | |
| throw "Checksum verification failed. Expected $expectedHash, got $actualHash" | |
| } | |
| } | |
| $expectedHash = '' | |
| try { | |
| $checksums = Invoke-WebRequest -Uri $checksumUrl -UseBasicParsing | Select-Object -ExpandProperty Content | |
| $expectedLine = $checksums -split "`n" | Where-Object { $_ -match $assetName } | Select-Object -First 1 | |
| if ($expectedLine) { | |
| $expectedHash = ($expectedLine -split '\s+')[0].ToLower() | |
| } | |
| } catch { | |
| throw "Checksum verification failed: could not fetch $checksumUrl ($_)" | |
| } | |
| if (-not $expectedHash) { | |
| throw "Checksum verification failed: no entry for $assetName in $checksumUrl" | |
| } | |
| $actualHash = (Get-FileHash -Path $binaryPath -Algorithm SHA256).Hash.ToLower() | |
| if ($expectedHash -ne $actualHash) { | |
| throw "Checksum verification failed. Expected $expectedHash, got $actualHash" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@action.yml` around lines 77 - 93, Change the checksum logic from "fail-open"
to "fail-closed": make fetching and verifying checksums using
Invoke-WebRequest/$checksumUrl mandatory unless a new opt-in input (e.g.
inputs.skip_checksum) is set; if Invoke-WebRequest throws or no matching
$expectedLine is found, throw an error instead of only Write-Warning, and only
bypass this error when inputs.skip_checksum=true. Locate the block that sets
$expectedHash, the catch that currently Write-Warning, and the verification that
compares $expectedHash to (Get-FileHash -Path $binaryPath -Algorithm
SHA256).Hash; replace the silent fallback (empty $expectedHash) with an explicit
throw on fetch/missing checksum unless the new skip flag is enabled, and surface
a clear error message explaining how to opt out via the new input.
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Documentation