feat(orchestrator): QoL for large projects — CLI providers, caching, LFS, hooks#777
Closed
frostebite wants to merge 17 commits intomainfrom
Closed
feat(orchestrator): QoL for large projects — CLI providers, caching, LFS, hooks#777frostebite wants to merge 17 commits intomainfrom
frostebite wants to merge 17 commits intomainfrom
Conversation
…ule profiles, caching, LFS, hooks Add generic enterprise-grade features to the orchestrator, enabling Unity projects with complex CI/CD pipelines to adopt game-ci/unity-builder with built-in support for: - CLI provider protocol: JSON-over-stdin/stdout bridge enabling providers in any language (Go, Python, Rust, shell) via the `providerExecutable` input - Submodule profiles: YAML-based selective submodule initialization with glob patterns and variant overlays (`submoduleProfilePath`, `submoduleVariantPath`) - Local build caching: Filesystem-based Library and LFS caching for local builds without external cache actions (`localCacheEnabled`, `localCacheRoot`) - Custom LFS transfer agents: Register external transfer agents like elastic-git-storage (`lfsTransferAgent`, `lfsTransferAgentArgs`, `lfsStoragePaths`) - Git hooks support: Detect and install lefthook/husky with configurable skip lists (`gitHooksEnabled`, `gitHooksSkipList`) Also removes all `orchestrator-develop` branch references, replacing with `main`. 13 new action inputs, 13 new files, 14 new CLI provider tests, 17 submodule tests, plus cache/LFS/hooks unit tests. All 452 tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds submodule profile initialization, LFS transfer-agent support, local Library/LFS caching, git hooks management, a new CLI-based orchestrator provider, CLI commands, many action inputs, and changes workflow fallback clones to prefer the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Entry as Entry (src/index.ts)
participant Remote as RemoteClient
participant Submodule as SubmoduleProfileService
participant LfsAgent as LfsAgentService
participant Cache as LocalCacheService
participant Hooks as GitHooksService
participant Orch as Orchestrator
participant CLIProv as External CLI Provider
GH->>Entry: workflow start with inputs
Entry->>Remote: bootstrapRepository (clone)
alt submoduleProfilePath set
Remote->>Submodule: createInitPlan & execute
end
alt lfsTransferAgent set
Remote->>LfsAgent: configure(agentPath, args, storagePaths, repoPath)
end
alt localCacheEnabled true
Entry->>Cache: restoreLibraryCache & restoreLfsCache
end
alt gitHooksEnabled true
Entry->>Hooks: installHooks + configureSkipList
else
Entry->>Hooks: disableHooks
end
Entry->>Orch: initialize provider (providerExecutable?)
alt providerExecutable set
Orch->>CLIProv: instantiate CLI provider (executable)
CLIProv->>Orch: run-task / stream outputs / return results
else
Orch->>Orch: use built-in provider
end
Entry->>Entry: run build workflow
alt localCacheEnabled true
Entry->>Cache: saveLibraryCache & saveLfsCache
end
Entry-->>GH: workflow finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
|
frostebite
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
src/model/cli/cli.ts-200-202 (1)
200-202:⚠️ Potential issue | 🟡 MinorRename
agentArgsto satisfy lint rule.This currently violates
unicorn/prevent-abbreviations.🔧 Proposed fix
- const agentArgs = Cli.options!['agentArgs'] || ''; + const agentArguments = Cli.options!['agentArgs'] || ''; const storagePaths = (Cli.options!['storagePaths'] || '').split(';').filter(Boolean); - await LfsAgentService.configure(agentPath, agentArgs, storagePaths, process.cwd()); + await LfsAgentService.configure(agentPath, agentArguments, storagePaths, process.cwd());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/cli/cli.ts` around lines 200 - 202, Rename the abbreviated variable agentArgs to a full-word name (e.g., agentArguments) to satisfy the unicorn/prevent-abbreviations lint rule: update the declaration const agentArgs = Cli.options!['agentArgs'] || ''; to use the new identifier, update any subsequent uses (notably the call to LfsAgentService.configure(agentPath, agentArgs, storagePaths, process.cwd())), and ensure the option key mapping (Cli.options!['agentArgs']) is normalized if necessary (or map the existing option name to the new variable) so all references to agentArgs are replaced with the new agentArguments identifier.src/model/orchestrator/orchestrator.ts-132-141 (1)
132-141:⚠️ Potential issue | 🟡 MinorFix lint failure in the new CLI provider block.
padding-line-between-statementsrequires a blank line beforereturnhere.🔧 Proposed fix
Orchestrator.Provider = new CliProvider( Orchestrator.buildParameters.providerExecutable, Orchestrator.buildParameters, ); OrchestratorLogger.log(`Using CLI provider executable: ${Orchestrator.buildParameters.providerExecutable}`); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/orchestrator.ts` around lines 132 - 141, Add a blank line before the `return` in the CLI provider block to satisfy the `padding-line-between-statements` rule: after calling `OrchestratorLogger.log(...)` and before the `return` statement in the branch that checks `Orchestrator.buildParameters.providerExecutable` (where `CliProvider` is imported and `Orchestrator.Provider` is set), insert an empty line so the `return` is separated by a padding line.src/model/orchestrator/services/hooks/git-hooks-service.test.ts-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorRemove unused
pathimport to fix lint failure.
pathis imported but never used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/hooks/git-hooks-service.test.ts` at line 2, Remove the unused import "path" from the top of the test file (the import statement importing from 'node:path') to resolve the lint error; open src/model/orchestrator/services/hooks/git-hooks-service.test.ts and delete the line "import path from 'node:path';" (or if the import was intended, instead use the imported symbol within the test to justify keeping it).src/model/orchestrator/services/lfs/lfs-agent-service.test.ts-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorRemove unused
pathimport.This import is currently unused and triggers lint failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/lfs/lfs-agent-service.test.ts` at line 2, Remove the unused import of path by deleting the line that imports "path" from "node:path" in the test file (the import statement "import path from 'node:path'"); ensure no remaining references to the symbol path in lfs-agent-service.test.ts and run lint/tests to confirm the lint error is resolved.src/model/orchestrator/services/submodule/submodule-profile-types.ts-7-8 (1)
7-8:⚠️ Potential issue | 🟡 MinorThese field names currently violate configured camelCase lint rules.
If these are wire-format keys, prefer camelCase in the TS model and map snake_case only at parse/serialization boundaries to keep CI lint passing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/submodule/submodule-profile-types.ts` around lines 7 - 8, The interface fields primary_submodule and product_name violate camelCase linting—rename them to primarySubmodule and productName in the TypeScript model (update any interface/type declarations and all usages referencing primary_submodule/product_name) and adjust JSON parsing/serialization code to map between snake_case (wire format) and camelCase (model) at the boundaries (e.g., in fromJSON/toJSON or parse/serialize helpers) so CI lint passes without changing external wire keys.src/model/orchestrator/services/hooks/git-hooks-service.test.ts-83-83 (1)
83-83:⚠️ Potential issue | 🟡 MinorAvoid explicit
undefinedin mock return.
mockReturnValue(undefined)tripsunicorn/no-useless-undefined; usemockImplementation(() => {})(or omit the explicit value).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/hooks/git-hooks-service.test.ts` at line 83, The test currently uses (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined) which triggers unicorn/no-useless-undefined; replace the explicit undefined return with a no-op implementation by calling mockImplementation(() => {}) on the mock (or remove the explicit mockReturnValue entirely if the default behavior is fine) so the mock for mkdirSync is a function that returns nothing; update the test where mockFs.mkdirSync is referenced to use mockImplementation and keep the rest of the assertions unchanged.src/model/orchestrator/services/lfs/lfs-agent-service.test.ts-52-58 (1)
52-58:⚠️ Potential issue | 🟡 MinorReset
LFS_STORAGE_PATHSafter this test to avoid env leakage.This test mutates global process env and can affect later cases in the same worker if not cleaned up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/lfs/lfs-agent-service.test.ts` around lines 52 - 58, The test 'should set LFS_STORAGE_PATHS environment variable when storagePaths provided' mutates process.env and should restore it to prevent leakage; update the test (or add an afterEach in this spec) to save the original process.env.LFS_STORAGE_PATHS before calling LfsAgentService.configure and restore it after the assertion (or delete it if it was undefined) so subsequent tests are unaffected; reference the test name and the LfsAgentService.configure invocation to locate where to add the save/restore logic.src/model/orchestrator/services/cache/local-cache-service.test.ts-46-46 (1)
46-46:⚠️ Potential issue | 🟡 MinorAddress current ESLint errors in this test file.
This segment contains lint violations (
unicorn/prevent-abbreviations,lines-around-comment, andunicorn/no-useless-undefined) that should be fixed to keep CI passing.Also applies to: 93-93, 99-99, 107-107, 121-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/cache/local-cache-service.test.ts` at line 46, The test file has ESLint issues around abbreviated names, comment spacing, and use of useless undefined: rename the variable originalEnv to originalEnvironment (and any other abbreviated vars at the other occurrences), ensure comments have blank lines above/below per lines-around-comment (adjust spacing around inline comments near the test code at the mentioned locations), and remove or replace explicit uses of undefined that trigger unicorn/no-useless-undefined (e.g., use typeof checks or delete process.env keys or set to void 0 if needed). Update references to the renamed symbol (originalEnvironment) across the tests so they still restore process.env, and adjust any comments to conform to the linter rules.src/model/orchestrator/services/submodule/submodule-profile-service.test.ts-86-86 (1)
86-86:⚠️ Potential issue | 🟡 MinorResolve lint spacing violations in the test helpers.
These lines trigger
padding-line-between-statementsand will keep ESLint red.Also applies to: 199-199, 252-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/submodule/submodule-profile-service.test.ts` at line 86, Several test helper functions currently end with the literal return '' which is causing padding-line-between-statements lint errors; update each helper around the return '' occurrences so there is a blank line separating adjacent statements (add an empty line before or after the return as appropriate) to satisfy ESLint. Locate the test helpers in submodule-profile-service.test.ts that use return '' and insert the necessary blank lines around those return statements so padding-line-between-statements is no longer violated.src/model/orchestrator/providers/cli/cli-provider.test.ts-22-23 (1)
22-23:⚠️ Potential issue | 🟡 MinorMove imports to the top of the module.
Lines 22-23 violate
import/first(imports after executable statements), which will fail lint.Proposed fix
import { EventEmitter } from 'events'; import { ProviderLoader } from '../provider-loader'; +import { spawn } from 'child_process'; +import CliProvider from './cli-provider'; // Mock child_process jest.mock('child_process', () => ({ spawn: jest.fn(), exec: jest.fn(), })); @@ -// Mock provider-git-manager (required by provider-loader) -jest.mock('../provider-git-manager'); - -import { spawn } from 'child_process'; -import CliProvider from './cli-provider'; +// Mock provider-git-manager (required by provider-loader) +jest.mock('../provider-git-manager');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/cli/cli-provider.test.ts` around lines 22 - 23, The test module currently has executable code before imports causing an import/first lint error; move the module imports (specifically the "spawn" import from 'child_process' and the "CliProvider" import from './cli-provider') to the very top of the file so all import statements appear before any executable statements or test setup code, and ensure no other non-import statements precede them (adjust any top-level requires/logic to run after imports).src/model/orchestrator/services/hooks/git-hooks-service.ts-60-77 (1)
60-77:⚠️ Potential issue | 🟡 MinorNormalize skip-list tokens before exporting env vars.
On Line 68, raw values are joined directly. If the input contains spaces or empty entries (for example from comma-separated user input),
LEFTHOOK_EXCLUDEmay include invalid hook names.Proposed fix
static configureSkipList(skipList: string[]): Record<string, string> { - if (skipList.length === 0) { + const normalizedSkipList = skipList.map((hook) => hook.trim()).filter(Boolean); + if (normalizedSkipList.length === 0) { return {}; } // Return both lefthook and husky env vars so the caller can apply whichever is relevant. // Lefthook supports selective hook exclusion. const env: Record<string, string> = { - LEFTHOOK_EXCLUDE: skipList.join(','), + LEFTHOOK_EXCLUDE: normalizedSkipList.join(','), }; // Husky only supports full disable (HUSKY=0), not selective skipping. // If any hooks are in the skip list, disable husky entirely. env.HUSKY = '0'; - OrchestratorLogger.log(`[GitHooks] Skip list configured: ${skipList.join(', ')}`); + OrchestratorLogger.log(`[GitHooks] Skip list configured: ${normalizedSkipList.join(', ')}`); return env; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/hooks/git-hooks-service.ts` around lines 60 - 77, The configureSkipList function currently joins raw skipList values into LEFTHOOK_EXCLUDE and sets HUSKY= '0' even when entries contain spaces or empty tokens; fix by normalizing the input in configureSkipList: map each token to token.trim(), filter out empty strings (token !== ''), then if the cleaned list is empty return {} otherwise set LEFTHOOK_EXCLUDE to cleaned.join(',') and only set HUSKY = '0' when there are valid tokens; update the OrchestratorLogger.log call to log the cleaned list so the exported env vars reflect normalized hook names.
🧹 Nitpick comments (1)
action.yml (1)
294-351: Add a brief security caution for token/executable inputs.Consider adding explicit wording that
submoduleTokenshould come from GitHub Secrets andproviderExecutablemust be trusted, since these are security-sensitive knobs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@action.yml` around lines 294 - 351, Add a short security caution sentence to the action inputs: for submoduleToken, state it should be provided via GitHub Secrets (not hard-coded) and warn about least-privilege scope; for providerExecutable, warn that the path must point to a trusted binary (do not run untrusted executables) because it can execute arbitrary code. Update the descriptions for the submoduleToken and providerExecutable inputs (referencing the input names submoduleToken and providerExecutable) to include these concise security notes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 101-111: The local cache save is executed unconditionally and can
persist invalid state on failed builds; update the logic to only run
LocalCacheService.saveLibraryCache and LocalCacheService.saveLfsCache when the
build succeeded (i.e. exitCode === 0) by moving or gating the block that checks
buildParameters.localCacheEnabled behind the post-build success check (use the
existing exitCode variable), keeping references to
buildParameters.localCacheLibrary and buildParameters.localCacheLfs to decide
whether to call saveLibraryCache(projectFullPath, cacheRoot, cacheKey) and
saveLfsCache(workspace, cacheRoot, cacheKey) respectively.
In `@src/model/orchestrator/providers/cli/cli-provider.ts`:
- Around line 90-111: The stdout handler for child processes in
runTaskInWorkflow and watchWorkflow assumes each data event contains whole
newline-delimited entries, causing JSON lines split across chunks to fail;
introduce a persistent buffer string (e.g., incompleteStdoutChunk) scoped to the
child process, on each child.stdout.on('data') prepend the buffer to
data.toString(), split into lines, keep the last partial line in
incompleteStdoutChunk (do not attempt to parse it yet), and only parse/handle
complete lines (updating lastJsonResponse, pushing to outputLines, and calling
OrchestratorLogger.log); also flush any remaining incompleteStdoutChunk on
child.stdout's 'end'/'close' to process the last line.
In `@src/model/orchestrator/providers/provider-loader.ts`:
- Line 61: The provider map includes 'cli' but loadProvider() constructs
providers using new Provider(buildParameters), which fails because CliProvider
requires an executablePath plus buildParameters; update provider-loader logic to
special-case the 'cli' key in loadProvider() (and the provider registration
around the cli entry) to instantiate CliProvider with both
buildParameters.providerExecutable (or buildParameters.providerExecutablePath)
and buildParameters, or change the provider factory for 'cli' to accept a single
composite argument that includes executablePath; reference loadProvider,
Provider, CliProvider, and buildParameters/providerExecutable when making the
change.
In `@src/model/orchestrator/remote-client/index.ts`:
- Around line 241-255: The call passes unvalidated, user-provided values
(Orchestrator.buildParameters.submoduleProfilePath, submoduleVariantPath,
submoduleToken, gitPrivateToken and OrchestratorFolders.repoPathAbsolute) into
services that build shell commands (SubmoduleProfileService.execute and
LfsAgentService.configure); add defensive validation/sanitization before calling
these methods: ensure file/path inputs are normalized and constrained (no .., no
null bytes, allowed chars/whitelist, exists under expected repo root), ensure
token/auth inputs match strict regex (base64/hex/URL-safe chars and length) or
are rejected, and prefer passing values via safe APIs or argument arrays rather
than concatenated shell strings; perform these checks in the caller (index.ts)
and reject/throw on invalid values before invoking
SubmoduleProfileService.execute or LfsAgentService.configure.
In `@src/model/orchestrator/services/cache/local-cache-service.ts`:
- Around line 68-73: The restore path creation currently only calls fs.mkdirSync
(e.g., for variables libraryDest and the .git/lfs destination) which merges
restored files with any preexisting content; before extracting (where tarPath is
used and OrchestratorSystem.Run executes the tar command), remove existing
destination contents (use fs.rmSync(destination, { recursive: true, force: true
}) or an equivalent safe recursive delete) then recreate the directory with
fs.mkdirSync(..., { recursive: true }); apply this change for the Library
restore (libraryDest/projectPath + tarPath extraction) and the .git/lfs restore
so stale artifacts are cleared before running OrchestratorSystem.Run.
In `@src/model/orchestrator/services/lfs/lfs-agent-service.ts`:
- Around line 34-36: Constructing git-config commands by interpolating repoPath,
agentPath, agentArgs, and agentName into a shell string is unsafe; instead
change the calls that use OrchestratorSystem.Run to pass the command and its
arguments as a safe argument array (or add a Run variant that accepts args) so
the shell will not reparse those values, e.g. call OrchestratorSystem.Run with
["git","-C", repoPath, "config", "lfs.customtransfer."+agentName+".path",
agentPath] and similarly for the args and standalonetransferagent entries;
additionally validate or restrict agentName to a safe pattern
(alphanumeric/[-_.]) before including it in the config key to prevent injection
via config key names.
- Around line 30-32: The code is logging sensitive LFS agent arguments via
OrchestratorLogger.log(`[LfsAgent] Args: ${agentArgs}`); remove or redact raw
agentArgs: replace the direct log with either no args or a safe summary (e.g.,
count of args or a redacted string), and if needed add a helper like
sanitizeArgs/safeArgs to mask tokens/credentials before logging; keep the
existing OrchestratorLogger usage and continue logging agentPath but never print
agentArgs verbatim (refer to OrchestratorLogger and the agentArgs variable in
lfs-agent-service.ts).
In `@src/model/orchestrator/services/submodule/submodule-profile-service.test.ts`:
- Around line 284-291: The tests currently assert raw interpolated shell command
strings (mockedSystem.Run calls) which mirrors unsafe construction using
action.path and action.branch; update the implementation in
submodule-profile-service.ts to stop building shell-interpolated commands and
instead pass argv-safe arguments (e.g., change Run usage to accept an argv array
or use a spawn-style API) and sanitize/validate action.path and action.branch
before use; then update these tests (references: mockedSystem.Run expectations
and the code that constructs commands around action.path/action.branch) to
assert the safer call shape (argv array or escaped/sanitized input) for the
init, checkout and deinit cases.
- Around line 299-301: The test currently asserts that mockedSystem.Run was
called with a command containing a raw secret token string, which exposes
credentials; change the test and (if needed) the implementation so credentials
are not embedded in command text: update the expectation on mockedSystem.Run in
submodule-profile-service.test.ts to assert that the command does not contain
the token (e.g., expect no plain "https://my-secret-token" substring) and
instead verifies use of a credential helper or env-based auth (for example
assert the call uses a credential helper flag or that the code calls the method
that sets GIT_ASKPASS/GIT_HTTP_USERNAME/credential helper), and update the code
path that builds the git command (the logic that triggers mockedSystem.Run) to
use git credential helpers or environment variables rather than interpolating
the token into the URL.
In `@src/model/orchestrator/services/submodule/submodule-profile-service.ts`:
- Around line 32-35: The mapping over parsed.submodules in
submodule-profile-service.ts currently coerces missing values via
String(entry.name)/String(entry.branch) which produces the literal "undefined";
instead, validate each entry's shape before including it: check that entry is an
object and that entry.name and entry.branch are present and are non-empty
strings, and either filter out invalid entries or throw a clear validation error
(depending on intended behavior). Replace the String(...) coercion in the
submodules mapping with a validation step (on parsed.submodules entries) that
returns only well-formed { name, branch } objects or surfaces a descriptive
error so invalid data doesn't propagate into plan execution.
- Around line 200-216: The code builds fullPath but still runs git commands with
unquoted/unscoped paths and interpolated values, which is unsafe and can fail
when CWD differs; update all OrchestratorSystem.Run calls in execute()
(including the token config, submodule update/init, checkout and deinit calls)
to use the computed fullPath variable (not action.path), pass git commands with
-C "<fullPath>" or run them with cwd set to repoPath, and quote/interpolate all
dynamic values (e.g., wrap ${token}, ${fullPath}, ${action.branch},
${action.name}) to prevent shell injection and whitespace issues; specifically
change the token git config invocation and the lines that call git submodule
update --init, git -C ... checkout, and git submodule deinit -f to use quoted
fullPath (or quoted args) and/or a repo-scoped -C "<fullPath>" so commands run
reliably regardless of CWD.
---
Minor comments:
In `@src/model/cli/cli.ts`:
- Around line 200-202: Rename the abbreviated variable agentArgs to a full-word
name (e.g., agentArguments) to satisfy the unicorn/prevent-abbreviations lint
rule: update the declaration const agentArgs = Cli.options!['agentArgs'] || '';
to use the new identifier, update any subsequent uses (notably the call to
LfsAgentService.configure(agentPath, agentArgs, storagePaths, process.cwd())),
and ensure the option key mapping (Cli.options!['agentArgs']) is normalized if
necessary (or map the existing option name to the new variable) so all
references to agentArgs are replaced with the new agentArguments identifier.
In `@src/model/orchestrator/orchestrator.ts`:
- Around line 132-141: Add a blank line before the `return` in the CLI provider
block to satisfy the `padding-line-between-statements` rule: after calling
`OrchestratorLogger.log(...)` and before the `return` statement in the branch
that checks `Orchestrator.buildParameters.providerExecutable` (where
`CliProvider` is imported and `Orchestrator.Provider` is set), insert an empty
line so the `return` is separated by a padding line.
In `@src/model/orchestrator/providers/cli/cli-provider.test.ts`:
- Around line 22-23: The test module currently has executable code before
imports causing an import/first lint error; move the module imports
(specifically the "spawn" import from 'child_process' and the "CliProvider"
import from './cli-provider') to the very top of the file so all import
statements appear before any executable statements or test setup code, and
ensure no other non-import statements precede them (adjust any top-level
requires/logic to run after imports).
In `@src/model/orchestrator/services/cache/local-cache-service.test.ts`:
- Line 46: The test file has ESLint issues around abbreviated names, comment
spacing, and use of useless undefined: rename the variable originalEnv to
originalEnvironment (and any other abbreviated vars at the other occurrences),
ensure comments have blank lines above/below per lines-around-comment (adjust
spacing around inline comments near the test code at the mentioned locations),
and remove or replace explicit uses of undefined that trigger
unicorn/no-useless-undefined (e.g., use typeof checks or delete process.env keys
or set to void 0 if needed). Update references to the renamed symbol
(originalEnvironment) across the tests so they still restore process.env, and
adjust any comments to conform to the linter rules.
In `@src/model/orchestrator/services/hooks/git-hooks-service.test.ts`:
- Line 2: Remove the unused import "path" from the top of the test file (the
import statement importing from 'node:path') to resolve the lint error; open
src/model/orchestrator/services/hooks/git-hooks-service.test.ts and delete the
line "import path from 'node:path';" (or if the import was intended, instead use
the imported symbol within the test to justify keeping it).
- Line 83: The test currently uses (mockFs.mkdirSync as
jest.Mock).mockReturnValue(undefined) which triggers
unicorn/no-useless-undefined; replace the explicit undefined return with a no-op
implementation by calling mockImplementation(() => {}) on the mock (or remove
the explicit mockReturnValue entirely if the default behavior is fine) so the
mock for mkdirSync is a function that returns nothing; update the test where
mockFs.mkdirSync is referenced to use mockImplementation and keep the rest of
the assertions unchanged.
In `@src/model/orchestrator/services/hooks/git-hooks-service.ts`:
- Around line 60-77: The configureSkipList function currently joins raw skipList
values into LEFTHOOK_EXCLUDE and sets HUSKY= '0' even when entries contain
spaces or empty tokens; fix by normalizing the input in configureSkipList: map
each token to token.trim(), filter out empty strings (token !== ''), then if the
cleaned list is empty return {} otherwise set LEFTHOOK_EXCLUDE to
cleaned.join(',') and only set HUSKY = '0' when there are valid tokens; update
the OrchestratorLogger.log call to log the cleaned list so the exported env vars
reflect normalized hook names.
In `@src/model/orchestrator/services/lfs/lfs-agent-service.test.ts`:
- Line 2: Remove the unused import of path by deleting the line that imports
"path" from "node:path" in the test file (the import statement "import path from
'node:path'"); ensure no remaining references to the symbol path in
lfs-agent-service.test.ts and run lint/tests to confirm the lint error is
resolved.
- Around line 52-58: The test 'should set LFS_STORAGE_PATHS environment variable
when storagePaths provided' mutates process.env and should restore it to prevent
leakage; update the test (or add an afterEach in this spec) to save the original
process.env.LFS_STORAGE_PATHS before calling LfsAgentService.configure and
restore it after the assertion (or delete it if it was undefined) so subsequent
tests are unaffected; reference the test name and the LfsAgentService.configure
invocation to locate where to add the save/restore logic.
In `@src/model/orchestrator/services/submodule/submodule-profile-service.test.ts`:
- Line 86: Several test helper functions currently end with the literal return
'' which is causing padding-line-between-statements lint errors; update each
helper around the return '' occurrences so there is a blank line separating
adjacent statements (add an empty line before or after the return as
appropriate) to satisfy ESLint. Locate the test helpers in
submodule-profile-service.test.ts that use return '' and insert the necessary
blank lines around those return statements so padding-line-between-statements is
no longer violated.
In `@src/model/orchestrator/services/submodule/submodule-profile-types.ts`:
- Around line 7-8: The interface fields primary_submodule and product_name
violate camelCase linting—rename them to primarySubmodule and productName in the
TypeScript model (update any interface/type declarations and all usages
referencing primary_submodule/product_name) and adjust JSON
parsing/serialization code to map between snake_case (wire format) and camelCase
(model) at the boundaries (e.g., in fromJSON/toJSON or parse/serialize helpers)
so CI lint passes without changing external wire keys.
---
Nitpick comments:
In `@action.yml`:
- Around line 294-351: Add a short security caution sentence to the action
inputs: for submoduleToken, state it should be provided via GitHub Secrets (not
hard-coded) and warn about least-privilege scope; for providerExecutable, warn
that the path must point to a trusted binary (do not run untrusted executables)
because it can execute arbitrary code. Update the descriptions for the
submoduleToken and providerExecutable inputs (referencing the input names
submoduleToken and providerExecutable) to include these concise security notes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a21522c5-53fb-45d1-8266-e4f9f301b9fd
⛔ 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 (25)
.github/workflows/orchestrator-async-checks.ymlaction.ymlsrc/index.tssrc/model/build-parameters.tssrc/model/cli/cli.tssrc/model/input.tssrc/model/orchestrator/orchestrator.tssrc/model/orchestrator/providers/cli/cli-provider-protocol.tssrc/model/orchestrator/providers/cli/cli-provider.test.tssrc/model/orchestrator/providers/cli/cli-provider.tssrc/model/orchestrator/providers/cli/index.tssrc/model/orchestrator/providers/provider-loader.tssrc/model/orchestrator/remote-client/index.tssrc/model/orchestrator/services/cache/local-cache-service.test.tssrc/model/orchestrator/services/cache/local-cache-service.tssrc/model/orchestrator/services/hooks/git-hooks-service.test.tssrc/model/orchestrator/services/hooks/git-hooks-service.tssrc/model/orchestrator/services/lfs/lfs-agent-service.test.tssrc/model/orchestrator/services/lfs/lfs-agent-service.tssrc/model/orchestrator/services/submodule/submodule-profile-service.test.tssrc/model/orchestrator/services/submodule/submodule-profile-service.tssrc/model/orchestrator/services/submodule/submodule-profile-types.tssrc/model/orchestrator/tests/e2e/orchestrator-end2end-caching.test.tssrc/model/orchestrator/workflows/async-workflow.tssrc/model/orchestrator/workflows/build-automation-workflow.ts
Comment on lines
+101
to
+111
| // Local build caching - save | ||
| if (buildParameters.localCacheEnabled) { | ||
| const { LocalCacheService } = await import('./model/orchestrator/services/cache/local-cache-service'); | ||
| if (buildParameters.localCacheLibrary) { | ||
| const projectFullPath = path.join(workspace, buildParameters.projectPath); | ||
| await LocalCacheService.saveLibraryCache(projectFullPath, cacheRoot, cacheKey); | ||
| } | ||
| if (buildParameters.localCacheLfs) { | ||
| await LocalCacheService.saveLfsCache(workspace, cacheRoot, cacheKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid saving local caches when the build fails.
This block runs before the failure check and can persist invalid Library/LFS state when exitCode !== 0, causing future cache poisoning.
Proposed fix
- // Local build caching - save
- if (buildParameters.localCacheEnabled) {
+ // Local build caching - save (only on successful build)
+ if (buildParameters.localCacheEnabled && exitCode === 0) {
const { LocalCacheService } = await import('./model/orchestrator/services/cache/local-cache-service');
if (buildParameters.localCacheLibrary) {
const projectFullPath = path.join(workspace, buildParameters.projectPath);
await LocalCacheService.saveLibraryCache(projectFullPath, cacheRoot, cacheKey);
}
if (buildParameters.localCacheLfs) {
await LocalCacheService.saveLfsCache(workspace, cacheRoot, cacheKey);
}
}📝 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
| // Local build caching - save | |
| if (buildParameters.localCacheEnabled) { | |
| const { LocalCacheService } = await import('./model/orchestrator/services/cache/local-cache-service'); | |
| if (buildParameters.localCacheLibrary) { | |
| const projectFullPath = path.join(workspace, buildParameters.projectPath); | |
| await LocalCacheService.saveLibraryCache(projectFullPath, cacheRoot, cacheKey); | |
| } | |
| if (buildParameters.localCacheLfs) { | |
| await LocalCacheService.saveLfsCache(workspace, cacheRoot, cacheKey); | |
| } | |
| } | |
| // Local build caching - save (only on successful build) | |
| if (buildParameters.localCacheEnabled && exitCode === 0) { | |
| const { LocalCacheService } = await import('./model/orchestrator/services/cache/local-cache-service'); | |
| if (buildParameters.localCacheLibrary) { | |
| const projectFullPath = path.join(workspace, buildParameters.projectPath); | |
| await LocalCacheService.saveLibraryCache(projectFullPath, cacheRoot, cacheKey); | |
| } | |
| if (buildParameters.localCacheLfs) { | |
| await LocalCacheService.saveLfsCache(workspace, cacheRoot, cacheKey); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 101 - 111, The local cache save is executed
unconditionally and can persist invalid state on failed builds; update the logic
to only run LocalCacheService.saveLibraryCache and
LocalCacheService.saveLfsCache when the build succeeded (i.e. exitCode === 0) by
moving or gating the block that checks buildParameters.localCacheEnabled behind
the post-build success check (use the existing exitCode variable), keeping
references to buildParameters.localCacheLibrary and
buildParameters.localCacheLfs to decide whether to call
saveLibraryCache(projectFullPath, cacheRoot, cacheKey) and
saveLfsCache(workspace, cacheRoot, cacheKey) respectively.
Comment on lines
+90
to
+111
| child.stdout.on('data', (data: Buffer) => { | ||
| const lines = data.toString().split('\n'); | ||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) continue; | ||
|
|
||
| // Try to parse as JSON response | ||
| try { | ||
| const parsed = JSON.parse(trimmed); | ||
| if (typeof parsed === 'object' && parsed !== null && 'success' in parsed) { | ||
| lastJsonResponse = parsed as CliProviderResponse; | ||
| continue; | ||
| } | ||
| } catch { | ||
| // Not JSON — treat as build output | ||
| } | ||
|
|
||
| // Forward non-JSON lines as real-time build output | ||
| OrchestratorLogger.log(trimmed); | ||
| outputLines.push(trimmed); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Buffer stdout across chunks before line/JSON parsing.
Lines 90-111 and 197-216 assume each data event contains complete newline-delimited entries. In Node streams, JSON lines can be split across chunks, which causes parse failures and incorrect fallback behavior.
Proposed fix pattern (apply to both `runTaskInWorkflow` and `watchWorkflow`)
- child.stdout.on('data', (data: Buffer) => {
- const lines = data.toString().split('\n');
+ let stdoutBuffer = '';
+ child.stdout.on('data', (data: Buffer) => {
+ stdoutBuffer += data.toString();
+ const lines = stdoutBuffer.split('\n');
+ stdoutBuffer = lines.pop() ?? '';
for (const line of lines) {
const trimmed = line.trim();
if (!trimmed) continue;
// existing JSON parsing + logging logic
}
});
+ child.on('close', (code: number | null) => {
+ const tail = stdoutBuffer.trim();
+ if (tail) {
+ try {
+ const parsed = JSON.parse(tail);
+ if (typeof parsed === 'object' && parsed !== null && 'success' in parsed) {
+ lastJsonResponse = parsed as CliProviderResponse;
+ } else {
+ OrchestratorLogger.log(tail);
+ outputLines.push(tail);
+ }
+ } catch {
+ OrchestratorLogger.log(tail);
+ outputLines.push(tail);
+ }
+ }
+ // existing close-handler logic
+ });Also applies to: 197-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/cli/cli-provider.ts` around lines 90 - 111,
The stdout handler for child processes in runTaskInWorkflow and watchWorkflow
assumes each data event contains whole newline-delimited entries, causing JSON
lines split across chunks to fail; introduce a persistent buffer string (e.g.,
incompleteStdoutChunk) scoped to the child process, on each
child.stdout.on('data') prepend the buffer to data.toString(), split into lines,
keep the last partial line in incompleteStdoutChunk (do not attempt to parse it
yet), and only parse/handle complete lines (updating lastJsonResponse, pushing
to outputLines, and calling OrchestratorLogger.log); also flush any remaining
incompleteStdoutChunk on child.stdout's 'end'/'close' to process the last line.
| const providerModuleMap: Record<string, string> = { | ||
| aws: './aws', | ||
| k8s: './k8s', | ||
| cli: './cli', |
There was a problem hiding this comment.
cli is advertised as loadable but cannot be constructed via this loader.
Because loadProvider() instantiates with a single argument (new Provider(buildParameters) on Line 91), loading 'cli' from this map/list will fail at runtime since CliProvider requires executablePath + buildParameters.
Proposed fix
default: {
// Fallback to built-in providers or direct import
const providerModuleMap: Record<string, string> = {
aws: './aws',
k8s: './k8s',
- cli: './cli',
test: './test',
'local-docker': './docker',
'local-system': './local',
local: './local',
}; static getAvailableProviders(): string[] {
- return ['aws', 'k8s', 'cli', 'test', 'local-docker', 'local-system', 'local'];
+ return ['aws', 'k8s', 'test', 'local-docker', 'local-system', 'local'];
}Or, alternatively, special-case 'cli' instantiation using buildParameters.providerExecutable.
Also applies to: 140-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/provider-loader.ts` at line 61, The provider
map includes 'cli' but loadProvider() constructs providers using new
Provider(buildParameters), which fails because CliProvider requires an
executablePath plus buildParameters; update provider-loader logic to
special-case the 'cli' key in loadProvider() (and the provider registration
around the cli entry) to instantiate CliProvider with both
buildParameters.providerExecutable (or buildParameters.providerExecutablePath)
and buildParameters, or change the provider factory for 'cli' to accept a single
composite argument that includes executablePath; reference loadProvider,
Provider, CliProvider, and buildParameters/providerExecutable when making the
change.
Comment on lines
+241
to
+255
| // Initialize submodules from profile if configured | ||
| if (Orchestrator.buildParameters.submoduleProfilePath) { | ||
| const { SubmoduleProfileService } = await import('../services/submodule/submodule-profile-service'); | ||
| RemoteClientLogger.log('Initializing submodules from profile...'); | ||
| const plan = await SubmoduleProfileService.createInitPlan( | ||
| Orchestrator.buildParameters.submoduleProfilePath, | ||
| Orchestrator.buildParameters.submoduleVariantPath, | ||
| OrchestratorFolders.repoPathAbsolute, | ||
| ); | ||
| await SubmoduleProfileService.execute( | ||
| plan, | ||
| OrchestratorFolders.repoPathAbsolute, | ||
| Orchestrator.buildParameters.submoduleToken || Orchestrator.buildParameters.gitPrivateToken, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Validate shell-sensitive inputs before passing to submodule/LFS services.
These values are input-derived and are forwarded to services that construct shell command strings. Add validation/sanitization before invoking SubmoduleProfileService.execute(...) and LfsAgentService.configure(...) to prevent command-break/injection cases.
🛡️ Proposed defensive fix in this call path
+ const assertSafeShellValue = (value: string, fieldName: string) => {
+ if (/[\r\n`$|;&<>"]/.test(value)) {
+ throw new Error(`Invalid characters in ${fieldName}`);
+ }
+ };
+
// Initialize submodules from profile if configured
if (Orchestrator.buildParameters.submoduleProfilePath) {
const { SubmoduleProfileService } = await import('../services/submodule/submodule-profile-service');
RemoteClientLogger.log('Initializing submodules from profile...');
+ const submoduleToken =
+ Orchestrator.buildParameters.submoduleToken || Orchestrator.buildParameters.gitPrivateToken;
+ if (submoduleToken) {
+ assertSafeShellValue(submoduleToken, 'submodule token');
+ }
const plan = await SubmoduleProfileService.createInitPlan(
Orchestrator.buildParameters.submoduleProfilePath,
Orchestrator.buildParameters.submoduleVariantPath,
OrchestratorFolders.repoPathAbsolute,
);
await SubmoduleProfileService.execute(
plan,
OrchestratorFolders.repoPathAbsolute,
- Orchestrator.buildParameters.submoduleToken || Orchestrator.buildParameters.gitPrivateToken,
+ submoduleToken,
);
}
@@
// Configure custom LFS transfer agent if specified
if (Orchestrator.buildParameters.lfsTransferAgent) {
const { LfsAgentService } = await import('../services/lfs/lfs-agent-service');
RemoteClientLogger.log('Configuring custom LFS transfer agent...');
+ assertSafeShellValue(Orchestrator.buildParameters.lfsTransferAgent, 'lfsTransferAgent');
+ if (Orchestrator.buildParameters.lfsTransferAgentArgs) {
+ assertSafeShellValue(Orchestrator.buildParameters.lfsTransferAgentArgs, 'lfsTransferAgentArgs');
+ }
+ const storagePaths = Orchestrator.buildParameters.lfsStoragePaths
+ ? Orchestrator.buildParameters.lfsStoragePaths.split(';').map((x) => x.trim()).filter(Boolean)
+ : [];
await LfsAgentService.configure(
Orchestrator.buildParameters.lfsTransferAgent,
Orchestrator.buildParameters.lfsTransferAgentArgs,
- Orchestrator.buildParameters.lfsStoragePaths ? Orchestrator.buildParameters.lfsStoragePaths.split(';') : [],
+ storagePaths,
OrchestratorFolders.repoPathAbsolute,
);
}Also applies to: 272-282
🤖 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 241 - 255, The
call passes unvalidated, user-provided values
(Orchestrator.buildParameters.submoduleProfilePath, submoduleVariantPath,
submoduleToken, gitPrivateToken and OrchestratorFolders.repoPathAbsolute) into
services that build shell commands (SubmoduleProfileService.execute and
LfsAgentService.configure); add defensive validation/sanitization before calling
these methods: ensure file/path inputs are normalized and constrained (no .., no
null bytes, allowed chars/whitelist, exists under expected repo root), ensure
token/auth inputs match strict regex (base64/hex/URL-safe chars and length) or
are rejected, and prefer passing values via safe APIs or argument arrays rather
than concatenated shell strings; perform these checks in the caller (index.ts)
and reject/throw on invalid values before invoking
SubmoduleProfileService.execute or LfsAgentService.configure.
Comment on lines
+68
to
+73
| // Ensure destination exists | ||
| fs.mkdirSync(libraryDest, { recursive: true }); | ||
|
|
||
| OrchestratorLogger.log(`[LocalCache] Library cache hit: restoring from ${tarPath}`); | ||
| await OrchestratorSystem.Run(`tar -xf "${tarPath}" -C "${projectPath}"`, true); | ||
| OrchestratorLogger.log(`[LocalCache] Library cache restored successfully`); |
There was a problem hiding this comment.
Clear destination directories before restoring cache archives.
Lines 69 and 158 only create destination folders; they do not remove previous content. That merges old and restored data, which can leave stale artifacts in Library or .git/lfs.
Proposed fix
const tarPath = path.join(cachePath, latestFile);
const libraryDest = path.join(projectPath, 'Library');
// Ensure destination exists
-fs.mkdirSync(libraryDest, { recursive: true });
+if (fs.existsSync(libraryDest)) {
+ fs.rmSync(libraryDest, { recursive: true, force: true });
+}
+fs.mkdirSync(libraryDest, { recursive: true });
@@
const tarPath = path.join(cachePath, latestFile);
const lfsDest = path.join(repoPath, '.git', 'lfs');
// Ensure destination exists
-fs.mkdirSync(lfsDest, { recursive: true });
+if (fs.existsSync(lfsDest)) {
+ fs.rmSync(lfsDest, { recursive: true, force: true });
+}
+fs.mkdirSync(lfsDest, { recursive: true });Also applies to: 157-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/cache/local-cache-service.ts` around lines 68
- 73, The restore path creation currently only calls fs.mkdirSync (e.g., for
variables libraryDest and the .git/lfs destination) which merges restored files
with any preexisting content; before extracting (where tarPath is used and
OrchestratorSystem.Run executes the tar command), remove existing destination
contents (use fs.rmSync(destination, { recursive: true, force: true }) or an
equivalent safe recursive delete) then recreate the directory with
fs.mkdirSync(..., { recursive: true }); apply this change for the Library
restore (libraryDest/projectPath + tarPath extraction) and the .git/lfs restore
so stale artifacts are cleared before running OrchestratorSystem.Run.
Comment on lines
+34
to
+36
| await OrchestratorSystem.Run(`git -C "${repoPath}" config lfs.customtransfer.${agentName}.path "${agentPath}"`); | ||
| await OrchestratorSystem.Run(`git -C "${repoPath}" config lfs.customtransfer.${agentName}.args "${agentArgs}"`); | ||
| await OrchestratorSystem.Run(`git -C "${repoPath}" config lfs.standalonetransferagent ${agentName}`); |
There was a problem hiding this comment.
Harden git-config command construction against shell injection.
Lines 34-36 interpolate input-derived values directly into shell commands (repoPath, agentPath, agentArgs, agentName). This can break command parsing and is exploitable.
Proposed fix
-const agentName = path.basename(agentPath, path.extname(agentPath));
+const agentName = path.basename(agentPath, path.extname(agentPath)).replace(/[^A-Za-z0-9_-]/g, '_');
+const shellEscape = (value: string) => value.replace(/(["\\$`])/g, '\\$1');
+const escapedRepoPath = shellEscape(repoPath);
+const escapedAgentPath = shellEscape(agentPath);
+const escapedAgentArgs = shellEscape(agentArgs);
// Set git config entries for the custom transfer agent
-await OrchestratorSystem.Run(`git -C "${repoPath}" config lfs.customtransfer.${agentName}.path "${agentPath}"`);
-await OrchestratorSystem.Run(`git -C "${repoPath}" config lfs.customtransfer.${agentName}.args "${agentArgs}"`);
-await OrchestratorSystem.Run(`git -C "${repoPath}" config lfs.standalonetransferagent ${agentName}`);
+await OrchestratorSystem.Run(
+ `git -C "${escapedRepoPath}" config lfs.customtransfer.${agentName}.path "${escapedAgentPath}"`,
+);
+await OrchestratorSystem.Run(
+ `git -C "${escapedRepoPath}" config lfs.customtransfer.${agentName}.args "${escapedAgentArgs}"`,
+);
+await OrchestratorSystem.Run(`git -C "${escapedRepoPath}" config lfs.standalonetransferagent "${agentName}"`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/lfs/lfs-agent-service.ts` around lines 34 -
36, Constructing git-config commands by interpolating repoPath, agentPath,
agentArgs, and agentName into a shell string is unsafe; instead change the calls
that use OrchestratorSystem.Run to pass the command and its arguments as a safe
argument array (or add a Run variant that accepts args) so the shell will not
reparse those values, e.g. call OrchestratorSystem.Run with ["git","-C",
repoPath, "config", "lfs.customtransfer."+agentName+".path", agentPath] and
similarly for the args and standalonetransferagent entries; additionally
validate or restrict agentName to a safe pattern (alphanumeric/[-_.]) before
including it in the config key to prevent injection via config key names.
Comment on lines
+284
to
+291
| expect(mockedSystem.Run).toHaveBeenCalledWith('git submodule update --init Assets/ModuleA'); | ||
|
|
||
| // ModuleB: init + checkout develop | ||
| expect(mockedSystem.Run).toHaveBeenCalledWith('git submodule update --init Assets/ModuleB'); | ||
| expect(mockedSystem.Run).toHaveBeenCalledWith('git -C Assets/ModuleB checkout develop'); | ||
|
|
||
| // ModuleC: deinit | ||
| expect(mockedSystem.Run).toHaveBeenCalledWith('git submodule deinit -f Assets/ModuleC 2>/dev/null || true'); |
There was a problem hiding this comment.
Current assertions lock in shell-injection-prone command construction.
These expectations mirror unsafe interpolation of action.path / action.branch into shell commands (see src/model/orchestrator/services/submodule/submodule-profile-service.ts, Line 206-217). Those values originate from profile files and should be escaped/argv-based before execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/submodule/submodule-profile-service.test.ts`
around lines 284 - 291, The tests currently assert raw interpolated shell
command strings (mockedSystem.Run calls) which mirrors unsafe construction using
action.path and action.branch; update the implementation in
submodule-profile-service.ts to stop building shell-interpolated commands and
instead pass argv-safe arguments (e.g., change Run usage to accept an argv array
or use a spawn-style API) and sanitize/validate action.path and action.branch
before use; then update these tests (references: mockedSystem.Run expectations
and the code that constructs commands around action.path/action.branch) to
assert the safer call shape (argv array or escaped/sanitized input) for the
init, checkout and deinit cases.
Comment on lines
+299
to
+301
| expect(mockedSystem.Run).toHaveBeenCalledWith( | ||
| 'git config url."https://my-secret-token@github.com/".insteadOf "https://github.com/"', | ||
| ); |
There was a problem hiding this comment.
Token is being validated in a raw command string (secret exposure risk).
This codifies behavior where credentials are embedded directly in shell command text. Prefer credential helpers or env-based auth so secrets are not present in command arguments/loggable strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/submodule/submodule-profile-service.test.ts`
around lines 299 - 301, The test currently asserts that mockedSystem.Run was
called with a command containing a raw secret token string, which exposes
credentials; change the test and (if needed) the implementation so credentials
are not embedded in command text: update the expectation on mockedSystem.Run in
submodule-profile-service.test.ts to assert that the command does not contain
the token (e.g., expect no plain "https://my-secret-token" substring) and
instead verifies use of a credential helper or env-based auth (for example
assert the call uses a credential helper flag or that the code calls the method
that sets GIT_ASKPASS/GIT_HTTP_USERNAME/credential helper), and update the code
path that builds the git command (the logic that triggers mockedSystem.Run) to
use git credential helpers or environment variables rather than interpolating
the token into the URL.
Comment on lines
+32
to
+35
| submodules: parsed.submodules.map((entry: any) => ({ | ||
| name: String(entry.name), | ||
| branch: String(entry.branch), | ||
| })), |
There was a problem hiding this comment.
Validate submodule entry shape instead of coercing invalid data.
Lines 33-34 convert missing values to literal 'undefined'. That propagates into plan execution as invalid branch/path behavior.
Proposed fix
- submodules: parsed.submodules.map((entry: any) => ({
- name: String(entry.name),
- branch: String(entry.branch),
- })),
+ submodules: parsed.submodules.map((entry: any, index: number) => {
+ if (!entry || typeof entry.name !== 'string' || typeof entry.branch !== 'string') {
+ throw new Error(`Invalid submodule entry at index ${index} in ${profilePath}`);
+ }
+ return {
+ name: entry.name,
+ branch: entry.branch,
+ };
+ }),📝 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
| submodules: parsed.submodules.map((entry: any) => ({ | |
| name: String(entry.name), | |
| branch: String(entry.branch), | |
| })), | |
| submodules: parsed.submodules.map((entry: any, index: number) => { | |
| if (!entry || typeof entry.name !== 'string' || typeof entry.branch !== 'string') { | |
| throw new Error(`Invalid submodule entry at index ${index} in ${profilePath}`); | |
| } | |
| return { | |
| name: entry.name, | |
| branch: entry.branch, | |
| }; | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/submodule/submodule-profile-service.ts`
around lines 32 - 35, The mapping over parsed.submodules in
submodule-profile-service.ts currently coerces missing values via
String(entry.name)/String(entry.branch) which produces the literal "undefined";
instead, validate each entry's shape before including it: check that entry is an
object and that entry.name and entry.branch are present and are non-empty
strings, and either filter out invalid entries or throw a clear validation error
(depending on intended behavior). Replace the String(...) coercion in the
submodules mapping with a validation step (on parsed.submodules entries) that
returns only well-formed { name, branch } objects or surfaces a descriptive
error so invalid data doesn't propagate into plan execution.
Comment on lines
+200
to
+216
| await OrchestratorSystem.Run(`git config url."https://${token}@github.com/".insteadOf "https://github.com/"`); | ||
| } | ||
|
|
||
| for (const action of plan) { | ||
| const fullPath = path.posix.join(repoPath, action.path).replace(/\\/g, '/'); | ||
|
|
||
| if (action.action === 'init') { | ||
| OrchestratorLogger.log(`Initializing submodule: ${action.name} (branch: ${action.branch})`); | ||
| await OrchestratorSystem.Run(`git submodule update --init ${action.path}`); | ||
|
|
||
| if (action.branch !== 'main') { | ||
| OrchestratorLogger.log(`Checking out branch '${action.branch}' for submodule: ${action.name}`); | ||
| await OrchestratorSystem.Run(`git -C ${action.path} checkout ${action.branch}`); | ||
| } | ||
| } else { | ||
| OrchestratorLogger.log(`Skipping submodule: ${action.name}`); | ||
| await OrchestratorSystem.Run(`git submodule deinit -f ${action.path} 2>/dev/null || true`); |
There was a problem hiding this comment.
Use repo-scoped and quoted git commands in execute().
Line 204 computes fullPath but Line 212 still uses git -C ${action.path}. Lines 200/208/212/216 also interpolate unquoted values into shell commands. This can fail when CWD differs from repoPath and is unsafe with profile-derived input.
Proposed fix
if (token) {
OrchestratorLogger.log('Configuring git authentication for submodule initialization...');
- await OrchestratorSystem.Run(`git config url."https://${token}@github.com/".insteadOf "https://github.com/"`);
+ await OrchestratorSystem.Run(
+ `git -C "${repoPath}" config url."https://${token}@github.com/".insteadOf "https://github.com/"`,
+ );
}
@@
if (action.action === 'init') {
OrchestratorLogger.log(`Initializing submodule: ${action.name} (branch: ${action.branch})`);
- await OrchestratorSystem.Run(`git submodule update --init ${action.path}`);
+ await OrchestratorSystem.Run(`git -C "${repoPath}" submodule update --init "${action.path}"`);
if (action.branch !== 'main') {
OrchestratorLogger.log(`Checking out branch '${action.branch}' for submodule: ${action.name}`);
- await OrchestratorSystem.Run(`git -C ${action.path} checkout ${action.branch}`);
+ await OrchestratorSystem.Run(`git -C "${fullPath}" checkout "${action.branch}"`);
}
} else {
OrchestratorLogger.log(`Skipping submodule: ${action.name}`);
- await OrchestratorSystem.Run(`git submodule deinit -f ${action.path} 2>/dev/null || true`);
+ await OrchestratorSystem.Run(`git -C "${repoPath}" submodule deinit -f "${action.path}"`, true);
}🧰 Tools
🪛 ESLint
[error] 204-204: 'fullPath' is assigned a value but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/submodule/submodule-profile-service.ts`
around lines 200 - 216, The code builds fullPath but still runs git commands
with unquoted/unscoped paths and interpolated values, which is unsafe and can
fail when CWD differs; update all OrchestratorSystem.Run calls in execute()
(including the token config, submodule update/init, checkout and deinit calls)
to use the computed fullPath variable (not action.path), pass git commands with
-C "<fullPath>" or run them with cwd set to repoPath, and quote/interpolate all
dynamic values (e.g., wrap ${token}, ${fullPath}, ${action.branch},
${action.name}) to prevent shell injection and whitespace issues; specifically
change the token git config invocation and the lines that call git submodule
update --init, git -C ... checkout, and git submodule deinit -f to use quoted
fullPath (or quoted args) and/or a repo-scoped -C "<fullPath>" so commands run
reliably regardless of CWD.
3 tasks
frostebite
changed the title
frostebite
changed the title
This was referenced Mar 5, 2026
frostebite
added a commit
to game-ci/documentation
that referenced
this pull request
Mar 5, 2026
Adds a new page documenting the CLI provider protocol that lets users write orchestrator providers in any language (Go, Python, Rust, shell). Covers: invocation model, JSON stdin/stdout protocol, streaming output, subcommands with timeouts, shell example, CLI vs TypeScript comparison. Related: game-ci/unity-builder#777 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
frostebite
added a commit
to game-ci/documentation
that referenced
this pull request
Mar 5, 2026
…, hooks Adds a new advanced topics page documenting orchestrator build services: - Submodule profiles (YAML, glob patterns, variant overlays) - Local build caching (Library + LFS filesystem cache) - Custom LFS transfer agents (elastic-git-storage, etc.) - Git hooks (lefthook/husky detection, skip lists) Related: game-ci/unity-builder#777 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 5, 2026
frostebite
added a commit
to game-ci/documentation
that referenced
this pull request
Mar 5, 2026
Adds a new page documenting the CLI provider protocol that lets users write orchestrator providers in any language (Go, Python, Rust, shell). Covers: invocation model, JSON stdin/stdout protocol, streaming output, subcommands with timeouts, shell example, CLI vs TypeScript comparison. Related: game-ci/unity-builder#777 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
frostebite
changed the title
frostebite
changed the title
frostebite
changed the title
frostebite
changed the title
frostebite
changed the title
Adds tests for cache hit restore (picks latest tar), LFS cache restore/save, garbage collection age filtering, and edge cases like permission errors and empty directories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/services/cache/local-cache-service.test.ts`:
- Line 103: The tests call (mockFs.mkdirSync as
jest.Mock).mockReturnValue(undefined), which triggers the
unicorn/no-useless-undefined lint rule; update each occurrence (e.g., the mock
setup for mockFs.mkdirSync in local-cache-service.test.ts and the similar mocks
at the other noted lines) to remove the explicit undefined return — use
mockReturnValue() with no argument or mockReturnValueOnce() without passing
undefined, or simply mockImplementation(() => {}) so the mock still returns void
but no useless undefined is present; update all four spots (the mkdirSync mock
and the three analogous mocks) accordingly.
- Line 55: Rename abbreviated variables and fix padding/formatting to satisfy
ESLint/Prettier: change originalEnv to originalEnvironment and dirPath to
directoryPath across this test file (references in setup/teardown and any helper
functions), add or remove the single blank line required around the statements
flagged by padding-line-between-statements (e.g., before/after the return or
between const declarations and assertions near the block around the former
dirPath usage), and reformat the multiline expect assertions at the four flagged
ranges into Prettier-compliant forms (either single-line expectations or use
chained/multi-assert helpers so they aren’t split across lines). Ensure all
references (setup/teardown, mocks, and deletes) use the new names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a7fbccbc-9bfa-49be-b2d5-f6282f5d4fb9
📒 Files selected for processing (1)
src/model/orchestrator/services/cache/local-cache-service.test.ts
| }); | ||
|
|
||
| describe('resolveCacheRoot', () => { | ||
| const originalEnv = process.env; |
There was a problem hiding this comment.
Resolve remaining ESLint errors (naming/padding/prettier) in this file.
Line [55] (originalEnv) and Line [143] (dirPath) trip unicorn/prevent-abbreviations; Line [147] trips padding-line-between-statements; and the multiline expectations at Lines [110-113], [157-160], [190-193], [234-237] are flagged by Prettier.
💡 Suggested fix (naming + padding)
- const originalEnv = process.env;
+ const originalEnvironment = process.env;
beforeEach(() => {
- process.env = { ...originalEnv };
+ process.env = { ...originalEnvironment };
});
afterEach(() => {
- process.env = originalEnv;
+ process.env = originalEnvironment;
});
- (mockFs.readdirSync as jest.Mock).mockImplementation((dirPath: string) => {
- if (String(dirPath).includes('Library') && !String(dirPath).includes('cache')) {
+ (mockFs.readdirSync as jest.Mock).mockImplementation((directoryPath: string) => {
+ if (String(directoryPath).includes('Library') && !String(directoryPath).includes('cache')) {
return ['file1.asset', 'file2.asset'];
}
+
return [];
});Also applies to: 143-147, 110-113, 157-160, 190-193, 234-237
🧰 Tools
🪛 ESLint
[error] 55-55: The variable originalEnv should be named originalEnvironment. A more descriptive name will do too.
(unicorn/prevent-abbreviations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/cache/local-cache-service.test.ts` at line
55, Rename abbreviated variables and fix padding/formatting to satisfy
ESLint/Prettier: change originalEnv to originalEnvironment and dirPath to
directoryPath across this test file (references in setup/teardown and any helper
functions), add or remove the single blank line required around the statements
flagged by padding-line-between-statements (e.g., before/after the return or
between const declarations and assertions near the block around the former
dirPath usage), and reformat the multiline expect assertions at the four flagged
ranges into Prettier-compliant forms (either single-line expectations or use
chained/multi-assert helpers so they aren’t split across lines). Ensure all
references (setup/teardown, mocks, and deletes) use the new names.
| (mockFs.statSync as jest.Mock).mockImplementation((filePath: string) => ({ | ||
| mtimeMs: String(filePath).includes('lib-2000') ? 2000 : 1000, | ||
| })); | ||
| (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined); |
There was a problem hiding this comment.
Remove explicit undefined in mock return setup.
Lines [103], [150], [184], and [229] violate unicorn/no-useless-undefined. This is a lint error and can fail CI.
💡 Suggested fix
- (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined);
+ (mockFs.mkdirSync as jest.Mock).mockReturnValue();
- (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined);
+ (mockFs.mkdirSync as jest.Mock).mockReturnValue();
- (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined);
+ (mockFs.mkdirSync as jest.Mock).mockReturnValue();
- (mockFs.rmSync as jest.Mock).mockReturnValue(undefined);
+ (mockFs.rmSync as jest.Mock).mockReturnValue();Also applies to: 150-150, 184-184, 229-229
🧰 Tools
🪛 ESLint
[error] 103-103: Do not use useless undefined.
(unicorn/no-useless-undefined)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/cache/local-cache-service.test.ts` at line
103, The tests call (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined),
which triggers the unicorn/no-useless-undefined lint rule; update each
occurrence (e.g., the mock setup for mockFs.mkdirSync in
local-cache-service.test.ts and the similar mocks at the other noted lines) to
remove the explicit undefined return — use mockReturnValue() with no argument or
mockReturnValueOnce() without passing undefined, or simply mockImplementation(()
=> {}) so the mock still returns void but no useless undefined is present;
update all four spots (the mkdirSync mock and the three analogous mocks)
accordingly.
Add comprehensive tests for CLI provider (cleanupWorkflow, garbageCollect, listWorkflow, watchWorkflow, stderr forwarding, timeout handling), local cache service (saveLfsCache full path and error handling), git hooks service (husky install, failure logging, edge cases), and LFS agent service (empty storagePaths, validate logging). 73 tests across 4 test files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
src/model/orchestrator/services/cache/local-cache-service.test.ts (3)
55-63:⚠️ Potential issue | 🟡 MinorResolve remaining naming/padding lint failures.
Line [55] (
originalEnv) and Lines [143], [213] (dirPath) still violateunicorn/prevent-abbreviations, and Lines [147], [217] need the required blank line beforereturn.Suggested lint-compliant patch
- const originalEnv = process.env; + const originalEnvironment = process.env; beforeEach(() => { - process.env = { ...originalEnv }; + process.env = { ...originalEnvironment }; }); afterEach(() => { - process.env = originalEnv; + process.env = originalEnvironment; }); - (mockFs.readdirSync as jest.Mock).mockImplementation((dirPath: string) => { - if (String(dirPath).includes('Library') && !String(dirPath).includes('cache')) { + (mockFs.readdirSync as jest.Mock).mockImplementation((directoryPath: string) => { + if (String(directoryPath).includes('Library') && !String(directoryPath).includes('cache')) { return ['file1.asset', 'file2.asset']; } + return []; }); - (mockFs.readdirSync as jest.Mock).mockImplementation((dirPath: string) => { - if (String(dirPath).includes('lfs') && !String(dirPath).includes('cache')) { + (mockFs.readdirSync as jest.Mock).mockImplementation((directoryPath: string) => { + if (String(directoryPath).includes('lfs') && !String(directoryPath).includes('cache')) { return ['objects', 'tmp']; } + return []; });Also applies to: 143-148, 213-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/cache/local-cache-service.test.ts` around lines 55 - 63, Rename the abbreviated variables and add the missing blank lines before returns: change originalEnv to a clear name like originalEnvVars (update its usage in the beforeEach and afterEach blocks and any references), rename dirPath to directoryPath wherever declared/used, and insert the required blank line immediately before each return statement in the affected test helper/functions (the ones using directoryPath at the earlier and later test blocks). Update all references to these symbols (originalEnv -> originalEnvVars, dirPath -> directoryPath) to keep the test file lint-compliant.
103-103:⚠️ Potential issue | 🟡 MinorDrop explicit
undefinedin mock return values.Lines [103], [150], [184], [220], and [261] trigger
unicorn/no-useless-undefined.Suggested patch
- (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined); + (mockFs.mkdirSync as jest.Mock).mockReturnValue(); - (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined); + (mockFs.mkdirSync as jest.Mock).mockReturnValue(); - (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined); + (mockFs.mkdirSync as jest.Mock).mockReturnValue(); - (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined); + (mockFs.mkdirSync as jest.Mock).mockReturnValue(); - (mockFs.rmSync as jest.Mock).mockReturnValue(undefined); + (mockFs.rmSync as jest.Mock).mockReturnValue();Also applies to: 150-150, 184-184, 220-220, 261-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/cache/local-cache-service.test.ts` at line 103, Tests call (mockFs.mkdirSync as jest.Mock).mockReturnValue(undefined) (and similar calls at the other locations); remove the explicit undefined by changing those invocations to mockReturnValue() so the mock returns undefined implicitly. Locate occurrences of mockReturnValue(undefined) in src/model/orchestrator/services/cache/local-cache-service.test.ts (including the calls on mockFs.mkdirSync and any other mocks at the flagged locations) and replace them with mockReturnValue() to satisfy the unicorn/no-useless-undefined rule.
110-113:⚠️ Potential issue | 🟡 MinorApply Prettier formatting to multiline
toHaveBeenCalledWithexpectations.These ranges still violate
prettier/prettierand can break lint CI.Suggested patch
- expect(OrchestratorSystem.Run).toHaveBeenCalledWith( - expect.stringContaining('lib-2000.tar'), - true, - ); + expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining('lib-2000.tar'), true); - expect(OrchestratorSystem.Run).toHaveBeenCalledWith( - expect.stringContaining('tar -cf'), - true, - ); + expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining('tar -cf'), true); - expect(OrchestratorSystem.Run).toHaveBeenCalledWith( - expect.stringContaining('lfs-200.tar'), - true, - ); + expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining('lfs-200.tar'), true); - expect(OrchestratorSystem.Run).toHaveBeenCalledWith( - expect.stringContaining('tar -cf'), - true, - ); + expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining('tar -cf'), true); - expect(mockFs.rmSync).toHaveBeenCalledWith( - path.join('/cache', 'old-cache'), - { recursive: true, force: true }, - ); + expect(mockFs.rmSync).toHaveBeenCalledWith(path.join('/cache', 'old-cache'), { recursive: true, force: true });Also applies to: 157-160, 190-193, 227-230, 266-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/cache/local-cache-service.test.ts` around lines 110 - 113, Reformat the multiline Jest expectations that call toHaveBeenCalledWith so they conform to Prettier style: locate usages of OrchestratorSystem.Run in the tests and reformat the toHaveBeenCalledWith calls (which use expect.stringContaining(...), boolean args, etc.) so each argument formatting matches the repo's Prettier rules (one argument per line, consistent trailing commas/spacing) — apply the same change to the other occurrences noted (around the other ranges) and run the project's Prettier/formatter to ensure the linter passes.
🧹 Nitpick comments (4)
src/model/orchestrator/services/cache/local-cache-service.test.ts (1)
116-124: Align test intent with assertions for error logging paths.The test names say warnings are logged, but they currently only assert return/no-throw. Add assertions on logger calls to prevent silent regressions.
Suggested patch
it('should return false and log warning on error', async () => { (mockFs.existsSync as jest.Mock).mockReturnValue(true); (mockFs.readdirSync as jest.Mock).mockImplementation(() => { throw new Error('Permission denied'); }); + const OrchestratorLogger = require('../core/orchestrator-logger').default; const result = await LocalCacheService.restoreLibraryCache('/project', '/cache', 'key1'); expect(result).toBe(false); + expect(OrchestratorLogger.logWarning).toHaveBeenCalledWith( + expect.stringContaining('Library cache restore failed: Permission denied'), + ); }); it('should handle save errors gracefully', async () => { (mockFs.existsSync as jest.Mock).mockReturnValue(true); (mockFs.readdirSync as jest.Mock).mockImplementation(() => { throw new Error('Disk full'); }); + const OrchestratorLogger = require('../core/orchestrator-logger').default; // Should not throw await LocalCacheService.saveLfsCache('/repo', '/cache', 'key1'); + expect(OrchestratorLogger.logWarning).toHaveBeenCalledWith( + expect.stringContaining('LFS cache save failed: Disk full'), + ); });Also applies to: 233-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/cache/local-cache-service.test.ts` around lines 116 - 124, The test "should return false and log warning on error" currently only asserts the boolean result; update it to also assert that the cache logger receives a warning when readdirSync throws: mock or spy the logger.warn used by LocalCacheService (e.g., mockLogger.warn or the logger instance LocalCacheService uses), trigger the error via (mockFs.readdirSync as jest.Mock).mockImplementation(() => { throw new Error('Permission denied'); }), then expect the logger.warn to have been called with a message containing the error or context about failing to restore cache; repeat the same pattern for the other test at the 233-241 location so both tests verify the logging side-effect as well as the boolean return value and reset/clear the mock between tests.src/model/orchestrator/services/lfs/lfs-agent-service.test.ts (1)
87-93: Strengthen this test to avoid a vacuous pass.By deleting
LFS_STORAGE_PATHSbefore invocation, this test doesn’t verify behavior when a value already exists. Set a sentinel first and assert it remains unchanged whenstoragePathsis empty.Suggested test adjustment
- const originalValue = process.env.LFS_STORAGE_PATHS; - delete process.env.LFS_STORAGE_PATHS; + process.env.LFS_STORAGE_PATHS = '/existing/path'; await LfsAgentService.configure('/usr/local/bin/agent', '', [], '/repo'); - expect(process.env.LFS_STORAGE_PATHS).toBeUndefined(); - - if (originalValue !== undefined) { - process.env.LFS_STORAGE_PATHS = originalValue; - } + expect(process.env.LFS_STORAGE_PATHS).toBe('/existing/path');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/lfs/lfs-agent-service.test.ts` around lines 87 - 93, The test currently deletes process.env.LFS_STORAGE_PATHS so it can vacuously pass; instead set a sentinel value into process.env.LFS_STORAGE_PATHS (e.g. originalValue or "SENTINEL"), call LfsAgentService.configure('/usr/local/bin/agent', '', [], '/repo') with an empty storagePaths array, and assert that process.env.LFS_STORAGE_PATHS still equals the sentinel afterward; also preserve and restore the originalValue variable around the test to avoid side effects.src/model/orchestrator/providers/cli/cli-provider.test.ts (2)
368-382: The test name claims stderr forwarding, but forwarding is not asserted.This currently only verifies “no rejection.” Add an assertion against the mocked logger to validate the behavior under test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/cli/cli-provider.test.ts` around lines 368 - 382, The test "forwards stderr lines to logger during execute" currently emits stderr data but doesn't assert that the provider forwarded it; update the test to assert that the mocked logger was called with the stderr content (e.g., verify mockLogger.warn/mockLogger.error was called once with a message containing "warning: something") after awaiting provider.listResources(); locate the test using createMockChildProcess, mockSpawn and the CliProvider instance and add the assertion against the logger mock to validate stderr forwarding behavior.
69-69: Extract JSON-line encoding into a helper function to eliminate string concatenation and duplication.This pattern repeats 14 times across the test file and violates the
prefer-templateESLint rule. All occurrences use the identical string concatenation:Buffer.from(JSON.stringify({...}) + '\n').Suggested fix
+const toJsonLine = (payload: unknown): Buffer => Buffer.from(`${JSON.stringify(payload)}\n`); @@ - child.stdout.emit('data', Buffer.from(JSON.stringify({ success: true, result: [] }) + '\n')); + child.stdout.emit('data', toJsonLine({ success: true, result: [] }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/cli/cli-provider.test.ts` at line 69, Extract the repeated Buffer.from(JSON.stringify(...) + '\n') pattern in the test file into a small helper (e.g., jsonLineBuffer or makeJsonLine) and replace each call site (for example the child.stdout.emit('data', Buffer.from(JSON.stringify({ success: true, result: [] }) + '\n')) usages) with the helper to eliminate string concatenation and satisfy prefer-template; the helper should accept the object, perform JSON.stringify and append the newline, and return the Buffer used by child.stdout.emit and other places.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/providers/cli/cli-provider.test.ts`:
- Line 155: The linter requires a blank line before inline (end-of-line)
comments; add an empty line immediately above the inline comment that reads
"listResources falls back to empty array when result is missing" and do the same
for the other inline comment occurrence in the CLI provider tests
(cli-provider.test.ts) so each line comment has a blank line before it, ensuring
the tests satisfy the lines-around-comment rule.
- Around line 22-23: The import statements for spawn and CliProvider are placed
after jest.mock() calls, violating import/first; move the two imports (the
"spawn" import from 'child_process' and the "CliProvider" default import from
'./cli-provider') to the very top of the test module so all imports occur before
any jest.mock() or other runtime calls.
In `@src/model/orchestrator/services/hooks/git-hooks-service.test.ts`:
- Around line 30-32: The existsSync mock currently checks
String(filePath).includes('lefthook.yml') && !String(filePath).startsWith('.')
which misclassifies paths like /repo/.lefthook.yml; update the predicate used in
the mockFs.existsSync implementation to check the basename of the path (e.g.,
use path.basename(filePath) === 'lefthook.yml') or an equivalent regex that
ensures the final filename is exactly lefthook.yml and does not start with a
dot; apply this change to both mock implementations of mockFs.existsSync in
git-hooks-service.test.ts (the one around the earlier mock and the one later in
the file).
- Line 83: Remove the unnecessary .mockReturnValue(undefined) calls for the
mockFs functions (e.g., the mock on mockFs.mkdirSync at the top and the other
mock at line 152) — simply delete those mockReturnValue(undefined) invocations
so the mocks use their default undefined return; and reformat the nearby test
blocks that span the failing regions (the two multi-line mock/setup blocks
around lines 144-146 and 160-162) to match Prettier styling (fix indentation and
spacing) so the file passes linting, focusing on the test file's setup/teardown
blocks and any jest.mock or mock.calls formatting.
In `@src/model/orchestrator/services/lfs/lfs-agent-service.test.ts`:
- Around line 119-121: The assertion using OrchestratorLogger.logWarning is
split across multiple lines and triggers Prettier; collapse the multiline
expect.toHaveBeenCalledWith call into a single-line argument so Prettier passes
— specifically update the test assertion in lfs-agent-service.test.ts that calls
OrchestratorLogger.logWarning to use
expect(OrchestratorLogger.logWarning).toHaveBeenCalledWith(expect.stringContaining('Agent
executable not found')) on one line.
- Line 2: Remove the unused import "path" from the top of the test file to
satisfy the no-unused-vars lint rule: edit
src/model/orchestrator/services/lfs/lfs-agent-service.test.ts and delete the
line importing path from 'node:path' (the import is unused in this file), then
run the linter/tests to confirm the warning is gone.
- Around line 24-26: The tests mutate process.env.LFS_STORAGE_PATHS but only
call jest.clearAllMocks() in beforeEach; update the test suite to save the
original process.env.LFS_STORAGE_PATHS (e.g., const original =
process.env.LFS_STORAGE_PATHS) and restore it in an afterEach (or restore at
start of beforeEach) so each test runs with a clean env; locate the hooks in
lfs-agent-service.test.ts where beforeEach() is defined and add the save/restore
around process.env.LFS_STORAGE_PATHS to ensure deterministic tests.
---
Duplicate comments:
In `@src/model/orchestrator/services/cache/local-cache-service.test.ts`:
- Around line 55-63: Rename the abbreviated variables and add the missing blank
lines before returns: change originalEnv to a clear name like originalEnvVars
(update its usage in the beforeEach and afterEach blocks and any references),
rename dirPath to directoryPath wherever declared/used, and insert the required
blank line immediately before each return statement in the affected test
helper/functions (the ones using directoryPath at the earlier and later test
blocks). Update all references to these symbols (originalEnv -> originalEnvVars,
dirPath -> directoryPath) to keep the test file lint-compliant.
- Line 103: Tests call (mockFs.mkdirSync as
jest.Mock).mockReturnValue(undefined) (and similar calls at the other
locations); remove the explicit undefined by changing those invocations to
mockReturnValue() so the mock returns undefined implicitly. Locate occurrences
of mockReturnValue(undefined) in
src/model/orchestrator/services/cache/local-cache-service.test.ts (including the
calls on mockFs.mkdirSync and any other mocks at the flagged locations) and
replace them with mockReturnValue() to satisfy the unicorn/no-useless-undefined
rule.
- Around line 110-113: Reformat the multiline Jest expectations that call
toHaveBeenCalledWith so they conform to Prettier style: locate usages of
OrchestratorSystem.Run in the tests and reformat the toHaveBeenCalledWith calls
(which use expect.stringContaining(...), boolean args, etc.) so each argument
formatting matches the repo's Prettier rules (one argument per line, consistent
trailing commas/spacing) — apply the same change to the other occurrences noted
(around the other ranges) and run the project's Prettier/formatter to ensure the
linter passes.
---
Nitpick comments:
In `@src/model/orchestrator/providers/cli/cli-provider.test.ts`:
- Around line 368-382: The test "forwards stderr lines to logger during execute"
currently emits stderr data but doesn't assert that the provider forwarded it;
update the test to assert that the mocked logger was called with the stderr
content (e.g., verify mockLogger.warn/mockLogger.error was called once with a
message containing "warning: something") after awaiting
provider.listResources(); locate the test using createMockChildProcess,
mockSpawn and the CliProvider instance and add the assertion against the logger
mock to validate stderr forwarding behavior.
- Line 69: Extract the repeated Buffer.from(JSON.stringify(...) + '\n') pattern
in the test file into a small helper (e.g., jsonLineBuffer or makeJsonLine) and
replace each call site (for example the child.stdout.emit('data',
Buffer.from(JSON.stringify({ success: true, result: [] }) + '\n')) usages) with
the helper to eliminate string concatenation and satisfy prefer-template; the
helper should accept the object, perform JSON.stringify and append the newline,
and return the Buffer used by child.stdout.emit and other places.
In `@src/model/orchestrator/services/cache/local-cache-service.test.ts`:
- Around line 116-124: The test "should return false and log warning on error"
currently only asserts the boolean result; update it to also assert that the
cache logger receives a warning when readdirSync throws: mock or spy the
logger.warn used by LocalCacheService (e.g., mockLogger.warn or the logger
instance LocalCacheService uses), trigger the error via (mockFs.readdirSync as
jest.Mock).mockImplementation(() => { throw new Error('Permission denied'); }),
then expect the logger.warn to have been called with a message containing the
error or context about failing to restore cache; repeat the same pattern for the
other test at the 233-241 location so both tests verify the logging side-effect
as well as the boolean return value and reset/clear the mock between tests.
In `@src/model/orchestrator/services/lfs/lfs-agent-service.test.ts`:
- Around line 87-93: The test currently deletes process.env.LFS_STORAGE_PATHS so
it can vacuously pass; instead set a sentinel value into
process.env.LFS_STORAGE_PATHS (e.g. originalValue or "SENTINEL"), call
LfsAgentService.configure('/usr/local/bin/agent', '', [], '/repo') with an empty
storagePaths array, and assert that process.env.LFS_STORAGE_PATHS still equals
the sentinel afterward; also preserve and restore the originalValue variable
around the test to avoid side effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c1882fa-fe91-455a-bd82-6b02538a82ae
📒 Files selected for processing (4)
src/model/orchestrator/providers/cli/cli-provider.test.tssrc/model/orchestrator/services/cache/local-cache-service.test.tssrc/model/orchestrator/services/hooks/git-hooks-service.test.tssrc/model/orchestrator/services/lfs/lfs-agent-service.test.ts
Comment on lines
+22
to
+23
| import { spawn } from 'child_process'; | ||
| import CliProvider from './cli-provider'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/model/orchestrator/providers/cli/cli-provider.test.ts | head -40Repository: game-ci/unity-builder
Length of output: 1369
🏁 Script executed:
# Find ESLint configuration files
fd -e '.eslintrc*' -e 'eslintrc.js' -e 'eslintrc.json' -o 'eslint.config' | head -20Repository: game-ci/unity-builder
Length of output: 210
🏁 Script executed:
# Find ESLint configuration files
find . -maxdepth 2 -type f \( -name '.eslintrc*' -o -name 'eslint.config.js' \) 2>/dev/nullRepository: game-ci/unity-builder
Length of output: 81
🏁 Script executed:
cat .eslintrc.jsonRepository: game-ci/unity-builder
Length of output: 2553
Move imports to the top of the module.
Lines 22-23 violate import/first because the imports come after jest.mock() calls. This will fail linting and block the build.
Suggested fix
import { EventEmitter } from 'events';
import { ProviderLoader } from '../provider-loader';
+import { spawn } from 'child_process';
+import CliProvider from './cli-provider';
// Mock child_process
jest.mock('child_process', () => ({
spawn: jest.fn(),
exec: jest.fn(),
}));
@@
-import { spawn } from 'child_process';
-import CliProvider from './cli-provider';📝 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
| import { spawn } from 'child_process'; | |
| import CliProvider from './cli-provider'; | |
| import { EventEmitter } from 'events'; | |
| import { ProviderLoader } from '../provider-loader'; | |
| import { spawn } from 'child_process'; | |
| import CliProvider from './cli-provider'; | |
| // Mock child_process | |
| jest.mock('child_process', () => ({ | |
| spawn: jest.fn(), | |
| exec: jest.fn(), | |
| })); |
🧰 Tools
🪛 ESLint
[error] 22-22: Import in body of module; reorder to top.
(import/first)
[error] 23-23: Import in body of module; reorder to top.
(import/first)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/cli/cli-provider.test.ts` around lines 22 -
23, The import statements for spawn and CliProvider are placed after jest.mock()
calls, violating import/first; move the two imports (the "spawn" import from
'child_process' and the "CliProvider" default import from './cli-provider') to
the very top of the test module so all imports occur before any jest.mock() or
other runtime calls.
| child.emit('close', 0); | ||
|
|
||
| const result = await promise; | ||
| // listResources falls back to empty array when result is missing |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and get its size
wc -l src/model/orchestrator/providers/cli/cli-provider.test.tsRepository: game-ci/unity-builder
Length of output: 126
🏁 Script executed:
# Read the area around line 155 to see the current state
sed -n '150,160p' src/model/orchestrator/providers/cli/cli-provider.test.ts | cat -nRepository: game-ci/unity-builder
Length of output: 491
🏁 Script executed:
# Read the area around line 380 to see the current state
sed -n '375,385p' src/model/orchestrator/providers/cli/cli-provider.test.ts | cat -nRepository: game-ci/unity-builder
Length of output: 632
🏁 Script executed:
# Check for linting configuration files
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" -o -name "package.json" \) | head -20Repository: game-ci/unity-builder
Length of output: 96
🏁 Script executed:
# Check the eslint configuration for lines-around-comment rule
cat .eslintrc.json | head -50Repository: game-ci/unity-builder
Length of output: 1399
Add blank lines before inline comments to satisfy the lines-around-comment linting rule.
The ESLint configuration requires beforeLineComment: true, which mandates blank lines before all line comments. This applies at lines 155 and 380:
Suggested fix
const result = await promise;
+
// listResources falls back to empty array when result is missing
expect(result).toEqual([]);
@@
await promise;
+
// stderr content included in error message if process fails
// Here it succeeds, so we just verify no rejection📝 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
| // listResources falls back to empty array when result is missing | |
| const result = await promise; | |
| // listResources falls back to empty array when result is missing | |
| expect(result).toEqual([]); |
Suggested change
| // listResources falls back to empty array when result is missing | |
| await promise; | |
| // stderr content included in error message if process fails | |
| // Here it succeeds, so we just verify no rejection |
🧰 Tools
🪛 ESLint
[error] 155-155: Expected line before comment.
(lines-around-comment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/cli/cli-provider.test.ts` at line 155, The
linter requires a blank line before inline (end-of-line) comments; add an empty
line immediately above the inline comment that reads "listResources falls back
to empty array when result is missing" and do the same for the other inline
comment occurrence in the CLI provider tests (cli-provider.test.ts) so each line
comment has a blank line before it, ensuring the tests satisfy the
lines-around-comment rule.
Comment on lines
+30
to
+32
| (mockFs.existsSync as jest.Mock).mockImplementation((filePath: string) => { | ||
| return String(filePath).includes('lefthook.yml') && !String(filePath).startsWith('.'); | ||
| }); |
There was a problem hiding this comment.
Tighten lefthook file matcher; current predicate can misclassify .lefthook.yml.
At Line 31 and Line 99, !String(filePath).startsWith('.') is ineffective for absolute paths (e.g. /repo/.lefthook.yml). This weakens test specificity.
Proposed fix
import fs from 'node:fs';
import path from 'node:path';
...
(mockFs.existsSync as jest.Mock).mockImplementation((filePath: string) => {
- return String(filePath).includes('lefthook.yml') && !String(filePath).startsWith('.');
+ return path.basename(String(filePath)) === 'lefthook.yml';
});
...
(mockFs.existsSync as jest.Mock).mockImplementation((filePath: string) => {
- return String(filePath).includes('lefthook.yml') && !String(filePath).startsWith('.');
+ return path.basename(String(filePath)) === 'lefthook.yml';
});Also applies to: 98-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/hooks/git-hooks-service.test.ts` around lines
30 - 32, The existsSync mock currently checks
String(filePath).includes('lefthook.yml') && !String(filePath).startsWith('.')
which misclassifies paths like /repo/.lefthook.yml; update the predicate used in
the mockFs.existsSync implementation to check the basename of the path (e.g.,
use path.basename(filePath) === 'lefthook.yml') or an equivalent regex that
ensures the final filename is exactly lefthook.yml and does not start with a
dot; apply this change to both mock implementations of mockFs.existsSync in
git-hooks-service.test.ts (the one around the earlier mock and the one later in
the file).
src/model/orchestrator/services/hooks/git-hooks-service.test.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,124 @@ | |||
| import fs from 'node:fs'; | |||
| import path from 'node:path'; | |||
There was a problem hiding this comment.
Remove unused path import (lint blocker).
Line 2 imports path but it is never used, which fails no-unused-vars.
Suggested fix
-import path from 'node:path';📝 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
| import path from 'node:path'; |
🧰 Tools
🪛 ESLint
[error] 2-2: 'path' is defined but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/lfs/lfs-agent-service.test.ts` at line 2,
Remove the unused import "path" from the top of the test file to satisfy the
no-unused-vars lint rule: edit
src/model/orchestrator/services/lfs/lfs-agent-service.test.ts and delete the
line importing path from 'node:path' (the import is unused in this file), then
run the linter/tests to confirm the warning is gone.
Comment on lines
+24
to
+26
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
Isolate process.env mutations across tests.
LFS_STORAGE_PATHS is mutated in this suite, but Line 24-Line 26 only clears mocks. Please restore process.env.LFS_STORAGE_PATHS in hooks so tests remain deterministic.
Suggested fix
+let originalLfsStoragePaths: string | undefined;
+
describe('LfsAgentService', () => {
beforeEach(() => {
jest.clearAllMocks();
+ originalLfsStoragePaths = process.env.LFS_STORAGE_PATHS;
});
+
+ afterEach(() => {
+ if (originalLfsStoragePaths === undefined) {
+ delete process.env.LFS_STORAGE_PATHS;
+ } else {
+ process.env.LFS_STORAGE_PATHS = originalLfsStoragePaths;
+ }
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/lfs/lfs-agent-service.test.ts` around lines
24 - 26, The tests mutate process.env.LFS_STORAGE_PATHS but only call
jest.clearAllMocks() in beforeEach; update the test suite to save the original
process.env.LFS_STORAGE_PATHS (e.g., const original =
process.env.LFS_STORAGE_PATHS) and restore it in an afterEach (or restore at
start of beforeEach) so each test runs with a clean env; locate the hooks in
lfs-agent-service.test.ts where beforeEach() is defined and add the save/restore
around process.env.LFS_STORAGE_PATHS to ensure deterministic tests.
First-class support for elastic-git-storage as a custom LFS transfer agent. When lfsTransferAgent is set to "elastic-git-storage" (or "elastic-git-storage@v1.0.0" for a specific version), the service automatically finds or installs the agent from GitHub releases, then configures it via git config. Supports version pinning via @Version suffix in the agent value, eliminating the need for a separate version parameter. Platform and architecture detection handles linux/darwin/windows on amd64/arm64. 37 unit tests covering detection, PATH lookup, installation, version parsing, and configuration delegation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/model/orchestrator/services/lfs/elastic-git-storage-service.test.ts (1)
116-185: Add regression tests for env-empty path fallback and unsafe version input.Please add explicit cases to prevent future regressions around (1) empty
RUNNER_TOOL_CACHE/LOCALAPPDATAyielding relative candidates and (2) rejecting malformedversionvalues before command construction.🧪 Suggested test additions
+ it('should not treat relative RUNNER_TOOL_CACHE fallback as installed', async () => { + const { OrchestratorSystem } = require('../core/orchestrator-system'); + mockOs.platform.mockReturnValue('linux'); + mockOs.homedir.mockReturnValue('/home/runner'); + OrchestratorSystem.Run.mockRejectedValue(new Error('not found')); + + const originalRunnerToolCache = process.env.RUNNER_TOOL_CACHE; + process.env.RUNNER_TOOL_CACHE = ''; + + (mockFs.existsSync as jest.Mock).mockImplementation((candidate: string) => { + return candidate === 'elastic-git-storage/elastic-git-storage'; + }); + + const result = await ElasticGitStorageService.findInstalled(); + expect(result).toBe(''); + + if (originalRunnerToolCache === undefined) delete process.env.RUNNER_TOOL_CACHE; + else process.env.RUNNER_TOOL_CACHE = originalRunnerToolCache; + }); + + it('should reject unsafe version values', async () => { + const { OrchestratorSystem } = require('../core/orchestrator-system'); + mockOs.platform.mockReturnValue('linux'); + mockOs.arch.mockReturnValue('x64'); + mockOs.tmpdir.mockReturnValue('/tmp'); + + const result = await ElasticGitStorageService.install('v1.2.3$(touch /tmp/pwned)'); + expect(result).toBe(''); + expect(OrchestratorSystem.Run).not.toHaveBeenCalled(); + });Also applies to: 187-312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/lfs/elastic-git-storage-service.test.ts` around lines 116 - 185, Add two regression tests: (1) in the suite around ElasticGitStorageService.findInstalled, add a case where RUNNER_TOOL_CACHE and LOCALAPPDATA are set to empty strings and assert returned candidate paths are absolute (no relative paths) and still correctly choose a valid absolute install location by mocking fs.existsSync and os.homedir; (2) add a test for the install/version flow (call the method that accepts a version string, e.g., ElasticGitStorageService.install or the method that builds the download command) that passes malformed/unsafe version inputs (e.g., '../1.0' or shell-meta characters) and assert the code rejects them before invoking OrchestratorSystem.Run (mock OrchestratorSystem.Run and expect it not to be called or expect an error to be thrown), ensuring validation occurs prior to command construction. Ensure tests reset process.env and restore mocks after each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/services/lfs/elastic-git-storage-service.ts`:
- Around line 90-101: The candidates array in findInstalled currently uses
path.join(process.env.RUNNER_TOOL_CACHE || '', ...) and similar, which produces
relative paths when the env vars are empty and allows repo-local hijack; change
these to only add entries when the corresponding env var is truthy (e.g., if
(process.env.RUNNER_TOOL_CACHE)
candidates.push(path.join(process.env.RUNNER_TOOL_CACHE, 'elastic-git-storage',
'elastic-git-storage')) ), and do the same for LOCALAPPDATA and any other
env-based entries (or alternatively use path.resolve on the env value after
checking it's non-empty) so no relative paths are generated; update the
candidates construction in findInstalled accordingly.
- Around line 139-147: Validate the incoming version before constructing
downloadUrl: allow only 'latest' or a strict tag format (e.g. semantic version
like v1.2.3 or 1.2.3 with optional pre-release/metadata) and reject/throw for
anything else. Update the logic that sets releaseTag and builds downloadUrl
(references: releaseTag, downloadUrl, ElasticGitStorageService.REPO_OWNER,
ElasticGitStorageService.REPO_NAME) to first run a regex check (e.g.
^v?\d+\.\d+\.\d+(-[A-Za-z0-9._-]+)?$) and error out if it fails; ensure the same
validation is applied to the code path that later executes the URL via shell
(the code around where downloadUrl is used) so no unvalidated version can reach
a shell invocation.
---
Nitpick comments:
In `@src/model/orchestrator/services/lfs/elastic-git-storage-service.test.ts`:
- Around line 116-185: Add two regression tests: (1) in the suite around
ElasticGitStorageService.findInstalled, add a case where RUNNER_TOOL_CACHE and
LOCALAPPDATA are set to empty strings and assert returned candidate paths are
absolute (no relative paths) and still correctly choose a valid absolute install
location by mocking fs.existsSync and os.homedir; (2) add a test for the
install/version flow (call the method that accepts a version string, e.g.,
ElasticGitStorageService.install or the method that builds the download command)
that passes malformed/unsafe version inputs (e.g., '../1.0' or shell-meta
characters) and assert the code rejects them before invoking
OrchestratorSystem.Run (mock OrchestratorSystem.Run and expect it not to be
called or expect an error to be thrown), ensuring validation occurs prior to
command construction. Ensure tests reset process.env and restore mocks after
each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9288134c-e74d-4e2b-a1a1-f377c08ba618
📒 Files selected for processing (3)
action.ymlsrc/model/orchestrator/services/lfs/elastic-git-storage-service.test.tssrc/model/orchestrator/services/lfs/elastic-git-storage-service.ts
Comment on lines
+90
to
+101
| const candidates = [ | ||
| path.join(process.env.RUNNER_TOOL_CACHE || '', 'elastic-git-storage', 'elastic-git-storage'), | ||
| '/usr/local/bin/elastic-git-storage', | ||
| path.join(os.homedir(), '.local', 'bin', 'elastic-git-storage'), | ||
| ]; | ||
|
|
||
| if (os.platform() === 'win32') { | ||
| candidates.push( | ||
| path.join(process.env.RUNNER_TOOL_CACHE || '', 'elastic-git-storage', 'elastic-git-storage.exe'), | ||
| path.join(process.env.LOCALAPPDATA || '', 'elastic-git-storage', 'elastic-git-storage.exe'), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Prevent repository-relative executable hijack in findInstalled.
Line 91, Line 98, and Line 99 use path.join(env || '', ...); when env vars are empty, this yields relative paths. A repo-local elastic-git-storage/... file can then be selected as “installed” and later configured as the transfer agent.
🔒 Suggested fix
- const candidates = [
- path.join(process.env.RUNNER_TOOL_CACHE || '', 'elastic-git-storage', 'elastic-git-storage'),
- '/usr/local/bin/elastic-git-storage',
- path.join(os.homedir(), '.local', 'bin', 'elastic-git-storage'),
- ];
+ const candidates: string[] = [
+ '/usr/local/bin/elastic-git-storage',
+ path.join(os.homedir(), '.local', 'bin', 'elastic-git-storage'),
+ ];
+ const runnerToolCache = process.env.RUNNER_TOOL_CACHE?.trim();
+ if (runnerToolCache && path.isAbsolute(runnerToolCache)) {
+ candidates.unshift(path.join(runnerToolCache, 'elastic-git-storage', 'elastic-git-storage'));
+ }
if (os.platform() === 'win32') {
- candidates.push(
- path.join(process.env.RUNNER_TOOL_CACHE || '', 'elastic-git-storage', 'elastic-git-storage.exe'),
- path.join(process.env.LOCALAPPDATA || '', 'elastic-git-storage', 'elastic-git-storage.exe'),
- );
+ if (runnerToolCache && path.isAbsolute(runnerToolCache)) {
+ candidates.push(path.join(runnerToolCache, 'elastic-git-storage', 'elastic-git-storage.exe'));
+ }
+ const localAppData = process.env.LOCALAPPDATA?.trim();
+ if (localAppData && path.isAbsolute(localAppData)) {
+ candidates.push(path.join(localAppData, 'elastic-git-storage', 'elastic-git-storage.exe'));
+ }
}Also applies to: 103-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/lfs/elastic-git-storage-service.ts` around
lines 90 - 101, The candidates array in findInstalled currently uses
path.join(process.env.RUNNER_TOOL_CACHE || '', ...) and similar, which produces
relative paths when the env vars are empty and allows repo-local hijack; change
these to only add entries when the corresponding env var is truthy (e.g., if
(process.env.RUNNER_TOOL_CACHE)
candidates.push(path.join(process.env.RUNNER_TOOL_CACHE, 'elastic-git-storage',
'elastic-git-storage')) ), and do the same for LOCALAPPDATA and any other
env-based entries (or alternatively use path.resolve on the env value after
checking it's non-empty) so no relative paths are generated; update the
candidates construction in findInstalled accordingly.
Comment on lines
+139
to
+147
| const releaseTag = version === 'latest' ? 'latest' : version; | ||
| const assetName = `elastic-git-storage_${osName}_${archName}${ext}`; | ||
|
|
||
| let downloadUrl: string; | ||
| if (releaseTag === 'latest') { | ||
| downloadUrl = `https://github.com/${ElasticGitStorageService.REPO_OWNER}/${ElasticGitStorageService.REPO_NAME}/releases/latest/download/${assetName}`; | ||
| } else { | ||
| downloadUrl = `https://github.com/${ElasticGitStorageService.REPO_OWNER}/${ElasticGitStorageService.REPO_NAME}/releases/download/${releaseTag}/${assetName}`; | ||
| } |
There was a problem hiding this comment.
Validate version before building the download command.
Line 139 feeds raw version into downloadUrl, then Line 152 executes that string through a shell command. Malformed input can inject shell syntax. Reject anything except latest or a strict version/tag format before URL construction.
🛡️ Suggested fix
- const releaseTag = version === 'latest' ? 'latest' : version;
+ const normalizedVersion = (version || 'latest').trim();
+ const validVersion =
+ normalizedVersion === 'latest' ||
+ /^v?\d+\.\d+\.\d+(?:[-+._0-9A-Za-z]+)?$/.test(normalizedVersion);
+ if (!validVersion) {
+ OrchestratorLogger.logWarning(
+ `[ElasticGitStorage] Invalid version "${normalizedVersion}". Skipping installation.`,
+ );
+ return '';
+ }
+ const releaseTag = normalizedVersion;Also applies to: 151-153
🧰 Tools
🪛 ESLint
[error] 143-147: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/lfs/elastic-git-storage-service.ts` around
lines 139 - 147, Validate the incoming version before constructing downloadUrl:
allow only 'latest' or a strict tag format (e.g. semantic version like v1.2.3 or
1.2.3 with optional pre-release/metadata) and reject/throw for anything else.
Update the logic that sets releaseTag and builds downloadUrl (references:
releaseTag, downloadUrl, ElasticGitStorageService.REPO_OWNER,
ElasticGitStorageService.REPO_NAME) to first run a regex check (e.g.
^v?\d+\.\d+\.\d+(-[A-Za-z0-9._-]+)?$) and error out if it fails; ensure the same
validation is applied to the code path that later executes the URL via shell
(the code around where downloadUrl is used) so no unvalidated version can reach
a shell invocation.
#777) Implement two-level workspace isolation pattern for enterprise-scale CI: - Atomic O(1) workspace restore via filesystem move (no tar/download/extract) - Separate Library caching for independent restore - .git preservation for delta operations - Stale workspace cleanup with configurable retention policies - 5 new action inputs: childWorkspacesEnabled, childWorkspaceName, childWorkspaceCacheRoot, childWorkspacePreserveGit, childWorkspaceSeparateLibrary - 28 unit tests covering all service methods This enables enterprise CI where workspaces are 50GB+ and traditional caching via actions/cache is impractical. On NTFS, workspace restore is O(1) via atomic rename when source and destination are on the same volume. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
Child Workspace Isolation (new commit: 007852a)Added New Action Inputs
How It Works
Test Coverage28 unit tests covering: workspace restore, fresh workspace, Library caching, workspace save, Files Changed
|
Prevent builds from hanging indefinitely when CLI provider subprocess is unresponsive. Default 2h for runTaskInWorkflow, 1h for watchWorkflow. Graceful SIGTERM with 10s grace before SIGKILL. - Added RUN_TASK_TIMEOUT_MS (2 hours) and WATCH_WORKFLOW_TIMEOUT_MS (1 hour) - Added gracefulKill helper: SIGTERM first, SIGKILL after 10s grace period - runTaskInWorkflow and watchWorkflow now have timeout protection - Existing execute() method upgraded to use gracefulKill - core.error() called with clear human-readable timeout message - Added comprehensive tests: timeout triggers, SIGKILL escalation, grace period cancellation on voluntary exit, normal completion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The monolithic orchestrator-integrity workflow runs 25+ tests sequentially in a single job, consistently hitting the 60-minute timeout on PR runs. Split into 4 parallel jobs (k8s, aws-provider, local-docker, rclone) each on its own runner, cutting wall-clock time from 3+ hours to ~1 hour and eliminating disk space exhaustion from shared runner contention. Adopts the parallel architecture from PR #809. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 5, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the else branch that actively called GitHooksService.disableHooks() for every user where gitHooksEnabled was false (the default). This was a breaking change that silently modified core.hooksPath to point at an empty directory, disabling any existing git hooks (husky, lefthook, pre-commit, etc.). When gitHooksEnabled is false (default), the action now does nothing regarding hooks — exactly matching the behavior on main before the hooks feature was added. The hooks feature only activates when users explicitly set gitHooksEnabled: true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eatures Add three test files covering the two highest-priority gaps in PR #777: 1. src/index-enterprise-features.test.ts (21 tests) - Integration wiring tests for index.ts that verify conditional gating of all enterprise services (GitHooks, LocalCache, ChildWorkspace, SubmoduleProfile, LfsAgent). Tests that disabled features (default) are never invoked, enabled features call the correct service methods, and the order of operations is correct (restore before build, save after build). Also tests non-local provider strategy skips all enterprise features. 2. src/model/enterprise-inputs.test.ts (103 tests) - Input/BuildParameters wiring tests for all 20 new enterprise properties. Covers defaults, explicit values, and boolean string parsing edge cases (the #1 source of bugs: 'false' as truthy, 'TRUE' case sensitivity, '1', 'yes'). Verifies BuildParameters.create() correctly maps all Input getters. 3. src/model/orchestrator/services/submodule/submodule-profile-service.test.ts (5 new tests) - Command construction safety tests for execute(), documenting how paths, branches, and tokens are passed into git commands and verifying the expected command strings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
frostebite
added a commit
that referenced
this pull request
Mar 6, 2026
…support' into release/lts-infrastructure Merges latest commits from PR #777 (git hooks fix + 129 new tests). Resolved merge conflicts: - dist/index.js, dist/index.js.map: rebuilt from merged source - src/model/input.ts: fixed 3 missing closing braces from merge - src/model/orchestrator/options/orchestrator-options.ts: fixed 1 missing closing brace from merge
frostebite
added a commit
that referenced
this pull request
Mar 6, 2026
…support' into release/lts-2.0.0 Merges latest commits from PR #777 (git hooks fix + 129 new tests). Resolved merge conflicts: - dist/index.js, dist/index.js.map: rebuilt from merged source - src/model/input.ts: fixed 5 missing closing braces and return statements from merge
Add a middleware system that wraps around build pipeline phases with before/after semantics, built on the existing command hook and container hook fundamentals. Middleware supports rich trigger conditions (phase, provider, platform, environment expressions), priority-based ordering with wrapping semantics, and both inline YAML and file-based definitions from game-ci/middleware/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use the Checks API to flip failed macOS build conclusions to neutral (gray dash) so unstable builds don't show red X marks on PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GitHub deprecated external check run conclusion updates (Feb 2025), so the Checks API approach to mark builds as neutral no longer works. Instead, move continue-on-error from the job level to the build step. The job always succeeds (green check), failed builds emit a warning annotation, and upload is skipped on failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Stop modifying the macOS build workflow — leave it identical to main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
Closing — all orchestrator code has been extracted to the standalone Content from this PR (CLI providers, caching, LFS, hooks, middleware pipeline) is fully present in the orchestrator repo. See PR #819 for the extraction. |
12 tasks
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
Enterprise support features for Orchestrator — submodule profiles, local caching, child workspaces, LFS transfer agent, git hooks, and CLI provider. All features are opt-in with safe defaults — existing workflows are completely unaffected. 19 new action inputs, 7 new service modules.
Features
1. Submodule Profiles
Selectively initialize submodules from a YAML config instead of cloning everything. Supports glob patterns, variant overlays, and token auth. Essential for monorepos where builds only need a subset of submodules.
Inputs:
submoduleProfilePath,submoduleVariantPath,submoduleToken2. Local Caching
Workspace-level caching with atomic moves. Caches the Unity Library folder and LFS objects between local builds without
actions/cache. Filesystem-based, keyed by platform/version/branch.Inputs:
localCacheEnabled,localCacheRoot,localCacheLibrary,localCacheLfs3. Child Workspaces
Isolated build workspaces for multi-target CI builds. Each workspace gets its own Library folder with optional cache root, git config preservation, and separate Library folder support. Prevents builds from interfering with each other in monorepo environments.
Inputs:
childWorkspacesEnabled,childWorkspaceName,childWorkspaceCacheRoot,childWorkspacePreserveGit,childWorkspaceSeparateLibrary4. LFS Transfer Agent
Custom Git LFS transfer agent support with first-class elastic-git-storage integration. Configures
git config lfs.customtransfer.*automatically. SetlfsTransferAgent: elastic-git-storagefor auto-download and configuration; append@versionfor release pinning (e.g.,elastic-git-storage@v1.0.0).Inputs:
lfsTransferAgent,lfsTransferAgentArgs,lfsStoragePaths5. Git Hooks
Unity git hooks support via lefthook or husky, with built-in Unity Git Hooks integration. Opt-in only — does NOT modify hooks when disabled (the default). When enabled, auto-detects
com.frostebite.unitygithooksinPackages/manifest.json, runs init scripts, and sets CI-friendly env vars.Inputs:
gitHooksEnabled,gitHooksSkipList,gitHooksRunBeforeBuild6. CLI Provider
Run builds via external CLI executables written in any language (Go, Python, Rust, shell). JSON on stdin/stdout protocol, executable path via
providerExecutableinput. No TypeScript required.Input:
providerExecutableNew Inputs (19 total)
submoduleProfilePathsubmoduleVariantPathsubmoduleTokenlocalCacheEnabledlocalCacheRootlocalCacheLibrarylocalCacheLfschildWorkspacesEnabledchildWorkspaceNamechildWorkspaceCacheRootchildWorkspacePreserveGitchildWorkspaceSeparateLibrarylfsTransferAgentelastic-git-storagefor built-in support, or any executable path;@versionfor release pinning)lfsTransferAgentArgslfsStoragePathsgitHooksEnabledgitHooksSkipListgitHooksRunBeforeBuildpre-commit,pre-push)providerExecutableNew Services (7 total)
SubmoduleProfileServiceservices/submodule/.gitmodules, generate init plans, execute selective submodule initLocalCacheServiceservices/cache/ChildWorkspaceServiceservices/workspace/LfsAgentServiceservices/lfs/git config, validate executables, configure storage pathsGitHooksServiceservices/hooks/CliProviderproviders/cli/ProviderInterface— spawns external executable per method callSecretSourceServiceservices/secrets/Breaking Changes
None. All features are opt-in with safe defaults. Default behavior is identical to current main.
The git hooks feature was audited and fixed (commit
7b8f109) to ensure it does not touchcore.hooksPathwhen disabled (the default). The original implementation had anelsebranch that calledGitHooksService.disableHooks()for every build wheregitHooksEnabledwas false — this would have silently pointedcore.hooksPathat an empty directory, breaking any existing git hooks (husky, lefthook, pre-commit, etc.) for all users. This was caught and removed before merge.Test Coverage
182 existing service unit tests (all passing).
129 new tests added across three files:
src/index-enterprise-features.test.tssrc/model/enterprise-inputs.test.ts'false'as truthy,'TRUE'case sensitivity,'1','yes')src/model/orchestrator/services/submodule/submodule-profile-service.test.tsTotal for this PR's functionality: 311 tests. Full suite: 518 tests across all test files.
Documentation
Tracking