Conversation
Breaking change: All CloudRunner classes, options, environment variables, and action.yml inputs have been renamed to Orchestrator equivalents. - Renamed src/model/cloud-runner/ directory to src/model/orchestrator/ - Renamed all cloud-runner-* files to orchestrator-* - Renamed all CloudRunner* classes to Orchestrator* (15+ classes) - Renamed all cloudRunner* properties to orchestrator* equivalents - Renamed CLOUD_RUNNER_* env vars to ORCHESTRATOR_* - Updated action.yml [CloudRunner] markers to [Orchestrator] - Updated workflow files and package.json test scripts - Updated all runtime strings (cache paths, log messages, branch refs) - Rebuilt dist/index.js No backward compatibility layer is provided. Co-Authored-By: Claude Opus 4.6 <[email protected]>
📝 WalkthroughWalkthroughThis pull request performs a comprehensive systematic rename of the "CloudRunner" component to "Orchestrator" across the entire codebase. CloudRunner classes, modules, configuration options, imports, and public APIs are renamed to Orchestrator equivalents. All workflow files, test suites, provider implementations, and utilities are updated consistently. No functional logic or control flows are modified. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/model/orchestrator/providers/aws/services/task-service.ts (1)
52-56:⚠️ Potential issue | 🟡 MinorDay-age logging is incorrect (D is always 0).
Using
Math.floor(ageDate.getHours() / 24)won't produce elapsed days becausegetHours()returns only the hour component (0–23) of the date object, not total hours elapsed. This affects lines 53–55, 74–76, and 193–195.Calculate elapsed time in milliseconds and derive days/hours/minutes from that instead:
- const ageDate: Date = new Date(Date.now() - (element.CreationTime?.getTime() ?? 0)); - OrchestratorLogger.log( - `Task Stack ${element.StackName} - Age D${Math.floor( - ageDate.getHours() / 24, - )} H${ageDate.getHours()} M${ageDate.getMinutes()}`, - ); + const ageMs = Date.now() - (element.CreationTime?.getTime() ?? 0); + const ageDays = Math.floor(ageMs / (24 * 60 * 60 * 1000)); + const ageHours = Math.floor(ageMs / (60 * 60 * 1000)) % 24; + const ageMinutes = Math.floor(ageMs / (60 * 1000)) % 60; + OrchestratorLogger.log( + `Task Stack ${element.StackName} - Age D${ageDays} H${ageHours} M${ageMinutes}`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/aws/services/task-service.ts` around lines 52 - 56, The logs use Math.floor(ageDate.getHours() / 24) which is wrong because getHours() is only 0–23; instead compute elapsed = Date.now() - <timestamp> (the timestamp used to build ageDate), then derive days = Math.floor(elapsed / (1000*60*60*24)), hours = Math.floor((elapsed % (1000*60*60*24)) / (1000*60*60)), minutes = Math.floor((elapsed % (1000*60*60)) / (1000*60)); replace the existing uses of ageDate.getHours()/getMinutes() in the Task Stack log(s) (the calls to OrchestratorLogger.log that mention "Task Stack ..." and other similar logs around where ageDate is created) to format using these computed days/hours/minutes so D/H/M reflect total elapsed time.src/model/orchestrator/providers/aws/services/garbage-collection-service.ts (1)
35-53:⚠️ Potential issue | 🟠 Major
returninside the stack loop aborts cleanup too early.Line 42 and Line 52 exit
cleanup()entirely, so later stacks/log-groups/locks are never processed. This should becontinueto skip only the current stack.Proposed fix
- return; + continue; ... - return; + continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/aws/services/garbage-collection-service.ts` around lines 35 - 53, The loop in GarbageCollectionService.cleanup() incorrectly uses return to skip a single stack, which aborts the entire cleanup; replace the two return statements inside the stack iteration (the one after logging "Skipping ${element.StackName} - active task..." and the one after logging "Skipping ${element.StackName} ignore list") with continue so only the current element is skipped and the loop proceeds to process remaining stacks, log-groups, and locks.src/model/orchestrator/tests/e2e/orchestrator-end2end-locking.test.ts (1)
46-46:⚠️ Potential issue | 🟠 MajorMissing assertion - statement has no effect.
This line evaluates a boolean but doesn't assert it. The
.includes()result is not wrapped inexpect().toBeTruthy(), so the check is silently skipped.Proposed fix
- expect(lines.map((x) => x.replace(`/`, ``)).includes(buildParameters.cacheKey)); + expect(lines.map((x) => x.replace(`/`, ``)).includes(buildParameters.cacheKey)).toBeTruthy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/tests/e2e/orchestrator-end2end-locking.test.ts` at line 46, The test currently computes a boolean but doesn't assert it; wrap the evaluated expression so the test actually asserts inclusion — e.g., change the bare expression using lines.map(...).includes(buildParameters.cacheKey) into an expect(...) call (for example expect(lines.map(x => x.replace('/', '')).includes(buildParameters.cacheKey)).toBeTruthy() or .toBe(true)) so the check with variables lines and buildParameters.cacheKey becomes an assertion.src/model/github.ts (1)
124-127:⚠️ Potential issue | 🟡 MinorPre-existing logic issue:
endedDateis only set when already defined.The condition
if (GitHub.endedDate !== undefined)meansendedDateonly gets set if it already has a value, but it starts asundefined(line 12). This appears inverted - it should likely be=== undefinedto set it on first completion.This is a pre-existing issue not introduced by this PR, but worth addressing.
Proposed fix
if (status === `completed`) { - if (GitHub.endedDate !== undefined) { + if (GitHub.endedDate === undefined) { GitHub.endedDate = new Date().toISOString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/github.ts` around lines 124 - 127, The current completion branch only assigns GitHub.endedDate when it is already defined due to the inverted check; in the block where you check if (status === `completed`) update the condition so the assignment runs when endedDate is not yet set (e.g., change the guard from GitHub.endedDate !== undefined to GitHub.endedDate === undefined or remove the guard entirely) and ensure GitHub.endedDate = new Date().toISOString() is executed to capture the first completion time.src/model/orchestrator/providers/test/index.ts (1)
45-64:⚠️ Potential issue | 🟠 MajorFix argument-order mismatch in
runTaskInWorkflow.This implementation orders the first three
stringargs differently thanProviderInterface, which can silently swapbuildGuid,image, andcommandsat runtime.🔧 Proposed fix
- public async runTaskInWorkflow( - commands: string, - buildGuid: string, - image: string, + public async runTaskInWorkflow( + buildGuid: string, + image: string, + commands: string, // eslint-disable-next-line no-unused-vars mountdir: string, // eslint-disable-next-line no-unused-vars workingdir: string, // eslint-disable-next-line no-unused-vars environment: OrchestratorEnvironmentVariable[], // eslint-disable-next-line no-unused-vars secrets: OrchestratorSecret[], ): Promise<string> { OrchestratorLogger.log(image); OrchestratorLogger.log(buildGuid); OrchestratorLogger.log(commands); - return await new Promise((result) => { - result(commands); - }); + return commands; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/test/index.ts` around lines 45 - 64, The runTaskInWorkflow implementation has its first three string parameters in the wrong order compared to ProviderInterface, causing buildGuid, image and commands to be passed incorrectly; update the function signature of runTaskInWorkflow so the parameters match ProviderInterface (reorder the first three string params to the same order used by the interface), then adjust any in-function uses (OrchestratorLogger.log calls and the returned Promise value) to refer to the correctly named variables (buildGuid, image, commands) so logging and the returned result remain consistent with the new parameter order.src/model/orchestrator/providers/aws/index.ts (1)
150-171:⚠️ Potential issue | 🟠 MajorGate cleanup-stack waiter with the same condition as delete.
At Line 150 deletion of
-cleanupstack is conditional, but waiting for its deletion is unconditional. This can fail cleanup whenuseCleanupCronis false.Proposed fix
await waitUntilStackDeleteComplete( { client: CF, maxWaitTime: stackWaitTimeSeconds, }, { StackName: taskDef.taskDefStackName, }, ); - await waitUntilStackDeleteComplete( - { - client: CF, - maxWaitTime: stackWaitTimeSeconds, - }, - { - StackName: `${taskDef.taskDefStackName}-cleanup`, - }, - ); + if (OrchestratorOptions.useCleanupCron) { + await waitUntilStackDeleteComplete( + { + client: CF, + maxWaitTime: stackWaitTimeSeconds, + }, + { + StackName: `${taskDef.taskDefStackName}-cleanup`, + }, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/aws/index.ts` around lines 150 - 171, The waiter for the cleanup stack is unconditionally invoked even when OrchestratorOptions.useCleanupCron is false, causing failures; modify the logic so the DeleteStackCommand and the corresponding waitUntilStackDeleteComplete call for `${taskDef.taskDefStackName}-cleanup` are both executed only when OrchestratorOptions.useCleanupCron is true — i.e., wrap the second waitUntilStackDeleteComplete (the one referencing `${taskDef.taskDefStackName}-cleanup`) inside the same if (OrchestratorOptions.useCleanupCron) block that calls CF.send(new DeleteStackCommand(...)), keeping the existing waitUntilStackDeleteComplete call for taskDef.taskDefStackName and using the same client CF and stackWaitTimeSeconds.
🟡 Minor comments (8)
src/model/orchestrator/providers/k8s/kubernetes-storage.ts-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorFix the duplicated phrase in the override log message.
Line 17 currently reads awkwardly ("was input was set"), which reduces log clarity during debugging.
Proposed wording fix
- OrchestratorLogger.log(`Kube Volume was input was set ${buildParameters.kubeVolume} overriding ${pvcName}`); + OrchestratorLogger.log(`Kube volume input was set to ${buildParameters.kubeVolume}; overriding ${pvcName}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/k8s/kubernetes-storage.ts` at line 17, The log message in OrchestratorLogger.log currently contains a duplicated phrase ("was input was set"); update the string in kubernetes-storage.ts where OrchestratorLogger.log is called with buildParameters.kubeVolume and pvcName to a clear message such as: "Kube volume was set to {buildParameters.kubeVolume}, overriding {pvcName}" (or "Kube volume input was set to ... overriding ...") so the duplicate "was" is removed and the variables remain included for context.src/model/input.ts-35-36 (1)
35-36:⚠️ Potential issue | 🟡 MinorAvoid double query calls and preserve empty-string overrides.
Line 35 and Line 36 call
OrchestratorQueryOverride.query(...)twice and use a truthy gate, which treats''as missing. Cache once and check!== undefined.Proposed fix
- if (OrchestratorQueryOverride.query(query, alternativeQuery)) { - return OrchestratorQueryOverride.query(query, alternativeQuery); - } + const orchestratorOverride = OrchestratorQueryOverride.query(query, alternativeQuery); + if (orchestratorOverride !== undefined) { + return orchestratorOverride; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/input.ts` around lines 35 - 36, The code currently calls OrchestratorQueryOverride.query(query, alternativeQuery) twice and uses a truthy check that treats an empty-string override as missing; fix by calling OrchestratorQueryOverride.query(query, alternativeQuery) once (store result in a local variable, e.g., override) and then return it only if override !== undefined (not a truthy check) so that empty string values are preserved; update the logic around the call sites referencing OrchestratorQueryOverride.query to use the cached local variable.src/model/cli/cli.ts-115-115 (1)
115-115:⚠️ Potential issue | 🟡 MinorSame grammar issue: "a orchestrator" should be "an orchestrator".
📝 Proposed fix
- `@CliFunction`(`async-workflow`, `runs a orchestrator build`) + `@CliFunction`(`async-workflow`, `runs an orchestrator build`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/cli/cli.ts` at line 115, The CLI help string has incorrect article usage in the `@CliFunction` decorator for the command labeled `async-workflow`; update the description text from "runs a orchestrator build" to "runs an orchestrator build" by editing the `@CliFunction`(`async-workflow`, `runs a orchestrator build`) decorator so the second argument reads `runs an orchestrator build`.src/model/cli/cli.ts-124-124 (1)
124-124:⚠️ Potential issue | 🟡 MinorSame grammar issue: "a orchestrator" should be "an orchestrator".
📝 Proposed fix
- `@CliFunction`(`checks-update`, `runs a orchestrator build`) + `@CliFunction`(`checks-update`, `runs an orchestrator build`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/cli/cli.ts` at line 124, Update the decorator metadata string on the `@CliFunction` annotation to fix the grammar: change the description from "runs a orchestrator build" to "runs an orchestrator build" in the `@CliFunction`(`checks-update`, `runs a orchestrator build`) usage so the annotation reads `@CliFunction`(`checks-update`, `runs an orchestrator build`).src/model/cli/cli.ts-107-107 (1)
107-107:⚠️ Potential issue | 🟡 MinorGrammar issue: "a orchestrator" should be "an orchestrator".
The description uses incorrect article before "orchestrator".
📝 Proposed fix
- `@CliFunction`(`cli-build`, `runs a orchestrator build`) + `@CliFunction`(`cli-build`, `runs an orchestrator build`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/cli/cli.ts` at line 107, Update the CliFunction decorator description for the cli-build command: change the string literal `runs a orchestrator build` to `runs an orchestrator build` in the `@CliFunction`(`cli-build`, `...`) invocation so the article is grammatically correct.src/model/orchestrator/providers/k8s/kubernetes-secret.ts-39-39 (1)
39-39:⚠️ Potential issue | 🟡 MinorMinor: Grammatically incorrect log message.
The message "Created secret failed" should be "Creating secret failed" or "Secret creation failed" for clarity.
Proposed fix
- OrchestratorLogger.log(`Created secret failed ${error}`); + OrchestratorLogger.log(`Creating secret failed: ${error}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/k8s/kubernetes-secret.ts` at line 39, The log message in the OrchestratorLogger.log call inside kubernetes-secret.ts is grammatically incorrect ("Created secret failed"); update that string to a clearer message such as "Creating secret failed" or "Secret creation failed" and keep including the ${error} interpolation so the call remains OrchestratorLogger.log(`Secret creation failed ${error}`) (locate the OrchestratorLogger.log invocation in the Kubernetes secret creation/error handling code and replace only the message text).src/model/orchestrator/providers/aws/aws-task-runner.ts-272-272 (1)
272-272:⚠️ Potential issue | 🟡 MinorFix typo in status log message.
The log text has a typo (
unknwon), which makes diagnostics harder to scan.✏️ Suggested fix
- OrchestratorLogger.log('## Orchestrator job unknwon'); + OrchestratorLogger.log('## Orchestrator job unknown');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/aws/aws-task-runner.ts` at line 272, Update the typo in the status log string used by OrchestratorLogger.log: replace the misspelled "Orchestrator job unknwon" message with "Orchestrator job unknown" (locate the call to OrchestratorLogger.log in aws-task-runner.ts that emits this message and correct the string).src/model/orchestrator/remote-client/index.ts-83-84 (1)
83-84:⚠️ Potential issue | 🟡 MinorMinor grammar issue in CLI function description.
The description should use "an" before "orchestrator" since it starts with a vowel sound.
- `@CliFunction`(`remote-cli-post-build`, `runs a orchestrator build`) + `@CliFunction`(`remote-cli-post-build`, `runs an orchestrator build`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/remote-client/index.ts` around lines 83 - 84, Update the `@CliFunction` decorator string for the remoteClientPostBuild method: change the description text from "runs a orchestrator build" to "runs an orchestrator build" so the annotation on remoteClientPostBuild uses correct grammar.
🧹 Nitpick comments (19)
action.yml (1)
237-237: Fix minor typo in user-facing text.Use “disk space” instead of “disc space”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@action.yml` at line 237, Update the user-facing description string that currently reads "Amount of disc space to assign the Kubernetes Persistent Volume" to use the correct word "disk" instead of "disc"; modify the description value in action.yml (the description key on the Kubernetes Persistent Volume input) so it reads "Amount of disk space to assign the Kubernetes Persistent Volume".src/model/orchestrator/options/orchestrator-query-override.ts (2)
33-45: Return an explicit boolean fromshouldUseOverride().Line 45 currently falls through with
undefined; returningfalseexplicitly makes override gating deterministic and easier to reason about.♻️ Proposed refactor
- private static shouldUseOverride(query: string) { - if (OrchestratorOptions.inputPullCommand !== '') { - if (OrchestratorOptions.pullInputList.length > 0) { - const doesInclude = - OrchestratorOptions.pullInputList.includes(query) || - OrchestratorOptions.pullInputList.includes(Input.ToEnvVarFormat(query)); - - return doesInclude ? true : false; - } else { - return true; - } - } - } + private static shouldUseOverride(query: string): boolean { + if (OrchestratorOptions.inputPullCommand === '') { + return false; + } + if (OrchestratorOptions.pullInputList.length === 0) { + return true; + } + return ( + OrchestratorOptions.pullInputList.includes(query) || + OrchestratorOptions.pullInputList.includes(Input.ToEnvVarFormat(query)) + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-query-override.ts` around lines 33 - 45, The method shouldUseOverride currently can return undefined when OrchestratorOptions.inputPullCommand is an empty string; update shouldUseOverride(query: string) to always return a boolean by adding an explicit false return for the fallback path and ensure all branches return boolean values — keep the existing checks using OrchestratorOptions.inputPullCommand, OrchestratorOptions.pullInputList, and Input.ToEnvVarFormat(query) but return true or false explicitly (e.g., return doesInclude when list exists, return true when pullInputList is empty, and return false when inputPullCommand is empty).
5-8:formatFunction()only replaces the first occurrence per token.If
{0}appears more than once in the template, later occurrences remain unresolved.🧩 Proposed tweak
const formatFunction = (value: string, arguments_: any[]) => { for (const element of arguments_) { - value = value.replace(`{${element.key}}`, element.value); + value = value.replaceAll(`{${element.key}}`, element.value); } return value; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-query-override.ts` around lines 5 - 8, formatFunction currently replaces only the first occurrence of each token because it uses String.replace; update it to replace all occurrences of each token (e.g., when `{0}` appears multiple times) by using String.prototype.replaceAll or a global RegExp for the pattern `{${key}}` (ensure the token/key is escaped before building the RegExp), iterating over arguments_ and replacing every match with element.value so all template instances are resolved.src/model/orchestrator/options/orchestrator-folders.ts (1)
15-19: Consider adding a null guard forOrchestrator.buildParameters.Multiple getters in this class access
Orchestrator.buildParametersdirectly. If these getters are called beforebuildParametersis initialized, they will throw runtime errors.The conditional on line 16 passes
Orchestrator.buildParameterstoshouldUseRetainedWorkspaceMode, but if it's undefined, the method call may behave unexpectedly, and line 18 will throw when accessing.buildGuid.♻️ Suggested defensive check
public static get uniqueOrchestratorJobFolderAbsolute(): string { + if (!Orchestrator.buildParameters) { + throw new Error('Orchestrator.buildParameters must be initialized before accessing folder paths'); + } return Orchestrator.buildParameters && BuildParameters.shouldUseRetainedWorkspaceMode(Orchestrator.buildParameters) ? path.join(`/`, OrchestratorFolders.buildVolumeFolder, Orchestrator.lockedWorkspace) : path.join(`/`, OrchestratorFolders.buildVolumeFolder, Orchestrator.buildParameters.buildGuid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-folders.ts` around lines 15 - 19, uniqueOrchestratorJobFolderAbsolute uses Orchestrator.buildParameters without guarding against undefined which can throw; first read Orchestrator.buildParameters into a local (e.g., const bp = Orchestrator.buildParameters) and guard it before calling BuildParameters.shouldUseRetainedWorkspaceMode(bp) or accessing bp.buildGuid; if bp is falsy return a safe fallback (for example path.join('/', OrchestratorFolders.buildVolumeFolder, Orchestrator.lockedWorkspace) or another explicit default) so the getter never dereferences undefined; apply the same null-guard pattern when referencing Orchestrator.buildParameters in other getters.src/model/orchestrator/providers/provider-git-manager.ts (1)
99-100: Use error-level logging for failure paths.Line 99, Line 168, and Line 275 are failure branches but currently use
OrchestratorLogger.log(...). Route these throughOrchestratorLogger.error(...)for clearer diagnostics.Proposed fix
- OrchestratorLogger.log(`Error: ${errorMessage}`); + OrchestratorLogger.error(`Error: ${errorMessage}`); ... - OrchestratorLogger.log(`Error: ${errorMessage}`); + OrchestratorLogger.error(`Error: ${errorMessage}`); ... - OrchestratorLogger.log(`Error during cleanup: ${error.message}`); + OrchestratorLogger.error(`Error during cleanup: ${error.message}`);Also applies to: 168-169, 275-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/provider-git-manager.ts` around lines 99 - 100, Replace the use of OrchestratorLogger.log(...) with OrchestratorLogger.error(...) in the failure branches inside provider-git-manager.ts where errors are currently being written (specifically the calls that log the error messages like "Error: ${errorMessage}" and similar failure-path logs). Locate the occurrences by searching for OrchestratorLogger.log used inside error/catch branches in this file and change them to OrchestratorLogger.error to ensure failures are logged at error level; keep the existing message text and interpolation intact. Ensure the three reported occurrences are updated consistently.src/model/orchestrator/providers/aws/aws-base-stack.ts (1)
25-31: Consider temporary fallback for legacy stack wait env var during migration.Without a fallback, old configurations silently revert to default
600sifCLOUD_RUNNER_AWS_STACK_WAIT_TIMEis still set, which can change runtime behavior unexpectedly.Suggested migration-safe diff
function getStackWaitTime(): number { - const overrideValue = Number(process.env.ORCHESTRATOR_AWS_STACK_WAIT_TIME ?? ''); + const overrideValue = Number( + process.env.ORCHESTRATOR_AWS_STACK_WAIT_TIME ?? process.env.CLOUD_RUNNER_AWS_STACK_WAIT_TIME ?? '', + ); if (!Number.isNaN(overrideValue) && overrideValue > 0) { return overrideValue; } return DEFAULT_STACK_WAIT_TIME_SECONDS; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/aws/aws-base-stack.ts` around lines 25 - 31, The getStackWaitTime() function currently only reads ORCHESTRATOR_AWS_STACK_WAIT_TIME and falls back to DEFAULT_STACK_WAIT_TIME_SECONDS, which ignores the legacy CLOUD_RUNNER_AWS_STACK_WAIT_TIME; change getStackWaitTime() to first attempt parsing ORCHESTRATOR_AWS_STACK_WAIT_TIME, and if that is missing/invalid, attempt parsing CLOUD_RUNNER_AWS_STACK_WAIT_TIME (using Number and Number.isNaN checks and requiring >0) before finally returning DEFAULT_STACK_WAIT_TIME_SECONDS; reference DEFAULT_STACK_WAIT_TIME_SECONDS, getStackWaitTime, and process.env for locating and updating the logic.src/model/orchestrator/remote-client/remote-client-logger.ts (1)
48-50: Flatten redundant k8s condition inhandleLogManagementPostJob.After the early return at Line 48, the nested
if (OrchestratorOptions.providerStrategy === 'k8s')is always true and can be simplified for readability.Also applies to: 60-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/remote-client/remote-client-logger.ts` around lines 48 - 50, The nested checks for OrchestratorOptions.providerStrategy === 'k8s' inside handleLogManagementPostJob are redundant because there is already an early return when the strategy is not 'k8s'; remove the inner if wrappers and dedent their bodies so the code executes directly after the early return, and apply the same flattening to the later repeated block (the second k8s check around the subsequent 60-71 region) to improve readability while keeping the initial guard and behavior unchanged.src/model/orchestrator/options/orchestrator-options-reader.ts (1)
5-6: Filter framework keys and dedupe property names.Current output can include non-option keys and duplicates. Normalizing once here makes downstream consumers safer and simpler.
Proposed refactor
class OrchestratorOptionsReader { static GetProperties() { - return [...Object.getOwnPropertyNames(Input), ...Object.getOwnPropertyNames(OrchestratorOptions)]; + const ignore = new Set(['length', 'name', 'prototype']); + return [...new Set([...Object.getOwnPropertyNames(Input), ...Object.getOwnPropertyNames(OrchestratorOptions)])] + .filter((property) => !ignore.has(property)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-options-reader.ts` around lines 5 - 6, GetProperties currently returns raw property names from Input and OrchestratorOptions which can include framework keys and duplicates; update static GetProperties() to combine Object.getOwnPropertyNames(Input) and Object.getOwnPropertyNames(OrchestratorOptions), filter out non-option/framework keys (e.g., 'prototype','constructor','length','name' and any keys starting with '_' or whose value is a function), dedupe the resulting names using a Set, and return the normalized array so downstream consumers get only unique option keys; reference the GetProperties method and the Input and OrchestratorOptions symbols when making this change.src/model/input-readers/git-repo.test.ts (2)
13-14: Unnecessaryawaitbefore synchronousjest.spyOncalls.
jest.spyOn()is synchronous and returns a mock object immediately. Theawaitis not needed here and could be misleading.♻️ Proposed fix
- await jest.spyOn(OrchestratorSystem, 'Run').mockReturnValue(Promise.resolve(mockValue)); - await jest.spyOn(OrchestratorOptions, 'providerStrategy', 'get').mockReturnValue('not-local'); + jest.spyOn(OrchestratorSystem, 'Run').mockReturnValue(Promise.resolve(mockValue)); + jest.spyOn(OrchestratorOptions, 'providerStrategy', 'get').mockReturnValue('not-local');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/input-readers/git-repo.test.ts` around lines 13 - 14, The test contains unnecessary awaits before synchronous jest.spyOn calls; remove the leading await from the two calls that spy on OrchestratorSystem.Run and the OrchestratorOptions providerStrategy getter so they are executed synchronously (i.e., change the statements that reference OrchestratorSystem.Run and OrchestratorOptions.providerStrategy to call jest.spyOn(...) without awaiting the result).
20-21: Same issue: unnecessaryawaitbefore synchronousjest.spyOncalls.♻️ Proposed fix
- await jest.spyOn(OrchestratorSystem, 'Run').mockReturnValue(Promise.resolve(mockValue)); - await jest.spyOn(OrchestratorOptions, 'providerStrategy', 'get').mockReturnValue('not-local'); + jest.spyOn(OrchestratorSystem, 'Run').mockReturnValue(Promise.resolve(mockValue)); + jest.spyOn(OrchestratorOptions, 'providerStrategy', 'get').mockReturnValue('not-local');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/input-readers/git-repo.test.ts` around lines 20 - 21, The two test lines use unnecessary await on synchronous jest.spyOn calls: remove the leading await from the jest.spyOn calls for OrchestratorSystem.Run and the OrchestratorOptions providerStrategy getter so they return spies immediately (e.g., jest.spyOn(OrchestratorSystem, 'Run').mockReturnValue(Promise.resolve(mockValue)) and jest.spyOn(OrchestratorOptions, 'providerStrategy', 'get').mockReturnValue('not-local')); keep awaits only on actual async operations in the test and, if needed, capture the returned spy into a variable for later assertions.src/model/orchestrator/tests/orchestrator-rclone-steps.test.ts (1)
55-82: Test timeout value seems excessive.The timeout of
1_000_000_000milliseconds (~11.5 days) appears to be a placeholder or copy-paste artifact. While this won't cause issues, it's unusual and could mask actual timeout problems in CI.Consider using a more reasonable timeout like
300_000(5 minutes) or600_000(10 minutes) depending on expected test duration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/tests/orchestrator-rclone-steps.test.ts` around lines 55 - 82, The test named 'Run build and prebuilt rclone cache pull, cache push and upload build' uses an excessively large timeout literal (1_000_000_000); replace that placeholder with a reasonable timeout (e.g., 300_000 or 600_000 milliseconds) to avoid masking CI timeouts. Edit the it(...) test block in orchestrator-rclone-steps.test.ts and change the trailing timeout argument from 1_000_000_000 to the chosen value (e.g., 600_000) so the test fails timely when it hangs.src/model/orchestrator/workflows/workflow-composition-root.ts (2)
35-37: Remove pointless try-catch that only rethrows.This try-catch block catches errors only to immediately rethrow them unchanged. It adds no value and can be removed to simplify the code.
Proposed fix
export class WorkflowCompositionRoot implements WorkflowInterface { async run(orchestratorStepState: OrchestratorStepParameters) { - try { if ( OrchestratorOptions.asyncOrchestrator && !Orchestrator.isOrchestratorAsyncEnvironment && !Orchestrator.isOrchestratorEnvironment ) { return await AsyncWorkflow.runAsyncWorkflow(orchestratorStepState.environment, orchestratorStepState.secrets); } if (Orchestrator.buildParameters.customJob !== '') { return await CustomWorkflow.runContainerJobFromString( Orchestrator.buildParameters.customJob, orchestratorStepState.environment, orchestratorStepState.secrets, ); } return await new BuildAutomationWorkflow().run( new OrchestratorStepParameters( orchestratorStepState.image.toString(), orchestratorStepState.environment, orchestratorStepState.secrets, ), ); - } catch (error) { - throw error; - } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/workflows/workflow-composition-root.ts` around lines 35 - 37, Remove the redundant try-catch that only rethrows the caught error in workflow-composition-root; delete the surrounding "try { ... } catch (error) { throw error; }" block and leave the original inner code path as-is so errors naturally propagate, ensuring any references to the caught variable (error) are removed and behavior remains unchanged (no additional logging or handling needed).
28-34: Redundant reconstruction ofOrchestratorStepParameters.
orchestratorStepStateis already anOrchestratorStepParametersinstance. Creating a new one with the same properties appears unnecessary unless.imagemight be a non-string type requiring.toString()coercion.If
.imageis already a string, consider passingorchestratorStepStatedirectly:Proposed simplification (if image is already a string)
- return await new BuildAutomationWorkflow().run( - new OrchestratorStepParameters( - orchestratorStepState.image.toString(), - orchestratorStepState.environment, - orchestratorStepState.secrets, - ), - ); + return await new BuildAutomationWorkflow().run(orchestratorStepState);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/workflows/workflow-composition-root.ts` around lines 28 - 34, The code redundantly constructs a new OrchestratorStepParameters when orchestratorStepState is already that type; update the call in BuildAutomationWorkflow().run to pass orchestratorStepState directly (instead of new OrchestratorStepParameters(...)) if orchestratorStepState.image is already a string; if image can be a non-string type, either coerce it once when orchestratorStepState is created or add a small helper that returns a normalized OrchestratorStepParameters (ensuring .image is string) and use that helper here—locate the usage around BuildAutomationWorkflow.run and the orchestratorStepState variable and remove the unnecessary reconstruction or centralize the .toString() coercion.src/model/index.ts (1)
27-27: Optional: Remove redundant alias.
Orchestrator as Orchestratoris redundant. Consider simplifying to justOrchestrator.Proposed fix
- Orchestrator as Orchestrator, + Orchestrator,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/index.ts` at line 27, Remove the redundant self-alias "Orchestrator as Orchestrator" and replace it with the plain symbol "Orchestrator" in the export/import list (i.e., change occurrences of Orchestrator as Orchestrator to simply Orchestrator) so the module exports/imports the symbol without an unnecessary alias; update any surrounding comma/spacing accordingly.src/model/orchestrator/providers/k8s/kubernetes-secret.ts (1)
35-35: Hardcoded 15-second delay without explanation.This
setTimeoutintroduces a 15-second delay before creating the secret. If this is a workaround for a timing issue (e.g., waiting for namespace readiness), it should be documented or replaced with proper polling/retry logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/k8s/kubernetes-secret.ts` at line 35, The hardcoded 15-second sleep in the secret creation flow must be replaced with deterministic waiting: remove the await new Promise(...) and implement a short polling/retry loop that checks the cluster/resource readiness (e.g., query the namespace/status or attempt a lightweight GET of the secret endpoint) inside the function that creates the secret (look for createSecret/createKubernetesSecret or whichever method in kubernetes-secret.ts performs the secret creation), using exponential backoff and a configurable timeout/attempts; if readiness isn't achieved within timeout, surface a clear error. If this wait was intended as a documented workaround, instead add a comment above the function describing why it's necessary and make the delay configurable via environment/config variables rather than hardcoding 15000ms.src/model/orchestrator/providers/aws/aws-job-stack.ts (1)
24-24: Consider a temporary fallback for the legacy stack wait env var.Using only the new key can cause silent config regressions in existing pipelines during migration. A short fallback/deprecation window would make rollout safer.
🔁 Suggested compatibility bridge
function getStackWaitTime(): number { - const overrideValue = Number(process.env.ORCHESTRATOR_AWS_STACK_WAIT_TIME ?? ''); + const rawOverride = + process.env.ORCHESTRATOR_AWS_STACK_WAIT_TIME ?? process.env.CLOUD_RUNNER_AWS_STACK_WAIT_TIME ?? ''; + const overrideValue = Number(rawOverride); + if (process.env.CLOUD_RUNNER_AWS_STACK_WAIT_TIME && !process.env.ORCHESTRATOR_AWS_STACK_WAIT_TIME) { + OrchestratorLogger.logWarning( + 'CLOUD_RUNNER_AWS_STACK_WAIT_TIME is deprecated; use ORCHESTRATOR_AWS_STACK_WAIT_TIME.', + ); + } if (!Number.isNaN(overrideValue) && overrideValue > 0) { return overrideValue; } return DEFAULT_STACK_WAIT_TIME_SECONDS; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/aws/aws-job-stack.ts` at line 24, Replace the direct parse of process.env.ORCHESTRATOR_AWS_STACK_WAIT_TIME with a compatibility lookup that checks the new key first and falls back to the legacy env var (e.g. process.env.ORCHESTRATOR_STACK_WAIT_TIME) when the new key is empty/undefined; parse the selected string with Number safely and provide a sensible default if parsing fails, and emit a deprecation warning (console.warn or existing logger) when the legacy key is used; update the variable overrideValue (and any callers expecting its type) accordingly.src/model/orchestrator/tests/e2e/orchestrator-end2end-caching.test.ts (1)
12-18: DuplicateCreateParametersfunction.Same as in the Kubernetes test file - this duplicates the helper from
../create-test-parameter.ts. Consider importing the shared version to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/tests/e2e/orchestrator-end2end-caching.test.ts` around lines 12 - 18, The CreateParameters helper function is duplicated here; instead of redefining it, import and use the shared helper from the existing module (../create-test-parameter.ts). Replace the local CreateParameters implementation with an import of the shared function and update usages to call that imported function (references: CreateParameters, BuildParameters.create, and Cli.options) so there is a single source of truth and no duplicate helper code.src/model/orchestrator/options/orchestrator-options.ts (1)
60-64: Redundant|| falsein boolean expression.The expression
value === 'true' || falseis redundant since the strict equality check already returns a boolean.static get githubChecks(): boolean { const value = OrchestratorOptions.getInput('githubChecks'); - - return value === `true` || false; + return value === 'true'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-options.ts` around lines 60 - 64, The githubChecks getter in OrchestratorOptions uses a redundant `|| false`; simplify the boolean return by evaluating the string directly: call OrchestratorOptions.getInput('githubChecks'), compare to `'true'` and return that result (remove the `|| false`) so githubChecks returns the boolean from the strict equality; update the static get githubChecks() implementation accordingly, keeping the input retrieval via OrchestratorOptions.getInput.src/model/orchestrator/tests/e2e/orchestrator-kubernetes.test.ts (1)
11-17: Consider using the sharedCreateParametershelper.This function duplicates
CreateParametersfrom../create-test-parameter.ts. To avoid code duplication and maintain consistency, consider importing the shared helper:-async function CreateParameters(overrides: any) { - if (overrides) { - Cli.options = overrides; - } - - return await BuildParameters.create(); -} +import { CreateParameters } from '../create-test-parameter';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/tests/e2e/orchestrator-kubernetes.test.ts` around lines 11 - 17, The test defines a local CreateParameters function that duplicates the shared helper from ../create-test-parameter.ts; remove this local CreateParameters and import the shared helper instead (e.g., import { CreateParameters } from '../create-test-parameter' or adjust to the actual named/default export), then update callers in orchestrator-kubernetes.test.ts to use the imported CreateParameters and ensure any required types/overrides handling matches the shared helper's signature.
ℹ️ Review info
Configuration used: Repository 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 (116)
$LOG_FILE.github/workflows/integrity-check.yml.github/workflows/orchestrator-async-checks.yml.github/workflows/orchestrator-integrity.ymlaction.ymlpackage.jsonsrc/index.tssrc/integration/orchestrator-github-checks-integration-test.tssrc/model/build-parameters.tssrc/model/cli/cli.tssrc/model/cloud-runner/error/cloud-runner-error.tssrc/model/cloud-runner/options/cloud-runner-constants.tssrc/model/cloud-runner/options/cloud-runner-environment-variable.tssrc/model/cloud-runner/options/cloud-runner-folders.tssrc/model/cloud-runner/options/cloud-runner-options-reader.tssrc/model/cloud-runner/options/cloud-runner-options.tssrc/model/cloud-runner/options/cloud-runner-query-override.tssrc/model/cloud-runner/options/cloud-runner-statics.tssrc/model/cloud-runner/options/cloud-runner-step-parameters.tssrc/model/cloud-runner/workflows/workflow-composition-root.tssrc/model/cloud-runner/workflows/workflow-interface.tssrc/model/github.tssrc/model/image-environment-factory.tssrc/model/index.tssrc/model/input-readers/generic-input-reader.tssrc/model/input-readers/git-repo.test.tssrc/model/input-readers/git-repo.tssrc/model/input-readers/github-cli.tssrc/model/input-readers/test-license-reader.tssrc/model/input.tssrc/model/orchestrator/error/orchestrator-error.tssrc/model/orchestrator/options/orchestrator-constants.tssrc/model/orchestrator/options/orchestrator-environment-variable.tssrc/model/orchestrator/options/orchestrator-folders.tssrc/model/orchestrator/options/orchestrator-guid.tssrc/model/orchestrator/options/orchestrator-options-reader.tssrc/model/orchestrator/options/orchestrator-options.tssrc/model/orchestrator/options/orchestrator-query-override.tssrc/model/orchestrator/options/orchestrator-secret.tssrc/model/orchestrator/options/orchestrator-statics.tssrc/model/orchestrator/options/orchestrator-step-parameters.tssrc/model/orchestrator/orchestrator.tssrc/model/orchestrator/providers/README.mdsrc/model/orchestrator/providers/aws/aws-base-stack.tssrc/model/orchestrator/providers/aws/aws-client-factory.tssrc/model/orchestrator/providers/aws/aws-cloud-formation-templates.tssrc/model/orchestrator/providers/aws/aws-error.tssrc/model/orchestrator/providers/aws/aws-job-stack.tssrc/model/orchestrator/providers/aws/aws-task-runner.tssrc/model/orchestrator/providers/aws/cloud-formations/base-stack-formation.tssrc/model/orchestrator/providers/aws/cloud-formations/cleanup-cron-formation.tssrc/model/orchestrator/providers/aws/cloud-formations/task-definition-formation.tssrc/model/orchestrator/providers/aws/index.tssrc/model/orchestrator/providers/aws/orchestrator-aws-task-def.tssrc/model/orchestrator/providers/aws/services/garbage-collection-service.tssrc/model/orchestrator/providers/aws/services/task-service.tssrc/model/orchestrator/providers/docker/index.tssrc/model/orchestrator/providers/k8s/index.tssrc/model/orchestrator/providers/k8s/kubernetes-job-spec-factory.tssrc/model/orchestrator/providers/k8s/kubernetes-pods.tssrc/model/orchestrator/providers/k8s/kubernetes-role.tssrc/model/orchestrator/providers/k8s/kubernetes-secret.tssrc/model/orchestrator/providers/k8s/kubernetes-service-account.tssrc/model/orchestrator/providers/k8s/kubernetes-storage.tssrc/model/orchestrator/providers/k8s/kubernetes-task-runner.tssrc/model/orchestrator/providers/local/index.tssrc/model/orchestrator/providers/provider-git-manager.tssrc/model/orchestrator/providers/provider-interface.tssrc/model/orchestrator/providers/provider-loader.tssrc/model/orchestrator/providers/provider-resource.tssrc/model/orchestrator/providers/provider-url-parser.tssrc/model/orchestrator/providers/provider-workflow.tssrc/model/orchestrator/providers/test/index.tssrc/model/orchestrator/remote-client/caching.tssrc/model/orchestrator/remote-client/index.tssrc/model/orchestrator/remote-client/remote-client-logger.tssrc/model/orchestrator/services/core/follow-log-stream-service.tssrc/model/orchestrator/services/core/orchestrator-logger.tssrc/model/orchestrator/services/core/orchestrator-result.tssrc/model/orchestrator/services/core/orchestrator-system.tssrc/model/orchestrator/services/core/resource-tracking.tssrc/model/orchestrator/services/core/shared-workspace-locking.tssrc/model/orchestrator/services/core/task-parameter-serializer.tssrc/model/orchestrator/services/hooks/command-hook-service.tssrc/model/orchestrator/services/hooks/command-hook.tssrc/model/orchestrator/services/hooks/container-hook-service.tssrc/model/orchestrator/services/hooks/container-hook.tssrc/model/orchestrator/services/utility/lfs-hashing.tssrc/model/orchestrator/tests/create-test-parameter.tssrc/model/orchestrator/tests/e2e/orchestrator-end2end-caching.test.tssrc/model/orchestrator/tests/e2e/orchestrator-end2end-locking.test.tssrc/model/orchestrator/tests/e2e/orchestrator-end2end-retaining.test.tssrc/model/orchestrator/tests/e2e/orchestrator-kubernetes.test.tssrc/model/orchestrator/tests/fixtures/invalid-provider.tssrc/model/orchestrator/tests/orchestrator-async-workflow.test.tssrc/model/orchestrator/tests/orchestrator-caching.test.tssrc/model/orchestrator/tests/orchestrator-environment.test.tssrc/model/orchestrator/tests/orchestrator-github-checks.test.tssrc/model/orchestrator/tests/orchestrator-hooks.test.tssrc/model/orchestrator/tests/orchestrator-image.test.tssrc/model/orchestrator/tests/orchestrator-local-persistence.test.tssrc/model/orchestrator/tests/orchestrator-locking-core.test.tssrc/model/orchestrator/tests/orchestrator-locking-get-locked.test.tssrc/model/orchestrator/tests/orchestrator-rclone-steps.test.tssrc/model/orchestrator/tests/orchestrator-s3-steps.test.tssrc/model/orchestrator/tests/orchestrator-suite.test.tssrc/model/orchestrator/tests/providers/provider-git-manager.test.tssrc/model/orchestrator/tests/providers/provider-loader.test.tssrc/model/orchestrator/tests/providers/provider-url-parser.test.tssrc/model/orchestrator/workflows/async-workflow.tssrc/model/orchestrator/workflows/build-automation-workflow.tssrc/model/orchestrator/workflows/custom-workflow.tssrc/model/orchestrator/workflows/workflow-composition-root.tssrc/model/orchestrator/workflows/workflow-interface.tssrc/test-utils/orchestrator-test-helpers.tstemp/job-log.txt
💤 Files with no reviewable changes (11)
- src/model/cloud-runner/options/cloud-runner-step-parameters.ts
- src/model/cloud-runner/options/cloud-runner-statics.ts
- src/model/cloud-runner/workflows/workflow-interface.ts
- src/model/cloud-runner/options/cloud-runner-options-reader.ts
- src/model/cloud-runner/options/cloud-runner-query-override.ts
- src/model/cloud-runner/options/cloud-runner-folders.ts
- src/model/cloud-runner/options/cloud-runner-options.ts
- src/model/cloud-runner/options/cloud-runner-constants.ts
- src/model/cloud-runner/options/cloud-runner-environment-variable.ts
- src/model/cloud-runner/error/cloud-runner-error.ts
- src/model/cloud-runner/workflows/workflow-composition-root.ts
| CHECKS_UPDATE: ${{ github.event.inputs.checksObject }} | ||
| run: | | ||
| git clone -b cloud-runner-develop https://github.com/game-ci/unity-builder | ||
| git clone -b orchestrator-develop https://github.com/game-ci/unity-builder |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check orchestrator-develop branch in game-ci/unity-builder via GitHub API
gh api repos/game-ci/unity-builder/branches --jq '.[].name' 2>/dev/null | grep -i 'orchestrator' | head -20Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
#!/bin/bash
# List all branches in game-ci/unity-builder
echo "=== All branches in game-ci/unity-builder ==="
gh api repos/game-ci/unity-builder/branches --paginate --jq '.[].name' 2>/dev/null | sortRepository: game-ci/unity-builder
Length of output: 223
🌐 Web query:
game-ci/unity-builder orchestrator-develop branch
💡 Result:
In game-ci/unity-builder, there is no branch named orchestrator-develop visible in the repository’s branch list (as of March 3, 2026). The only “orchestrator”-related active branch shown is rename-orchestrator; the default branch is main. [1]
If you meant to use that work-in-progress branch, try:
git fetch origin
git checkout -b rename-orchestrator origin/rename-orchestrator[1]
Sources
[1] game-ci/unity-builder — Branches page (shows main and active branch rename-orchestrator, no orchestrator-develop). (github.com)
Citations:
Fix branch reference: orchestrator-develop branch does not exist in game-ci/unity-builder.
The workflow will fail at line 57 when attempting to clone a non-existent branch. The orchestrator-develop branch is not present in the repository. Available orchestrator-related branch is rename-orchestrator; the default branch is main. Correct the branch reference before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/orchestrator-async-checks.yml at line 57, The workflow's
git clone command is pointing to a non-existent branch name
'orchestrator-develop'; update the branch argument in the git clone invocation
(the line containing "git clone -b orchestrator-develop
https://github.com/game-ci/unity-builder") to use an existing branch such as
'rename-orchestrator' or 'main' so the checkout succeeds during the workflow
run.
$LOG_FILE
Outdated
| cloud runner build workflow starting | ||
| cloud runner build workflow starting | ||
| cloud runner build workflow starting | ||
| cloud runner build workflow starting |
There was a problem hiding this comment.
Replace stale “cloud runner” wording with “orchestrator” in this log artifact.
Line 1–Line 4 still use old terminology, which conflicts with the rename objective and can fail grep-based integrity checks. If the 4 entries are intentional retry markers, keep the count but rename the message text.
Proposed fix
-cloud runner build workflow starting
-cloud runner build workflow starting
-cloud runner build workflow starting
-cloud runner build workflow starting
+orchestrator build workflow starting
+orchestrator build workflow starting
+orchestrator build workflow starting
+orchestrator build workflow startingBased on learnings: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
📝 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.
| cloud runner build workflow starting | |
| cloud runner build workflow starting | |
| cloud runner build workflow starting | |
| cloud runner build workflow starting | |
| orchestrator build workflow starting | |
| orchestrator build workflow starting | |
| orchestrator build workflow starting | |
| orchestrator build workflow starting |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @$LOG_FILE around lines 1 - 4, Update the log artifact entries that currently
contain the string "cloud runner build workflow starting" to use the new
terminology "orchestrator build workflow starting" while preserving the existing
four duplicate entries (these are intentional retry markers); search for the
exact literal "cloud runner build workflow starting" in the log
generation/location (the artifact referenced by $LOG_FILE) and replace the
message text only, keeping the same count and placement so retry semantics
remain unchanged.
| orchestratorRepoName: | ||
| default: 'game-ci/unity-builder' | ||
| required: false | ||
| description: | ||
| '[CloudRunner] Specifies the repo for the unity builder. Useful if you forked the repo for testing, features, or | ||
| '[Orchestrator] Specifies the repo for the unity builder. Useful if you forked the repo for testing, features, or | ||
| fixes.' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and examine the action.yml file
echo "=== Checking action.yml for input definitions ==="
fd -t f "action.yml" | head -5
# Search for getInput calls in TypeScript files as suggested
echo -e "\n=== Searching for getInput calls with input names ==="
rg -nP --type=ts -C2 "getInput\(['\"](cloudRunnerRepoName|orchestratorRepoName)['\"]" 2>/dev/null || echo "No direct matches found"
# Also search more broadly for these input names in TS files
echo -e "\n=== Searching for both input names in TypeScript files ==="
rg -n "(cloudRunnerRepoName|orchestratorRepoName)" --type=ts 2>/dev/null || echo "No matches found"Repository: game-ci/unity-builder
Length of output: 1454
🏁 Script executed:
#!/bin/bash
# Examine the action.yml file to see the current input definitions
echo "=== Examining action.yml ==="
cat action.ymlRepository: game-ci/unity-builder
Length of output: 11400
🏁 Script executed:
#!/bin/bash
# Search for TypeScript files that might handle input parsing
echo "=== Finding TypeScript/JavaScript files that might process inputs ==="
fd -t f \( -name "*.ts" -o -name "*.js" \) | head -20
# Check for any fallback or input handling logic
echo -e "\n=== Searching for fallback patterns ==="
rg -n "cloudRunnerRepoName" --type=ts --type=js 2>/dev/null || echo "No results"Repository: game-ci/unity-builder
Length of output: 434
🏁 Script executed:
#!/bin/bash
# Find and examine the getInput method implementation
echo "=== Searching for getInput method definition ==="
rg -n "getInput" --type=ts -B2 -A5 "src/model/orchestrator/options/orchestrator-options.ts" | head -50
# Look for the parent class or method definition
echo -e "\n=== Looking for OrchestratorOptions class definition and getInput ==="
fd -t f -name "orchestrator-options.ts" -exec cat -n {} \;Repository: game-ci/unity-builder
Length of output: 2060
🏁 Script executed:
#!/bin/bash
# Search for any fallback or conditional logic handling cloudRunnerRepoName anywhere
echo "=== Comprehensive search for cloudRunnerRepoName in entire codebase ==="
rg "cloudRunnerRepoName" --type=ts --type=js --type=yaml 2>/dev/null || echo "No occurrences found"
# Also check git history for this input name (case-insensitive)
echo -e "\n=== Check if cloudRunnerRepoName exists in recent git history ==="
git log --all --oneline -- action.yml | head -10Repository: game-ci/unity-builder
Length of output: 904
Add a compatibility path for the renamed input to avoid workflow breakage.
Renaming the input from cloudRunnerRepoName to orchestratorRepoName is a breaking change. The old input was completely removed during the "Cloud Runner" to "Orchestrator" rename (commit c219e2c), and workflows still passing cloudRunnerRepoName will silently fall back to the default repo since no fallback logic exists in the code.
🔧 Suggested manifest compatibility patch
+ cloudRunnerRepoName:
+ default: ''
+ required: false
+ description: '[Deprecated] Use orchestratorRepoName instead.'
orchestratorRepoName:
default: 'game-ci/unity-builder'
required: false
description:
'[Orchestrator] Specifies the repo for the unity builder. Useful if you forked the repo for testing, features, or
fixes.'Also ensure runtime logic falls back from the deprecated input when orchestratorRepoName is unset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@action.yml` around lines 276 - 281, Add backwards compatibility for the
renamed input by accepting the deprecated input name and using it when the new
one is not provided: update the action metadata to keep a cloudRunnerRepoName
input (marked deprecated in its description) or add a note that
orchestratorRepoName should fallback to cloudRunnerRepoName, and update the
runtime input retrieval (where you call core.getInput('orchestratorRepoName') or
similar) to fall back to core.getInput('cloudRunnerRepoName') when the new input
is empty; ensure any variable/constant that holds the repo name (e.g.,
orchestratorRepoName) is set to the deprecated value if present so existing
workflows keep working.
| public static async handleException(error: unknown, buildParameters: BuildParameters, secrets: OrchestratorSecret[]) { | ||
| OrchestratorLogger.error(JSON.stringify(error, undefined, 4)); | ||
| core.setFailed('Orchestrator failed'); | ||
| if (Orchestrator.Provider !== undefined) { | ||
| await Orchestrator.Provider.cleanupWorkflow(buildParameters, buildParameters.branch, secrets); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "orchestrator-error.ts" | head -20Repository: game-ci/unity-builder
Length of output: 117
🏁 Script executed:
cat -n src/model/orchestrator/error/orchestrator-error.tsRepository: game-ci/unity-builder
Length of output: 867
🏁 Script executed:
# Check Node.js version requirements
rg "node.*version|engines.*node" package.json -A 2 -B 2Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
# Search for other JSON.stringify usage on errors or in error handling
rg "JSON\.stringify.*error" --type ts --type js -C 2Repository: game-ci/unity-builder
Length of output: 48228
🏁 Script executed:
# Check if util module is used elsewhere in the codebase
rg "from.*node:util|require.*util" --type ts --type jsRepository: game-ci/unity-builder
Length of output: 1926
🏁 Script executed:
# Quick verification: JSON.stringify behavior on Error objects
node -e "const e = new Error('test'); console.log('JSON.stringify(Error):', JSON.stringify(e)); console.log('Expected message and stack:', e.message, '|||', e.stack.split('\n')[0]);"Repository: game-ci/unity-builder
Length of output: 139
🏁 Script executed:
# Check if the cleanup method is documented or if there's a pattern
rg "cleanupWorkflow" --type ts -B 3 -A 3 | head -40Repository: game-ci/unity-builder
Length of output: 2889
Do not use JSON.stringify(error) for logging; it returns empty objects for Error instances.
Line 9 loses critical error details—JSON.stringify(Error) returns {} instead of message and stack. Additionally, the async cleanup call (lines 11–13) has no error handling and can fail silently.
🛠️ Proposed fix
+import util from 'node:util';
+
export class OrchestratorError {
public static async handleException(error: unknown, buildParameters: BuildParameters, secrets: OrchestratorSecret[]) {
- OrchestratorLogger.error(JSON.stringify(error, undefined, 4));
- core.setFailed('Orchestrator failed');
+ const errorMessage =
+ error instanceof Error
+ ? `${error.message}\n${error.stack ?? ''}`
+ : util.inspect(error, { depth: 5, breakLength: 120 });
+ OrchestratorLogger.error(errorMessage);
+ core.setFailed(`Orchestrator failed: ${errorMessage}`);
if (Orchestrator.Provider !== undefined) {
- await Orchestrator.Provider.cleanupWorkflow(buildParameters, buildParameters.branch, secrets);
+ try {
+ await Orchestrator.Provider.cleanupWorkflow(buildParameters, buildParameters.branch, secrets);
+ } catch (cleanupError) {
+ OrchestratorLogger.error(
+ `Cleanup failed: ${
+ cleanupError instanceof Error ? cleanupError.message : String(cleanupError)
+ }`,
+ );
+ }
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/error/orchestrator-error.ts` around lines 8 - 13,
handleException currently logs the incoming error with JSON.stringify (which
drops Error.message/stack) and calls Orchestrator.Provider.cleanupWorkflow
without error handling; replace the JSON.stringify call on error in
handleException with explicit extraction of error details (e.g., message and
stack or util.format) and pass those into OrchestratorLogger.error and
core.setFailed, and wrap the await
Orchestrator.Provider.cleanupWorkflow(buildParameters, buildParameters.branch,
secrets) in a try/catch that logs any cleanup failures via
OrchestratorLogger.error (including the caught error's message/stack) so cleanup
failures don't fail silently.
| public static get unityBuilderRepoUrl(): string { | ||
| return `https://${Orchestrator.buildParameters.gitPrivateToken}@github.com/${Orchestrator.buildParameters.orchestratorRepoName}.git`; | ||
| } | ||
|
|
||
| public static get targetBuildRepoUrl(): string { | ||
| return `https://${Orchestrator.buildParameters.gitPrivateToken}@github.com/${Orchestrator.buildParameters.githubRepo}.git`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if unityBuilderRepoUrl or targetBuildRepoUrl are logged anywhere
rg -n "(unityBuilderRepoUrl|targetBuildRepoUrl)" --type ts -B2 -A2Repository: game-ci/unity-builder
Length of output: 3222
🏁 Script executed:
#!/bin/bash
# Search for logging statements that might use these URLs or their values
rg -n "(console\.(log|info|debug|warn|error)|logger\.|log\()" --type ts -A2 -B2 | head -100Repository: game-ci/unity-builder
Length of output: 8453
🏁 Script executed:
#!/bin/bash
# Check for any usage of gitPrivateToken in logging contexts
rg -n "gitPrivateToken" --type ts -B3 -A3Repository: game-ci/unity-builder
Length of output: 7314
🏁 Script executed:
#!/bin/bash
# Find OrchestratorSystem.Run implementation
rg -n "class OrchestratorSystem|Run\(" src/model/orchestrator/system --type ts -A5Repository: game-ci/unity-builder
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check orchestrator.ts context around line 280 to understand the masking
sed -n '275,290p' src/model/orchestrator/orchestrator.tsRepository: game-ci/unity-builder
Length of output: 644
🏁 Script executed:
#!/bin/bash
# Check if the workflow shell scripts are logged or output anywhere
rg -n "async-workflow\|build-automation-workflow" --type ts -B2 -A2 | grep -i "log\|console\|output\|run\|exec" -A2 -B2 | head -80Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for OrchestratorSystem.Run method more broadly
rg -n "class OrchestratorSystem|Run\(" --type ts | grep -i "system\|run" | head -20Repository: game-ci/unity-builder
Length of output: 2602
🏁 Script executed:
#!/bin/bash
# Find the OrchestratorSystem file
fd -t f "system" src/Repository: game-ci/unity-builder
Length of output: 206
🏁 Script executed:
#!/bin/bash
# Search for Run method in orchestrator folder
rg -n "async Run\(|Run\(" src/model/orchestrator --type ts -A5 | head -100Repository: game-ci/unity-builder
Length of output: 10519
🏁 Script executed:
#!/bin/bash
# Read the full OrchestratorSystem.Run implementation
cat -n src/model/orchestrator/services/core/orchestrator-system.tsRepository: game-ci/unity-builder
Length of output: 2569
🏁 Script executed:
#!/bin/bash
# Check the exact OrchestratorSystem.Run call in remote-client/index.ts around line 311
sed -n '305,320p' src/model/orchestrator/remote-client/index.tsRepository: game-ci/unity-builder
Length of output: 899
Credential leakage through command logging - escalate to major issue
The targetBuildRepoUrl getter (and similarly unityBuilderRepoUrl) embeds gitPrivateToken directly in the URL. At src/model/orchestrator/remote-client/index.ts:311-315, this URL is passed directly to OrchestratorSystem.Run() without suppressing logs. Since suppressLogs defaults to false, the full command including the embedded token is logged via RemoteClientLogger.log() at execution time and again with output, leaking credentials to logs.
Use OrchestratorSystem.Run(..., false, true) to suppress logging, or refactor to use environment variables for authentication instead of embedding tokens in URLs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/options/orchestrator-folders.ts` around lines 75 - 81,
The getters unityBuilderRepoUrl and targetBuildRepoUrl currently embed
Orchestrator.buildParameters.gitPrivateToken in the URL which causes credential
leakage when passed to OrchestratorSystem.Run (observed in
remote-client/index.ts where RemoteClientLogger.log prints the command); update
the call to OrchestratorSystem.Run to suppress logging (pass suppressLogs =
true, e.g., OrchestratorSystem.Run(..., false, true) as suggested) OR refactor
authentication to use environment-variable based credentials (remove token from
URL and use GIT_ASKPASS/credential helper or env var for auth) so tokens are
never included in command strings; ensure changes reference the getters
unityBuilderRepoUrl and targetBuildRepoUrl and the caller
OrchestratorSystem.Run/RemoteClientLogger.log.
There was a problem hiding this comment.
yea the private token thing is a bit scary. Have you considered alternative approaches?
There was a problem hiding this comment.
🧩 Analysis chain
@webbertakken, great question! Here are the main alternatives, ranked by security/ergonomics for a containerized CI context like this:
Option 1 — GIT_ASKPASS (recommended)
Use GIT_ASKPASS and SSH_ASKPASS first because they have the benefit of being able to use an API token in an environment config without actually writing any secrets to disk. Wherever git clones an authenticated HTTPS repository and gets a 401, it checks the GIT_ASKPASS environment variable to see if you've specified a program that can present the token.
The clone URL becomes a plain https://github.com/<repo>.git — no token embedded at all:
public static get unityBuilderRepoUrl(): string {
return `https://github.com/${Orchestrator.buildParameters.orchestratorRepoName}.git`;
}
public static get targetBuildRepoUrl(): string {
return `https://github.com/${Orchestrator.buildParameters.githubRepo}.git`;
}Then, before calling OrchestratorSystem.Run(git clone ...), configure GIT_ASKPASS in a pre-step:
// Write a temp script that echoes the token
const askPassScript = `/tmp/.git-askpass-${buildGuid}`;
await OrchestratorSystem.Run(
`printf '#!/bin/sh\nexec echo "%s"\n' "${token}" > ${askPassScript} && chmod +x ${askPassScript}`,
false,
true // suppressLogs — never log the token write
);
// Then clone with the env var set
await OrchestratorSystem.Run(`GIT_ASKPASS=${askPassScript} git clone ${url}`, false, false);
// Cleanup
await OrchestratorSystem.Run(`rm -f ${askPassScript}`, false, true);A GIT_ASKPASS script that echoes the token environment variable, made executable, means any automated tool cloning git repositories won't be obstructed by username or password prompts.
Option 2 — git credential.helper store with a temp file
Write the token to a temp credentials store file, configure git to use it, perform the clone, then delete the file:
echo "https://git:${TOKEN}@github.com" > /tmp/git-credentials.store
git config --global credential.helper 'store --file /tmp/git-credentials.store'
git clone https://github.com/repo.git
rm -f /tmp/git-credentials.storeThis keeps the token out of the clone URL and the command log, though it briefly touches disk.
Option 3 — git -c http.extraHeader (header injection)
Pass the token as an HTTP Authorization header rather than embedding it in the URL:
const authHeader = `Authorization: Bearer ${token}`;
await OrchestratorSystem.Run(
`git -c http.extraHeader="${authHeader}" clone https://github.com/${repo}.git`,
false,
true // suppressLogs — header contains token
);The URL itself is clean, though you still need suppressLogs = true since the header value appears in the command string.
Option 4 — Short-term: suppressLogs = true
The quickest fix — just suppress logging for the affected OrchestratorSystem.Run() calls that use these URLs. The token stays embedded in the URL (a code smell), but at least it won't appear in logs:
await OrchestratorSystem.Run(url, false, true /* suppressLogs */);This doesn't fix the underlying pattern, but stops the immediate leakage.
Summary recommendation: Option 1 (GIT_ASKPASS) is the cleanest — the token never appears in any URL, command string, or log. Option 2 is a solid runner-up. Option 4 is a quick patch but doesn't eliminate the embedded token. Would you like me to open a follow-up issue to track this, or plan the code changes for Option 1?
🌐 Web query:
git clone with token without embedding in URL alternatives GIT_ASKPASS credential helper
💡 Result:
Here are common ways to git clone with an HTTPS token without putting the token in the URL.
1) Use a credential helper (recommended for interactive use)
Configure a credential helper so Git prompts once and stores it (OS keychain / secure store / cache), instead of requiring a token in the URL. Git supports helpers like cache and store, plus third‑party helpers, configured via credential.helper. Git can also key credentials by full path with credential.useHttpPath=true when needed. [1]
For GitHub specifically, GitHub recommends Git Credential Manager (GCM) (or GitHub CLI) to handle auth (often via browser/OAuth) and store credentials securely; alternatively use SSH keys. [2]
2) Use GIT_ASKPASS / core.askPass to supply the token at prompt time (good for CI/scripts)
Git’s credential prompting order includes:
GIT_ASKPASS- then
core.askPass - then terminal prompt, etc. [1]
So you can set GIT_ASKPASS to a script/program that prints the token (and optionally username) when Git asks—keeping the token out of the clone URL. [1]
3) Send the token as an HTTP header (Bearer) instead of a URL credential
You can pass an auth header via config for a single command:
git -c http.extraHeader='Authorization: Bearer <token>' clone https://host/org/repo.git(commonly used/required by some providers) [3][4]
To avoid placing the token directly on the command line, you can also feed it from an environment variable using --config-env:
git clone --config-env=http.extraHeader=GIT_AUTHORIZATION_HEADER https://host/org/repo.git(whereGIT_AUTHORIZATION_HEADER="Authorization: Bearer <token>") [5]
4) Switch to SSH (no token needed for Git HTTPS at all)
If your host supports it, using SSH keys avoids PATs for Git-over-HTTPS entirely (GitHub explicitly calls this out). [2]
Sources: Git credential prompting & helpers (GIT_ASKPASS, credential.helper, etc.) [1]; GitHub guidance on GCM/CLI and SSH [2]; examples of http.extraHeader bearer-token cloning [3][4] and --config-env pattern [5].
@webbertakken, yes — here's a concrete breakdown of the strongest alternatives for this containerized CI context:
Option 1 — GIT_ASKPASS (good for CI/scripts)
Git's credential prompting order includes GIT_ASKPASS, then core.askPass, then terminal prompt. You can set GIT_ASKPASS to a script/program that prints the token when Git asks, keeping the token out of the clone URL entirely.
The unityBuilderRepoUrl/targetBuildRepoUrl getters would become plain https://github.com/<repo>.git, with a short-lived ASKPASS script providing the token at prompt time:
public static get unityBuilderRepoUrl(): string {
return `https://github.com/${Orchestrator.buildParameters.orchestratorRepoName}.git`;
}
public static get targetBuildRepoUrl(): string {
return `https://github.com/${Orchestrator.buildParameters.githubRepo}.git`;
}Option 2 — http.extraHeader + --config-env (elegant for CI)
You can pass an auth header via --config-env to avoid placing the token directly on the command line: git clone --config-env=http.extraHeader=GIT_AUTHORIZATION_HEADER https://host/org/repo.git where GIT_AUTHORIZATION_HEADER="Authorization: Bearer <token>".
This is particularly clean: the URL has no token, the command string has no token, and the secret only lives in an environment variable passed to the subprocess. No suppressLogs workaround needed:
await OrchestratorSystem.Run(
`git clone --config-env=http.extraHeader=GIT_AUTHORIZATION_HEADER ${plainUrl}`,
false,
false,
{ GIT_AUTHORIZATION_HEADER: `Authorization: Bearer ${token}` }
);Option 3 — SSH keys (most robust long-term)
GitHub explicitly recommends SSH keys to avoid PATs for Git-over-HTTPS entirely. If the orchestrator's build containers can be provisioned with a deploy key or SSH agent forwarding, this eliminates the HTTPS token problem entirely at the infrastructure level.
Option 4 — Quick patch: suppressLogs = true
The fastest fix — just suppress logging on the OrchestratorSystem.Run() calls that use these URLs. The token stays embedded (a code smell), but stops the immediate leakage.
Recommendation: Option 2 (--config-env) is arguably the best fit here — no changes to the container setup, no temp files, no token in any command string or URL, and it works in any environment that has access to the gitPrivateToken as an env var. Would you like me to open a follow-up issue to track this, or plan the code changes?
| public static async handleCachePurging() { | ||
| if (process.env.PURGE_REMOTE_BUILDER_CACHE !== undefined) { | ||
| RemoteClientLogger.log(`purging ${CloudRunnerFolders.purgeRemoteCaching}`); | ||
| fs.promises.rmdir(CloudRunnerFolders.cacheFolder, { recursive: true }); | ||
| RemoteClientLogger.log(`purging ${OrchestratorFolders.purgeRemoteCaching}`); | ||
| fs.promises.rmdir(OrchestratorFolders.cacheFolder, { recursive: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the OrchestratorFolders definition
find . -name "orchestrator-folders.ts" -o -name "*orchestrator-folders*" | head -20Repository: game-ci/unity-builder
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Check the cacheFolder and cacheFolderForAllFull definitions
rg "cacheFolder|cacheFolderForAllFull" src/model/orchestrator/ --type ts -B2 -A3Repository: game-ci/unity-builder
Length of output: 15223
🏁 Script executed:
#!/bin/bash
# Check usage of fs.promises.rmdir with cacheFolder or related paths
rg "rmdir|cacheFolder" src/model/orchestrator/remote-client/caching.ts -B3 -A3Repository: game-ci/unity-builder
Length of output: 4215
Relative path will delete from wrong directory; missing await on async call.
OrchestratorFolders.cacheFolder returns 'cache' (a relative path). Using fs.promises.rmdir() with a relative path will delete ./cache relative to the current working directory, not the intended cache location. Use OrchestratorFolders.cacheFolderForAllFull instead for the absolute path. Additionally, the fs.promises.rmdir() call is missing await.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/remote-client/caching.ts` around lines 477 - 482,
handleCachePurging currently calls fs.promises.rmdir with the relative
OrchestratorFolders.cacheFolder and without awaiting it; change the call in
RemoteClient.caching.handleCachePurging to await
fs.promises.rmdir(OrchestratorFolders.cacheFolderForAllFull, { recursive: true
}) (use the absolute cacheFolderForAllFull constant instead of cacheFolder) and
ensure the call is awaited so the purge completes before the function returns;
keep the RemoteClientLogger.log message as-is but log the absolute path if
desired.
| const startPath = process.cwd(); | ||
| process.chdir(folder); | ||
| const result = await (await CloudRunnerSystem.Run(`find -type f -exec md5sum "{}" + | sort | md5sum`)) | ||
| const result = await (await OrchestratorSystem.Run(`find -type f -exec md5sum "{}" + | sort | md5sum`)) | ||
| .replace(/\n/g, '') | ||
| .split(` `)[0]; | ||
| process.chdir(startPath); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/model/orchestrator/services/utility/lfs-hashing.ts | head -50Repository: game-ci/unity-builder
Length of output: 1875
🏁 Script executed:
fd -t f "orchestrator-system.ts" src/ | head -5Repository: game-ci/unity-builder
Length of output: 124
🏁 Script executed:
cat -n src/model/orchestrator/services/core/orchestrator-system.ts | head -100Repository: game-ci/unity-builder
Length of output: 2569
🏁 Script executed:
cat -n src/model/orchestrator/services/utility/lfs-hashing.ts | sed -n '35,42p'Repository: game-ci/unity-builder
Length of output: 329
Always restore cwd with try/finally in hashAllFiles().
If the OrchestratorSystem.Run() call fails, line 30 is skipped and the process stays in the wrong working directory, breaking subsequent filesystem operations.
🔧 Proposed fix
const startPath = process.cwd();
process.chdir(folder);
- const result = await (await OrchestratorSystem.Run(`find -type f -exec md5sum "{}" + | sort | md5sum`))
+ try {
+ const result = (await OrchestratorSystem.Run(`find -type f -exec md5sum "{}" + | sort | md5sum`))
.replace(/\n/g, '')
.split(` `)[0];
- process.chdir(startPath);
-
- return result;
+ return result;
+ } finally {
+ process.chdir(startPath);
+ }📝 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.
| const startPath = process.cwd(); | |
| process.chdir(folder); | |
| const result = await (await CloudRunnerSystem.Run(`find -type f -exec md5sum "{}" + | sort | md5sum`)) | |
| const result = await (await OrchestratorSystem.Run(`find -type f -exec md5sum "{}" + | sort | md5sum`)) | |
| .replace(/\n/g, '') | |
| .split(` `)[0]; | |
| process.chdir(startPath); | |
| const startPath = process.cwd(); | |
| process.chdir(folder); | |
| try { | |
| const result = (await OrchestratorSystem.Run(`find -type f -exec md5sum "{}" + | sort | md5sum`)) | |
| .replace(/\n/g, '') | |
| .split(` `)[0]; | |
| return result; | |
| } finally { | |
| process.chdir(startPath); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/utility/lfs-hashing.ts` around lines 25 - 30,
The current hashAllFiles implementation changes cwd with process.chdir(folder)
but restores it only after the async OrchestratorSystem.Run call, so an
exception would leave the process in the wrong directory; update hashAllFiles to
capture the original cwd (startPath), perform process.chdir(folder) and run
OrchestratorSystem.Run inside a try block, and ensure process.chdir(startPath)
is executed in a finally block so the working directory is always restored even
if OrchestratorSystem.Run throws.
| await OrchestratorSystem.Run( | ||
| `aws configure set aws_access_key_id "${process.env.AWS_ACCESS_KEY_ID}" --profile default || true`, | ||
| ); | ||
| await CloudRunnerSystem.Run( | ||
| await OrchestratorSystem.Run( | ||
| `aws configure set aws_secret_access_key "${process.env.AWS_SECRET_ACCESS_KEY}" --profile default || true`, | ||
| ); |
There was a problem hiding this comment.
Do not interpolate AWS secrets into logged shell commands.
OrchestratorSystem.Run logs command strings by default. On Line 149 and Line 153, secret values are embedded directly, which can expose credentials in CI logs.
🔐 Proposed fix
- await OrchestratorSystem.Run(
- `aws configure set aws_access_key_id "${process.env.AWS_ACCESS_KEY_ID}" --profile default || true`,
- );
+ await OrchestratorSystem.Run(
+ `aws configure set aws_access_key_id "$AWS_ACCESS_KEY_ID" --profile default || true`,
+ false,
+ true,
+ );
- await OrchestratorSystem.Run(
- `aws configure set aws_secret_access_key "${process.env.AWS_SECRET_ACCESS_KEY}" --profile default || true`,
- );
+ await OrchestratorSystem.Run(
+ `aws configure set aws_secret_access_key "$AWS_SECRET_ACCESS_KEY" --profile default || true`,
+ false,
+ true,
+ );
- await OrchestratorSystem.Run(
- `aws configure set region "${process.env.AWS_REGION}" --profile default || true`,
- );
+ await OrchestratorSystem.Run(
+ `aws configure set region "$AWS_REGION" --profile default || true`,
+ false,
+ true,
+ );Also applies to: 156-157, 168-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/tests/orchestrator-s3-steps.test.ts` around lines 149
- 154, Tests currently interpolate process.env secrets into the command strings
passed to OrchestratorSystem.Run, exposing credentials in logs; change the
commands in orchestrator-s3-steps.test.ts to avoid inserting secret values by
referencing environment variables in the shell (e.g., use aws configure set
aws_access_key_id "$AWS_ACCESS_KEY_ID" --profile default and aws configure set
aws_secret_access_key "$AWS_SECRET_ACCESS_KEY" --profile default) or by passing
an env object into OrchestratorSystem.Run so the child process receives the
credentials without them appearing in the command string; update all occurrences
(the shown calls and the ones at 156-157, 168-173) to use this approach and
ensure the test environment sets the corresponding env vars before running.
| BRANCH="${Orchestrator.buildParameters.orchestratorBranch}" | ||
| REPO="${OrchestratorFolders.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 orchestrator-develop "$REPO" /builder \ | ||
| || git clone -q -b main "$REPO" /builder \ | ||
| || git clone -q "$REPO" /builder | ||
| fi | ||
| git clone -q -b ${CloudRunner.buildParameters.branch} ${CloudRunnerFolders.targetBuildRepoUrl} /repo | ||
| git clone -q -b ${Orchestrator.buildParameters.branch} ${OrchestratorFolders.targetBuildRepoUrl} /repo |
There was a problem hiding this comment.
Quote dynamic branch/repo values in clone commands.
At Line 40, branch/repo are injected unquoted into shell. This can break for special characters and is avoidable by consistently quoting variables.
Proposed fix
BRANCH="${Orchestrator.buildParameters.orchestratorBranch}"
REPO="${OrchestratorFolders.unityBuilderRepoUrl}"
+TARGET_BRANCH="${Orchestrator.buildParameters.branch}"
+TARGET_REPO="${OrchestratorFolders.targetBuildRepoUrl}"
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 orchestrator-develop "$REPO" /builder \
|| git clone -q -b main "$REPO" /builder \
|| git clone -q "$REPO" /builder
fi
-git clone -q -b ${Orchestrator.buildParameters.branch} ${OrchestratorFolders.targetBuildRepoUrl} /repo
+git clone -q -b "$TARGET_BRANCH" "$TARGET_REPO" /repo📝 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="${Orchestrator.buildParameters.orchestratorBranch}" | |
| REPO="${OrchestratorFolders.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 orchestrator-develop "$REPO" /builder \ | |
| || git clone -q -b main "$REPO" /builder \ | |
| || git clone -q "$REPO" /builder | |
| fi | |
| git clone -q -b ${CloudRunner.buildParameters.branch} ${CloudRunnerFolders.targetBuildRepoUrl} /repo | |
| git clone -q -b ${Orchestrator.buildParameters.branch} ${OrchestratorFolders.targetBuildRepoUrl} /repo | |
| BRANCH="${Orchestrator.buildParameters.orchestratorBranch}" | |
| REPO="${OrchestratorFolders.unityBuilderRepoUrl}" | |
| TARGET_BRANCH="${Orchestrator.buildParameters.branch}" | |
| TARGET_REPO="${OrchestratorFolders.targetBuildRepoUrl}" | |
| 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 orchestrator-develop "$REPO" /builder \ | |
| || git clone -q -b main "$REPO" /builder \ | |
| || git clone -q "$REPO" /builder | |
| fi | |
| git clone -q -b "$TARGET_BRANCH" "$TARGET_REPO" /repo |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/workflows/async-workflow.ts` around lines 30 - 40, The
shell injection risk comes from unquoted variable expansions in the final clone
command; change git clone -q -b ${Orchestrator.buildParameters.branch}
${OrchestratorFolders.targetBuildRepoUrl} /repo to quote both expansions (e.g.
git clone -q -b "${Orchestrator.buildParameters.branch}"
"${OrchestratorFolders.targetBuildRepoUrl}" /repo) and ensure any other git
clone or git ls-remote usages consistently wrap BRANCH, REPO,
Orchestrator.buildParameters.orchestratorBranch,
OrchestratorFolders.unityBuilderRepoUrl, Orchestrator.buildParameters.branch and
OrchestratorFolders.targetBuildRepoUrl in double quotes to handle spaces/special
characters.
Remove $LOG_FILE and temp/job-log.txt debug artifacts that should not be in the repository. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Companion docs PR: game-ci/documentation#531 |
| public static get unityBuilderRepoUrl(): string { | ||
| return `https://${Orchestrator.buildParameters.gitPrivateToken}@github.com/${Orchestrator.buildParameters.orchestratorRepoName}.git`; | ||
| } | ||
|
|
||
| public static get targetBuildRepoUrl(): string { | ||
| return `https://${Orchestrator.buildParameters.gitPrivateToken}@github.com/${Orchestrator.buildParameters.githubRepo}.git`; | ||
| } |
There was a problem hiding this comment.
yea the private token thing is a bit scary. Have you considered alternative approaches?
| class OrchestratorQueryOverride { | ||
| static queryOverrides: { [key: string]: string } | undefined; | ||
|
|
||
| // TODO accept premade secret sources or custom secret source definition yamls |
There was a problem hiding this comment.
you want to still address this?
There was a problem hiding this comment.
Will address here
2 participants
Warning
Summary
src/model/cloud-runner/directory tosrc/model/orchestrator/cloud-runner-*files toorchestrator-*(~88 files)CloudRunner*classes toOrchestrator*(15+ classes)cloudRunner*properties/variables toorchestrator*equivalentsCLOUD_RUNNER_*environment variables toORCHESTRATOR_*action.ymlinputs and[CloudRunner]markers to[Orchestrator]cloud-runner-integrity.yml→orchestrator-integrity.yml, etc.)package.jsontest scriptsdist/index.jsBreaking Changes
CloudRunnerclass references are nowOrchestratorcloudRunnerRepoNameis noworchestratorRepoNameCLOUD_RUNNER_*are nowORCHESTRATOR_*Test plan
tsc --noEmit)dist/index.jsrebuilt and committed🤖 Generated with Claude Code
Summary by CodeRabbit