Closed
Conversation
Adds a GameCIPlugin-compatible export so the CLI repo can consume the orchestrator as a plugin. This includes: - build-parameters-adapter: maps flat yargs options to BuildParameters - orchestrator-options-plugin: registers all orchestrator options via yargs - provider-adapter: wraps ProviderInterface constructors for CLI PluginRegistry - index.ts: exports the orchestratorPlugin with all providers (aws, k8s, docker, local, test) - cli-plugin.test.ts: 7 tests covering plugin shape, options registration, and parameter mapping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a GameCI CLI plugin system by exposing a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI System
participant Plugin as orchestratorPlugin
participant OptConfig as configureOrchestratorOptions
participant BuildAdapter as createBuildParametersFromCliOptions
participant ProviderAdapter as createProviderAdapter
participant Provider as Actual Provider
CLI->>Plugin: Load plugin
activate Plugin
Note over Plugin: name: 'orchestrator'<br/>version: '3.0.0'
Plugin-->>CLI: Return orchestratorPlugin metadata
deactivate Plugin
CLI->>OptConfig: Register CLI options
activate OptConfig
Note over OptConfig: Register 30+ flags:<br/>region, containerCpu,<br/>kubeConfig, etc.
OptConfig-->>CLI: Options registered
deactivate OptConfig
CLI->>CLI: User provides CLI arguments
CLI->>BuildAdapter: Convert options
activate BuildAdapter
Note over BuildAdapter: Map CLI options to<br/>BuildParameters with<br/>env var fallback & defaults
BuildAdapter-->>CLI: Return BuildParameters
deactivate BuildAdapter
CLI->>ProviderAdapter: Create provider instance
activate ProviderAdapter
ProviderAdapter->>Provider: new ProviderClass(buildParameters)
activate Provider
Provider-->>ProviderAdapter: Provider ready
deactivate Provider
ProviderAdapter-->>CLI: Provider adapter ready
deactivate ProviderAdapter
CLI->>ProviderAdapter: Call workflow methods
activate ProviderAdapter
ProviderAdapter->>Provider: Forward method call
activate Provider
Provider-->>ProviderAdapter: Return result
deactivate Provider
ProviderAdapter-->>CLI: Return result
deactivate ProviderAdapter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/cli-plugin/cli-plugin.test.ts (1)
60-93: Add regression tests for currently uncovered adapter edge cases.Please add assertions for
region,resourceTracking,useSharedBuilder,useCleanupCron, andgarbageMaxAge = 0so silent mapping regressions are caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-plugin/cli-plugin.test.ts` around lines 60 - 93, The tests for createBuildParametersFromCliOptions are missing assertions for several adapter edge-case fields; update the two test cases to assert mapping and defaults for region, resourceTracking, useSharedBuilder, useCleanupCron, and garbageMaxAge (including explicit check that garbageMaxAge defaults to 0). In the 'maps yargs options to BuildParameters' test add assertions that the returned BuildParameters contains the provided region value and boolean flags resourceTracking, useSharedBuilder, useCleanupCron and garbageMaxAge set from the input; in the 'applies defaults for missing options' test assert the expected default values for region, resourceTracking, useSharedBuilder, useCleanupCron and that garbageMaxAge === 0 so regressions in the mapping are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 6-9: The package exports point "./cli-plugin" at the raw
TypeScript source ("./src/cli-plugin/index.ts"), which will be published and
break Node consumers; change this by adding a "files" field to package.json to
explicitly include the compiled output (e.g., "dist/") and exclude raw "src",
update the "./cli-plugin" export to point to the compiled entry (e.g.,
"dist/cli-plugin/index.js"), and adjust the build pipeline (the ncc build or
your build script) to emit a dist/cli-plugin/index.js artifact (or alternatively
document that TypeScript runtime tooling is required); reference the "exports"
key and the "./cli-plugin" subpath and ensure the build task that produces
"dist/" is updated to generate the cli-plugin subpath.
In `@src/cli-plugin/build-parameters-adapter.ts`:
- Line 103: The default buildPlatform assignment in build-parameters-adapter.ts
incorrectly treats only providerStrategy === 'local' as host-local; update the
logic around bp.buildPlatform (the assignment that uses options.buildPlatform ||
(bp.providerStrategy !== 'local' ? 'linux' : process.platform)) so that
providerStrategy values 'local' and 'local-system' are both considered
host-local (i.e., if bp.providerStrategy is neither 'local' nor 'local-system'
default to 'linux', otherwise use process.platform); change the conditional to
check for both identifiers (e.g., via explicit comparisons or an inclusion
check) to ensure local-system uses the host platform default.
- Line 115: The assignment bp.garbageMaxAge = Number(options.garbageMaxAge) ||
24 erroneously treats 0 as falsy and resets it to 24; instead parse the value
and only fallback when the parsed value is NaN/invalid—e.g., compute const
parsed = Number(options.garbageMaxAge) and set bp.garbageMaxAge =
Number.isFinite(parsed) ? parsed : 24 (or use a nullish/NaN check) so that 0 is
preserved; update the code around bp.garbageMaxAge and options.garbageMaxAge
accordingly.
- Around line 53-124: The BuildParameters mapping in build-parameters-adapter.ts
is dropping several registered CLI flags (region, containerHookFiles,
commandHookFiles, resourceTracking, useSharedBuilder, useCleanupCron); update
the bp assignments (the BuildParameters object referenced as bp) to copy these
options from the incoming options object: set bp.region = options.region || ''
(or appropriate default), map bp.containerHookFiles and bp.commandHookFiles to
arrays (e.g. options.containerHookFiles ? options.containerHookFiles.split(',')
: []), set bp.resourceTracking = options.resourceTracking === true ||
options.resourceTracking === 'true', and set bp.useSharedBuilder and
bp.useCleanupCron similarly to boolean conversions (=== true || === 'true'); add
these lines alongside the other mappings in the same adapter so user-provided
values are preserved.
In `@src/cli-plugin/index.ts`:
- Line 57: The Test provider is violating the provider interface because
TestOrchestrator implements runTaskInWorkflow(commands, buildGuid, image, ...)
while the expected signature is runTaskInWorkflow(buildGuid, image, commands,
...); update TestOrchestrator.runTaskInWorkflow to take parameters in the
correct order (buildGuid, image, commands, ...) and adjust the internal use of
those parameters accordingly so that createProviderAdapter(TestOrchestrator)
forwards arguments correctly to the implementation.
---
Nitpick comments:
In `@src/cli-plugin/cli-plugin.test.ts`:
- Around line 60-93: The tests for createBuildParametersFromCliOptions are
missing assertions for several adapter edge-case fields; update the two test
cases to assert mapping and defaults for region, resourceTracking,
useSharedBuilder, useCleanupCron, and garbageMaxAge (including explicit check
that garbageMaxAge defaults to 0). In the 'maps yargs options to
BuildParameters' test add assertions that the returned BuildParameters contains
the provided region value and boolean flags resourceTracking, useSharedBuilder,
useCleanupCron and garbageMaxAge set from the input; in the 'applies defaults
for missing options' test assert the expected default values for region,
resourceTracking, useSharedBuilder, useCleanupCron and that garbageMaxAge === 0
so regressions in the mapping are caught.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 067f94e5-a5e7-4b64-8cd3-aebe063e0820
📒 Files selected for processing (6)
package.jsonsrc/cli-plugin/build-parameters-adapter.tssrc/cli-plugin/cli-plugin.test.tssrc/cli-plugin/index.tssrc/cli-plugin/orchestrator-options-plugin.tssrc/cli-plugin/provider-adapter.ts
Comment on lines
+6
to
+9
| "exports": { | ||
| ".": "./dist/index.js", | ||
| "./cli-plugin": "./src/cli-plugin/index.ts" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "package exports:"
jq '.exports' package.json
echo "tsconfig output dirs:"
jq '.compilerOptions | {rootDir, outDir}' tsconfig.json
echo "cli-plugin source entry:"
fd -p '^index\.ts$' src/cli-plugin
echo "compiled cli-plugin JS tracked in repo (if present):"
fd -p '^index\.js$' lib/cli-plugin dist/cli-plugin || trueRepository: game-ci/unity-builder
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tsconfig.json content ==="
cat tsconfig.json | head -30
echo "=== Checking for compiled cli-plugin output ==="
ls -la lib/cli-plugin 2>/dev/null || echo "lib/cli-plugin does not exist"
ls -la dist/cli-plugin 2>/dev/null || echo "dist/cli-plugin does not exist"
echo "=== Build scripts in package.json ==="
jq '.scripts | keys[] as $k | "\($k): \(.[$k])"' package.json 2>/dev/null | head -20 || grep -A 20 '"scripts"' package.json
echo "=== cli-plugin source files ==="
find src/cli-plugin -type f -name "*.ts" -o -name "*.js" 2>/dev/null | head -10Repository: game-ci/unity-builder
Length of output: 2985
🏁 Script executed:
#!/bin/bash
echo "=== Directory structure of dist/ ==="
ls -la dist/ 2>/dev/null | head -20 || echo "dist does not exist or is empty"
echo "=== Directory structure of lib/ ==="
ls -la lib/ 2>/dev/null | head -20 || echo "lib does not exist or is empty"
echo "=== Checking .gitignore for exclusions ==="
cat .gitignore | grep -E "(^lib|^dist|/lib|/dist)" || echo "No explicit lib/dist gitignore rules found"
echo "=== Content of dist/index.js (first 50 lines) ==="
head -50 dist/index.js 2>/dev/null || echo "dist/index.js not found"Repository: game-ci/unity-builder
Length of output: 3115
🏁 Script executed:
#!/bin/bash
echo "=== package.json files and exports fields ==="
jq '{files, exports, main, bin}' package.json 2>/dev/null || cat package.json | grep -A 10 '"files"\|"exports"\|"main"\|"bin"'
echo "=== Checking for ncc or webpack config ==="
ls -la | grep -E "(ncc|webpack|build)" || echo "No ncc/webpack config files found"
echo "=== How cli-plugin is imported in tests/usage ==="
grep -r "cli-plugin" src/ --include="*.ts" | grep -E "(import|require)" | head -5 || echo "No local imports of cli-plugin found"
echo "=== Check if cli-plugin is meant to be external or internal ==="
grep -r "from.*cli-plugin\|require.*cli-plugin" . --include="*.ts" --include="*.js" 2>/dev/null | grep -v node_modules | head -10Repository: game-ci/unity-builder
Length of output: 459
🏁 Script executed:
#!/bin/bash
echo "=== Check if cli-plugin is referenced in README or docs ==="
find . -name "README*" -o -name "*.md" 2>/dev/null | xargs grep -l "cli-plugin" 2>/dev/null || echo "No docs mention cli-plugin"
echo "=== Check GitHub workflows/actions for how this is published ==="
cat .github/workflows/*.yml 2>/dev/null | grep -A 5 -B 5 "publish\|npm\|registry" || echo "No publish workflows found"
echo "=== Check if ts-node or similar is listed as dependency ==="
jq '{devDependencies, dependencies}' package.json | grep -E "ts-node|tsx|register" || echo "Checking dependencies..."
grep -E "ts-node|tsx" package.json || echo "No ts-node detected"
echo "=== What does the cli-plugin export contain? ==="
head -30 src/cli-plugin/index.tsRepository: game-ci/unity-builder
Length of output: 2325
🏁 Script executed:
#!/bin/bash
echo "=== Check what npm would publish ==="
npm ls --depth=0 2>/dev/null || echo "Checking published files..."
echo "=== Simulate npm publish: list files that would be included ==="
git ls-files 2>/dev/null | head -30
echo "=== Is src/ tracked in git? ==="
git ls-files | grep "^src/" | wc -l
echo "=== Is lib/ tracked? ==="
git ls-files | grep "^lib/" | wc -l
echo "=== Full exports and file info ==="
cat package.json | jq '{name, version, main, exports, files, publishConfig}'
echo "=== What actually happens when you require the subpath? ==="
echo "Checking if there are any test files that import cli-plugin..."
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "cli-plugin" 2>/dev/null | head -3Repository: game-ci/unity-builder
Length of output: 3397
Subpath export points to raw TypeScript source that requires TypeScript runtime support.
"./cli-plugin": "./src/cli-plugin/index.ts" will be published as-is since src/ is tracked in git and no "files" field limits what npm includes. This breaks import 'unity-builder/cli-plugin' for standard Node.js consumers without TypeScript loaders (ts-node, tsx, etc.).
The proposed fix of "./cli-plugin": "./lib/cli-plugin/index.js" is incorrect—lib/ is gitignored and will not be included in the published package.
Recommended solutions:
-
Add
"files"field to explicitly control published outputs and ensuredist/is included:{ "files": ["dist", "src"] }Then update the export to point to a pre-compiled
dist/cli-plugin/index.js(requires build changes to output subpaths). -
Or update the build to generate
dist/cli-plugin/index.jsduringncc buildinstead of bundling everything intodist/index.js. -
Or document that this package requires TypeScript tooling and update consumers to use
ts-nodeor similar.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 6 - 9, The package exports point "./cli-plugin" at
the raw TypeScript source ("./src/cli-plugin/index.ts"), which will be published
and break Node consumers; change this by adding a "files" field to package.json
to explicitly include the compiled output (e.g., "dist/") and exclude raw "src",
update the "./cli-plugin" export to point to the compiled entry (e.g.,
"dist/cli-plugin/index.js"), and adjust the build pipeline (the ncc build or
your build script) to emit a dist/cli-plugin/index.js artifact (or alternatively
document that TypeScript runtime tooling is required); reference the "exports"
key and the "./cli-plugin" subpath and ensure the build task that produces
"dist/" is updated to generate the cli-plugin subpath.
Comment on lines
+53
to
+124
| // Provider / orchestrator settings | ||
| bp.providerStrategy = options.providerStrategy || 'local-docker'; | ||
| bp.customParameters = options.customParameters || ''; | ||
| bp.sshAgent = options.sshAgent || ''; | ||
| bp.sshPublicKeysDirectoryPath = options.sshPublicKeysDirectoryPath || ''; | ||
| bp.gitPrivateToken = options.gitPrivateToken || ''; | ||
| bp.runAsHostUser = options.runAsHostUser || 'false'; | ||
| bp.chownFilesTo = options.chownFilesTo || ''; | ||
|
|
||
| // AWS | ||
| bp.awsStackName = options.awsStackName || 'game-ci'; | ||
| bp.awsEndpoint = options.awsEndpoint; | ||
| bp.awsCloudFormationEndpoint = options.awsCloudFormationEndpoint || options.awsEndpoint; | ||
| bp.awsEcsEndpoint = options.awsEcsEndpoint || options.awsEndpoint; | ||
| bp.awsKinesisEndpoint = options.awsKinesisEndpoint || options.awsEndpoint; | ||
| bp.awsCloudWatchLogsEndpoint = options.awsCloudWatchLogsEndpoint || options.awsEndpoint; | ||
| bp.awsS3Endpoint = options.awsS3Endpoint || options.awsEndpoint; | ||
|
|
||
| // Storage | ||
| bp.storageProvider = options.storageProvider || 's3'; | ||
| bp.rcloneRemote = options.rcloneRemote || ''; | ||
|
|
||
| // Kubernetes | ||
| bp.kubeConfig = options.kubeConfig || ''; | ||
| bp.kubeVolume = options.kubeVolume || ''; | ||
| bp.kubeVolumeSize = options.kubeVolumeSize || '25Gi'; | ||
| bp.kubeStorageClass = options.kubeStorageClass || ''; | ||
|
|
||
| // Container resources | ||
| bp.containerMemory = options.containerMemory || '3072'; | ||
| bp.containerCpu = options.containerCpu || '1024'; | ||
| bp.containerNamespace = options.containerNamespace || 'default'; | ||
|
|
||
| // Hooks | ||
| bp.commandHooks = options.commandHooks || ''; | ||
| bp.postBuildContainerHooks = options.postBuildContainerHooks || ''; | ||
| bp.preBuildContainerHooks = options.preBuildContainerHooks || ''; | ||
| bp.customJob = options.customJob || ''; | ||
|
|
||
| // Git / CI | ||
| bp.runNumber = options.runNumber || process.env.GITHUB_RUN_NUMBER || '0'; | ||
| bp.branch = options.branch || process.env.GITHUB_REF?.replace('refs/', '').replace('heads/', '') || ''; | ||
| bp.githubRepo = options.githubRepo || process.env.GITHUB_REPOSITORY || ''; | ||
| bp.orchestratorRepoName = options.orchestratorRepoName || 'game-ci/unity-builder'; | ||
| bp.cloneDepth = Number.parseInt(options.cloneDepth || '50', 10); | ||
| bp.gitSha = options.gitSha || process.env.GITHUB_SHA || ''; | ||
| bp.orchestratorBranch = options.orchestratorBranch || 'main'; | ||
| bp.orchestratorDebug = options.orchestratorDebug === true || options.orchestratorDebug === 'true'; | ||
|
|
||
| // Build platform | ||
| bp.buildPlatform = options.buildPlatform || (bp.providerStrategy !== 'local' ? 'linux' : process.platform); | ||
| bp.isCliMode = true; | ||
|
|
||
| // Caching | ||
| bp.cacheKey = options.cacheKey || bp.branch; | ||
| bp.pullInputList = options.pullInputList ? options.pullInputList.split(',') : []; | ||
| bp.inputPullCommand = options.inputPullCommand || ''; | ||
|
|
||
| // Advanced | ||
| bp.maxRetainedWorkspaces = Number.parseInt(options.maxRetainedWorkspaces || '0', 10); | ||
| bp.useLargePackages = options.useLargePackages === true || options.useLargePackages === 'true'; | ||
| bp.useCompressionStrategy = options.useCompressionStrategy === true || options.useCompressionStrategy === 'true'; | ||
| bp.garbageMaxAge = Number(options.garbageMaxAge) || 24; | ||
| bp.githubChecks = options.githubChecks === true || options.githubChecks === 'true'; | ||
| bp.asyncWorkflow = options.asyncOrchestrator === true || options.asyncOrchestrator === 'true'; | ||
| bp.githubCheckId = options.githubCheckId || ''; | ||
| bp.finalHooks = options.finalHooks ? options.finalHooks.split(',') : []; | ||
| bp.skipLfs = options.skipLfs === true || options.skipLfs === 'true'; | ||
| bp.skipCache = options.skipCache === true || options.skipCache === 'true'; | ||
| bp.cacheUnityInstallationOnMac = | ||
| options.cacheUnityInstallationOnMac === true || options.cacheUnityInstallationOnMac === 'true'; | ||
| bp.unityHubVersionOnMac = options.unityHubVersionOnMac || ''; |
There was a problem hiding this comment.
Several declared CLI flags are silently discarded during mapping.
region, containerHookFiles, commandHookFiles, resourceTracking, useSharedBuilder, and useCleanupCron are registered options but never copied into BuildParameters, so user-provided values are lost.
🔧 Proposed mapping additions
// Provider / orchestrator settings
+ bp.region = options.region || 'eu-west-2';
bp.providerStrategy = options.providerStrategy || 'local-docker';
bp.customParameters = options.customParameters || '';
@@
// Hooks
+ bp.containerHookFiles = options.containerHookFiles || '';
+ bp.commandHookFiles = options.commandHookFiles || '';
bp.commandHooks = options.commandHooks || '';
@@
// Advanced
+ bp.resourceTracking = options.resourceTracking === true || options.resourceTracking === 'true';
+ bp.useSharedBuilder = options.useSharedBuilder === true || options.useSharedBuilder === 'true';
+ bp.useCleanupCron = options.useCleanupCron === true || options.useCleanupCron === 'true';
bp.maxRetainedWorkspaces = Number.parseInt(options.maxRetainedWorkspaces || '0', 10);📝 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.
Suggested change
| // Provider / orchestrator settings | |
| bp.providerStrategy = options.providerStrategy || 'local-docker'; | |
| bp.customParameters = options.customParameters || ''; | |
| bp.sshAgent = options.sshAgent || ''; | |
| bp.sshPublicKeysDirectoryPath = options.sshPublicKeysDirectoryPath || ''; | |
| bp.gitPrivateToken = options.gitPrivateToken || ''; | |
| bp.runAsHostUser = options.runAsHostUser || 'false'; | |
| bp.chownFilesTo = options.chownFilesTo || ''; | |
| // AWS | |
| bp.awsStackName = options.awsStackName || 'game-ci'; | |
| bp.awsEndpoint = options.awsEndpoint; | |
| bp.awsCloudFormationEndpoint = options.awsCloudFormationEndpoint || options.awsEndpoint; | |
| bp.awsEcsEndpoint = options.awsEcsEndpoint || options.awsEndpoint; | |
| bp.awsKinesisEndpoint = options.awsKinesisEndpoint || options.awsEndpoint; | |
| bp.awsCloudWatchLogsEndpoint = options.awsCloudWatchLogsEndpoint || options.awsEndpoint; | |
| bp.awsS3Endpoint = options.awsS3Endpoint || options.awsEndpoint; | |
| // Storage | |
| bp.storageProvider = options.storageProvider || 's3'; | |
| bp.rcloneRemote = options.rcloneRemote || ''; | |
| // Kubernetes | |
| bp.kubeConfig = options.kubeConfig || ''; | |
| bp.kubeVolume = options.kubeVolume || ''; | |
| bp.kubeVolumeSize = options.kubeVolumeSize || '25Gi'; | |
| bp.kubeStorageClass = options.kubeStorageClass || ''; | |
| // Container resources | |
| bp.containerMemory = options.containerMemory || '3072'; | |
| bp.containerCpu = options.containerCpu || '1024'; | |
| bp.containerNamespace = options.containerNamespace || 'default'; | |
| // Hooks | |
| bp.commandHooks = options.commandHooks || ''; | |
| bp.postBuildContainerHooks = options.postBuildContainerHooks || ''; | |
| bp.preBuildContainerHooks = options.preBuildContainerHooks || ''; | |
| bp.customJob = options.customJob || ''; | |
| // Git / CI | |
| bp.runNumber = options.runNumber || process.env.GITHUB_RUN_NUMBER || '0'; | |
| bp.branch = options.branch || process.env.GITHUB_REF?.replace('refs/', '').replace('heads/', '') || ''; | |
| bp.githubRepo = options.githubRepo || process.env.GITHUB_REPOSITORY || ''; | |
| bp.orchestratorRepoName = options.orchestratorRepoName || 'game-ci/unity-builder'; | |
| bp.cloneDepth = Number.parseInt(options.cloneDepth || '50', 10); | |
| bp.gitSha = options.gitSha || process.env.GITHUB_SHA || ''; | |
| bp.orchestratorBranch = options.orchestratorBranch || 'main'; | |
| bp.orchestratorDebug = options.orchestratorDebug === true || options.orchestratorDebug === 'true'; | |
| // Build platform | |
| bp.buildPlatform = options.buildPlatform || (bp.providerStrategy !== 'local' ? 'linux' : process.platform); | |
| bp.isCliMode = true; | |
| // Caching | |
| bp.cacheKey = options.cacheKey || bp.branch; | |
| bp.pullInputList = options.pullInputList ? options.pullInputList.split(',') : []; | |
| bp.inputPullCommand = options.inputPullCommand || ''; | |
| // Advanced | |
| bp.maxRetainedWorkspaces = Number.parseInt(options.maxRetainedWorkspaces || '0', 10); | |
| bp.useLargePackages = options.useLargePackages === true || options.useLargePackages === 'true'; | |
| bp.useCompressionStrategy = options.useCompressionStrategy === true || options.useCompressionStrategy === 'true'; | |
| bp.garbageMaxAge = Number(options.garbageMaxAge) || 24; | |
| bp.githubChecks = options.githubChecks === true || options.githubChecks === 'true'; | |
| bp.asyncWorkflow = options.asyncOrchestrator === true || options.asyncOrchestrator === 'true'; | |
| bp.githubCheckId = options.githubCheckId || ''; | |
| bp.finalHooks = options.finalHooks ? options.finalHooks.split(',') : []; | |
| bp.skipLfs = options.skipLfs === true || options.skipLfs === 'true'; | |
| bp.skipCache = options.skipCache === true || options.skipCache === 'true'; | |
| bp.cacheUnityInstallationOnMac = | |
| options.cacheUnityInstallationOnMac === true || options.cacheUnityInstallationOnMac === 'true'; | |
| bp.unityHubVersionOnMac = options.unityHubVersionOnMac || ''; | |
| // Provider / orchestrator settings | |
| bp.region = options.region || 'eu-west-2'; | |
| bp.providerStrategy = options.providerStrategy || 'local-docker'; | |
| bp.customParameters = options.customParameters || ''; | |
| bp.sshAgent = options.sshAgent || ''; | |
| bp.sshPublicKeysDirectoryPath = options.sshPublicKeysDirectoryPath || ''; | |
| bp.gitPrivateToken = options.gitPrivateToken || ''; | |
| bp.runAsHostUser = options.runAsHostUser || 'false'; | |
| bp.chownFilesTo = options.chownFilesTo || ''; | |
| // AWS | |
| bp.awsStackName = options.awsStackName || 'game-ci'; | |
| bp.awsEndpoint = options.awsEndpoint; | |
| bp.awsCloudFormationEndpoint = options.awsCloudFormationEndpoint || options.awsEndpoint; | |
| bp.awsEcsEndpoint = options.awsEcsEndpoint || options.awsEndpoint; | |
| bp.awsKinesisEndpoint = options.awsKinesisEndpoint || options.awsEndpoint; | |
| bp.awsCloudWatchLogsEndpoint = options.awsCloudWatchLogsEndpoint || options.awsEndpoint; | |
| bp.awsS3Endpoint = options.awsS3Endpoint || options.awsEndpoint; | |
| // Storage | |
| bp.storageProvider = options.storageProvider || 's3'; | |
| bp.rcloneRemote = options.rcloneRemote || ''; | |
| // Kubernetes | |
| bp.kubeConfig = options.kubeConfig || ''; | |
| bp.kubeVolume = options.kubeVolume || ''; | |
| bp.kubeVolumeSize = options.kubeVolumeSize || '25Gi'; | |
| bp.kubeStorageClass = options.kubeStorageClass || ''; | |
| // Container resources | |
| bp.containerMemory = options.containerMemory || '3072'; | |
| bp.containerCpu = options.containerCpu || '1024'; | |
| bp.containerNamespace = options.containerNamespace || 'default'; | |
| // Hooks | |
| bp.containerHookFiles = options.containerHookFiles || ''; | |
| bp.commandHookFiles = options.commandHookFiles || ''; | |
| bp.commandHooks = options.commandHooks || ''; | |
| bp.postBuildContainerHooks = options.postBuildContainerHooks || ''; | |
| bp.preBuildContainerHooks = options.preBuildContainerHooks || ''; | |
| bp.customJob = options.customJob || ''; | |
| // Git / CI | |
| bp.runNumber = options.runNumber || process.env.GITHUB_RUN_NUMBER || '0'; | |
| bp.branch = options.branch || process.env.GITHUB_REF?.replace('refs/', '').replace('heads/', '') || ''; | |
| bp.githubRepo = options.githubRepo || process.env.GITHUB_REPOSITORY || ''; | |
| bp.orchestratorRepoName = options.orchestratorRepoName || 'game-ci/unity-builder'; | |
| bp.cloneDepth = Number.parseInt(options.cloneDepth || '50', 10); | |
| bp.gitSha = options.gitSha || process.env.GITHUB_SHA || ''; | |
| bp.orchestratorBranch = options.orchestratorBranch || 'main'; | |
| bp.orchestratorDebug = options.orchestratorDebug === true || options.orchestratorDebug === 'true'; | |
| // Build platform | |
| bp.buildPlatform = options.buildPlatform || (bp.providerStrategy !== 'local' ? 'linux' : process.platform); | |
| bp.isCliMode = true; | |
| // Caching | |
| bp.cacheKey = options.cacheKey || bp.branch; | |
| bp.pullInputList = options.pullInputList ? options.pullInputList.split(',') : []; | |
| bp.inputPullCommand = options.inputPullCommand || ''; | |
| // Advanced | |
| bp.resourceTracking = options.resourceTracking === true || options.resourceTracking === 'true'; | |
| bp.useSharedBuilder = options.useSharedBuilder === true || options.useSharedBuilder === 'true'; | |
| bp.useCleanupCron = options.useCleanupCron === true || options.useCleanupCron === 'true'; | |
| bp.maxRetainedWorkspaces = Number.parseInt(options.maxRetainedWorkspaces || '0', 10); | |
| bp.useLargePackages = options.useLargePackages === true || options.useLargePackages === 'true'; | |
| bp.useCompressionStrategy = options.useCompressionStrategy === true || options.useCompressionStrategy === 'true'; | |
| bp.garbageMaxAge = Number(options.garbageMaxAge) || 24; | |
| bp.githubChecks = options.githubChecks === true || options.githubChecks === 'true'; | |
| bp.asyncWorkflow = options.asyncOrchestrator === true || options.asyncOrchestrator === 'true'; | |
| bp.githubCheckId = options.githubCheckId || ''; | |
| bp.finalHooks = options.finalHooks ? options.finalHooks.split(',') : []; | |
| bp.skipLfs = options.skipLfs === true || options.skipLfs === 'true'; | |
| bp.skipCache = options.skipCache === true || options.skipCache === 'true'; | |
| bp.cacheUnityInstallationOnMac = | |
| options.cacheUnityInstallationOnMac === true || options.cacheUnityInstallationOnMac === 'true'; | |
| bp.unityHubVersionOnMac = options.unityHubVersionOnMac || ''; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli-plugin/build-parameters-adapter.ts` around lines 53 - 124, The
BuildParameters mapping in build-parameters-adapter.ts is dropping several
registered CLI flags (region, containerHookFiles, commandHookFiles,
resourceTracking, useSharedBuilder, useCleanupCron); update the bp assignments
(the BuildParameters object referenced as bp) to copy these options from the
incoming options object: set bp.region = options.region || '' (or appropriate
default), map bp.containerHookFiles and bp.commandHookFiles to arrays (e.g.
options.containerHookFiles ? options.containerHookFiles.split(',') : []), set
bp.resourceTracking = options.resourceTracking === true ||
options.resourceTracking === 'true', and set bp.useSharedBuilder and
bp.useCleanupCron similarly to boolean conversions (=== true || === 'true'); add
these lines alongside the other mappings in the same adapter so user-provided
values are preserved.
| bp.orchestratorDebug = options.orchestratorDebug === true || options.orchestratorDebug === 'true'; | ||
|
|
||
| // Build platform | ||
| bp.buildPlatform = options.buildPlatform || (bp.providerStrategy !== 'local' ? 'linux' : process.platform); |
There was a problem hiding this comment.
local-system gets the wrong default buildPlatform.
Current default treats only providerStrategy === 'local' as host-local. local-system is also host-local and should not default to 'linux' on macOS/Windows.
🔧 Proposed fix
- bp.buildPlatform = options.buildPlatform || (bp.providerStrategy !== 'local' ? 'linux' : process.platform);
+ const isHostLocalProvider =
+ bp.providerStrategy === 'local' || bp.providerStrategy === 'local-system';
+ bp.buildPlatform = options.buildPlatform || (isHostLocalProvider ? process.platform : 'linux');📝 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.
Suggested change
| bp.buildPlatform = options.buildPlatform || (bp.providerStrategy !== 'local' ? 'linux' : process.platform); | |
| const isHostLocalProvider = | |
| bp.providerStrategy === 'local' || bp.providerStrategy === 'local-system'; | |
| bp.buildPlatform = options.buildPlatform || (isHostLocalProvider ? process.platform : 'linux'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli-plugin/build-parameters-adapter.ts` at line 103, The default
buildPlatform assignment in build-parameters-adapter.ts incorrectly treats only
providerStrategy === 'local' as host-local; update the logic around
bp.buildPlatform (the assignment that uses options.buildPlatform ||
(bp.providerStrategy !== 'local' ? 'linux' : process.platform)) so that
providerStrategy values 'local' and 'local-system' are both considered
host-local (i.e., if bp.providerStrategy is neither 'local' nor 'local-system'
default to 'linux', otherwise use process.platform); change the conditional to
check for both identifiers (e.g., via explicit comparisons or an inclusion
check) to ensure local-system uses the host platform default.
| bp.maxRetainedWorkspaces = Number.parseInt(options.maxRetainedWorkspaces || '0', 10); | ||
| bp.useLargePackages = options.useLargePackages === true || options.useLargePackages === 'true'; | ||
| bp.useCompressionStrategy = options.useCompressionStrategy === true || options.useCompressionStrategy === 'true'; | ||
| bp.garbageMaxAge = Number(options.garbageMaxAge) || 24; |
There was a problem hiding this comment.
garbageMaxAge cannot be set to 0.
Number(options.garbageMaxAge) || 24 forces 0 back to 24. This breaks a valid boundary value.
🔧 Proposed fix
- bp.garbageMaxAge = Number(options.garbageMaxAge) || 24;
+ const parsedGarbageMaxAge = Number(options.garbageMaxAge);
+ bp.garbageMaxAge = Number.isNaN(parsedGarbageMaxAge) ? 24 : parsedGarbageMaxAge;📝 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.
Suggested change
| bp.garbageMaxAge = Number(options.garbageMaxAge) || 24; | |
| const parsedGarbageMaxAge = Number(options.garbageMaxAge); | |
| bp.garbageMaxAge = Number.isNaN(parsedGarbageMaxAge) ? 24 : parsedGarbageMaxAge; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli-plugin/build-parameters-adapter.ts` at line 115, The assignment
bp.garbageMaxAge = Number(options.garbageMaxAge) || 24 erroneously treats 0 as
falsy and resets it to 24; instead parse the value and only fallback when the
parsed value is NaN/invalid—e.g., compute const parsed =
Number(options.garbageMaxAge) and set bp.garbageMaxAge = Number.isFinite(parsed)
? parsed : 24 (or use a nullish/NaN check) so that 0 is preserved; update the
code around bp.garbageMaxAge and options.garbageMaxAge accordingly.
| 'local-docker': createProviderAdapter(LocalDockerOrchestrator), | ||
| 'local-system': createProviderAdapter(LocalOrchestrator), | ||
| local: createProviderAdapter(LocalOrchestrator), | ||
| test: createProviderAdapter(TestOrchestrator), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=ts -C2 'runTaskInWorkflow\s*\(' \
src/model/orchestrator/providers/provider-interface.ts \
src/model/orchestrator/providers/test/index.ts \
src/cli-plugin/provider-adapter.tsRepository: game-ci/unity-builder
Length of output: 1646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full runTaskInWorkflow signature from test provider
echo "=== Test Provider Implementation ==="
sed -n '45,60p' src/model/orchestrator/providers/test/index.ts
echo -e "\n=== Provider Interface Definition ==="
sed -n '26,42p' src/model/orchestrator/providers/provider-interface.ts
echo -e "\n=== Adapter Implementation ==="
sed -n '33,43p' src/cli-plugin/provider-adapter.ts
echo -e "\n=== Line 57 context in index.ts ==="
sed -n '50,65p' src/cli-plugin/index.tsRepository: game-ci/unity-builder
Length of output: 2340
test provider exposes a critical interface violation: TestOrchestrator.runTaskInWorkflow parameters are in the wrong order.
The interface defines runTaskInWorkflow(buildGuid, image, commands, ...), but TestOrchestrator implements runTaskInWorkflow(commands, buildGuid, image, ...). The adapter forwards arguments in interface-compliant order, causing TestOrchestrator to receive them misaligned: buildGuid receives the buildGuid value interpreted as commands, image receives buildGuid as image, and commands receives image as commands. This violates the provider contract and produces incorrect runtime behavior when test is selected in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli-plugin/index.ts` at line 57, The Test provider is violating the
provider interface because TestOrchestrator implements
runTaskInWorkflow(commands, buildGuid, image, ...) while the expected signature
is runTaskInWorkflow(buildGuid, image, commands, ...); update
TestOrchestrator.runTaskInWorkflow to take parameters in the correct order
(buildGuid, image, commands, ...) and adjust the internal use of those
parameters accordingly so that createProviderAdapter(TestOrchestrator) forwards
arguments correctly to the implementation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #824 +/- ##
==========================================
+ Coverage 31.23% 33.36% +2.12%
==========================================
Files 84 88 +4
Lines 4565 4736 +171
Branches 1103 1137 +34
==========================================
+ Hits 1426 1580 +154
- Misses 3139 3156 +17
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GameCIPlugin-compatible export atsrc/cli-plugin/so the CLI repo can consume the orchestrator as a plugin viaPluginRegistry.register(orchestratorPlugin)BuildParametersobject that all providers expectProviderInterfaceconstructors so the CLI can instantiate them with yargs options instead ofBuildParametersdirectlyexportsfield topackage.jsonfor./cli-pluginsubpath importRelated
Test plan
npx jest src/cli-plugin/cli-plugin.test.ts)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests