Next-Gen Features -- Combined Integration#814
Conversation
Initial scaffold for the test workflow engine service directory. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Initial scaffold for the runner registration and hot editor provider module. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…, and collection service Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ut, and storage-backed sync Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add scheduled workflow that validates community Unity packages compile and build correctly using unity-builder. Runs weekly on Sunday. Includes: - YAML plugin registry (community-plugins.yml) for package listings - Matrix expansion across plugins and platforms - Automatic failure reporting via GitHub issues - Manual trigger with plugin filter and Unity version override Co-Authored-By: Claude Opus 4.6 <[email protected]>
… monitoring, and job dispatch (#791) Adds persistent Unity editor instance support to reduce build iteration time by eliminating cold-start overhead. Includes: - HotRunnerTypes: interfaces for config, status, job request/result, transport - HotRunnerRegistry: in-memory runner management with file-based persistence - HotRunnerHealthMonitor: periodic health checks, idle recycling, job-count recycling - HotRunnerDispatcher: job routing with wait-for-runner, timeout, and output streaming - HotRunnerService: high-level API integrating registry, health, and dispatch - 34 unit tests covering registration, filtering, health, dispatch, timeout, fallback - action.yml inputs for hot runner configuration (7 new inputs) - Input/BuildParameters integration for hot runner settings - index.ts wiring with cold-build fallback when hot runner unavailable Co-Authored-By: Claude Opus 4.6 <[email protected]>
…s, tests, and action integration (#798) - Add ArtifactUploadHandler with support for github-artifacts, storage (rclone), and local copy upload targets, including large file chunking for GitHub Artifacts - Add 44 unit tests covering OutputTypeRegistry, OutputService, and ArtifactUploadHandler (config parsing, upload coordination, file collection) - Add 6 new action.yml inputs for artifact configuration - Add artifactManifestPath action output - Wire artifact collection and upload into index.ts post-build flow Co-Authored-By: Claude Opus 4.6 <[email protected]>
…omy filtering, and structured results (#790) Co-Authored-By: Claude Opus 4.6 <[email protected]>
…te management, and tests (#799) - Add storage-pull strategy: rclone-based sync from remote storage with overlay and clean modes, URI parsing (storage://remote:bucket/path), transfer parallelism, and automatic rclone availability checking - Add SyncStateManager: persistent state load/save with configurable paths, workspace hash calculation via SHA-256 of key project files, and drift detection for external modification awareness - Add action.yml inputs: syncStrategy, syncInputRef, syncStorageRemote, syncRevertAfter, syncStatePath with sensible defaults - Wire sync into Input (5 getters), BuildParameters (5 fields), index.ts (local build path), and RemoteClient (orchestrator path) with post-job overlay revert when syncRevertAfter is true - Add 42 unit tests covering all strategies, URI parsing, state management, hash calculation, drift detection, error handling, and edge cases (missing rclone, invalid URIs, absent state, empty diffs) - Add root:true to eslintrc to prevent plugin resolution conflicts Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace execSync with promisified exec so Promise.all actually runs test groups in parallel. Add native timeout support via exec options. Add 50MB maxBuffer for large Unity output. Fix ESLint violations (variable naming, padding lines, array push consolidation). Co-Authored-By: Claude Opus 4.6 <[email protected]>
Check for rclone binary before attempting storage-based uploads. Validate storage destination URI format (remoteName:path). Provide clear error message with install link when rclone is missing. Fail gracefully instead of cryptic ENOENT crash. Co-Authored-By: Claude Opus 4.6 <[email protected]>
… safeguards Validate runner entries when loading from hot-runners.json. Discard corrupted entries with warnings. Add validateAndRepair() method for runtime recovery. Validate data before persisting to prevent writing corrupt state. Handle corrupt persistence files (invalid JSON) gracefully. Rewrite executeWithTimeout using Promise.race to clean up transport connections on timeout. Fix pre-existing ESLint violations in dispatcher and test files. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
…commands Introduces a yargs-based CLI entry point (src/cli.ts) distributed as the `game-ci` command. The CLI reuses existing unity-builder modules — Input, BuildParameters, Orchestrator, Docker, MacBuilder — so the same build engine powers both the GitHub Action and the standalone CLI. Commands: build, activate, orchestrate, cache (list/restore/clear), status, version. Closes #812 Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add .github/workflows/publish-cli.yml for publishing the CLI to npm on release or via manual workflow_dispatch with dry-run support. Add comprehensive test coverage for the CLI: - input-mapper.test.ts: 16 tests covering argument mapping, boolean conversion, yargs internal property filtering, and Cli.options population - commands.test.ts: 26 tests verifying command exports, builder flags, default values, and camelCase aliases for all six commands - cli-integration.test.ts: 8 integration tests spawning the CLI process to verify help output, version info, and error handling Co-Authored-By: Claude Opus 4.6 <[email protected]>
…mand Replace the npm-only publish-cli.yml with a comprehensive release-cli.yml that builds standalone binaries via pkg for all platforms (Linux/macOS/Windows, x64/arm64), uploads them as GitHub Release assets with SHA256 checksums, and retains npm publish as an optional job. Add curl-pipe-sh installer (install.sh) and PowerShell installer (install.ps1) for one-liner installation from GitHub Releases. Both scripts auto-detect platform/architecture, verify checksums, and guide PATH configuration. Add `game-ci update` command for self-updating standalone binaries: checks GitHub releases for newer versions, downloads the correct platform binary, verifies it, and atomically replaces the running executable. Distribution strategy: GitHub Releases (primary), npm (optional), with winget/Homebrew/Chocolatey/Scoop as future providers. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…safety - Add process.exit(1) in cli.ts catch block so failures produce non-zero exit codes - Add 6 missing build inputs: containerRegistryRepository, containerRegistryImageVersion, dockerIsolationMode, sshPublicKeysDirectoryPath, cacheUnityInstallationOnMac, unityHubVersionOnMac - Add 6 missing orchestrate inputs: kubeStorageClass, readInputFromOverrideList, readInputOverrideCommand, postBuildSteps, preBuildSteps, customJob - Fix activate command description to accurately reflect verification behavior - Add null check before accessing result.BuildResults in orchestrate handler Co-Authored-By: Claude Opus 4.6 <[email protected]>
…to release/next-gen
…o release/next-gen # Conflicts: # dist/index.js # dist/index.js.map # src/index.ts # src/model/build-parameters.ts # src/model/input.ts
… into release/next-gen # Conflicts: # dist/index.js # dist/index.js.map # src/index.ts # src/model/build-parameters.ts
…l' into release/next-gen # Conflicts: # action.yml # dist/index.js # dist/index.js.map # src/index.ts # src/model/build-parameters.ts
…ion' into release/next-gen # Conflicts: # dist/index.js.map
…e/next-gen # Conflicts: # dist/index.js.map
frostebite
added
enhancement
|
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 a new CLI and command modules, hot-runner subsystem, incremental sync, test workflow, artifact/output system, four orchestrator providers, installers/workflows/scripts, many tests, and numerous Input/BuildParameters getters — plus unresolved merge conflicts in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Mapper as Input Mapper
participant Index as index.ts
participant Sync as IncrementalSyncService
participant Hot as HotRunnerService
participant Orch as Orchestrator/Build
participant Output as OutputService
participant Artifact as ArtifactUploadHandler
CLI->>Mapper: parse args
Mapper->>Index: mapCliArgumentsToInput()
alt testSuitePath set
Index->>Orch: execute TestWorkflowService
Orch-->>Index: TestResult[]
end
alt syncStrategy configured
Index->>Sync: applySyncStrategy()
Sync-->>Index: workspace synced
end
alt hotRunnerEnabled
Index->>Hot: initialize()
Index->>Hot: submitBuild(params)
Hot-->>Index: job result
Index->>Hot: shutdown()
else cold/orchestrated build
Index->>Orch: run build (local/orchestrator)
Orch-->>Index: exit code
end
Index->>Output: collectOutputs()
Output-->>Index: OutputManifest
Index->>Artifact: uploadArtifacts()
Artifact-->>Index: UploadResult
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| private buildPwshCommand(script: string): string { | ||
| return `pwsh -NoProfile -NonInteractive -Command "${script.replace(/"/g, '\\"')}"`; |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
In general, the fix is to ensure that when we embed an arbitrary string into a double-quoted PowerShell command string, we escape both backslashes and double quotes in a consistent, order-aware way. The usual safe pattern is: first escape backslashes, then escape double quotes in the already backslash-escaped string, so that we do not end up reinterpreting newly added backslashes.
For buildPwshCommand, instead of only calling script.replace(/"/g, '\\"'), we should first replace each backslash (\) with a double backslash (\\), and then escape each double quote by prefixing it with a backslash. That yields something like:
const escapedScript = script.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
return `pwsh -NoProfile -NonInteractive -Command "${escapedScript}"`;This keeps the existing semantics (embedding the same script text into the -Command argument) but ensures that any backslashes already present in script do not accidentally combine with quotes to terminate the string early or otherwise break parsing. All required functionality can be implemented inline in this method; no new imports or helpers are necessary. No changes are needed to buildInvokeCommand for this specific alert, since CodeQL reported line 140 only, and we must avoid speculative changes.
| @@ -137,7 +137,8 @@ | ||
| } | ||
|
|
||
| private buildPwshCommand(script: string): string { | ||
| return `pwsh -NoProfile -NonInteractive -Command "${script.replace(/"/g, '\\"')}"`; | ||
| const escapedScript = script.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); | ||
| return `pwsh -NoProfile -NonInteractive -Command "${escapedScript}"`; | ||
| } | ||
|
|
||
| private buildInvokeCommand(remoteScript: string): string { |
| } | ||
|
|
||
| private buildInvokeCommand(remoteScript: string): string { | ||
| const escapedScript = remoteScript.replace(/"/g, '\\"').replace(/'/g, "''"); |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
In general, the problem is that the manual escaping of metacharacters is incomplete: it escapes double and single quotes but not backslashes. To avoid inconsistent states like \" becoming \\\" (double-escaped backslash, singly escaped quote) or leaving quote characters effectively unescaped after the shell and PowerShell re-parse the string, we should first escape backslashes and then escape quotes. This way, any backslash present in the original input is preserved as a literal backslash in the final command string, and quotes cannot be “unescaped” by preceding them with a backslash.
The best way to fix this specific instance, without changing the external behavior, is to adjust the escapedScript computation in buildInvokeCommand so that it (1) escapes backslashes, (2) escapes double quotes, and (3) keeps the existing treatment for single quotes. That means replacing the line:
const escapedScript = remoteScript.replace(/"/g, '\\"').replace(/'/g, "''");with a version that first escapes \ characters globally, then double quotes globally, then single quotes globally, for example:
const escapedScript = remoteScript
.replace(/\\/g, '\\\\')
.replace(/"/g, '\\"')
.replace(/'/g, "''");No new imports or helper methods are required; the change is limited to the buildInvokeCommand method in src/model/orchestrator/providers/remote-powershell/index.ts. All surrounding code (including the similar buildPwshCommand method) remains unchanged, because CodeQL only flagged the direct manipulation of remoteScript in buildInvokeCommand, and we must avoid altering behavior beyond what is necessary.
| @@ -141,7 +141,10 @@ | ||
| } | ||
|
|
||
| private buildInvokeCommand(remoteScript: string): string { | ||
| const escapedScript = remoteScript.replace(/"/g, '\\"').replace(/'/g, "''"); | ||
| const escapedScript = remoteScript | ||
| .replace(/\\/g, '\\\\') | ||
| .replace(/"/g, '\\"') | ||
| .replace(/'/g, "''"); | ||
|
|
||
| if (this.transport === 'ssh') { | ||
| return `pwsh -NoProfile -NonInteractive -Command "Invoke-Command -HostName '${this.host}' -ScriptBlock { ${escapedScript} }"`; |
| name: Build ${{ matrix.target }} | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| include: | ||
| - target: linux-x64 | ||
| os: ubuntu-latest | ||
| pkg-target: node20-linux-x64 | ||
| binary-name: game-ci-linux-x64 | ||
| - target: linux-arm64 | ||
| os: ubuntu-latest | ||
| pkg-target: node20-linux-arm64 | ||
| binary-name: game-ci-linux-arm64 | ||
| - target: macos-x64 | ||
| os: macos-latest | ||
| pkg-target: node20-macos-x64 | ||
| binary-name: game-ci-macos-x64 | ||
| - target: macos-arm64 | ||
| os: macos-latest | ||
| pkg-target: node20-macos-arm64 | ||
| binary-name: game-ci-macos-arm64 | ||
| - target: windows-x64 | ||
| os: windows-latest | ||
| pkg-target: node20-win-x64 | ||
| binary-name: game-ci-windows-x64.exe | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.release.tag_name || inputs.tag || github.ref }} | ||
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
|
|
||
| - name: Install dependencies | ||
| run: yarn install --frozen-lockfile | ||
|
|
||
| - name: Build TypeScript | ||
| run: yarn build | ||
|
|
||
| - name: Verify CLI before packaging | ||
| run: node lib/cli.js version | ||
|
|
||
| - name: Build standalone binary | ||
| run: npx pkg lib/cli.js --target ${{ matrix.pkg-target }} --output ${{ matrix.binary-name }} --compress GZip | ||
|
|
||
| - name: Verify standalone binary (non-cross-compiled) | ||
| if: | | ||
| (matrix.target == 'linux-x64' && runner.os == 'Linux') || | ||
| (matrix.target == 'macos-arm64' && runner.os == 'macOS' && runner.arch == 'ARM64') || | ||
| (matrix.target == 'macos-x64' && runner.os == 'macOS' && runner.arch == 'X64') || | ||
| (matrix.target == 'windows-x64' && runner.os == 'Windows') | ||
| run: ./${{ matrix.binary-name }} version | ||
| shell: bash | ||
|
|
||
| - uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: binary-${{ matrix.target }} | ||
| path: ${{ matrix.binary-name }} | ||
| retention-days: 5 | ||
|
|
||
| create-checksums-and-upload: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 15 days ago
In general, the fix is to explicitly define permissions: for each job (or at the workflow root) to avoid relying on repository defaults. For the build-binaries job, it only needs to read source code and use artifacts; it does not need to write to repository contents or perform other privileged operations. Therefore, we can safely grant it contents: read as a minimal token permission.
The best targeted fix is to add a permissions: section under the build-binaries job definition, right after the runs-on: or before strategy:. We’ll set contents: read because the job checks out the repository and builds binaries but does not modify GitHub resources. Other jobs already have their own explicit permissions blocks and do not need changes.
Concretely, in .github/workflows/release-cli.yml, under jobs: build-binaries:, add:
permissions:
contents: readindented to align with other job-level keys. No imports or additional definitions are required, and this change does not affect existing functionality.
| @@ -23,6 +23,8 @@ | ||
| build-binaries: | ||
| name: Build ${{ matrix.target }} | ||
| runs-on: ${{ matrix.os }} | ||
| permissions: | ||
| contents: read | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟡 Minor comments (9)
src/model/orchestrator/runners/README.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorReplace placeholder issue text with a concrete link.
Line 5 says “See GitHub Issue” but does not provide a resolvable reference, so readers can’t find the protocol spec directly.
Suggested patch
-See GitHub Issue for full specification. +See GitHub Issue `#795` for full specification: https://github.com/game-ci/unity-builder/issues/795🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/runners/README.md` at line 5, Replace the placeholder text "See GitHub Issue for full specification." with a concrete, resolvable link to the actual GitHub issue or PR (e.g. a full HTTPS URL or a markdown link like [Issue `#123`](https://github.com/owner/repo/issues/123)); update the README string where it appears so readers can navigate directly to the protocol spec and include a short context/issue title in the link text for clarity..github/workflows/release-cli.yml-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorDispatch input description does not match implemented behavior.
Line 9 says “Uses latest release if empty,” but Lines 116-118 explicitly set an empty tag and skip upload. Please align the description or implement latest-tag lookup.
💡 Proposed fix (description-only)
- description: 'Release tag to build (e.g., v2.0.0). Uses latest release if empty.' + description: 'Release tag to build/upload (e.g., v2.0.0). Leave empty to build current ref without release upload.'Also applies to: 109-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-cli.yml at line 9, The dispatch input description claims an empty value uses the latest release, but the workflow contains a conditional that sets an empty tag and skips upload (the step that checks for tag == "" and skips upload); either update the input description to state that an empty value will skip upload, or implement latest-release lookup: add a pre-step (e.g., using actions/github-script or gh) that calls the GitHub API (octokit.rest.repos.getLatestRelease) to fetch the latest release tag and sets the workflow variable/output (the TAG env/output used later) when the dispatch input is empty so subsequent steps use that tag instead of skipping upload..github/workflows/validate-community-plugins.yml-126-140 (1)
126-140:⚠️ Potential issue | 🟡 MinorQuote
$GITHUB_STEP_SUMMARYand use one redirect block.Line 126 script writes repeatedly with unquoted
$GITHUB_STEP_SUMMARY, matching shellcheck SC2086/SC2129 warnings.💡 Proposed fix
- STATUS="${{ steps.build.outcome }}" - echo "## ${{ matrix.name }} — ${{ matrix.platform }}" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - if [ "$STATUS" = "success" ]; then - echo "✅ **PASSED** — Compiled and built successfully" >> $GITHUB_STEP_SUMMARY - else - echo "❌ **FAILED** — Build or compilation failed" >> $GITHUB_STEP_SUMMARY - fi - echo "" >> $GITHUB_STEP_SUMMARY - echo "- Unity: ${{ matrix.unity }}" >> $GITHUB_STEP_SUMMARY - echo "- Platform: ${{ matrix.platform }}" >> $GITHUB_STEP_SUMMARY - echo "- Source: ${{ matrix.source }}" >> $GITHUB_STEP_SUMMARY - echo "- Package: \`${{ matrix.package }}\`" >> $GITHUB_STEP_SUMMARY + STATUS="${{ steps.build.outcome }}" + { + echo "## ${{ matrix.name }} — ${{ matrix.platform }}" + echo "" + if [ "$STATUS" = "success" ]; then + echo "✅ **PASSED** — Compiled and built successfully" + else + echo "❌ **FAILED** — Build or compilation failed" + fi + echo "" + echo "- Unity: ${{ matrix.unity }}" + echo "- Platform: ${{ matrix.platform }}" + echo "- Source: ${{ matrix.source }}" + echo "- Package: \`${{ matrix.package }}\`" + } >> "$GITHUB_STEP_SUMMARY"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/validate-community-plugins.yml around lines 126 - 140, The workflow repeatedly appends to $GITHUB_STEP_SUMMARY without quoting and with multiple redirects, triggering shellcheck SC2086/SC2129; change the block that builds the summary to use a single here-doc redirected once into "$GITHUB_STEP_SUMMARY" and quote variable expansions (e.g., "${{ matrix.name }}", "${{ matrix.platform }}", "${{ matrix.unity }}", "${{ matrix.source }}", "${{ matrix.package }}") inside the here-doc so all uses of GITHUB_STEP_SUMMARY are written in one atomic, quoted redirect and all matrix variables are safely quoted.src/model/orchestrator/services/test-workflow/test-result-reporter.ts-279-281 (1)
279-281:⚠️ Potential issue | 🟡 MinorSanitize
]]>in stack traces before writing CDATA.Line 280 can emit invalid XML when a stack trace contains
]]>.💡 Proposed fix
lines.push(` <failure message="${TestResultReporter.escapeXml(failure.message)}">`); if (failure.stackTrace) { - lines.push(` <![CDATA[${failure.stackTrace}]]>`); + const safeStackTrace = failure.stackTrace.replace(/]]>/g, ']]]]><![CDATA[>'); + lines.push(` <![CDATA[${safeStackTrace}]]>`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test-workflow/test-result-reporter.ts` around lines 279 - 281, Sanitize failure.stackTrace before embedding it into CDATA: replace any occurrence of "]]>" with "]]]]><![CDATA[>" (or equivalent CDATA-safe split) so the resulting string cannot prematurely close the CDATA section, then use the sanitized value in the lines.push call that currently does lines.push(` <![CDATA[${failure.stackTrace}]]>`); — update the code that references failure.stackTrace in test-result-reporter.ts to perform this replacement prior to pushing the CDATA-wrapped line.src/cli.ts-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorRemove unused ESLint disable directive.
The disable comment is no longer needed and triggers
eslint-comments/no-unused-disable.Proposed cleanup diff
-// eslint-disable-next-line import/no-unresolved import { hideBin } from 'yargs/helpers';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` at line 4, Remove the now-unused ESLint disable directive "// eslint-disable-next-line import/no-unresolved" from src/cli.ts; locate the exact comment in the top of the file and delete it so eslint-comments/no-unused-disable is no longer triggered and normal eslint import/no-unresolved rules apply.src/cli/__tests__/input-mapper.test.ts-5-8 (1)
5-8:⚠️ Potential issue | 🟡 MinorReset
GitHub.githubInputEnabledbetween tests to avoid global-state leakage.This suite mutates global state but only resets
Cli.options, which can affect other test files.Proposed test isolation fix
afterEach(() => { jest.restoreAllMocks(); Cli.options = undefined; + GitHub.githubInputEnabled = true; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/__tests__/input-mapper.test.ts` around lines 5 - 8, The test teardown in afterEach currently restores mocks and resets Cli.options but misses resetting the global GitHub.githubInputEnabled flag; update the afterEach block (the one calling jest.restoreAllMocks() and setting Cli.options = undefined) to also reset GitHub.githubInputEnabled (e.g., set it back to undefined or its default boolean) so the global state is cleared between tests and prevents leakage across suites.src/model/orchestrator/services/output/output-manifest.ts-6-8 (1)
6-8:⚠️ Potential issue | 🟡 MinorFix lint failures for comment spacing (Line 7 and Line 33).
ESLint reports missing blank lines before those comments; this will keep lint red until addressed.
💡 Proposed fix
export interface OutputEntry { + /** Output type identifier (e.g., 'build', 'test-results', 'images') */ type: string; @@ export interface OutputManifest { + /** Unique build identifier */ buildGuid: string;Also applies to: 32-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/output/output-manifest.ts` around lines 6 - 8, The lint failures are due to missing blank lines before inline comment blocks in this module; update the comment spacing by inserting a blank line immediately before the JSDoc-style comment that documents the OutputEntry.type and before the later comment block around the other export (the comment near the symbol referenced on lines ~32–34), ensuring each comment has a preceding empty line to satisfy ESLint's comment-spacing rule while leaving the comment text and surrounding code unchanged.src/model/orchestrator/services/test-workflow/test-workflow-service.ts-65-69 (1)
65-69:⚠️ Potential issue | 🟡 MinorValidate
testResultFormatat runtime before casting.The cast on Line 68 bypasses type safety for runtime inputs; invalid values should be normalized or rejected explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test-workflow/test-workflow-service.ts` around lines 65 - 69, The code casts parameters.testResultFormat to 'junit' | 'json' | 'both' without runtime validation; update the TestWorkflow handler to validate/normalize the value in parameters.testResultFormat (e.g., check it equals 'junit', 'json', or 'both') before calling TestResultReporter.writeResults, and if invalid either map to a default ('json' or 'both') or throw/log a clear error and skip writing; use the validated variable (e.g., resultFormatValidated) when invoking TestResultReporter.writeResults to avoid unsafe casting.src/model/orchestrator/providers/ansible/index.ts-103-109 (1)
103-109:⚠️ Potential issue | 🟡 MinorThe
ansibleExtraVarsfallback message is inaccurate.On JSON parse failure, the code logs “using as-is” but actually discards user extra vars. Either fail fast with a clear error or implement a real raw-string fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/ansible/index.ts` around lines 103 - 109, The current catch for JSON.parse(this.extraVariables) logs "[Ansible] Failed to parse ansibleExtraVars as JSON, using as-is" but actually discards the input; change this to fail-fast with a clear error: when JSON.parse throws, log the parse error including the offending this.extraVariables and then throw a new Error (or rethrow) so callers (e.g., the method containing this.extraVariables parsing and playbookVariables assignment) cannot proceed with invalid input; update OrchestratorLogger.logWarning to include the error message and the raw this.extraVariables and ensure playbookVariables is not modified on parse failure.
🧹 Nitpick comments (9)
src/model/orchestrator/providers/gitlab-ci/index.ts (2)
103-122: Consider adding a maximum timeout to the polling loop.The polling loop continues indefinitely until a terminal status is reached. If the GitLab API consistently returns errors or the pipeline hangs in a non-terminal state, this could block forever.
⏱️ Proposed fix to add a timeout
// Poll until completion let status = 'pending'; const terminalStatuses = new Set(['success', 'failed', 'canceled', 'skipped']); + const maxWaitMs = 4 * 60 * 60 * 1000; // 4 hours max + const startTime = Date.now(); while (!terminalStatuses.has(status)) { + if (Date.now() - startTime > maxWaitMs) { + throw new Error(`Pipeline ${this.pipelineId} timed out after 4 hours`); + } + await new Promise((resolve) => setTimeout(resolve, 15_000));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/gitlab-ci/index.ts` around lines 103 - 122, The polling loop using status / terminalStatuses that calls OrchestratorSystem.Run to check this.pipelineId can hang indefinitely; add a maximum timeout (or max attempts) variable and break the loop when exceeded, logging an error via OrchestratorLogger.logWarning (or log) and returning/throwing appropriately; implement the timeout check inside the while loop (incrementing attempt counter or comparing elapsed time before awaiting) so that if OrchestratorSystem.Run keeps failing or status never becomes terminal you stop polling and surface a clear error for the caller.
173-173: Use primitive typenumberinstead of wrapper typeNumber.TypeScript style guides recommend using lowercase primitive types (
number,string,boolean) rather than their wrapper object types (Number,String,Boolean).✏️ Proposed fix
async garbageCollect( // eslint-disable-next-line no-unused-vars filter: string, // eslint-disable-next-line no-unused-vars previewOnly: boolean, // eslint-disable-next-line no-unused-vars - olderThan: Number, + olderThan: number, // eslint-disable-next-line no-unused-vars fullCache: boolean, // eslint-disable-next-line no-unused-vars baseDependencies: boolean, ): Promise<string> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/gitlab-ci/index.ts` at line 173, The property olderThan is typed using the wrapper `Number`; change it to the primitive `number` to follow TypeScript style (replace `olderThan: Number` with `olderThan: number`) wherever this property is declared (e.g., in the relevant interface/type in the GitLab CI orchestrator provider code) and run type-check to ensure no other uses rely on the wrapper type.delete-me-update-all-integration-branches.ps1 (1)
80-80: Remove unused git-result variables.
$refExistsand$resultare assigned but never consumed; this adds noise without value.Also applies to: 95-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@delete-me-update-all-integration-branches.ps1` at line 80, Remove the unused git-result variables by eliminating the assignments to $refExists and $result; instead run the git commands (e.g., git rev-parse --verify $remoteBranch and the other git invocation) without capturing their output into unused variables, or if you need to check success use the command's exit code directly. Update occurrences that assign to $refExists and $result so they no longer create unused variables (references: $refExists, $result, remoteBranch).src/model/orchestrator/providers/github-actions/index.ts (1)
262-263: Preserve error context inwatchWorkflowfailures.Returning
''hides operational failures; return a message with the run id/error for troubleshooting.🛠️ Suggested tweak
- } catch { - return ''; + } catch (error: any) { + return `Failed to watch run ${this.runId}: ${error.message || error}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/github-actions/index.ts` around lines 262 - 263, The catch in watchWorkflow currently swallows errors by returning an empty string; update the catch to accept the error (e.g., catch (err)) and return a descriptive message that includes the workflow run id and the error/details (use err.message or inspect err) so callers can see operational failures (reference the watchWorkflow function and the run id variable used there).community-plugins.yml (1)
16-26: Consider pinning plugin git refs for deterministic validation runs.Lines 16, 21, and 25 use floating git URLs, which can cause scheduled compatibility checks to drift unpredictably when upstream defaults change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@community-plugins.yml` around lines 16 - 26, The plugin entries with floating git URLs (the UniTask package URL, the NaughtyAttributes package URL, and the Unity Atoms package URL) should be pinned to specific refs so CI validation is deterministic; update those entries in community-plugins.yml by adding a fixed ref (tag or commit SHA) to each git URL (or add a ref: field) for UniTask, NaughtyAttributes, and Unity Atoms so the resolver always fetches the exact revision instead of the repo default branch.src/model/orchestrator/services/test/README.md (1)
5-5: Make the specification reference actionable.“See GitHub Issue for full specification.” should include the direct issue URL/ID to avoid dead-end docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/test/README.md` at line 5, Replace the vague line "See GitHub Issue for full specification." in the README (the exact string appears in src/model/orchestrator/services/test/README.md) with an actionable reference: insert the full GitHub issue URL or the issue number (e.g., "See GitHub Issue `#123`: https://github.com/OWNER/REPO/issues/123") and optionally a short title so readers can navigate directly to the specification.src/model/orchestrator/services/output/artifact-service.test.ts (1)
546-550: Preferjest.spyOn(...).mockImplementation(...)over manualexecFileSyncreassignment.Directly mutating
child_process.execFileSyncis brittle; if the test exits early, restoration may be skipped and contaminate later cases.♻️ Safer mock/restore pattern
- const childProcess = require('node:child_process'); - const originalExecFileSync = childProcess.execFileSync; - childProcess.execFileSync = jest.fn(() => { - throw new Error('ENOENT'); - }); + const childProcess = require('node:child_process'); + const execFileSyncSpy = jest.spyOn(childProcess, 'execFileSync').mockImplementation(() => { + throw new Error('ENOENT'); + }); // ... assertions ... - childProcess.execFileSync = originalExecFileSync; + execFileSyncSpy.mockRestore();Also applies to: 572-574, 578-580, 603-604
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/output/artifact-service.test.ts` around lines 546 - 550, Replace direct reassignment of child_process.execFileSync with a Jest spy to avoid leaking state: use jest.spyOn(childProcess, 'execFileSync').mockImplementation(() => { throw new Error('ENOENT'); }) in the test(s) that currently reassign execFileSync (locations around the tests referencing childProcess.execFileSync at lines ~546, ~572, ~578, ~603) and ensure you call mockRestore() (or use afterEach to restore) after the test so the original implementation is always restored; update each occurrence that manually sets originalExecFileSync and reassigns it to instead use jest.spyOn(...) and mockRestore().src/model/orchestrator/services/output/output-type-registry.ts (1)
112-125: Deduplicate parsed output type names before resolving.Repeated names (e.g.
logs,logs) currently produce duplicate type entries and duplicate downstream work.♻️ Suggested fix
- const names = outputTypesInput - .split(',') - .map((s) => s.trim()) - .filter(Boolean); + const names = [...new Set( + outputTypesInput + .split(',') + .map((s) => s.trim()) + .filter(Boolean), + )];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/output/output-type-registry.ts` around lines 112 - 125, Parsed output type names (from outputTypesInput → names) are not deduplicated so duplicate names produce duplicate entries; deduplicate the names array before resolving with OutputTypeRegistry.getType to avoid duplicate type entries being pushed into types. Replace the current iteration over names with one over a deduplicated, order-preserving collection (e.g., create a Set or use Array.from(new Set(names)) or filter by indexOf) and then call OutputTypeRegistry.getType(name) for each unique name, pushing valid defs into the types array and logging via OrchestratorLogger.logWarning for unknown names as before.src/model/orchestrator/services/sync/incremental-sync-service.ts (1)
170-175:syncRevertAfteris exposed but unused insyncStoragePull.Either wire this option into behavior or remove it from the method options to avoid a misleading API surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around lines 170 - 175, The option syncRevertAfter is declared but never used in syncStoragePull, so either remove it from the options to avoid a misleading API or wire it into the method: if you intend revert behavior, read options.syncRevertAfter inside syncStoragePull and after a successful pull invoke the existing revert routine (e.g., call the revert method used elsewhere such as revertStorageChanges or revertSyncState) or dispatch the appropriate revert flow, and update callers/tests; otherwise remove syncRevertAfter from the options interface and from all call sites to keep the API surface accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f009e7ca-0380-40a6-b585-5e85373b4888
⛔ Files ignored due to path filters (3)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (57)
.eslintrc.json.github/workflows/release-cli.yml.github/workflows/validate-community-plugins.ymlaction.ymlcommunity-plugins.ymldelete-me-update-all-integration-branches.ps1delete-me-update-this-integration-branch.ps1install.ps1install.shpackage.jsonsrc/cli.tssrc/cli/__tests__/cli-integration.test.tssrc/cli/__tests__/commands.test.tssrc/cli/__tests__/input-mapper.test.tssrc/cli/commands/activate.tssrc/cli/commands/build.tssrc/cli/commands/cache.tssrc/cli/commands/orchestrate.tssrc/cli/commands/status.tssrc/cli/commands/update.tssrc/cli/commands/version.tssrc/cli/input-mapper.tssrc/index.tssrc/model/build-parameters.tssrc/model/input.tssrc/model/orchestrator/providers/ansible/index.tssrc/model/orchestrator/providers/github-actions/index.tssrc/model/orchestrator/providers/gitlab-ci/index.tssrc/model/orchestrator/providers/remote-powershell/index.tssrc/model/orchestrator/remote-client/index.tssrc/model/orchestrator/runners/README.mdsrc/model/orchestrator/services/hot-runner/hot-runner-dispatcher.tssrc/model/orchestrator/services/hot-runner/hot-runner-health-monitor.tssrc/model/orchestrator/services/hot-runner/hot-runner-registry.tssrc/model/orchestrator/services/hot-runner/hot-runner-service.tssrc/model/orchestrator/services/hot-runner/hot-runner-types.tssrc/model/orchestrator/services/hot-runner/hot-runner.test.tssrc/model/orchestrator/services/hot-runner/index.tssrc/model/orchestrator/services/output/artifact-service.test.tssrc/model/orchestrator/services/output/artifact-upload-handler.tssrc/model/orchestrator/services/output/index.tssrc/model/orchestrator/services/output/output-manifest.tssrc/model/orchestrator/services/output/output-service.tssrc/model/orchestrator/services/output/output-type-registry.tssrc/model/orchestrator/services/sync/incremental-sync-service.tssrc/model/orchestrator/services/sync/incremental-sync.test.tssrc/model/orchestrator/services/sync/index.tssrc/model/orchestrator/services/sync/sync-state-manager.tssrc/model/orchestrator/services/sync/sync-state.tssrc/model/orchestrator/services/test-workflow/index.tssrc/model/orchestrator/services/test-workflow/taxonomy-filter-service.tssrc/model/orchestrator/services/test-workflow/test-result-reporter.tssrc/model/orchestrator/services/test-workflow/test-suite-parser.tssrc/model/orchestrator/services/test-workflow/test-workflow-service.tssrc/model/orchestrator/services/test-workflow/test-workflow-types.tssrc/model/orchestrator/services/test-workflow/test-workflow.test.tssrc/model/orchestrator/services/test/README.md
| foreach ($integration in $integrationBranches) { | ||
| $name = $integration.Name | ||
| Write-Host "`n========================================" -ForegroundColor Cyan | ||
| Write-Host "Updating $name" -ForegroundColor Cyan | ||
| Write-Host "========================================" -ForegroundColor Cyan | ||
|
|
||
| # Check if branch exists locally | ||
| $exists = git branch --list $name | ||
| if (-not $exists) { | ||
| Write-Host "Creating local branch from origin/$name..." -ForegroundColor Yellow | ||
| git checkout -b $name "origin/$name" | ||
| } else { | ||
| git checkout $name | ||
| git pull origin $name --ff-only 2>$null | ||
| if ($LASTEXITCODE -ne 0) { | ||
| git pull origin $name --no-edit | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*delete-me-update-all-integration-branches*" -type fRepository: game-ci/unity-builder
Length of output: 112
🏁 Script executed:
cat -n ./delete-me-update-all-integration-branches.ps1Repository: game-ci/unity-builder
Length of output: 5107
Add error handling for git checkout and push operations to prevent wrong-branch merges.
The git checkout operations at lines 67 and 69 and the git push at line 112 have no error checking. If these commands fail, the script continues executing, which can lead to merges from an unintended branch state or failed pushes that go unnoticed. Although $ErrorActionPreference = 'Stop' is set, git commands don't raise PowerShell terminating errors—they exit with non-zero codes that must be explicitly checked.
The suggested fail-fast wrapper approach (throwing exceptions on git failures) is appropriate to prevent downstream merges and silent push failures.
Suggested fail-fast wrapper
+function Invoke-Git {
+ param([string[]]$Args, [string]$ErrorMessage)
+ & git `@Args`
+ if ($LASTEXITCODE -ne 0) {
+ throw $ErrorMessage
+ }
+}
@@
- git checkout -b $name "origin/$name"
+ Invoke-Git @('checkout','-b',$name,"origin/$name") "Failed to create $name from origin/$name"
@@
- git checkout $name
- git pull origin $name --ff-only 2>$null
+ Invoke-Git @('checkout',$name) "Failed to checkout $name"
+ git pull origin $name --ff-only 2>$null
if ($LASTEXITCODE -ne 0) {
- git pull origin $name --no-edit
+ Invoke-Git @('pull','origin',$name,'--no-edit') "Failed to update $name"
}
@@
- git push origin $name
+ Invoke-Git @('push','origin',$name) "Failed to push $name"🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[warning] 59-59: File 'delete-me-update-all-integration-branches.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 60-60: File 'delete-me-update-all-integration-branches.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 61-61: File 'delete-me-update-all-integration-branches.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 66-66: File 'delete-me-update-all-integration-branches.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delete-me-update-all-integration-branches.ps1` around lines 57 - 74, The
script lacks explicit error handling for git commands and must fail fast on
non-zero exits to avoid wrong-branch merges; add a small helper (e.g.,
Invoke-Git or FailIfGitFailed) that runs a git command, checks $LASTEXITCODE,
and throws/Exit 1 on failure, then replace direct calls to git checkout (both
when creating and switching branches in the foreach over $integrationBranches
and where git checkout $name is used) and the git push (the push near line 112)
with this wrapper or, after each git invocation, immediately test $LASTEXITCODE
and throw/Exit 1 with a clear message including the branch name and the failing
git command so the script stops instead of continuing on errors.
| Write-Host "Fetching latest from origin..." -ForegroundColor Cyan | ||
| git fetch origin | ||
|
|
||
| $failed = @() | ||
| foreach ($branch in $branches) { | ||
| Write-Host "`nMerging origin/$branch..." -ForegroundColor Yellow | ||
| $result = git merge "origin/$branch" --no-edit 2>&1 | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Host " CONFLICT merging $branch - resolve manually" -ForegroundColor Red | ||
| $failed += $branch | ||
| # Abort this merge so we can continue with others | ||
| git merge --abort | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "delete-me-update-this-integration-branch.ps1" 2>/dev/nullRepository: game-ci/unity-builder
Length of output: 111
🏁 Script executed:
cat -n ./delete-me-update-this-integration-branch.ps1Repository: game-ci/unity-builder
Length of output: 2319
🏁 Script executed:
cat > /tmp/test_powershell_git.ps1 << 'EOF'
# Test how PowerShell handles git command failures with $ErrorActionPreference
$ErrorActionPreference = 'Stop'
Write-Host "Testing git fetch with non-existent remote..."
git fetch nonexistent-remote 2>&1
Write-Host "After git fetch - script continued? LASTEXITCODE=$LASTEXITCODE"
Write-Host "This line should not execute if ErrorActionPreference=Stop affects native commands"
EOF
# Show the test file for reference
cat /tmp/test_powershell_git.ps1Repository: game-ci/unity-builder
Length of output: 443
🌐 Web query:
PowerShell $ErrorActionPreference native command git behavior
💡 Result:
In PowerShell, $ErrorActionPreference primarily controls PowerShell cmdlet/function (non-terminating) errors; it doesn’t inherently make native executables like git.exe “throw” just because they returned a non-zero exit code. For native commands you normally must check $LASTEXITCODE (and/or $?) yourself and throw if you want terminating behavior. (stackoverflow.com)
Why git can still “fail” (or look like it failed) under $ErrorActionPreference = 'Stop'
There are two commonly-confused behaviors:
- Exit code vs. PowerShell errors
gitreports failure via its process exit code (read it via$LASTEXITCODE).- Historically, PowerShell did not automatically convert non-zero native exit codes into terminating errors (unless you add your own check / wrapper). (stackoverflow.com)
stderrrouting/redirection pitfalls (older behavior)
- In Windows PowerShell 5.1 and PowerShell 7.0–7.1, when native
stderrwas redirected (e.g.2>or sometimes2>&1scenarios), PowerShell could route thatstderrtext through PowerShell’s error stream asNativeCommandError, and then$ErrorActionPreference='Stop'could turn it into a terminating error even if the tool is just being chatty on stderr (whichgitoften is). (stackoverflow.com) - PowerShell 7.2+ changed this so native-command
stderris no longer treated as PowerShell error output in the same way, meaning$ErrorActionPreferenceno longer “trips” just because something wrote tostderr. (devblogs.microsoft.com)
“Make native commands respect ErrorActionPreference” (PowerShell 7.4+)
If your goal is: “non-zero exit code from git should behave like a terminating error”, PowerShell added a dedicated preference variable:
$PSNativeCommandUseErrorActionPreference(when enabled, PowerShell can treat non-zero native exit codes as errors in a way that integrates with-ErrorAction/$ErrorActionPreference). (learn.microsoft.com)
Practical pattern (works everywhere)
git describe --tags
if ($LASTEXITCODE -ne 0) { throw "git failed with exit code $LASTEXITCODE" }This is the most version-proof approach for git and other native tools. (devblogs.microsoft.com)
Citations:
- 1: https://stackoverflow.com/questions/48864988/powershell-with-git-command-error-handling-automatically-abort-on-non-zero-exi?utm_source=openai
- 2: https://stackoverflow.com/questions/48864988/powershell-with-git-command-error-handling-automatically-abort-on-non-zero-exi?utm_source=openai
- 3: https://stackoverflow.com/questions/66726049/how-can-i-redirect-stdout-and-stderr-without-polluting-powershell-error-output?utm_source=openai
- 4: https://devblogs.microsoft.com/powershell/powershell-7-1-preview-6/?utm_source=openai
- 5: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_preference_variables?+view+=+powershell-6&view=powershell-7.4&utm_source=openai
- 6: https://devblogs.microsoft.com/powershell/powershell-7-1-preview-6/?utm_source=openai
Add error check after git fetch to prevent misleading merge status.
Line 25 (git fetch origin) lacks an error check. If the fetch fails, the script continues and subsequent merge attempts may behave unpredictably or report incorrect conflict status. Add an explicit check after the fetch:
git fetch origin
if ($LASTEXITCODE -ne 0) {
Write-Error "Failed to fetch origin."
exit 1
}Additionally, consider validating that remote branches exist before attempting to merge:
git rev-parse --verify "origin/$branch" 2>$null
if ($LASTEXITCODE -ne 0) {
Write-Host " SKIP $branch - remote branch not found" -ForegroundColor DarkGray
$failed += $branch
continue
}And use a more defensive approach when aborting merges by checking if a merge is actually in progress before calling abort.
🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[warning] 24-24: File 'delete-me-update-this-integration-branch.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 29-29: File 'delete-me-update-this-integration-branch.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 32-32: File 'delete-me-update-this-integration-branch.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 30-30: The variable 'result' is assigned but never used.
(PSUseDeclaredVarsMoreThanAssignments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delete-me-update-this-integration-branch.ps1` around lines 24 - 36, Add an
explicit error check after the git fetch origin call by inspecting $LASTEXITCODE
and exit with an error message if fetch failed; before attempting git merge
"origin/$branch" validate the remote branch exists using git rev-parse --verify
"origin/$branch" and if not found log a SKIP message, add the branch to $failed
and continue; when handling merge failures, only call git merge --abort if a
merge is actually in progress (defensive check) to avoid spurious errors; ensure
all references use the existing variables and symbols ($branches, $failed,
$LASTEXITCODE, git fetch origin, git rev-parse --verify "origin/$branch", git
merge --abort, git merge "origin/$branch") so behavior is predictable and
failure states are clearly reported.
| # Verify the binary works | ||
| if "${INSTALL_DIR}/${BINARY_NAME}" version > /dev/null 2>&1; then | ||
| INSTALLED_VERSION=$("${INSTALL_DIR}/${BINARY_NAME}" version 2>&1 | head -1) | ||
| info "Verified: ${INSTALLED_VERSION}" | ||
| else | ||
| warn "Binary downloaded but could not verify. It may still work." | ||
| fi |
There was a problem hiding this comment.
Verify checksum before executing the downloaded binary.
Line 122 executes the binary before integrity verification, while checksum validation is deferred until Line 196. This should be reversed.
Suggested patch
chmod +x "${INSTALL_DIR}/${BINARY_NAME}"
+ verify_checksum
+
# Verify the binary works
if "${INSTALL_DIR}/${BINARY_NAME}" version > /dev/null 2>&1; then
INSTALLED_VERSION=$("${INSTALL_DIR}/${BINARY_NAME}" version 2>&1 | head -1)
@@
detect_platform
get_latest_version
install
-verify_checksumAlso applies to: 193-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 121 - 127, The script currently executes the
downloaded binary (${INSTALL_DIR}/${BINARY_NAME}) before performing checksum
validation; reverse this flow so checksum verification runs first: locate the
existing checksum verification logic (the block around lines 193-196) and invoke
it immediately after download and before any call to
"${INSTALL_DIR}/${BINARY_NAME}" (which sets INSTALLED_VERSION). If checksum
fails, abort with an error/warn and do not execute the binary; only when
checksum validation succeeds proceed to run "${INSTALL_DIR}/${BINARY_NAME}" to
set INSTALLED_VERSION and log Verified.
| const credentialPart = this.credential | ||
| ? `-Credential (New-Object PSCredential('${this.credential.split(':')[0]}', (ConvertTo-SecureString '${ | ||
| this.credential.split(':')[1] | ||
| }' -AsPlainText -Force)))` | ||
| : ''; | ||
|
|
||
| return `pwsh -NoProfile -NonInteractive -Command "Invoke-Command -ComputerName '${this.host}' ${credentialPart} -ScriptBlock { ${escapedScript} }"`; |
There was a problem hiding this comment.
Credential parsing is unsafe and truncates passwords containing :.
Lines 152-154 split on :, which corrupts credentials when passwords contain colons. It also embeds the password in the process command line.
💡 Proposed fix (parsing correctness)
- const credentialPart = this.credential
- ? `-Credential (New-Object PSCredential('${this.credential.split(':')[0]}', (ConvertTo-SecureString '${
- this.credential.split(':')[1]
- }' -AsPlainText -Force)))`
+ const credentialPart = this.credential
+ ? (() => {
+ const index = this.credential.indexOf(':');
+ if (index <= 0) throw new Error('remotePowershellCredential must be in "username:password" format');
+ const username = this.credential.slice(0, index);
+ const password = this.credential.slice(index + 1);
+ return `-Credential (New-Object PSCredential('${username}', (ConvertTo-SecureString '${password}' -AsPlainText -Force)))`;
+ })()
: '';A follow-up hardening pass should avoid placing the password directly in command-line arguments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/remote-powershell/index.ts` around lines 151
- 157, The current credential parsing (this.credential.split(':')) corrupts
passwords containing ':' and embeds the password on the command line; change
parsing to split only on the first ':' by using indexOf to get the first
separator and then slice to derive username and password (e.g., compute sep =
this.credential.indexOf(':'), username = this.credential.slice(0, sep), password
= this.credential.slice(sep+1)); ensure you escape single quotes in
username/password before interpolating into credentialPart (the credentialPart
construction) and replace the existing this.credential.split(...) usage;
additionally add a TODO on the return string building (the Invoke-Command
construction) to avoid placing plaintext passwords in process command arguments
in a subsequent hardening pass.
| const outputPath = path.join( | ||
| projectPath, | ||
| typeDef.defaultPath.replace('{platform}', process.env.BUILD_TARGET || 'Unknown'), | ||
| ); | ||
|
|
||
| if (!fs.existsSync(outputPath)) { | ||
| OrchestratorLogger.log(`[Output] No output found for '${typeDef.name}' at ${outputPath}`); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the specific lines
head -60 src/model/orchestrator/services/output/output-service.ts | tail -20Repository: game-ci/unity-builder
Length of output: 666
🏁 Script executed:
# Get more context around lines 47-55
wc -l src/model/orchestrator/services/output/output-service.tsRepository: game-ci/unity-builder
Length of output: 125
🏁 Script executed:
# Read the full file to understand the context
cat -n src/model/orchestrator/services/output/output-service.ts | head -80Repository: game-ci/unity-builder
Length of output: 3259
🏁 Script executed:
# Find the OutputTypeRegistry file
fd -t f output-type-registryRepository: game-ci/unity-builder
Length of output: 127
🏁 Script executed:
# Read the OutputTypeRegistry file
cat -n src/model/orchestrator/services/output/output-type-registry.tsRepository: game-ci/unity-builder
Length of output: 4766
🏁 Script executed:
# Search for all calls to registerType
rg -n 'registerType' --type tsRepository: game-ci/unity-builder
Length of output: 926
🏁 Script executed:
# Also check if there's any configuration loading that might accept custom types
rg -n 'OutputTypeRegistry|customType|registerType' src/ -A 3 -B 3Repository: game-ci/unity-builder
Length of output: 23575
🏁 Script executed:
# Read the context around line 126 in src/index.ts
sed -n '110,140p' src/index.tsRepository: game-ci/unity-builder
Length of output: 1258
🏁 Script executed:
# Verify path.join behavior with ../ sequences using a simple test
cat > /tmp/test_path.js << 'EOF'
const path = require('path');
// Test if path.join allows path traversal
const projectPath = '/home/user/project';
const maliciousPath = '../../../etc/passwd';
const result = path.join(projectPath, maliciousPath);
console.log('Base:', projectPath);
console.log('Malicious:', maliciousPath);
console.log('Result:', result);
console.log('Escapes project?', !result.startsWith(projectPath));
EOF
node /tmp/test_path.jsRepository: game-ci/unity-builder
Length of output: 162
🏁 Script executed:
# Check if artifactCustomTypes is validated anywhere
rg -n 'artifactCustomTypes' --type ts -B 5 -A 5Repository: game-ci/unity-builder
Length of output: 3690
🏁 Script executed:
# Check the artifact upload handler to see what happens with collected files
head -100 src/model/orchestrator/services/output/artifact-upload-handler.tsRepository: game-ci/unity-builder
Length of output: 2586
🏁 Script executed:
# Continue reading the artifact upload handler to see how manifest entries are used
sed -n '100,200p' src/model/orchestrator/services/output/artifact-upload-handler.tsRepository: game-ci/unity-builder
Length of output: 3087
🏁 Script executed:
# Check what happens after OutputService.collectOutputs() is called
sed -n '139,160p' src/index.tsRepository: game-ci/unity-builder
Length of output: 876
Block path traversal in output path resolution at multiple locations.
Path traversal vulnerability: custom output types accept arbitrary defaultPath values that can include ../ sequences. When joined with projectPath using path.join(), these escape the project root and enable reading arbitrary files from the filesystem. Files are then listed in the manifest and uploaded to configured artifact targets.
The vulnerability exists in two locations:
- src/model/orchestrator/services/output/output-service.ts (lines 47-50): Resolves output paths without boundary enforcement
- src/model/orchestrator/services/output/artifact-upload-handler.ts (line 159): Same issue during upload entry resolution
Use path.resolve() with root-boundary validation in both locations to ensure resolved paths stay within the project directory.
Proposed fix
For output-service.ts (lines 47-55):
- const outputPath = path.join(
- projectPath,
- typeDef.defaultPath.replace('{platform}', process.env.BUILD_TARGET || 'Unknown'),
- );
+ const projectRoot = path.resolve(projectPath);
+ const candidatePath = path.resolve(
+ projectRoot,
+ typeDef.defaultPath.replace('{platform}', process.env.BUILD_TARGET || 'Unknown'),
+ );
+ const isInsideProject =
+ candidatePath === projectRoot || candidatePath.startsWith(`${projectRoot}${path.sep}`);
+ if (!isInsideProject) {
+ OrchestratorLogger.logWarning(
+ `[Output] Skipping '${typeDef.name}' because resolved path escapes project root: ${candidatePath}`,
+ );
+ continue;
+ }
+ const outputPath = candidatePath;Apply the same pattern to artifact-upload-handler.ts (line 159) in the uploadEntry method.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const outputPath = path.join( | |
| projectPath, | |
| typeDef.defaultPath.replace('{platform}', process.env.BUILD_TARGET || 'Unknown'), | |
| ); | |
| if (!fs.existsSync(outputPath)) { | |
| OrchestratorLogger.log(`[Output] No output found for '${typeDef.name}' at ${outputPath}`); | |
| continue; | |
| } | |
| const projectRoot = path.resolve(projectPath); | |
| const candidatePath = path.resolve( | |
| projectRoot, | |
| typeDef.defaultPath.replace('{platform}', process.env.BUILD_TARGET || 'Unknown'), | |
| ); | |
| const isInsideProject = | |
| candidatePath === projectRoot || candidatePath.startsWith(`${projectRoot}${path.sep}`); | |
| if (!isInsideProject) { | |
| OrchestratorLogger.logWarning( | |
| `[Output] Skipping '${typeDef.name}' because resolved path escapes project root: ${candidatePath}`, | |
| ); | |
| continue; | |
| } | |
| const outputPath = candidatePath; | |
| if (!fs.existsSync(outputPath)) { | |
| OrchestratorLogger.log(`[Output] No output found for '${typeDef.name}' at ${outputPath}`); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/output/output-service.ts` around lines 47 -
55, The output path resolution currently uses path.join which allows path
traversal via typeDef.defaultPath; update the logic that builds outputPath in
output-service (the block using projectPath, typeDef.defaultPath and outputPath)
to use path.resolve and then enforce a boundary check (e.g., ensure the resolved
path is within projectPath via path.relative or startsWith check); if the
resolved path escapes the project root, log an error on OrchestratorLogger and
continue (do not process). Apply the same fix in artifact-upload-handler inside
the uploadEntry method (the code resolving the upload entry path, e.g.,
entryPath/target path) so resolved upload paths are checked against projectPath
and rejected if outside the project.
| * @returns The loaded sync state, or undefined if no state exists or parsing fails | ||
| */ | ||
| static loadState(workspacePath: string, statePath?: string): SyncState | undefined { | ||
| const resolvedPath = path.join(workspacePath, statePath || SyncStateManager.DEFAULT_STATE_PATH); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/model/orchestrator/services/sync/sync-state-manager.tsRepository: game-ci/unity-builder
Length of output: 5324
🏁 Script executed:
rg -r "loadState\s*\(|saveState\s*\(" --type ts -B 2 -A 2Repository: game-ci/unity-builder
Length of output: 461
🏁 Script executed:
rg "\.loadState\(|\.saveState\(" --type ts -B 2 -A 2Repository: game-ci/unity-builder
Length of output: 9474
🏁 Script executed:
cat -n src/model/orchestrator/services/sync/incremental-sync-service.ts | head -100Repository: game-ci/unity-builder
Length of output: 4311
🏁 Script executed:
grep -n "statePath" src/model/orchestrator/services/sync/incremental-sync-service.ts | head -20Repository: game-ci/unity-builder
Length of output: 1598
🏁 Script executed:
sed -n '160,240p' src/model/orchestrator/services/sync/incremental-sync-service.tsRepository: game-ci/unity-builder
Length of output: 3125
🏁 Script executed:
rg "IncrementalSyncService\." --type ts | grep -E "(syncStoragePull|syncGitDelta|sync|resolveStrategy)" | head -20Repository: game-ci/unity-builder
Length of output: 2987
🏁 Script executed:
rg "syncStatePath" --type ts -B 2 -A 2 | head -50Repository: game-ci/unity-builder
Length of output: 2936
🏁 Script executed:
find . -name "action.yml" -o -name "action.yaml" | head -5Repository: game-ci/unity-builder
Length of output: 77
🏁 Script executed:
grep -A 5 -B 2 "syncStatePath" action.ymlRepository: game-ci/unity-builder
Length of output: 253
🏁 Script executed:
cat src/model/input.ts | grep -A 3 "syncStatePath"Repository: game-ci/unity-builder
Length of output: 222
🏁 Script executed:
node -e "const path = require('path'); console.log('Case 1:', path.join('/home/runner', '../../../../etc/passwd')); console.log('Case 2:', path.join('/home/runner', '../../../sensitive.txt')); console.log('Case 3:', path.resolve('/home/runner', '../../../../etc/passwd'));"Repository: game-ci/unity-builder
Length of output: 127
🏁 Script executed:
node -e "
const path = require('path');
// Simulating the proposed fix
function resolveStatePath(workspacePath, statePath) {
const workspaceRoot = path.resolve(workspacePath);
const resolvedPath = path.resolve(workspaceRoot, statePath || '.game-ci/sync-state.json');
const isInsideWorkspace =
resolvedPath === workspaceRoot || resolvedPath.startsWith(\`\${workspaceRoot}\${path.sep}\`);
if (!isInsideWorkspace) {
throw new Error(\`Invalid state path outside workspace: \${statePath}\`);
}
return resolvedPath;
}
// Test cases
try {
console.log('Valid:', resolveStatePath('/home/runner/workspace', 'custom/state.json'));
} catch(e) { console.log('Error:', e.message); }
try {
console.log('Traversal attack:', resolveStatePath('/home/runner/workspace', '../../../../etc/passwd'));
} catch(e) { console.log('Error:', e.message); }
try {
console.log('Default:', resolveStatePath('/home/runner/workspace', undefined));
} catch(e) { console.log('Error:', e.message); }
"Repository: game-ci/unity-builder
Length of output: 237
Constrain syncStatePath to workspace root to prevent directory traversal.
syncStatePath is user-controllable input from GitHub Actions that is directly joined with the workspace path, allowing attackers to escape via ../ sequences and read/write arbitrary files on the system. For example, a malicious input like ../../../../etc/passwd bypasses the workspace boundary entirely.
Extract path resolution into a validation helper that rejects paths outside the workspace:
Proposed fix
export class SyncStateManager {
static readonly DEFAULT_STATE_PATH = '.game-ci/sync-state.json';
+
+ private static resolveStatePath(workspacePath: string, statePath?: string): string {
+ const workspaceRoot = path.resolve(workspacePath);
+ const resolvedPath = path.resolve(workspaceRoot, statePath || SyncStateManager.DEFAULT_STATE_PATH);
+ const isInsideWorkspace =
+ resolvedPath === workspaceRoot || resolvedPath.startsWith(`${workspaceRoot}${path.sep}`);
+ if (!isInsideWorkspace) {
+ throw new Error(`[SyncState] Invalid state path outside workspace: ${statePath}`);
+ }
+ return resolvedPath;
+ }
@@
- const resolvedPath = path.join(workspacePath, statePath || SyncStateManager.DEFAULT_STATE_PATH);
+ const resolvedPath = SyncStateManager.resolveStatePath(workspacePath, statePath);
@@
- const resolvedPath = path.join(workspacePath, statePath || SyncStateManager.DEFAULT_STATE_PATH);
+ const resolvedPath = SyncStateManager.resolveStatePath(workspacePath, statePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/sync/sync-state-manager.ts` at line 37, The
current join of workspacePath and statePath in SyncStateManager (resolvedPath)
allows directory traversal via user-controlled syncStatePath; update
SyncStateManager to resolve and validate the path: use
path.resolve(workspacePath, statePath || SyncStateManager.DEFAULT_STATE_PATH) to
compute resolvedPath, then compute path.relative(workspacePath, resolvedPath)
and reject (throw) if the relative path starts with '..' or is absolute,
ensuring resolvedPath remains inside workspacePath; centralize this logic in a
small helper (e.g., validateSyncStatePath or resolveSyncStatePath) and call it
where resolvedPath is created so malicious '../' inputs are refused.
| if (trimmed.startsWith('/') && trimmed.endsWith('/') && trimmed.length > 2) { | ||
| const pattern = trimmed.slice(1, -1); | ||
| try { | ||
| const regex = new RegExp(pattern); | ||
| return regex.test(testValue); | ||
| } catch { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TaxonomyFilterService call sites =="
ast-grep --pattern 'TaxonomyFilterService.buildFilterArgs($_)' || true
ast-grep --pattern 'TaxonomyFilterService.matchesFilter($_, $_)' || true
echo
echo "== Potential external/user-controlled filter sources =="
rg -nP --type=ts 'workflow_dispatch|github\.event\.inputs|process\.argv|yargs|filters\s*[:=]' || trueRepository: game-ci/unity-builder
Length of output: 7459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== taxonomy-filter-service.ts (full file) ==="
wc -l src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts
echo
echo "=== Lines around 160-165 and context ==="
cat -n src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts | sed -n '150,175p'
echo
echo "=== test-suite-parser.ts - filter assignment ==="
rg -n -B5 -A5 'run\.filters\s*=' src/model/orchestrator/services/test-workflow/test-suite-parser.ts || true
echo
echo "=== test-workflow-service.ts line 210 and context ==="
cat -n src/model/orchestrator/services/test-workflow/test-workflow-service.ts | sed -n '200,220p'Repository: game-ci/unity-builder
Length of output: 2520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== test-suite-parser.ts - full file context ==="
wc -l src/model/orchestrator/services/test-workflow/test-suite-parser.ts
echo
echo "=== test-suite-parser.ts - beginning to understand raw.filters origin ==="
cat -n src/model/orchestrator/services/test-workflow/test-suite-parser.ts | head -100
echo
echo "=== buildFilterArgs implementation ==="
rg -n -A20 'static buildFilterArgs' src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts || true
echo
echo "=== Who calls parseTestSuite ==="
rg -n 'parseTestSuite\(' src/ --type=ts | head -20Repository: game-ci/unity-builder
Length of output: 4908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Who calls parseSuiteFile ==="
rg -n 'parseSuiteFile\|parseTestSuite' src/ --type=ts 2>/dev/null || true
echo
echo "=== buildFilterArgs full implementation ==="
cat -n src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts | sed -n '69,95p'
echo
echo "=== buildDimensionExpression implementation ==="
cat -n src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts | sed -n '100,145p'
echo
echo "=== How test workflow is provided to orchestrator ==="
rg -n 'testWorkflow\|suite.*path' src/model/orchestrator/services/ --type=ts -A2 | head -40Repository: game-ci/unity-builder
Length of output: 3158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for TestSuiteParser usage across entire codebase ==="
rg -n 'TestSuiteParser' . --type=ts 2>/dev/null || true
echo
echo "=== Search for .yml/.yaml file references in orchestrator ==="
rg -n '\.(yml|yaml)' src/model/orchestrator --type=ts 2>/dev/null | head -20 || true
echo
echo "=== Check test-workflow-service.ts for context around line 210 ==="
cat -n src/model/orchestrator/services/test-workflow/test-workflow-service.ts | head -50
echo
echo "=== Check if workflow definition file comes from parameters ==="
rg -n 'testWorkflow|workflow.*file|definition' src/model/orchestrator/services/test-workflow/test-workflow-service.ts --type=ts | head -20Repository: game-ci/unity-builder
Length of output: 7962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Who calls TestWorkflowService.executeTestSuite ==="
rg -n 'executeTestSuite' src/ --type=ts 2>/dev/null | grep -v test.ts || true
echo
echo "=== Check test-workflow.test.ts for test cases with regex patterns ==="
rg -n 'Maturity.*Trusted.*Adolescent\|\/.*\/' src/model/orchestrator/services/test-workflow/test-workflow.test.ts | head -10
echo
echo "=== Check matchesDimensionFilter for error handling ==="
cat -n src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts | sed -n '156,170p'
echo
echo "=== Check entire matchesFilter flow ==="
cat -n src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts | sed -n '136,151p'Repository: game-ci/unity-builder
Length of output: 652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Who calls TestWorkflowService.executeTestSuite ==="
rg -n 'executeTestSuite' src/ --type=ts 2>/dev/null | grep -v test.ts || true
echo
echo "=== Check test-workflow.test.ts for test cases with regex patterns ==="
rg -n 'Maturity.*Trusted' src/model/orchestrator/services/test-workflow/test-workflow.test.ts | head -10
echo
echo "=== Check where buildParameters.testSuitePath comes from ==="
rg -n 'testSuitePath' src/ --type=ts | head -20
echo
echo "=== Check BuildParameters source ==="
rg -n 'class BuildParameters\|interface BuildParameters' src/ --type=ts -A5 | head -30Repository: game-ci/unity-builder
Length of output: 2099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check Input.getInput source ==="
rg -n 'getInput.*testSuitePath\|class Input' src/model/input.ts --type=ts -B5 -A10 | head -40
echo
echo "=== Check for any validation on filter patterns ==="
rg -n 'validation\|validate\|limit\|length' src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts --type=ts
echo
echo "=== Full matchesDimensionFilter function for complete context ==="
cat -n src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts | sed -n '156,190p'
echo
echo "=== Check if pattern size is limited anywhere ==="
rg -n 'pattern.*length\|maxLength\|pattern.*limit' src/model/orchestrator/services/test-workflow/ --type=tsRepository: game-ci/unity-builder
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check Input class definition ==="
cat -n src/model/input.ts | sed -n '1,50p'
echo
echo "=== Check testSuitePath getter ==="
cat -n src/model/input.ts | sed -n '325,340p'
echo
echo "=== Full matchesDimensionFilter with context ==="
cat -n src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts | sed -n '156,190p'
echo
echo "=== Check if any regex limits exist ==="
rg -n 'pattern\|regex\|RegExp' src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts --type=ts -B1 -A1 | head -40Repository: game-ci/unity-builder
Length of output: 4032
Add pattern validation to prevent ReDoS attacks.
Line 163 constructs a RegExp from user-controlled YAML input without validation. The try-catch only prevents compilation errors but allows patterns susceptible to pathological backtracking (ReDoS). Add a length limit and consider disabling backtracking features (e.g., via atomic groups or validation rules) to prevent regex engine stalls when filters are defined in user-provided YAML files.
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 162-162: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 ESLint
[error] 164-164: Expected blank line before this statement.
(padding-line-between-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/taxonomy-filter-service.ts`
around lines 160 - 165, The RegExp is built directly from user YAML in the block
that slices `trimmed` into `pattern` and calls `new RegExp(pattern)`; add
validation before constructing the RegExp: enforce a max pattern length (e.g.,
100-500 chars) and reject patterns that contain known catastrophic constructs
(nested quantifiers/backtracking patterns such as "(.+)+", "(.*){", "(a|a)+",
backreferences like "\1", or excessive repetition tokens). If validation fails,
avoid calling `new RegExp` and return a safe fallback (e.g., false) or treat the
filter as invalid; keep the existing try-catch but perform these checks on
`pattern` prior to `new RegExp(pattern)` to mitigate ReDoS in the
`taxonomy-filter-service.ts` code path where `trimmed`, `pattern`, and
`regex.test` are used.
| // Build the full Unity command | ||
| const unityPath = TestWorkflowService.resolveUnityPath(parameters); | ||
| const command = `"${unityPath}" ${unityArguments} -testResults "${resultFile}"`; | ||
|
|
||
| core.info(`[TestWorkflow] Executing: ${command}`); | ||
|
|
||
| await execAsync(command, { | ||
| timeout: timeoutMs, | ||
| maxBuffer: 50 * 1024 * 1024, // 50 MB to handle large Unity output | ||
| cwd: parameters.projectPath || process.cwd(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "test-workflow-service.ts" -type fRepository: game-ci/unity-builder
Length of output: 137
🏁 Script executed:
cat -n src/model/orchestrator/services/test-workflow/test-workflow-service.ts | head -250Repository: game-ci/unity-builder
Length of output: 10802
🏁 Script executed:
# Check the buildUnityArgs function definition
rg -n "buildUnityArgs" src/model/orchestrator/services/test-workflow/test-workflow-service.ts -A 20Repository: game-ci/unity-builder
Length of output: 1975
🏁 Script executed:
# Check the imports at the top of the file
head -20 src/model/orchestrator/services/test-workflow/test-workflow-service.tsRepository: game-ci/unity-builder
Length of output: 867
Use execFile with array arguments instead of exec with shell strings to prevent command injection.
Interpolating user-controlled values (projectPath, builtClientPath, environment variables) into a shell command creates a command injection vulnerability. Using exec() interprets the string as shell, allowing attackers to inject arbitrary commands.
🛡️ Required changes
-import { exec } from 'node:child_process';
+import { execFile } from 'node:child_process';
import { promisify } from 'node:util';
...
-const execAsync = promisify(exec);
+const execFileAsync = promisify(execFile);
...
- static buildUnityArgs(run: TestRunDefinition, parameters: BuildParameters): string {
+ static buildUnityArgs(run: TestRunDefinition, parameters: BuildParameters): string[] {
const unityArguments: string[] = ['-batchmode', '-nographics'];
// Project path
if (parameters.projectPath) {
- unityArguments.push(`-projectPath "${parameters.projectPath}"`);
+ unityArguments.push('-projectPath', parameters.projectPath);
}
// Test mode
if (run.builtClient && run.builtClientPath) {
unityArguments.push(
'-runTests',
`-testPlatform StandalonePlayer`,
`-assemblyNames Assembly-CSharp-Tests`,
- `-builtPlayerPath "${run.builtClientPath}"`,
+ '-builtPlayerPath', run.builtClientPath,
);
}
...
- return unityArguments.join(' ');
+ return unityArguments;
}
static async executeTestRun(run: TestRunDefinition, parameters: BuildParameters): Promise<TestResult> {
...
const unityArguments = TestWorkflowService.buildUnityArgs(run, parameters);
...
- const command = `"${unityPath}" ${unityArguments} -testResults "${resultFile}"`;
- core.info(`[TestWorkflow] Executing: ${command}`);
- await execAsync(command, {
+ const args = [...unityArguments, '-testResults', resultFile];
+ core.info(`[TestWorkflow] Executing Unity test run`);
+ await execFileAsync(unityPath, args, {
timeout: timeoutMs,
maxBuffer: 50 * 1024 * 1024,
cwd: parameters.projectPath || process.cwd(),
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/test-workflow/test-workflow-service.ts`
around lines 99 - 109, The current TestWorkflowService uses execAsync with a
single shell command string which risks command injection; instead build an argv
array and call a non-shell exec variant (execFile or spawn) — call
TestWorkflowService.resolveUnityPath(parameters) to get the executable path and
construct an args array containing unityArguments split/parsed safely plus
"-testResults" and resultFile as separate entries, then invoke
execFile/child_process.spawn with that path and args while passing the existing
options (timeout, maxBuffer/capture logic, cwd=parameters.projectPath ||
process.cwd(), env) so no user-controlled value is interpolated into a shell
string; update the execAsync call site to use execFile/ spawn (or promisified
execFile) and ensure any quoting/whitespace in unityArguments is handled by
explicit argument elements rather than concatenation.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
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 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
delete-me-update-all-integration-branches.ps1 (1)
94-94: Remove unused variable assignments.
$refExistsand$resultare assigned but never used. The commands are run for their exit codes and side effects only.🧹 Suggested cleanup
- $refExists = git rev-parse --verify $remoteBranch 2>$null + git rev-parse --verify $remoteBranch 2>$null | Out-Null- $result = git merge $remoteBranch --no-edit 2>&1 + git merge $remoteBranch --no-edit 2>&1 | Out-NullAlso applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@delete-me-update-all-integration-branches.ps1` at line 94, The script assigns command outputs to unused variables ($refExists and $result) at the git calls; remove those unused assignments so the commands are executed for their exit codes/side effects only (i.e., replace occurrences like "$refExists = git rev-parse --verify $remoteBranch 2>$null" and the similar "$result =" assignment with a bare command invocation) and ensure no other code depends on those variables (update any downstream uses if they exist).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-tests-mac.yml:
- Around line 109-111: The current check run filter uses
run.name.includes('StandaloneOSX') or run.name.includes('iOS') which is too
broad and can match runs from other workflows; update the job name (the matrix
job name defined where name: "${{ matrix.targetPlatform }} on ${{
matrix.unityVersion }}" is set) to include a macOS identifier (e.g., "macOS") so
it becomes unique, and then change the filtering logic in the loop that iterates
checkRuns.check_runs to check for that identifier (replace the
run.name.includes('StandaloneOSX') / 'iOS' checks with a
run.name.includes('macOS') check), or alternatively narrow the filter by
checking workflow-specific metadata such as check_suite or head_branch if
available; ensure the conditional that currently uses run.conclusion and
run.app?.slug is updated to include the new name/metadata check so only macOS
workflow runs are affected.
- Around line 102-107: The current call to github.rest.checks.listForRef with
per_page: 100 can miss check runs beyond the first page; replace the single-list
call with a paginated fetch (e.g., use github.paginate or loop over pages) to
collect all check runs for the ref, then continue using the aggregated checkRuns
array; update references to the single "const { data: checkRuns }" to use the
combined results so no check runs are skipped.
In `@delete-me-update-all-integration-branches.ps1`:
- Around line 133-136: When restoring stashed changes after checking $stashed,
run the git stash pop and check its exit status/output (instead of assuming
success); if git stash pop fails or reports conflicts, print a clear warning
telling the user that the stash was not dropped, that conflicts must be resolved
manually, and suggest commands like git status and git stash list to
inspect/restore the stash; ensure this logic surrounds the existing git stash
pop call so you do not inadvertently drop or ignore the stash on failure.
- Line 7: The assignment of $originalBranch from `git rev-parse --abbrev-ref
HEAD` lacks validation; immediately after that command check its success (e.g.,
inspect `$?` or test for empty string) and if it failed or is empty, emit a
clear error message and exit the script (or set a safe fallback) to avoid later
doing `git checkout $originalBranch`. Update the code around the
`$originalBranch` assignment to validate the variable and handle failure paths
before any further git operations like `git checkout $originalBranch`.
---
Nitpick comments:
In `@delete-me-update-all-integration-branches.ps1`:
- Line 94: The script assigns command outputs to unused variables ($refExists
and $result) at the git calls; remove those unused assignments so the commands
are executed for their exit codes/side effects only (i.e., replace occurrences
like "$refExists = git rev-parse --verify $remoteBranch 2>$null" and the similar
"$result =" assignment with a bare command invocation) and ensure no other code
depends on those variables (update any downstream uses if they exist).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aface71f-05b7-497f-820c-fb5a9d77d0a1
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
.github/workflows/build-tests-mac.ymldelete-me-update-all-integration-branches.ps1
| const { data: checkRuns } = await github.rest.checks.listForRef({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| ref: context.sha, | ||
| per_page: 100, | ||
| }); |
There was a problem hiding this comment.
Pagination not handled for check runs listing.
The script uses per_page: 100 without handling pagination. If there are more than 100 check runs for a commit (e.g., in a busy CI environment with many workflows), some failing macOS builds may not be processed.
For most typical use cases this is acceptable, but consider using github.paginate() if the repo frequently exceeds 100 check runs per commit.
📄 Optional: Use pagination for robustness
- const { data: checkRuns } = await github.rest.checks.listForRef({
- owner: context.repo.owner,
- repo: context.repo.repo,
- ref: context.sha,
- per_page: 100,
- });
-
- for (const run of checkRuns.check_runs) {
+ const checkRuns = await github.paginate(github.rest.checks.listForRef, {
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ ref: context.sha,
+ per_page: 100,
+ });
+
+ for (const run of checkRuns) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-tests-mac.yml around lines 102 - 107, The current
call to github.rest.checks.listForRef with per_page: 100 can miss check runs
beyond the first page; replace the single-list call with a paginated fetch
(e.g., use github.paginate or loop over pages) to collect all check runs for the
ref, then continue using the aggregated checkRuns array; update references to
the single "const { data: checkRuns }" to use the combined results so no check
runs are skipped.
| for (const run of checkRuns.check_runs) { | ||
| if (run.conclusion !== 'failure' || run.app?.slug !== 'github-actions') continue; | ||
| if (!run.name.includes('StandaloneOSX') && !run.name.includes('iOS')) continue; |
There was a problem hiding this comment.
Check run filtering may incorrectly affect other workflows.
The filter run.name.includes('StandaloneOSX') or run.name.includes('iOS') is too broad. Per the context snippet, build-tests-ubuntu.yml also runs StandaloneOSX and iOS builds with the same job naming pattern (${{ matrix.targetPlatform }} on ${{ matrix.unityVersion }}). This script could inadvertently mark Ubuntu-workflow failures as neutral.
Consider adding a workflow-specific identifier to distinguish check runs. Options:
-
Update the job name (line 16) to include "macOS":
name: ${{ matrix.targetPlatform }} on macOS / ${{ matrix.unityVersion }}
Then filter forrun.name.includes('macOS'). -
Filter by workflow name using the check_suite's head_branch or additional metadata.
🐛 Proposed fix: Add macOS identifier to job name and filter
In the job definition around line 16:
- name: ${{ matrix.targetPlatform }} on ${{ matrix.unityVersion }}
+ name: ${{ matrix.targetPlatform }} on macOS (${{ matrix.unityVersion }})Then update the filter in the script:
- if (!run.name.includes('StandaloneOSX') && !run.name.includes('iOS')) continue;
+ if (!run.name.includes('on macOS')) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-tests-mac.yml around lines 109 - 111, The current
check run filter uses run.name.includes('StandaloneOSX') or
run.name.includes('iOS') which is too broad and can match runs from other
workflows; update the job name (the matrix job name defined where name: "${{
matrix.targetPlatform }} on ${{ matrix.unityVersion }}" is set) to include a
macOS identifier (e.g., "macOS") so it becomes unique, and then change the
filtering logic in the loop that iterates checkRuns.check_runs to check for that
identifier (replace the run.name.includes('StandaloneOSX') / 'iOS' checks with a
run.name.includes('macOS') check), or alternatively narrow the filter by
checking workflow-specific metadata such as check_suite or head_branch if
available; ensure the conditional that currently uses run.conclusion and
run.app?.slug is updated to include the new name/metadata check so only macOS
workflow runs are affected.
|
|
||
| $ErrorActionPreference = 'Stop' | ||
|
|
||
| $originalBranch = git rev-parse --abbrev-ref HEAD |
There was a problem hiding this comment.
Critical: Missing error handling for initial branch detection.
If git rev-parse fails (e.g., script run outside a git repo), $originalBranch will be empty or contain an error message. The script will later attempt git checkout $originalBranch (line 131), which will fail or checkout an unintended ref. Add validation immediately after line 7:
$originalBranch = git rev-parse --abbrev-ref HEAD
+if ($LASTEXITCODE -ne 0 -or -not $originalBranch) {
+ throw "Not in a git repository or unable to determine current 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.
| $originalBranch = git rev-parse --abbrev-ref HEAD | |
| $originalBranch = git rev-parse --abbrev-ref HEAD | |
| if ($LASTEXITCODE -ne 0 -or -not $originalBranch) { | |
| throw "Not in a git repository or unable to determine current branch" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delete-me-update-all-integration-branches.ps1` at line 7, The assignment of
$originalBranch from `git rev-parse --abbrev-ref HEAD` lacks validation;
immediately after that command check its success (e.g., inspect `$?` or test for
empty string) and if it failed or is empty, emit a clear error message and exit
the script (or set a safe fallback) to avoid later doing `git checkout
$originalBranch`. Update the code around the `$originalBranch` assignment to
validate the variable and handle failure paths before any further git operations
like `git checkout $originalBranch`.
| if ($stashed) { | ||
| Write-Host "Restoring stashed changes..." -ForegroundColor Cyan | ||
| git stash pop | ||
| } |
There was a problem hiding this comment.
Handle stash pop failure gracefully.
If git stash pop encounters conflicts, the stash is not dropped and the user's changes remain in the stash. The script should warn the user to manually resolve this.
🛡️ Suggested improvement
if ($stashed) {
Write-Host "Restoring stashed changes..." -ForegroundColor Cyan
git stash pop
+ if ($LASTEXITCODE -ne 0) {
+ Write-Host "WARNING: Failed to restore stash cleanly. Run 'git stash list' to check." -ForegroundColor Yellow
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($stashed) { | |
| Write-Host "Restoring stashed changes..." -ForegroundColor Cyan | |
| git stash pop | |
| } | |
| if ($stashed) { | |
| Write-Host "Restoring stashed changes..." -ForegroundColor Cyan | |
| git stash pop | |
| if ($LASTEXITCODE -ne 0) { | |
| Write-Host "WARNING: Failed to restore stash cleanly. Run 'git stash list' to check." -ForegroundColor Yellow | |
| } | |
| } |
🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[warning] 134-134: File 'delete-me-update-all-integration-branches.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@delete-me-update-all-integration-branches.ps1` around lines 133 - 136, When
restoring stashed changes after checking $stashed, run the git stash pop and
check its exit status/output (instead of assuming success); if git stash pop
fails or reports conflicts, print a clear warning telling the user that the
stash was not dropped, that conflicts must be resolved manually, and suggest
commands like git status and git stash list to inspect/restore the stash; ensure
this logic surrounds the existing git stash pop call so you do not inadvertently
drop or ignore the stash on failure.
Stop modifying the macOS build workflow — leave it identical to main. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The test mock was missing gitAuthMode, causing useHeaderAuth to default to true and strip the token from repo URLs. Adding gitAuthMode: 'url' restores the expected URL-mode behavior. Co-Authored-By: Claude Opus 4.6 <[email protected]>
[email protected] requires Node >=20.19.0, so Node 18 is no longer compatible. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #814 +/- ##
==========================================
+ Coverage 31.25% 39.28% +8.03%
==========================================
Files 84 113 +29
Lines 4563 6519 +1956
Branches 1103 1597 +494
==========================================
+ Hits 1426 2561 +1135
- Misses 3137 3957 +820
- Partials 0 1 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/model/orchestrator/options/orchestrator-folders.test.ts (1)
132-139: Cover the non-urlauth branch too.These assertions only lock in the
gitAuthMode: 'url'path because the mock is fixed to that mode. Please add one complementary case for a non-urlmode so this suite also catches regressions in the alternate repo URL behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-folders.test.ts` around lines 132 - 139, Add a complementary test that sets the mocked auth mode to a non-'url' value (e.g., set gitAuthMode to 'ssh' or 'tokenHeader' in the same test harness) and assert OrchestratorFolders.unityBuilderRepoUrl and OrchestratorFolders.targetBuildRepoUrl reflect the alternate-auth format (i.e., do not include the token in the URL and match the expected SSH/alternate string). Locate where the current tests read the mock auth config and duplicate a case that flips gitAuthMode away from 'url', then assert the resulting values from OrchestratorFolders.unityBuilderRepoUrl and OrchestratorFolders.targetBuildRepoUrl match the non-url behavior.
🤖 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/options/orchestrator-folders.test.ts`:
- Around line 144-159: The two tests that mutate
process.env.PURGE_REMOTE_BUILDER_CACHE around
OrchestratorFolders.purgeRemoteCaching are not exception-safe; wrap each env
mutation in a try/finally or move setup/teardown into a per-test afterEach to
guarantee restoration of the original value. Concretely, for the tests named
"returns false when env var is not set" and "returns true when env var is set",
capture the original process.env.PURGE_REMOTE_BUILDER_CACHE at the start,
perform the mutation and assertion inside try, and restore the original value in
finally (or instead use afterEach to reset
process.env.PURGE_REMOTE_BUILDER_CACHE to the saved original), ensuring no
leaked state affects other tests.
---
Nitpick comments:
In `@src/model/orchestrator/options/orchestrator-folders.test.ts`:
- Around line 132-139: Add a complementary test that sets the mocked auth mode
to a non-'url' value (e.g., set gitAuthMode to 'ssh' or 'tokenHeader' in the
same test harness) and assert OrchestratorFolders.unityBuilderRepoUrl and
OrchestratorFolders.targetBuildRepoUrl reflect the alternate-auth format (i.e.,
do not include the token in the URL and match the expected SSH/alternate
string). Locate where the current tests read the mock auth config and duplicate
a case that flips gitAuthMode away from 'url', then assert the resulting values
from OrchestratorFolders.unityBuilderRepoUrl and
OrchestratorFolders.targetBuildRepoUrl match the non-url behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 079e124c-162e-4a67-8396-abc526556f5a
📒 Files selected for processing (2)
.github/workflows/integrity-check.ymlsrc/model/orchestrator/options/orchestrator-folders.test.ts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/integrity-check.yml
| it('returns false when env var is not set', () => { | ||
| const original = process.env.PURGE_REMOTE_BUILDER_CACHE; | ||
| delete process.env.PURGE_REMOTE_BUILDER_CACHE; | ||
| expect(OrchestratorFolders.purgeRemoteCaching).toBe(false); | ||
| if (original !== undefined) process.env.PURGE_REMOTE_BUILDER_CACHE = original; | ||
| }); | ||
|
|
||
| it('returns true when env var is set', () => { | ||
| const original = process.env.PURGE_REMOTE_BUILDER_CACHE; | ||
| process.env.PURGE_REMOTE_BUILDER_CACHE = 'true'; | ||
| expect(OrchestratorFolders.purgeRemoteCaching).toBe(true); | ||
| if (original !== undefined) { | ||
| process.env.PURGE_REMOTE_BUILDER_CACHE = original; | ||
| } else { | ||
| delete process.env.PURGE_REMOTE_BUILDER_CACHE; | ||
| } |
There was a problem hiding this comment.
Make the env cleanup exception-safe.
Lines 145-158 restore PURGE_REMOTE_BUILDER_CACHE only on the happy path. If either assertion fails, the mutated env leaks into later tests. Please move this cleanup into afterEach or wrap each mutation in try/finally.
♻️ Suggested pattern
describe('purgeRemoteCaching', () => {
+ let original: string | undefined;
+
+ beforeEach(() => {
+ original = process.env.PURGE_REMOTE_BUILDER_CACHE;
+ });
+
+ afterEach(() => {
+ if (original === undefined) {
+ delete process.env.PURGE_REMOTE_BUILDER_CACHE;
+ } else {
+ process.env.PURGE_REMOTE_BUILDER_CACHE = original;
+ }
+ });
+
it('returns false when env var is not set', () => {
- const original = process.env.PURGE_REMOTE_BUILDER_CACHE;
delete process.env.PURGE_REMOTE_BUILDER_CACHE;
expect(OrchestratorFolders.purgeRemoteCaching).toBe(false);
- if (original !== undefined) process.env.PURGE_REMOTE_BUILDER_CACHE = original;
});
it('returns true when env var is set', () => {
- const original = process.env.PURGE_REMOTE_BUILDER_CACHE;
process.env.PURGE_REMOTE_BUILDER_CACHE = 'true';
expect(OrchestratorFolders.purgeRemoteCaching).toBe(true);
- if (original !== undefined) {
- process.env.PURGE_REMOTE_BUILDER_CACHE = original;
- } else {
- delete process.env.PURGE_REMOTE_BUILDER_CACHE;
- }
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/options/orchestrator-folders.test.ts` around lines 144
- 159, The two tests that mutate process.env.PURGE_REMOTE_BUILDER_CACHE around
OrchestratorFolders.purgeRemoteCaching are not exception-safe; wrap each env
mutation in a try/finally or move setup/teardown into a per-test afterEach to
guarantee restoration of the original value. Concretely, for the tests named
"returns false when env var is not set" and "returns true when env var is set",
capture the original process.env.PURGE_REMOTE_BUILDER_CACHE at the start,
perform the mutation and assertion inside try, and restore the original value in
finally (or instead use afterEach to reset
process.env.PURGE_REMOTE_BUILDER_CACHE to the saved original), ensuring no
leaked state affects other tests.
yargs@18 requires Node >=20.19.0. Downgrade to [email protected] to maintain Node 18 support. Revert integrity-check.yml back to Node 18. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The orchestrator-develop branch no longer exists. Update all fallback clone commands and test fixtures to use main instead. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/model/orchestrator/workflows/build-automation-workflow.ts (1)
95-104: Consider centralizing this clone fallback script.This
git ls-remote/clone requested branch -> main -> repo defaultblock now exists here and insrc/model/orchestrator/workflows/async-workflow.ts:30-37. Pulling it into a shared helper would reduce the chance of the fallback policy drifting again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/workflows/build-automation-workflow.ts` around lines 95 - 104, Extract the repeated git clone fallback block into a single reusable helper (e.g., a function named ensureRepoCloned or cloneWithBranchFallback) and replace the inline script in build-automation-workflow.ts and async-workflow.ts with calls to that helper, passing the branch, repo and destination (BRANCH/REPO/DEST or the corresponding Orchestrator/OrchestratorFolders values); implement the helper to run the same logic (git ls-remote check for branch, clone -b branch, else clone -b main || clone) and keep log/error messages identical to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/orchestrator-async-checks.yml:
- Line 57: Replace the hardcoded branch in the clone step so the workflow clones
the branch that triggered the dispatch; change the git clone command (the line
containing "git clone -b main https://github.com/game-ci/unity-builder") to use
the ORCHESTRATOR_BRANCH variable (e.g., git clone -b "${{
env.ORCHESTRATOR_BRANCH }}" <repo>) so it matches the existing
ORCHESTRATOR_BRANCH set from ${{ github.ref }}.
---
Nitpick comments:
In `@src/model/orchestrator/workflows/build-automation-workflow.ts`:
- Around line 95-104: Extract the repeated git clone fallback block into a
single reusable helper (e.g., a function named ensureRepoCloned or
cloneWithBranchFallback) and replace the inline script in
build-automation-workflow.ts and async-workflow.ts with calls to that helper,
passing the branch, repo and destination (BRANCH/REPO/DEST or the corresponding
Orchestrator/OrchestratorFolders values); implement the helper to run the same
logic (git ls-remote check for branch, clone -b branch, else clone -b main ||
clone) and keep log/error messages identical to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c27edec-9bd1-4678-9a8d-390bf99a5913
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (4)
.github/workflows/orchestrator-async-checks.ymlsrc/model/orchestrator/tests/e2e/orchestrator-end2end-caching.test.tssrc/model/orchestrator/workflows/async-workflow.tssrc/model/orchestrator/workflows/build-automation-workflow.ts
| CHECKS_UPDATE: ${{ github.event.inputs.checksObject }} | ||
| run: | | ||
| git clone -b orchestrator-develop https://github.com/game-ci/unity-builder | ||
| git clone -b main https://github.com/game-ci/unity-builder |
There was a problem hiding this comment.
Clone the dispatched branch instead of always cloning main.
This now runs async checks against main even when the workflow is dispatched from release/next-gen or another branch, so the workflow stops validating the branch that triggered it. It also conflicts with ORCHESTRATOR_BRANCH on Line 28, which still points at ${{ github.ref }}.
Suggested fix
- git clone -b main https://github.com/game-ci/unity-builder
+ git clone -b "${{ github.ref_name }}" https://github.com/game-ci/unity-builder📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| git clone -b main https://github.com/game-ci/unity-builder | |
| git clone -b "${{ github.ref_name }}" https://github.com/game-ci/unity-builder |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/orchestrator-async-checks.yml at line 57, Replace the
hardcoded branch in the clone step so the workflow clones the branch that
triggered the dispatch; change the git clone command (the line containing "git
clone -b main https://github.com/game-ci/unity-builder") to use the
ORCHESTRATOR_BRANCH variable (e.g., git clone -b "${{ env.ORCHESTRATOR_BRANCH
}}" <repo>) so it matches the existing ORCHESTRATOR_BRANCH set from ${{
github.ref }}.
A leftover <<<<<<< HEAD marker was causing all builds on this branch to fail with YAML parse error at line 305. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Cache is an orchestrator feature, so it belongs under `game-ci orchestrate cache` rather than as a top-level `game-ci cache` command. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Closing — superseded by the orchestrator extraction (PR #819). All next-gen orchestrator features are now in the standalone |
1 participant
Next-Gen Features — Combined Integration
This is the next-gen subset of the full LTS 2.0.0 release (#815). It excludes the 9 infrastructure PRs which are available separately on
release/lts-infrastructure(#816).What's in this PR
Official CLI —
game-ciRun builds from your terminal without GitHub Actions. Top-level commands:
build,test,activate,orchestrate,status,version,update. Orchestrator subcommands:orchestrate cache list|restore|clear.Hot runner protocol —
hotRunnerEnabledPersistent Unity editor instances for faster subsequent builds. Includes registry, health monitoring, and automatic fallback to cold builds.
Test workflow engine —
testSuitePathYAML-based test suite definitions with taxonomy filters, structured results (JUnit/JSON), and event-based suite selection (PR vs push vs release).
Generic artifact system —
artifactOutputTypes,artifactUploadTargetConfigurable output collection (build, logs, test-results, coverage, etc.) with upload to GitHub Artifacts, rclone storage, or local paths.
Incremental sync —
syncStrategyDelta-based workspace updates instead of full clone. Strategies:
full,git-delta,direct-input,storage-pull.Community plugin validation — scheduled compile and build checks for community plugins.
Component PRs
Related Issues
Tracks #795
Integration branches
release/lts-infrastructurerelease/next-genrelease/lts-2.0.0Merge conflict notes
All conflicts were additive — multiple branches adding properties/inputs to shared files. Resolved by keeping all additions from each branch.