Cloud Runner Improvements - S3 Locking, Aws Local Stack (Pipelines), Testing Improvements, Rclone storage support, Provider plugin system #731
Conversation
- Implemented a primary attempt to pull LFS files using GIT_PRIVATE_TOKEN. - Added a fallback mechanism to use GITHUB_TOKEN if the initial attempt fails. - Configured git to replace SSH and HTTPS URLs with token-based authentication for the fallback. - Improved error handling to log specific failure messages for both token attempts. This change ensures more robust handling of LFS file retrieval in various authentication scenarios.
- Added permissions for packages, pull-requests, statuses, and id-token to enhance workflow capabilities. - This change improves the CI pipeline's ability to manage pull requests and access necessary resources.
…ation - Added configuration to use GIT_PRIVATE_TOKEN for git operations, replacing SSH and HTTPS URLs with token-based authentication. - Improved error handling to ensure GIT_PRIVATE_TOKEN availability before attempting to pull LFS files. - This change streamlines the process of pulling LFS files in environments requiring token authentication.
…entication - Enhanced the process of configuring git to use GIT_PRIVATE_TOKEN and GITHUB_TOKEN by clearing existing URL configurations before setting new ones. - Improved the clarity of the URL replacement commands for better readability and maintainability. - This change ensures a more robust setup for pulling LFS files in environments requiring token authentication.
… pipeline - Replaced instances of GITHUB_TOKEN with GIT_PRIVATE_TOKEN in the cloud-runner CI pipeline configuration. - This change ensures consistent use of token-based authentication across various jobs in the workflow, enhancing security and functionality.
…L unsetting - Modified the git configuration commands to append '|| true' to prevent errors if the specified URLs do not exist. - This change enhances the reliability of the URL clearing process in the RemoteClient class, ensuring smoother execution during token-based authentication setups.
…tion - Updated comments for clarity regarding the purpose of URL configuration changes. - Simplified the git configuration commands by removing redundant lines while maintaining functionality for HTTPS token-based authentication. - This change enhances the readability and maintainability of the RemoteClient class's git setup process.
# Conflicts: # dist/index.js # dist/index.js.map # jest.config.js # yarn.lock
… log lines for test assertions
…off; lint/format fixes
… cache key for retained workspace (#379)
…logs; tests: retained workspace AWS assertion (#381)
…rd local provider steps
…nd log management; update builder path logic based on provider strategy
…sed on provider strategy and credentials; update binary files
…ained markers; hooks: include AWS S3 hooks on aws provider
…t:ci script; fix(windows): skip grep-based version regex tests; logs: echo CACHE_KEY/retained markers; hooks: include AWS hooks on aws provider
… update binary files
…rintf for empty input
…I hangs; s3 steps pass again
frostebite
requested review from
AndrewKahr,
EricRibeiro,
GabLeRoux,
MichaelBuhler,
cloudymax,
davidmfinol and
webbertakken
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| // prettier-ignore | ||
| return ` | ||
| echo "game ci start" | ||
| echo "game ci start" >> /home/job-log.txt | ||
| /entrypoint.sh | node ${builderPath} -m remote-cli-log-stream --logFile /home/job-log.txt | ||
| echo "game ci start" >> "$LOG_FILE" | ||
| timeout 3s node ${builderPath} -m remote-cli-log-stream --logFile "$LOG_FILE" || true | ||
| node ${builderPath} -m remote-cli-post-build`; |
There was a problem hiding this comment.
[P1] Local provider no longer executes build entrypoint
When the provider strategy is non‑containerized (local/local-system), BuildCommands now returns only a few log statements and remote-cli helpers but never stages the Unity build scripts or runs /entrypoint.sh. Before this change the same branch cloned the default build script, marked it executable and piped its output through the log stream, which actually performed the build. With the current code, local runs will immediately log success without invoking any build logic, so local provider builds will silently do nothing.
Useful? React with 👍 / 👎.
* feat: Add dynamic provider loader with improved error handling - Create provider-loader.ts with function-based dynamic import functionality - Update CloudRunner.setupSelectedBuildPlatform to use dynamic loader for unknown providers - Add comprehensive error handling for missing packages and interface validation - Include test coverage for successful loading and error scenarios - Maintain backward compatibility with existing built-in providers - Add ProviderLoader class wrapper for backward compatibility - Support both built-in providers (via switch) and external providers (via dynamic import) * fix: Resolve linting errors in provider loader - Fix TypeError usage instead of Error for type checking - Add missing blank lines for proper code formatting - Fix comment spacing issues * build: Update built artifacts after linting fixes - Rebuild dist/ with latest changes - Include updated provider loader in built bundle - Ensure all changes are reflected in compiled output * build: Update built artifacts after linting fixes - Rebuild dist/ with latest changes - Include updated provider loader in built bundle - Ensure all changes are reflected in compiled output * build: Update built artifacts after linting fixes - Rebuild dist/ with latest changes - Include updated provider loader in built bundle - Ensure all changes are reflected in compiled output * build: Update built artifacts after linting fixes - Rebuild dist/ with latest changes - Include updated provider loader in built bundle - Ensure all changes are reflected in compiled output * fix: Fix AWS job dependencies and remove duplicate localstack tests - Update AWS job to depend on both k8s and localstack jobs - Remove duplicate localstack tests from k8s job (now only runs k8s tests) - Remove unused cloud-runner-localstack job from main integrity check - Fix AWS SDK warnings by using Uint8Array(0) instead of empty string for S3 PutObject - Rename localstack-and-k8s job to k8s job for clarity * feat: Implement provider loader dynamic imports with GitHub URL support - Add URL detection and parsing utilities for GitHub URLs, local paths, and NPM packages - Implement git operations for cloning and updating repositories with local caching - Add automatic update checking mechanism for GitHub repositories - Update provider-loader.ts to support multiple source types with comprehensive error handling - Add comprehensive test coverage for all new functionality - Include complete documentation with usage examples - Support GitHub URLs: https://github.com/user/repo, user/repo@branch - Support local paths: ./path, /absolute/path - Support NPM packages: package-name, @scope/package - Maintain backward compatibility with existing providers - Add fallback mechanisms and interface validation * feat: Implement provider loader dynamic imports with GitHub URL support - Add URL detection and parsing utilities for GitHub URLs, local paths, and NPM packages - Implement git operations for cloning and updating repositories with local caching - Add automatic update checking mechanism for GitHub repositories - Update provider-loader.ts to support multiple source types with comprehensive error handling - Add comprehensive test coverage for all new functionality - Include complete documentation with usage examples - Support GitHub URLs: https://github.com/user/repo, user/repo@branch - Support local paths: ./path, /absolute/path - Support NPM packages: package-name, @scope/package - Maintain backward compatibility with existing providers - Add fallback mechanisms and interface validation * feat: Fix provider-loader tests and URL parser consistency - Fixed provider-loader test failures (constructor validation, module imports) - Fixed provider-url-parser to return consistent base URLs for GitHub sources - Updated error handling to use TypeError consistently - All provider-loader and provider-url-parser tests now pass - Fixed prettier and eslint formatting issues * feat: Implement provider loader dynamic imports with GitHub URL support - Add URL detection and parsing utilities for GitHub URLs, local paths, and NPM packages - Implement git operations for cloning and updating repositories with local caching - Add automatic update checking mechanism for GitHub repositories - Update provider-loader.ts to support multiple source types with comprehensive error handling - Add comprehensive test coverage for all new functionality - Include complete documentation with usage examples - Support GitHub URLs: https://github.com/user/repo, user/repo@branch - Support local paths: ./path, /absolute/path - Support NPM packages: package-name, @scope/package - Maintain backward compatibility with existing providers - Add fallback mechanisms and interface validation * feat: Implement provider loader dynamic imports with GitHub URL support - Add URL detection and parsing utilities for GitHub URLs, local paths, and NPM packages - Implement git operations for cloning and updating repositories with local caching - Add automatic update checking mechanism for GitHub repositories - Update provider-loader.ts to support multiple source types with comprehensive error handling - Add comprehensive test coverage for all new functionality - Include complete documentation with usage examples - Support GitHub URLs: https://github.com/user/repo, user/repo@branch - Support local paths: ./path, /absolute/path - Support NPM packages: package-name, @scope/package - Maintain backward compatibility with existing providers - Add fallback mechanisms and interface validation * m * m
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/model/cloud-runner/services/core/shared-workspace-locking.ts (3)
199-200: Bug: invalid Array.sort comparator breaks lock ordering.Provide two-arg comparator; current version returns arbitrary positives.
- .sort((x) => x.timestamp); + .sort((a, b) => a.timestamp - b.timestamp);
244-244: Bug: invalid sort comparator for workspaces.Same issue as above; fix comparator.
- ordered.sort((x) => x.timestamp); + ordered.sort((a, b) => a.timestamp - b.timestamp);
271-273: Bug: timestamp parsed as Number of whole filename → NaN.Split on "_" before Number().
- .map((x) => Number(x))[0]; + .map((x) => Number(x.split(`_`)[0]))[0];
♻️ Duplicate comments (9)
.github/workflows/cloud-runner-integrity.yml (5)
55-59: Avoid curl | bash; pin k3d with checksum verification.Reduces supply-chain risk and improves reproducibility.
- - name: Install k3d + - name: Install k3d (pinned + checksum) if: ${{ matrix.provider == 'k8s' }} run: | - curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash - k3d version | cat + set -euo pipefail + K3D_VERSION=v5.7.3 + curl -fsSL -o k3d "https://github.com/k3d-io/k3d/releases/download/${K3D_VERSION}/k3d-linux-amd64" + curl -fsSL -o k3d.sha256 "https://github.com/k3d-io/k3d/releases/download/${K3D_VERSION}/k3d-linux-amd64.sha256" + sha256sum --check k3d.sha256 + install -m 0755 k3d /usr/local/bin/k3d + k3d version | cat
12-19: Tighten workflow permissions (least privilege).Drop global actions: write and pull-requests: write; keep only what’s needed. If a job needs more, scope it at job-level.
permissions: - checks: write - contents: read - actions: write - packages: read - pull-requests: write - statuses: write - id-token: write + contents: read + checks: write + statuses: write + id-token: write
75-77: Create S3 bucket with region-aware API (non-us-east-1 needs LocationConstraint).Prevents failures when AWS_REGION != us-east-1 (you set eu-west-2).
- awslocal s3 mb s3://$AWS_STACK_NAME || true - awslocal s3 ls + if [ "${AWS_REGION}" = "us-east-1" ]; then + awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" || true + else + awslocal s3api create-bucket --bucket "$AWS_STACK_NAME" \ + --create-bucket-configuration LocationConstraint="${AWS_REGION}" || true + fi + awslocal s3 ls
103-104: Do not overwrite GITHUB_TOKEN with a private token.Use the default GitHub-provided token; only pass a PAT as a separate var if truly required.
- GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} - GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} + GITHUB_TOKEN: ${{ github.token }}Repeat the same fix in LocalStack and AWS jobs. If private repo access is needed, keep:
GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }}but do not assign it to GITHUB_TOKEN.
Also applies to: 154-155, 195-196
172-176: aws-actions/configure-aws-credentials@v1 is deprecated on current runners. Upgrade to v4.This will otherwise fail at runtime (actionlint already flags it).
- - name: Configure AWS Credentials - uses: aws-actions/configure-aws-credentials@v1 + - name: Configure AWS Credentials + uses: aws-actions/configure-aws-credentials@v4 with: aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} aws-region: ${{ env.AWS_REGION }}src/model/cloud-runner/services/core/shared-workspace-locking.ts (3)
46-48: Consider exposing workspacePrefix (public) for reuse.Helps other modules avoid hardcoding "locks/".
- private static get workspacePrefix() { + public static get workspacePrefix() { return `locks/`; }
14-16: Prevent command injection: replace exec('rclone …') with execFile and pass args.Use execFile with an args array and shell: false; update all rclone call sites accordingly.
-import { promisify } from 'node:util'; -import { exec as execCb } from 'node:child_process'; -const exec = promisify(execCb); +import { promisify } from 'node:util'; +import { execFile } from 'node:child_process'; +const execFileAsync = promisify(execFile); @@ - private static async rclone(command: string): Promise<string> { - const { stdout } = await exec(`rclone ${command}`); - return stdout.toString(); - } + private static async rclone(...args: string[]): Promise<string> { + const { stdout } = await execFileAsync('rclone', args, { shell: false, windowsHide: true }); + return stdout.toString(); + } @@ - await SharedWorkspaceLocking.rclone(`lsf ${bucket}`); + await SharedWorkspaceLocking.rclone('lsf', bucket); } catch { - await SharedWorkspaceLocking.rclone(`mkdir ${bucket}`); + await SharedWorkspaceLocking.rclone('mkdir', bucket); } @@ - const output = await SharedWorkspaceLocking.rclone(`lsjson ${path}`); + const output = await SharedWorkspaceLocking.rclone('lsjson', path); @@ - if (SharedWorkspaceLocking.useRclone) { - await SharedWorkspaceLocking.rclone(`touch ${SharedWorkspaceLocking.bucket}/${key}`); - } else { + if (SharedWorkspaceLocking.useRclone) { + await SharedWorkspaceLocking.rclone('touch', `${SharedWorkspaceLocking.bucket}/${key}`); + } else { @@ - if (SharedWorkspaceLocking.useRclone) { - await SharedWorkspaceLocking.rclone(`touch ${SharedWorkspaceLocking.bucket}/${key}`); - } else { + if (SharedWorkspaceLocking.useRclone) { + await SharedWorkspaceLocking.rclone('touch', `${SharedWorkspaceLocking.bucket}/${key}`); + } else { @@ - } else { + } else { await SharedWorkspaceLocking.s3.send( new PutObjectCommand({ Bucket: SharedWorkspaceLocking.bucket, Key: key, Body: new Uint8Array(0) }), ); } const hasLock = await SharedWorkspaceLocking.HasWorkspaceLock(workspace, runId, buildParametersContext); @@ - if (SharedWorkspaceLocking.useRclone) { - await SharedWorkspaceLocking.rclone(`delete ${SharedWorkspaceLocking.bucket}/${key}`); - } else { + if (SharedWorkspaceLocking.useRclone) { + await SharedWorkspaceLocking.rclone('delete', `${SharedWorkspaceLocking.bucket}/${key}`); + } else { await SharedWorkspaceLocking.s3.send( new DeleteObjectCommand({ Bucket: SharedWorkspaceLocking.bucket, Key: key }), ); } @@ - if (SharedWorkspaceLocking.useRclone) { - await SharedWorkspaceLocking.rclone( - `delete ${SharedWorkspaceLocking.bucket}/${SharedWorkspaceLocking.workspacePrefix}${buildParametersContext.cacheKey}/${file}`, - ); - } else { + if (SharedWorkspaceLocking.useRclone) { + await SharedWorkspaceLocking.rclone( + 'delete', + `${SharedWorkspaceLocking.bucket}/${SharedWorkspaceLocking.workspacePrefix}${buildParametersContext.cacheKey}/${file}`, + ); + } else { await SharedWorkspaceLocking.s3.send( new DeleteObjectCommand({ Bucket: SharedWorkspaceLocking.bucket, Key: `${SharedWorkspaceLocking.workspacePrefix}${buildParametersContext.cacheKey}/${file}`, }), ); } @@ - if (SharedWorkspaceLocking.useRclone) { - await SharedWorkspaceLocking.rclone(`delete ${SharedWorkspaceLocking.bucket}/${prefix}${file}`); - } else { + if (SharedWorkspaceLocking.useRclone) { + await SharedWorkspaceLocking.rclone('delete', `${SharedWorkspaceLocking.bucket}/${prefix}${file}`); + } else { await SharedWorkspaceLocking.s3.send( new DeleteObjectCommand({ Bucket: SharedWorkspaceLocking.bucket, Key: `${prefix}${file}` }), ); }Also applies to: 29-32, 53-56, 78-84, 299-305, 331-337, 344-350, 368-379, 389-395
85-96: ListObjectsV2 not paginated — results truncated at 1,000 keys.Paginate to avoid missing workspaces/locks in larger repos.
- const result = await SharedWorkspaceLocking.s3.send( - new ListObjectsV2Command({ Bucket: bucket, Prefix: prefix, Delimiter: '/' }), - ); - const entries: string[] = []; - for (const p of result.CommonPrefixes || []) { - if (p.Prefix) entries.push(p.Prefix.slice(prefix.length)); - } - for (const c of result.Contents || []) { - if (c.Key && c.Key !== prefix) entries.push(c.Key.slice(prefix.length)); - } - return entries; + const entries: string[] = []; + let token: string | undefined; + do { + const result = await SharedWorkspaceLocking.s3.send( + new ListObjectsV2Command({ + Bucket: bucket, + Prefix: prefix, + Delimiter: '/', + ContinuationToken: token, + }), + ); + for (const p of result.CommonPrefixes || []) { + if (p.Prefix) entries.push(p.Prefix.slice(prefix.length)); + } + for (const c of result.Contents || []) { + if (c.Key && c.Key !== prefix) entries.push(c.Key.slice(prefix.length)); + } + token = result.NextContinuationToken; + } while (token); + return entries;src/model/cloud-runner/cloud-runner.ts (1)
94-123: Fail fast on unknown provider instead of silently falling back to localDefaulting to LocalCloudRunner can mask misconfiguration and run workloads in unintended environments. Prefer throwing (or gate fallback behind an explicit flag).
Two options:
- Throw on unknown provider:
default: - // Try to load provider using the dynamic loader for unknown providers - try { - CloudRunner.Provider = await loadProvider(provider, CloudRunner.buildParameters); - } catch (error: any) { - CloudRunnerLogger.log(`Failed to load provider '${provider}' using dynamic loader: ${error.message}`); - CloudRunnerLogger.log('Falling back to local provider...'); - CloudRunner.Provider = new LocalCloudRunner(); - } - break; + // Unknown provider -> fail fast + throw new CloudRunnerError(`Unknown or failed provider: "${provider}". Refusing to fall back to 'local'.`);
- Gate fallback with an env flag:
default: - // Try to load provider using the dynamic loader for unknown providers + // Try to load provider using the dynamic loader for unknown providers try { CloudRunner.Provider = await loadProvider(provider, CloudRunner.buildParameters); } catch (error: any) { - CloudRunnerLogger.log(`Failed to load provider '${provider}' using dynamic loader: ${error.message}`); - CloudRunnerLogger.log('Falling back to local provider...'); - CloudRunner.Provider = new LocalCloudRunner(); + CloudRunnerLogger.log(`Failed to load provider '${provider}' using dynamic loader: ${error.message}`); + if (process.env.CLOUD_RUNNER_ALLOW_PROVIDER_FALLBACK === 'true') { + CloudRunnerLogger.log('Falling back to local provider due to CLOUD_RUNNER_ALLOW_PROVIDER_FALLBACK=true'); + CloudRunner.Provider = new LocalCloudRunner(); + } else { + throw new CloudRunnerError( + `Unknown or failed provider: "${provider}". Set CLOUD_RUNNER_ALLOW_PROVIDER_FALLBACK=true to allow fallback.` + ); + } } break;
🧹 Nitpick comments (19)
.github/workflows/cloud-runner-integrity.yml (5)
71-74: Pin LocalStack action to a specific version/commit.Use a tagged version or commit SHA to prevent supply-chain drift.
- - name: Start LocalStack (S3) - uses: localstack/[email protected] + - name: Start LocalStack (S3) + uses: localstack/[email protected] # consider pinning by commit SHA
1-11: Optional: expose region as an input for reuse and flexibility.Let callers set AWS region; default to eu-west-2.
on: workflow_call: inputs: + awsRegion: + description: 'AWS region for tests' + required: false + default: 'eu-west-2' + type: string runGithubIntegrationTests: description: 'Run GitHub Checks integration tests' required: false default: 'false' type: stringAnd in env:
- AWS_REGION: eu-west-2 - AWS_DEFAULT_REGION: eu-west-2 + AWS_REGION: ${{ inputs.awsRegion }} + AWS_DEFAULT_REGION: ${{ inputs.awsRegion }}
1-1: Optional: add concurrency to cancel superseded runs.Prevents CI pileups on fast pushes.
name: cloud-runner-integrity +concurrency: + group: cloud-runner-integrity-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
188-194: Optional: avoid duplicating AWS creds in env after configure-aws-credentials.The action exports creds; unless tests explicitly require env injection, you can remove these to reduce exposure.
- AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} - AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}If tests need them, keep as-is.
25-26: Confirm branch/ref format.CLOUD_RUNNER_BRANCH uses github.ref (e.g., refs/heads/feature/x). If consumers expect a raw branch name, switch to github.ref_name.
- CLOUD_RUNNER_BRANCH: ${{ github.ref }} + CLOUD_RUNNER_BRANCH: ${{ github.ref_name }}src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
257-257: Type: returnnumbernotNumber.Use primitive type to match conventions and avoid subtle typing pitfalls.
- ): Promise<Number> { + ): Promise<number> {
63-68: Nit: rename createParams → createParameters (ESLint).Minor naming clarity; aligns with unicorn/prevent-abbreviations.
- const createParams: any = { Bucket: bucket }; + const createParameters: any = { Bucket: bucket }; if (region && region !== 'us-east-1') { - createParams.CreateBucketConfiguration = { LocationConstraint: region }; + createParameters.CreateBucketConfiguration = { LocationConstraint: region }; } - await SharedWorkspaceLocking.s3.send(new CreateBucketCommand(createParams)); + await SharedWorkspaceLocking.s3.send(new CreateBucketCommand(createParameters));src/model/cloud-runner/providers/provider-url-parser.ts (2)
48-60: Extend SSH/HTTPS parsing to more GitHub URL shapes (optional)Support
ssh://[email protected]/owner/repo.gitandgit+https://variants; they’re common in package metadata.Example addition:
-const githubSshMatch = source.match(/^git@github\.com:([^/]+)\/([^/]+?)(?:\.git)?\/?(?:tree\/([^/]+))?(?:\/(.+))?$/); +const githubSshMatch = + source.match(/^git@github\.com:([^/]+)\/([^/]+?)(?:\.git)?\/?(?:tree\/([^/]+))?(?:\/(.+))?$/) || + source.match(/^ssh:\/\/git@github\.com\/([^/]+)\/([^/]+?)(?:\.git)?(?:#([^/]+))?(?:\/(.+))?$/);
118-128: Avoid logging 'Branch: undefined'Tiny UX tweak: show '(default)' when branch is not specified.
- CloudRunnerLogger.log(` Branch: ${parsed.branch}`); + CloudRunnerLogger.log(` Branch: ${parsed.branch ?? '(default)'}`);src/model/cloud-runner/tests/providers/provider-git-manager.test.ts (1)
48-75: These tests only assert mocked return plumbingConsider at least asserting the called-with arguments to catch regressions in API usage.
Example:
-const result = await mockProviderGitManager.cloneRepository(mockUrlInfo); +const result = await mockProviderGitManager.cloneRepository(mockUrlInfo); expect(result.success).toBe(true); expect(result.localPath).toBe('/path/to/cloned/repo'); +expect(mockProviderGitManager.cloneRepository).toHaveBeenCalledWith(mockUrlInfo);src/model/cloud-runner/tests/providers/provider-loader.test.ts (2)
25-36: Also assert getProviderModulePath is invoked for GitHub sourcesImproves coverage of the Git path resolution.
await expect(loadProvider('https://github.com/user/repo', {} as any)).rejects.toThrow(); expect(mockProviderGitManager.ensureRepositoryAvailable).toHaveBeenCalled(); +expect(mockProviderGitManager.getProviderModulePath).toHaveBeenCalledWith( + expect.objectContaining({ owner: 'user', repo: 'repo' }), + '/path/to/cloned/repo', +);
5-8: Stabilize class static method mocksAuto-mocking classes with static methods can be flaky. Provide an explicit mock factory to ensure methods exist.
-jest.mock('../../providers/provider-git-manager'); +jest.mock('../../providers/provider-git-manager', () => ({ + ProviderGitManager: { + ensureRepositoryAvailable: jest.fn(), + getProviderModulePath: jest.fn(), + cleanupOldRepositories: jest.fn(), + }, +}));src/model/cloud-runner/tests/providers/provider-url-parser.test.ts (2)
29-39: Add edge-case tests (trailing slash, shorthand with path, invalid/ambiguous inputs)To lock behavior and prevent regressions, add a few more assertions:
- Trailing slash on GitHub URL.
- Shorthand with path but no branch (user/repo/path).
- Reject/handle odd inputs (e.g., git+https://, enterprise GitHub domains if unsupported).
Apply near existing cases:
@@ it('parses HTTPS GitHub URLs with path', () => { @@ }); + + it('parses HTTPS GitHub URLs with trailing slash', () => { + const result = parseProviderSource('https://github.com/user/repo/'); + expect(result).toEqual({ + type: 'github', + owner: 'user', + repo: 'repo', + branch: 'main', + path: '', + url: 'https://github.com/user/repo', + }); + }); @@ it('parses shorthand GitHub references with path', () => { @@ }); + + it('parses shorthand with path and default branch', () => { + const result = parseProviderSource('user/repo/path/to/dir'); + expect(result).toEqual({ + type: 'github', + owner: 'user', + repo: 'repo', + branch: 'main', + path: 'path/to/dir', + url: 'https://github.com/user/repo', + }); + });Also applies to: 65-87, 89-99
170-185: Clarify branch-with-slash supportThe parser currently can’t represent branches containing “/” (e.g., feature/x) when using HTTPS “tree/…” or shorthand “@…/…”. Either document this limitation or extend parsing to support URL-encoded branches. Add a test once the decision is made.
src/model/cloud-runner/cloud-runner.ts (1)
70-92: Broaden LocalStack detection heuristics (low risk improvement)Consider also matching 0.0.0.0, host.docker.internal, and LocalStack’s s3.localhost.localstack.cloud endpoints to reduce false negatives in CI.
- const isLocalStack = /localstack|localhost|127\.0\.0\.1/i.test(endpointsToCheck); + const isLocalStack = /localstack|localhost|127\.0\.0\.1|0\.0\.0\.0|host\.docker\.internal|s3\.localhost\.localstack\.cloud/i.test(endpointsToCheck);src/model/cloud-runner/providers/provider-git-manager.ts (2)
97-106: Log errors with error severityUse CloudRunnerLogger.error for failures to improve observability.
- CloudRunnerLogger.log(`Error: ${errorMessage}`); + CloudRunnerLogger.error(`Error: ${errorMessage}`); @@ - CloudRunnerLogger.log(`Error: ${errorMessage}`); + CloudRunnerLogger.error(`Error: ${errorMessage}`); @@ - CloudRunnerLogger.log(`Error during cleanup: ${error.message}`); + CloudRunnerLogger.error(`Error during cleanup: ${error.message}`);Also applies to: 167-175, 274-276
25-37: Cache location: consider OS tempdir to avoid polluting repo root (optional)Storing under process.cwd() ties the cache to the workspace. Using os.tmpdir() or a configurable path can reduce noise and avoid permission issues.
- private static readonly CACHE_DIR = path.join(process.cwd(), '.provider-cache'); + private static readonly CACHE_DIR = + process.env.PROVIDER_CACHE_DIR || path.join(require('os').tmpdir(), 'unity-builder-provider-cache');src/model/cloud-runner/providers/provider-loader.ts (2)
95-112: Reject non-constructors early with clearer messageThe current message is fine; consider hinting expected export shape (default class or constructor function).
- if (typeof Provider !== 'function') { - throw new TypeError(`Provider package '${providerSource}' does not export a constructor function`); - } + if (typeof Provider !== 'function') { + throw new TypeError( + `Provider '${providerSource}' must export a constructor (default export or module.exports = class ...)` + ); + }
138-141: Expose built-in provider map from a single source to avoid drift (optional)Consider centralizing the map used here and in CloudRunner to a shared constant to prevent mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (14)
.cursor/settings.json(1 hunks).github/workflows/cloud-runner-integrity.yml(1 hunks).github/workflows/integrity-check.yml(1 hunks)src/model/cloud-runner/cloud-runner.ts(4 hunks)src/model/cloud-runner/providers/README.md(1 hunks)src/model/cloud-runner/providers/provider-git-manager.ts(1 hunks)src/model/cloud-runner/providers/provider-loader.ts(1 hunks)src/model/cloud-runner/providers/provider-url-parser.ts(1 hunks)src/model/cloud-runner/services/core/shared-workspace-locking.ts(8 hunks)src/model/cloud-runner/tests/fixtures/invalid-provider.ts(1 hunks)src/model/cloud-runner/tests/providers/provider-git-manager.test.ts(1 hunks)src/model/cloud-runner/tests/providers/provider-loader.test.ts(1 hunks)src/model/cloud-runner/tests/providers/provider-url-parser.test.ts(1 hunks)src/model/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .cursor/settings.json
- src/model/cloud-runner/providers/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integrity-check.yml
🧰 Additional context used
🧬 Code graph analysis (8)
src/model/cloud-runner/providers/provider-url-parser.ts (1)
src/model/github.ts (2)
owner(44-46)repo(48-50)
src/model/cloud-runner/providers/provider-loader.ts (5)
src/model/index.ts (3)
loadProvider(28-28)BuildParameters(17-17)ProviderLoader(29-29)src/model/cloud-runner/providers/provider-interface.ts (1)
ProviderInterface(7-57)src/model/cloud-runner/providers/provider-url-parser.ts (3)
parseProviderSource(29-90)logProviderSource(117-138)ProviderSourceInfo(22-22)src/model/cloud-runner/providers/provider-git-manager.ts (1)
ProviderGitManager(25-278)src/model/cloud-runner/services/core/cloud-runner-logger.ts (1)
error(24-26)
src/model/cloud-runner/tests/providers/provider-git-manager.test.ts (2)
src/model/cloud-runner/providers/provider-git-manager.ts (1)
ProviderGitManager(25-278)src/model/cloud-runner/providers/provider-url-parser.ts (1)
GitHubUrlInfo(3-10)
src/model/cloud-runner/tests/providers/provider-loader.test.ts (3)
src/model/cloud-runner/providers/provider-git-manager.ts (1)
ProviderGitManager(25-278)src/model/cloud-runner/providers/provider-interface.ts (1)
ProviderInterface(7-57)src/model/cloud-runner/providers/provider-loader.ts (3)
loadProvider(15-117)loadProvider(130-132)ProviderLoader(122-158)
src/model/cloud-runner/tests/providers/provider-url-parser.test.ts (1)
src/model/cloud-runner/providers/provider-url-parser.ts (3)
parseProviderSource(29-90)generateCacheKey(97-99)isGitHubSource(106-110)
src/model/cloud-runner/providers/provider-git-manager.ts (2)
src/model/cloud-runner/providers/provider-url-parser.ts (2)
GitHubUrlInfo(3-10)generateCacheKey(97-99)src/model/cloud-runner/services/core/cloud-runner-logger.ts (1)
error(24-26)
src/model/cloud-runner/cloud-runner.ts (2)
src/model/cloud-runner/providers/provider-loader.ts (2)
loadProvider(15-117)loadProvider(130-132)src/index.ts (1)
runMain(7-51)
src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
src/model/cloud-runner/providers/aws/aws-client-factory.ts (1)
AwsClientFactory(9-71)src/model/input.ts (1)
region(48-50)
🪛 ESLint
src/model/cloud-runner/tests/providers/provider-git-manager.test.ts
[error] 15-15: 'mockExecAsync' is assigned a value but never used.
(no-unused-vars)
[error] 18-26: Expected blank line before this statement.
(padding-line-between-statements)
[error] 29-29: 'mockFs' is assigned a value but never used.
(no-unused-vars)
[error] 32-32: Import in body of module; reorder to top.
(import/first)
src/model/cloud-runner/services/core/shared-workspace-locking.ts
[error] 15-15: The variable execCb should be named execCallback. A more descriptive name will do too.
(unicorn/prevent-abbreviations)
[error] 24-24: Expected blank line before this statement.
(padding-line-between-statements)
[error] 31-31: Expected blank line before this statement.
(padding-line-between-statements)
[error] 57-57: Expected blank line before this statement.
(padding-line-between-statements)
[error] 63-63: The variable createParams should be named createParameters. A more descriptive name will do too.
(unicorn/prevent-abbreviations)
[error] 80-80: Expected blank line before this statement.
(padding-line-between-statements)
[error] 80-80: Please rename the variable e. Suggested names are: error, event. A more descriptive name will do too.
(unicorn/prevent-abbreviations)
[error] 95-95: Expected blank line before this statement.
(padding-line-between-statements)
[error] 299-305: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 331-337: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 344-350: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 368-379: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 389-395: This if statement can be replaced by a ternary expression.
(unicorn/prefer-ternary)
[error] 404-404: Expected blank line before this statement.
(padding-line-between-statements)
🪛 actionlint (1.7.7)
.github/workflows/cloud-runner-integrity.yml
172-172: the runner of "aws-actions/configure-aws-credentials@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: iOS on 2021.3.45f1
🔇 Additional comments (3)
src/model/cloud-runner/tests/fixtures/invalid-provider.ts (1)
1-1: Fixture looks goodMinimal class is fine for negative-path tests.
src/model/index.ts (1)
13-13: Public re-exports LGTMRe-exporting loader APIs here is convenient for consumers.
Also applies to: 28-30
src/model/cloud-runner/tests/providers/provider-url-parser.test.ts (1)
3-16: Solid coverage of parser happy-pathsThese tests exercise HTTPS/SSH/shorthand/local/npm flows well and mirror the current parser behavior.
| UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} | ||
| UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} | ||
| UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} | ||
| PROJECT_PATH: test-project | ||
| TARGET_PLATFORM: StandaloneWindows64 | ||
| cloudRunnerTests: true | ||
| versioning: None | ||
| KUBE_STORAGE_CLASS: local-path | ||
| PROVIDER_STRATEGY: aws | ||
| AWS_ACCESS_KEY_ID: test | ||
| AWS_SECRET_ACCESS_KEY: test | ||
| AWS_ENDPOINT: http://localhost:4566 | ||
| AWS_ENDPOINT_URL: http://localhost:4566 | ||
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align LocalStack env with k8s job (path-style + IMDS off).
LocalStack works more reliably with path-style addressing and disabled EC2 metadata.
env:
UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
@@
- AWS_ENDPOINT: http://localhost:4566
- AWS_ENDPOINT_URL: http://localhost:4566
+ AWS_ENDPOINT: http://localhost:4566
+ AWS_ENDPOINT_URL: http://localhost:4566
+ AWS_S3_FORCE_PATH_STYLE: 'true'
+ AWS_EC2_METADATA_DISABLED: 'true'📝 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.
| UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} | |
| UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} | |
| UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} | |
| PROJECT_PATH: test-project | |
| TARGET_PLATFORM: StandaloneWindows64 | |
| cloudRunnerTests: true | |
| versioning: None | |
| KUBE_STORAGE_CLASS: local-path | |
| PROVIDER_STRATEGY: aws | |
| AWS_ACCESS_KEY_ID: test | |
| AWS_SECRET_ACCESS_KEY: test | |
| AWS_ENDPOINT: http://localhost:4566 | |
| AWS_ENDPOINT_URL: http://localhost:4566 | |
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | |
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | |
| UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} | |
| UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} | |
| UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} | |
| PROJECT_PATH: test-project | |
| TARGET_PLATFORM: StandaloneWindows64 | |
| cloudRunnerTests: true | |
| versioning: None | |
| KUBE_STORAGE_CLASS: local-path | |
| PROVIDER_STRATEGY: aws | |
| AWS_ACCESS_KEY_ID: test | |
| AWS_SECRET_ACCESS_KEY: test | |
| AWS_ENDPOINT: http://localhost:4566 | |
| AWS_ENDPOINT_URL: http://localhost:4566 | |
| AWS_S3_FORCE_PATH_STYLE: 'true' | |
| AWS_EC2_METADATA_DISABLED: 'true' | |
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | |
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} |
🤖 Prompt for AI Agents
In .github/workflows/cloud-runner-integrity.yml around lines 141 to 155, the
LocalStack-related environment for the job needs path-style S3 addressing and
EC2 metadata disabled; add AWS_S3_FORCE_PATH_STYLE: "true" and
AWS_EC2_METADATA_DISABLED: "true" (or true without quotes consistent with
existing YAML style) to the env block alongside AWS_ENDPOINT/AWS_ENDPOINT_URL so
the job uses path-style requests and ignores IMDS when interacting with
LocalStack.
| import { exec } from 'child_process'; | ||
| import { promisify } from 'util'; | ||
| import * as fs from 'fs'; | ||
| import path from 'path'; | ||
| import CloudRunnerLogger from '../services/core/cloud-runner-logger'; | ||
| import { GitHubUrlInfo, generateCacheKey } from './provider-url-parser'; | ||
|
|
||
| const execAsync = promisify(exec); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent command injection and hangs; use execFile with prompts disabled
Building git commands via string interpolation is unsafe and may hang on auth prompts. Switch to execFile with args and GIT_TERMINAL_PROMPT=0; also make timeout configurable.
- import { exec } from 'child_process';
+ import { exec, execFile } from 'child_process';
@@
-const execAsync = promisify(exec);
+const execAsync = promisify(exec);
+const execFileAsync = promisify(execFile);
@@
- private static readonly GIT_TIMEOUT = 30000; // 30 seconds
+ private static readonly GIT_TIMEOUT = Number(process.env.PROVIDER_GIT_TIMEOUT_MS) || 30000; // 30 seconds (configurable)
@@
- const cloneCommand = `git clone --depth 1 --branch ${urlInfo.branch} ${urlInfo.url} "${localPath}"`;
- CloudRunnerLogger.log(`Executing: ${cloneCommand}`);
-
- const { stderr } = await execAsync(cloneCommand, {
- timeout: this.GIT_TIMEOUT,
- cwd: this.CACHE_DIR,
- });
+ const args = ['clone', '--depth', '1', '--branch', String(urlInfo.branch), urlInfo.url, localPath];
+ CloudRunnerLogger.log(`Executing: git ${args.join(' ')}`);
+ const { stderr } = await execFileAsync('git', args, {
+ timeout: this.GIT_TIMEOUT,
+ cwd: this.CACHE_DIR,
+ env: { ...process.env, GIT_TERMINAL_PROMPT: '0' },
+ });
@@
- await execAsync('git fetch origin', {
+ await execFileAsync('git', ['fetch', 'origin'], {
timeout: this.GIT_TIMEOUT,
cwd: localPath,
+ env: { ...process.env, GIT_TERMINAL_PROMPT: '0' },
});
@@
- const { stdout: statusOutput } = await execAsync(`git status -uno`, {
- timeout: this.GIT_TIMEOUT,
- cwd: localPath,
- });
+ const { stdout: head } = await execFileAsync('git', ['rev-parse', 'HEAD'], { timeout: this.GIT_TIMEOUT, cwd: localPath });
+ const { stdout: remote } = await execFileAsync('git', ['rev-parse', `origin/${String(urlInfo.branch)}`], { timeout: this.GIT_TIMEOUT, cwd: localPath });Also applies to: 76-86, 128-139, 146-151
| const hasUpdates = | ||
| statusOutput.includes('Your branch is behind') || statusOutput.includes('can be fast-forwarded'); | ||
|
|
||
| if (hasUpdates) { | ||
| CloudRunnerLogger.log(`Updates available, pulling latest changes...`); | ||
|
|
||
| // Reset to origin/branch to get latest changes | ||
| await execAsync(`git reset --hard origin/${urlInfo.branch}`, { | ||
| timeout: this.GIT_TIMEOUT, | ||
| cwd: localPath, | ||
| }); | ||
|
|
||
| CloudRunnerLogger.log(`Repository updated successfully`); | ||
|
|
||
| return { | ||
| success: true, | ||
| updated: true, | ||
| }; | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use commit hashes to detect updates instead of parsing “git status” output
Comparing HEAD vs origin/branch is more reliable across locales and git versions.
- const hasUpdates =
- statusOutput.includes('Your branch is behind') || statusOutput.includes('can be fast-forwarded');
+ const hasUpdates = head.trim() !== remote.trim();
@@
- await execAsync(`git reset --hard origin/${urlInfo.branch}`, {
- timeout: this.GIT_TIMEOUT,
- cwd: localPath,
- });
+ await execFileAsync('git', ['reset', '--hard', `origin/${String(urlInfo.branch)}`], {
+ timeout: this.GIT_TIMEOUT,
+ cwd: localPath,
+ env: { ...process.env, GIT_TERMINAL_PROMPT: '0' },
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/model/cloud-runner/providers/provider-git-manager.ts around lines 140 to
158, replace the brittle text-based detection of updates with commit-hash
comparison: run a git fetch first, then obtain the local commit hash (git
rev-parse HEAD) and the remote branch commit hash (git rev-parse
origin/<branch>), compare the two hashes and treat any difference as
"hasUpdates"; if different, perform the existing reset/pull flow and return
updated: true. Use the same execAsync calls with the existing timeout and cwd
options, and handle errors as currently done.
| // Look for common provider entry points | ||
| const commonEntryPoints = [ | ||
| 'index.js', | ||
| 'index.ts', | ||
| 'src/index.js', | ||
| 'src/index.ts', | ||
| 'lib/index.js', | ||
| 'lib/index.ts', | ||
| 'dist/index.js', | ||
| 'dist/index.js.map', | ||
| ]; | ||
|
|
||
| for (const entryPoint of commonEntryPoints) { | ||
| const fullPath = path.join(localPath, entryPoint); | ||
| if (fs.existsSync(fullPath)) { | ||
| CloudRunnerLogger.log(`Found provider entry point: ${entryPoint}`); | ||
|
|
||
| return fullPath; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not treat source maps or TypeScript files as entry points
Importing dist/index.js.map or .ts will fail at runtime in Node without a TS loader. Prefer JS artifacts only; throw if none found.
- const commonEntryPoints = [
- 'index.js',
- 'index.ts',
- 'src/index.js',
- 'src/index.ts',
- 'lib/index.js',
- 'lib/index.ts',
- 'dist/index.js',
- 'dist/index.js.map',
- ];
+ const commonEntryPoints = [
+ 'dist/index.cjs',
+ 'dist/index.js',
+ 'lib/index.cjs',
+ 'lib/index.js',
+ 'index.cjs',
+ 'index.js',
+ 'src/index.js', // in case sources are JS
+ ];
@@
- // Default to repository root
- CloudRunnerLogger.log(`No specific entry point found, using repository root`);
-
- return localPath;
+ // No valid entry point found
+ throw new Error(`No loadable JS entry point found in provider repo at ${localPath}`);📝 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.
| // Look for common provider entry points | |
| const commonEntryPoints = [ | |
| 'index.js', | |
| 'index.ts', | |
| 'src/index.js', | |
| 'src/index.ts', | |
| 'lib/index.js', | |
| 'lib/index.ts', | |
| 'dist/index.js', | |
| 'dist/index.js.map', | |
| ]; | |
| for (const entryPoint of commonEntryPoints) { | |
| const fullPath = path.join(localPath, entryPoint); | |
| if (fs.existsSync(fullPath)) { | |
| CloudRunnerLogger.log(`Found provider entry point: ${entryPoint}`); | |
| return fullPath; | |
| // Look for common provider entry points | |
| const commonEntryPoints = [ | |
| 'dist/index.cjs', | |
| 'dist/index.js', | |
| 'lib/index.cjs', | |
| 'lib/index.js', | |
| 'index.cjs', | |
| 'index.js', | |
| 'src/index.js', // in case sources are JS | |
| ]; | |
| for (const entryPoint of commonEntryPoints) { | |
| const fullPath = path.join(localPath, entryPoint); | |
| if (fs.existsSync(fullPath)) { | |
| CloudRunnerLogger.log(`Found provider entry point: ${entryPoint}`); | |
| return fullPath; | |
| } | |
| } | |
| // No valid entry point found | |
| throw new Error(`No loadable JS entry point found in provider repo at ${localPath}`); |
🤖 Prompt for AI Agents
In src/model/cloud-runner/providers/provider-git-manager.ts around lines 224 to
241, the code currently treats .ts and .map files as valid entry points which
will break at runtime; update the commonEntryPoints array to include only
JavaScript artifacts (e.g., index.js, src/index.js, lib/index.js, dist/index.js)
remove any .ts and .map entries, iterate those paths and return the first
existing JS file, and if none are found throw a clear error indicating no
JavaScript entry point was located so callers can fail fast.
| let modulePath: string; | ||
| let importedModule: any; | ||
|
|
||
| try { | ||
| // Handle different source types | ||
| switch (sourceInfo.type) { | ||
| case 'github': { | ||
| CloudRunnerLogger.log(`Processing GitHub repository: ${sourceInfo.owner}/${sourceInfo.repo}`); | ||
|
|
||
| // Ensure the repository is available locally | ||
| const localRepoPath = await ProviderGitManager.ensureRepositoryAvailable(sourceInfo); | ||
|
|
||
| // Get the path to the provider module within the repository | ||
| modulePath = ProviderGitManager.getProviderModulePath(sourceInfo, localRepoPath); | ||
|
|
||
| CloudRunnerLogger.log(`Loading provider from: ${modulePath}`); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Robust import of absolute file paths (ESM/CJS compatibility) and error-level logging
When modulePath is an absolute filesystem path, import via file:// URL to avoid resolution edge cases. Also log errors with error severity before rethrowing.
-import { parseProviderSource, logProviderSource, ProviderSourceInfo } from './provider-url-parser';
+import { parseProviderSource, logProviderSource, ProviderSourceInfo } from './provider-url-parser';
+import path from 'path';
+import { pathToFileURL } from 'url';
@@
- // Import the module
- importedModule = await import(modulePath);
+ // Import the module (support absolute filesystem paths)
+ const specifier = path.isAbsolute(modulePath) ? pathToFileURL(modulePath).href : modulePath;
+ importedModule = await import(specifier);
} catch (error) {
- throw new Error(`Failed to load provider package '${providerSource}': ${(error as Error).message}`);
+ CloudRunnerLogger.error(
+ `Failed to load provider package '${providerSource}': ${(error as Error).message}`
+ );
+ throw new Error(`Failed to load provider package '${providerSource}': ${(error as Error).message}`);
}Also applies to: 73-77
🤖 Prompt for AI Agents
In src/model/cloud-runner/providers/provider-loader.ts around lines 25-41 (and
apply the same change to lines 73-77), ensure absolute filesystem modulePath
values are converted to a file:// URL (use pathToFileURL from 'url') before
dynamic import to avoid ESM/CJS resolution edge cases; wrap the import/require
in a try/catch, log failures with CloudRunnerLogger.error including the
modulePath and error, then rethrow the error so callers can handle it.
| branch: branch || 'main', | ||
| path: path || '', | ||
| url: `https://github.com/${owner}/${repo}`, | ||
| }; |
There was a problem hiding this comment.
Don't hardcode 'main' — leave branch optional and adjust cache key
Defaulting to 'main' breaks repos whose default branch isn't 'main'. Let the branch be undefined and handle it in the git layer; update cache key accordingly.
Apply:
repo,
- branch: branch || 'main',
+ branch,
path: path || '',
url: `https://github.com/${owner}/${repo}`,
};
@@
repo,
- branch: branch || 'main',
+ branch,
path: path || '',
url: `https://github.com/${owner}/${repo}`,
};
@@
repo,
- branch: branch || 'main',
+ branch,
path: path || '',
url: `https://github.com/${owner}/${repo}`,
};
@@
export function generateCacheKey(urlInfo: GitHubUrlInfo): string {
- return `github_${urlInfo.owner}_${urlInfo.repo}_${urlInfo.branch}`.replace(/[^\w-]/g, '_');
+ const branch = urlInfo.branch ?? 'HEAD';
+ return `github_${urlInfo.owner}_${urlInfo.repo}_${branch}`.replace(/[^\w-]/g, '_');
}Follow-up outside this file (ProviderGitManager.cloneRepository): include --branch only if provided; otherwise clone default branch:
// inside ProviderGitManager.cloneRepository
const branchArg = urlInfo.branch ? `--branch ${urlInfo.branch}` : '';
const cloneCommand = `git clone --depth 1 ${branchArg} ${urlInfo.url} "${localPath}"`;Also applies to: 56-59, 71-74, 97-99
| import { GitHubUrlInfo } from '../../providers/provider-url-parser'; | ||
| import * as fs from 'fs'; | ||
|
|
||
| // Mock @actions/core to fix fs.promises compatibility issue | ||
| jest.mock('@actions/core', () => ({ | ||
| info: jest.fn(), | ||
| warning: jest.fn(), | ||
| error: jest.fn(), | ||
| })); | ||
|
|
||
| // Mock fs module | ||
| jest.mock('fs'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix ESLint errors: import order and unused vars
Resolve import/first and no-unused-vars to keep CI green.
Apply:
+import { ProviderGitManager } from '../../providers/provider-git-manager';
import { GitHubUrlInfo } from '../../providers/provider-url-parser';
import * as fs from 'fs';
// Mock @actions/core to fix fs.promises compatibility issue
@@
-const mockExecAsync = jest.fn();
@@
-const mockFs = fs as jest.Mocked<typeof fs>;
@@
-// Import the mocked ProviderGitManager
-import { ProviderGitManager } from '../../providers/provider-git-manager';
+// Import the mocked ProviderGitManager (import/first rule satisfied above)
const mockProviderGitManager = ProviderGitManager as jest.Mocked<typeof ProviderGitManager>;Also applies to: 15-15, 29-29, 32-34
🤖 Prompt for AI Agents
In src/model/cloud-runner/tests/providers/provider-git-manager.test.ts around
lines 1-12 (also apply fixes at 15, 29, and 32-34): ESLint complains about
import/order and no-unused-vars; reorder imports so built-ins come first (import
* as fs from 'fs';) then relative imports (import type { GitHubUrlInfo } from
'../../providers/provider-url-parser';) and convert the unused GitHubUrlInfo to
a type-only import or remove it if truly unused, then keep the jest.mock calls
after all imports; ensure any other unused variables at the noted lines are
removed or converted to type-only imports to satisfy no-unused-vars.
| describe('loadProvider', () => { | ||
| it('loads a built-in provider dynamically', async () => { | ||
| const provider: ProviderInterface = await loadProvider('./test', {} as any); | ||
| expect(typeof provider.runTaskInWorkflow).toBe('function'); | ||
| }); | ||
|
|
||
| it('loads a local provider from relative path', async () => { | ||
| const provider: ProviderInterface = await loadProvider('./test', {} as any); | ||
| expect(typeof provider.runTaskInWorkflow).toBe('function'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Regression risk: built-in names like 'aws' resolve as NPM, not internal modules
Current flow parses 'aws' as npm and never reaches the built-in map in provider-loader. Add a test and fix loader to map built-ins before parsing.
Add test:
it('loads a built-in provider by name', async () => {
const provider = await loadProvider('test', {} as any);
expect(typeof provider.runTaskInWorkflow).toBe('function');
});Update provider-loader (outside this file) before calling parseProviderSource:
const builtins: Record<string, string> = {
aws: './aws',
k8s: './k8s',
test: './test',
'local-docker': './docker',
'local-system': './local',
local: './local',
};
if (providerSource in builtins) {
importedModule = await import(builtins[providerSource]);
// ... instantiate/validate and return
}
frostebite
changed the title
GabLeRoux
left a comment
GabLeRoux
left a comment
There was a problem hiding this comment.
I took a look at the first half, but overall, that looks really good ❤️
| env: | ||
| UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} | ||
| UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} | ||
| UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} | ||
| PROJECT_PATH: test-project | ||
| TARGET_PLATFORM: StandaloneWindows64 | ||
| cloudRunnerTests: true | ||
| versioning: None | ||
| KUBE_STORAGE_CLASS: local-path | ||
| PROVIDER_STRATEGY: aws | ||
| AWS_ACCESS_KEY_ID: test | ||
| AWS_SECRET_ACCESS_KEY: test | ||
| GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} | ||
| GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} |
There was a problem hiding this comment.
Is it the case? If so, looks like a good suggestion.
| @@ -0,0 +1,214 @@ | |||
| # Provider Loader Dynamic Imports | |||
|
|
|||
| The provider loader now supports dynamic loading of providers from multiple sources including local file paths, GitHub repositories, and NPM packages. | |||
There was a problem hiding this comment.
I think we should also document what is a provider so we can have a better idea of why this is useful. We should probably document this in here as well: https://game.ci/docs/github-cloud-runner/introduction/
Something like this:
A provider is a pluggable backend that Cloud Runner uses to run builds and workflows. Examples include AWS, Kubernetes, or local execution. Each provider implements the ProviderInterface, which defines the common lifecycle methods (setup, run, cleanup, garbage collection, etc.).
This abstraction makes Cloud Runner flexible: you can switch execution environments or add your own provider (via npm package, GitHub repo, or local path) without changing the rest of your pipeline.
Co-authored-by: Gabriel Le Breton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
.github/workflows/cloud-runner-integrity-localstack.yml (1)
66-79: Clean up test environment and fix token usage.Per previous review feedback, the environment configuration still has issues:
- Line 74:
KUBE_STORAGE_CLASSis Kubernetes-specific and not relevant for LocalStack (AWS provider)- Lines 78-79: Use
github.tokeninstead ofGIT_PRIVATE_TOKENforGITHUB_TOKEN- Missing LocalStack-specific environment variables for proper S3 and metadata service configuration
Apply this diff to align with LocalStack best practices:
env: UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} PROJECT_PATH: test-project TARGET_PLATFORM: StandaloneWindows64 cloudRunnerTests: true versioning: None - KUBE_STORAGE_CLASS: local-path PROVIDER_STRATEGY: aws AWS_ACCESS_KEY_ID: test AWS_SECRET_ACCESS_KEY: test - GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} - GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} + AWS_S3_ENDPOINT: http://localhost:4566 + AWS_S3_FORCE_PATH_STYLE: 'true' + AWS_EC2_METADATA_DISABLED: 'true' + GITHUB_TOKEN: ${{ github.token }}.github/workflows/cloud-runner-integrity.yml (6)
51-51: Consider updating kubectl to latest stable version.Per previous review feedback, the latest kubectl version is v1.34.1 (released 2025-09-09). However, as noted in past comments, verify compatibility before updating as there may be breaking changes.
52-56: Pin k3d version and avoid curl | bash for security.Per previous review feedback, the current installation method using
curl | bashpresents supply-chain risks and lacks reproducibility. Consider pinning a specific k3d release version and downloading directly from GitHub releases.
68-68: Update to latest LocalStack action version.Per previous review feedback, the latest version is v0.2.4 with no reported breaking changes.
72-74: Use region-aware S3 bucket creation for LocalStack.Per previous review feedback (accepted by maintainer), S3 bucket creation should use
s3api create-bucketwithLocationConstraintfor regions other than us-east-1 to ensure LocalStack compatibility.
100-101: Use github.token instead of private token.Per previous review feedback, avoid using
GIT_PRIVATE_TOKENforGITHUB_TOKENto prevent PAT leakage and respect fork limitations. Use the built-ingithub.tokeninstead.
145-152: Clean up LocalStack job environment configuration.The localstack job has several environment configuration issues:
- Line 145:
KUBE_STORAGE_CLASS: local-pathis Kubernetes-specific and irrelevant for AWS provider strategy- Lines 149-150: Missing
AWS_S3_FORCE_PATH_STYLE: 'true'andAWS_EC2_METADATA_DISABLED: 'true'(present in k8s job lines 98-99)- Lines 151-152: Should use
github.tokeninstead ofGIT_PRIVATE_TOKENforGITHUB_TOKENApply this diff to align with the k8s job configuration:
env: UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }} UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }} UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }} PROJECT_PATH: test-project TARGET_PLATFORM: StandaloneWindows64 cloudRunnerTests: true versioning: None - KUBE_STORAGE_CLASS: local-path PROVIDER_STRATEGY: aws AWS_ACCESS_KEY_ID: test AWS_SECRET_ACCESS_KEY: test AWS_ENDPOINT: http://localhost:4566 AWS_ENDPOINT_URL: http://localhost:4566 - GIT_PRIVATE_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} - GITHUB_TOKEN: ${{ secrets.GIT_PRIVATE_TOKEN }} + AWS_S3_FORCE_PATH_STYLE: 'true' + AWS_EC2_METADATA_DISABLED: 'true' + GITHUB_TOKEN: ${{ github.token }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/licenses.txtis excluded by!**/dist/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.github/workflows/cleanup.yml(1 hunks).github/workflows/cloud-runner-async-checks.yml(2 hunks).github/workflows/cloud-runner-integrity-localstack.yml(1 hunks).github/workflows/cloud-runner-integrity.yml(1 hunks).github/workflows/integrity-check.yml(2 hunks)package.json(4 hunks)src/model/cloud-runner/providers/local/index.ts(2 hunks)tsconfig.json(1 hunks)types/shell-quote.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/cloud-runner-async-checks.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-25T18:45:10.112Z
Learnt from: ejnarvala
Repo: game-ci/unity-builder PR: 661
File: src/model/platform-setup.ts:35-35
Timestamp: 2024-10-25T18:45:10.112Z
Learning: Configure code analysis tools to recognize TypeScript files to ensure accurate detection of properties like `unityLicensingProductIds` in future reviews.
Applied to files:
tsconfig.json
🧬 Code graph analysis (1)
src/model/cloud-runner/providers/local/index.ts (1)
src/model/cloud-runner/services/core/cloud-runner-system.ts (1)
CloudRunnerSystem(4-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: WebGL on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
🔇 Additional comments (6)
tsconfig.json (1)
12-12: Good improvement to be explicit about TypeScript file inclusion.Adding an explicit
includefield ensures all source files undersrc/and type declarations undertypes/are consistently compiled and type-checked. This is a solid improvement that aligns with the PR's extensive additions of new Cloud Runner models, providers, and type definitions.package.json (1)
22-22: LGTM! Well-justified dependency and script additions.The new
test:ciscript,shell-quoteruntime dependency, andnode-fetchdev dependency are all appropriate:
- Line 22: CI-specific Jest config with serial execution for reliable CI runs
- Line 53:
shell-quoteenables secure command escaping for Windows path handling (addresses command injection concerns)- Line 79:
node-fetch@2provides fetch polyfill for Node test environmentsAlso applies to: 53-53, 79-79
src/model/cloud-runner/providers/local/index.ts (1)
9-9: Security improvement: proper shell escaping implemented.The implementation correctly uses
shell-quoteto address the incomplete string escaping security concern identified in previous reviews. This properly handles backslashes, quotes, and other shell metacharacters that could lead to command injection vulnerabilities.Based on learnings from previous security review feedback, using a well-tested library like
shell-quoteis the recommended approach over manual escaping.Also applies to: 78-78
types/shell-quote.d.ts (1)
1-16: LGTM! Accurate type declarations for shell-quote.The TypeScript declaration file correctly defines the
shell-quotemodule's public API with appropriate JSDoc documentation. The type signatures match the library's actual usage in the codebase (specifically thequotefunction used in the local provider)..github/workflows/integrity-check.yml (2)
7-9: LGTM! Security and CI improvements.Lines 7-9: The
permissionsblock addresses the security concern from previous reviews by explicitly limiting GITHUB_TOKEN permissions tocontents: read.Line 28: The switch to
yarn test:cienables CI-specific Jest configuration for more reliable test execution in the GitHub Actions environment.Also applies to: 28-28
33-36: LGTM! Clean Cloud Runner integration.The new
cloud-runnerjob properly integrates the Cloud Runner integrity tests using workflow composition. Thesecrets: inheritpattern is appropriate for passing required Unity credentials and GitHub tokens to the called workflow.
| # Commented out: Using LocalStack tests instead of real AWS | ||
| # - run: yarn run cli --help | ||
| # env: | ||
| # AWS_REGION: eu-west-2 | ||
| # AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| # AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| # AWS_DEFAULT_REGION: eu-west-2 | ||
| # - run: yarn run cli -m list-resources | ||
| # env: | ||
| # AWS_REGION: eu-west-2 | ||
| # AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| # AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| # AWS_DEFAULT_REGION: eu-west-2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/cleanup.ymlRepository: game-ci/unity-builder
Length of output: 1540
🏁 Script executed:
grep -r "localstack\|LocalStack" .github/workflows/ --color=neverRepository: game-ci/unity-builder
Length of output: 1745
🏁 Script executed:
grep -r "AWS_ENDPOINT\|localhost" .github/workflows/ --color=neverRepository: game-ci/unity-builder
Length of output: 821
🏁 Script executed:
grep -r "yarn run cli" . --include="*.yml" --include="*.yaml" -B2 -A2 | head -50Repository: game-ci/unity-builder
Length of output: 985
🏁 Script executed:
# Check if there's a CLI defined in package.json or similar
find . -name "package.json" -o -name ".yarnrc*" | head -5Repository: game-ci/unity-builder
Length of output: 89
🏁 Script executed:
cat package.json | head -80Repository: game-ci/unity-builder
Length of output: 3579
🏁 Script executed:
grep -r "list-resources" . --include="*.ts" --include="*.js" -B3 -A3 | head -40Repository: game-ci/unity-builder
Length of output: 974
🏁 Script executed:
# Check if there are any other references to this cleanup workflow or AWS resource cleanup
grep -r "cleanupCloudRunner\|cleanup\.yml" . --include="*.md" --include="*.ts" --include="*.js" 2>/dev/null | head -20Repository: game-ci/unity-builder
Length of output: 47
🏁 Script executed:
# Check the garbage-collect command to understand what actual cleanup exists
grep -r "garbageCollect\|garbage-collect" . --include="*.ts" -B2 -A5 | head -60Repository: game-ci/unity-builder
Length of output: 4297
Remove commented code; cleanup job is incomplete.
Commented-out code should be removed from the repository. The cleanupCloudRunner job currently only runs yarn install and performs no actual cleanup operations. The commented --help and list-resources commands were diagnostics, not cleanup logic. If resource cleanup is still needed for the cleanup workflow during LocalStack migration, update the job to call yarn run cli garbage-collect or remove the entire job if it's no longer required.
🤖 Prompt for AI Agents
.github/workflows/cleanup.yml lines 26-38: the cleanup job contains
commented-out diagnostic commands and performs only yarn install, leaving the
workflow incomplete; either remove the commented code and implement real cleanup
by replacing the job steps to run the CLI garbage collection (e.g., invoke yarn
run cli garbage-collect with appropriate AWS/LocalStack env vars) or delete the
entire cleanupCloudRunner job if cleanup is no longer required; ensure any added
step sets necessary environment variables and secrets for LocalStack/AWS before
calling the garbage-collect command.
| // On Windows, many built-in hooks use POSIX shell syntax. Execute via bash if available. | ||
| if (process.platform === 'win32') { | ||
| const inline = commands | ||
| .replace(/\r/g, '') | ||
| .split('\n') | ||
| .filter((x) => x.trim().length > 0) | ||
| .join(' ; '); | ||
| // Use shell-quote to properly escape the command string, preventing command injection | ||
| const bashWrapped = `bash -lc ${quote([inline])}`; | ||
| return await CloudRunnerSystem.Run(bashWrapped); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for bash availability checks
rg -n "which bash|command -v bash|bash.*version" --type ts --type js -C 2
# Look for Windows execution or bash requirement documentation
fd -e md . | head -20 | xargs rg -l "Windows|bash|WSL" 2>/dev/null
# Check CloudRunnerSystem.Run implementation to understand error handling
rg -n "CloudRunnerSystem" --type ts | head -10
# Look for how CloudRunnerSystem.Run is defined and handles errors
fd -name "*CloudRunnerSystem*" --type fRepository: game-ci/unity-builder
Length of output: 2191
🏁 Script executed:
# Search for bash availability or Windows-specific handling in the local provider
rg -n "process.platform|bash" src/model/cloud-runner/providers/local/ --type ts -B 2 -A 2
# Check if there's any try-catch or error handling around the bash execution
rg -n "try|catch|throw" src/model/cloud-runner/providers/local/index.ts -B 1 -A 3Repository: game-ci/unity-builder
Length of output: 1690
🏁 Script executed:
# Look for documentation about Windows requirements
find . -name "*.md" -type f | xargs grep -l "Windows\|WSL\|bash" 2>/dev/null | head -10
# Check the broader context of the file to understand error handling patterns
wc -l src/model/cloud-runner/providers/local/index.ts
head -100 src/model/cloud-runner/providers/local/index.tsRepository: game-ci/unity-builder
Length of output: 3440
The "if available" comment is misleading—add bash availability verification or clarify the requirement.
The code assumes bash is available on Windows but doesn't verify it. The comment states "Execute via bash if available," but there is no availability check. If bash is not in PATH (e.g., WSL, Git Bash, or Cygwin not installed), the command will fail with an unclear error message.
Either add a pre-flight check to verify bash availability before attempting execution, or update the comment to clearly document that bash is a required dependency for Windows execution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/model/cloud-runner/providers/aws/services/task-service.ts (4)
34-85: Consider adding pagination for large stack lists.While the factory usage and type improvements are good,
ListStacksCommandsupports pagination viaNextToken. If your environment may have more than 100 stacks, results could be incomplete.Apply similar pagination logic as in
getTasks():public static async getCloudFormationJobStacks(): Promise<StackSummary[]> { - const result: StackSummary[] = []; CloudRunnerLogger.log(``); CloudRunnerLogger.log(`List Cloud Formation Stacks`); process.env.AWS_REGION = Input.region; const CF = AwsClientFactory.getCloudFormation(); - const stacks = - (await CF.send(new ListStacksCommand({}))).StackSummaries?.filter( - (_x) => - _x.StackStatus !== 'DELETE_COMPLETE' && _x.TemplateDescription !== BaseStackFormation.baseStackDecription, - ) || []; + let allStacks: StackSummary[] = []; + { + let nextToken: string | undefined; + do { + const response = await CF.send(new ListStacksCommand({ NextToken: nextToken })); + allStacks.push(...(response.StackSummaries ?? [])); + nextToken = response.NextToken; + } while (nextToken); + } + const stacks = allStacks.filter( + (_x) => + _x.StackStatus !== 'DELETE_COMPLETE' && _x.TemplateDescription !== BaseStackFormation.baseStackDecription, + ); CloudRunnerLogger.log(``); CloudRunnerLogger.log(`Cloud Formation Stacks ${stacks.length}`); + const result: StackSummary[] = []; for (const element of stacks) { // ... existing loop code } - const baseStacks = - (await CF.send(new ListStacksCommand({}))).StackSummaries?.filter( - (_x) => - _x.StackStatus !== 'DELETE_COMPLETE' && _x.TemplateDescription === BaseStackFormation.baseStackDecription, - ) || []; + const baseStacks = allStacks.filter( + (_x) => + _x.StackStatus !== 'DELETE_COMPLETE' && _x.TemplateDescription === BaseStackFormation.baseStackDecription, + ); CloudRunnerLogger.log(``); CloudRunnerLogger.log(`Base Stacks ${baseStacks.length}`); for (const element of baseStacks) { // ... existing loop code }
170-204: LGTM! Type safety and pagination properly implemented.The factory usage and type improvements are solid. Pagination correctly handles
nextToken.Minor: You could simplify the pagination loop slightly:
let logGroupsDescribe = await cwl.send(new DescribeLogGroupsCommand(logStreamInput)); const logGroups = logGroupsDescribe.logGroups || []; while (logGroupsDescribe.nextToken) { - logStreamInput = { - /* logGroupNamePrefix: 'game-ci',*/ - nextToken: logGroupsDescribe.nextToken, - }; - logGroupsDescribe = await cwl.send(new DescribeLogGroupsCommand(logStreamInput)); + logGroupsDescribe = await cwl.send( + new DescribeLogGroupsCommand({ nextToken: logGroupsDescribe.nextToken }) + ); logGroups.push(...(logGroupsDescribe?.logGroups || [])); }
208-211: Simplify the type assertion with a dedicated interface.The inline type assertion is verbose and harder to maintain. Consider defining a proper interface as suggested in previous reviews.
At the top of the file or in a separate types file:
interface IWorkspaceLocking { listObjects(prefix: string): Promise<string[]>; }Then simplify line 209:
- const objects = await (SharedWorkspaceLocking as unknown as { listObjects(prefix: string): Promise<string[]> }).listObjects(''); + const objects = await (SharedWorkspaceLocking as unknown as IWorkspaceLocking).listObjects('');
206-220: Consider adding pagination for S3 lock listing.
ListObjectsV2Commandreturns at most 1,000 objects by default. If you have many locks, useIsTruncatedandNextContinuationTokento paginate.Apply pagination similar to other methods:
const s3 = AwsClientFactory.getS3(); - const listRequest = { - Bucket: CloudRunner.buildParameters.awsStackName, - }; - - const results = await s3.send(new ListObjectsV2Command(listRequest)); - - return (results.Contents || []).map((obj) => ({ Key: obj.Key || '' })); + const allObjects: Array<{ Key: string }> = []; + let continuationToken: string | undefined; + do { + const results = await s3.send( + new ListObjectsV2Command({ + Bucket: CloudRunner.buildParameters.awsStackName, + ContinuationToken: continuationToken, + }) + ); + allObjects.push(...(results.Contents || []).map((obj) => ({ Key: obj.Key || '' }))); + continuationToken = results.NextContinuationToken; + } while (continuationToken); + return allObjects;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/licenses.txtis excluded by!**/dist/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.eslintrc.json(1 hunks).github/workflows/integrity-check.yml(2 hunks)package.json(3 hunks)src/model/cloud-runner/providers/aws/services/task-service.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- .eslintrc.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/model/cloud-runner/providers/aws/services/task-service.ts (2)
src/model/cloud-runner/providers/aws/aws-client-factory.ts (1)
AwsClientFactory(9-71)src/model/cloud-runner/services/core/shared-workspace-locking.ts (2)
SharedWorkspaceLocking(17-406)s3(19-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: StandaloneOSX on 6000.0.36f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (LocalStack) (cloud-runner-end2end-retaining)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (LocalStack) (cloud-runner-hooks)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (LocalStack) (cloud-runner-end2end-caching)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (K8s) (cloud-runner-end2end-retaining, k8s)
- GitHub Check: Cloud Runner Integrity / Cloud Runner Tests (K8s) (cloud-runner-end2end-caching, k8s)
🔇 Additional comments (7)
.github/workflows/integrity-check.yml (4)
7-10: ✓ Permissions block correctly addresses security findings.The explicitly scoped permissions now align with least-privilege best practices and resolve the GitHub Advanced Security warnings from previous reviews. The scopes (
contents: read,checks: write,statuses: write) are appropriate for this workflow's operations.
30-30: ✓ Coverage reporting addition is appropriate.Adding
--coverageto the test command improves observability of test coverage metrics, aligning with the CI improvements mentioned in the PR summary.
33-33: Clarify intent of commented dist-guard line.The dist-guard build check is now commented out rather than deleted. Confirm whether this is intentional (for future reference) or an incomplete refactoring. If no longer needed, remove it entirely to maintain clean code.
35-38: The cloud-runner-integrity workflow file exists and is correctly configured. It is properly defined as a reusable workflow (workflow_call) with appropriate permissions (contents: read,checks: write,statuses: write) matching those in the parent workflow. Thesecrets: inheritapproach is the correct pattern for passing repository secrets to reusable workflows, and the cloud-runner-integrity workflow properly accesses the inherited secrets for its k8s and localstack test jobs.src/model/cloud-runner/providers/aws/services/task-service.ts (3)
6-6: LGTM! Type-only imports properly address previous feedback.The type-only imports for
StackSummary,LogGroup, andTaskimprove type safety without runtime overhead, and the switch toListObjectsV2Commandenables proper pagination support.Also applies to: 11-11, 13-13, 14-14
86-137: LGTM! Excellent improvements to type safety and pagination.The method now properly:
- Uses typed return value with
Taskinstead ofany- Implements pagination for both clusters and tasks
- Avoids mutating AWS SDK objects
- Leverages the factory for consistent client configuration
138-169: LGTM! Factory usage correctly applied.The migration to
AwsClientFactory.getCloudFormation()maintains existing error handling while ensuring consistent endpoint configuration.
Co-authored-by: Gabriel Le Breton <[email protected]>
Co-authored-by: Gabriel Le Breton <[email protected]>
Changes
Related Issues
Related PRs
Successful Workflow Run Link
PRs don't have access to secrets so you will need to provide a link to a successful run of the workflows from your own
repo.
Checklist
code of conduct
in the documentation repo)
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.