Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/build-tests-mac.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ jobs:
buildForAllPlatformsMacOS:
name: ${{ matrix.targetPlatform }} on ${{ matrix.unityVersion }}
runs-on: macos-latest
continue-on-error: true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Inconsistent error handling across platform workflows.

Adding continue-on-error: true at the job level means macOS build failures won't fail the overall workflow, while Windows and Ubuntu workflows (per the context snippets) will still fail on errors. This asymmetry could mask real macOS build failures and allow PRs to merge with broken builds.

This change also doesn't appear related to the PR's stated objectives (git integrity checks, reserved filename cleanup, build archival). If this is intentional for testing the new reliability features, consider:

  1. Documenting why macOS specifically needs this
  2. Applying consistently across all platform workflows if the intent is uniform
  3. Removing after testing is complete
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/build-tests-mac.yml at line 15, The macOS workflow sets
continue-on-error: true which makes macOS job failures silent and inconsistent
with Windows/Ubuntu; either remove the continue-on-error: true line from the
macOS job so failures fail the workflow, or if intentional, add the same
continue-on-error: true to the Windows and Ubuntu workflow jobs and add a
comment explaining the temporary rationale and when it will be removed; ensure
the change is committed alongside documentation/PR description clarifying
intent.

strategy:
fail-fast: false
matrix:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/orchestrator-async-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ jobs:
# AWS_STACK_NAME: game-ci-github-pipelines
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
cd unity-builder
yarn
ls
Expand Down
28 changes: 26 additions & 2 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ inputs:
required: false
default: ''
description:
'[Orchestrator] Run a custom job instead of the standard build automation for orchestrator (in yaml format with the
keys image, secrets (name, value object array), command line string)'
'[Orchestrator] Run a custom job instead of the standard build automation for orchestrator (in yaml format with
the keys image, secrets (name, value object array), command line string)'
awsStackName:
default: 'game-ci'
required: false
Expand Down Expand Up @@ -279,6 +279,30 @@ inputs:
description:
'[Orchestrator] Specifies the repo for the unity builder. Useful if you forked the repo for testing, features, or
fixes.'
gitIntegrityCheck:
description: 'Run git integrity checks before build (fsck, lock cleanup, submodule validation)'
required: false
default: 'false'
gitAutoRecover:
description: 'Attempt automatic recovery if git corruption is detected'
required: false
default: 'false'
Comment on lines +286 to +289
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Set gitAutoRecover default to match the intended recovery behavior.

At Line 289, the default is 'false', which makes recovery opt-in even when integrity checks are enabled. That contradicts the stated behavior for this feature set.

🔧 Suggested change
  gitAutoRecover:
    description: 'Attempt automatic recovery if git corruption is detected'
    required: false
-   default: 'false'
+   default: '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.

Suggested change
gitAutoRecover:
description: 'Attempt automatic recovery if git corruption is detected'
required: false
default: 'false'
gitAutoRecover:
description: 'Attempt automatic recovery if git corruption is detected'
required: false
default: 'true'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@action.yml` around lines 286 - 289, The default for the gitAutoRecover input
is currently 'false' which contradicts the intended behavior; update the input
definition for gitAutoRecover so its default is 'true' (keep the YAML string
form used for action inputs) to make automatic recovery opt-out rather than
opt-in, ensuring the gitAutoRecover field and its description remain unchanged.

cleanReservedFilenames:
description: 'Remove Windows reserved filenames that cause Unity import loops'
required: false
default: 'false'
buildArchiveEnabled:
description: 'Archive build output after successful build'
required: false
default: 'false'
buildArchivePath:
description: 'Path to store build archives'
required: false
default: './build-archives'
buildArchiveRetention:
description: 'Days to retain build archives before cleanup'
required: false
default: '30'

outputs:
volume:
Expand Down
593 changes: 589 additions & 4 deletions dist/index.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/index.js.map

Large diffs are not rendered by default.

35 changes: 35 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Action, BuildParameters, Cache, Orchestrator, Docker, ImageTag, Output
import { Cli } from './model/cli/cli';
import MacBuilder from './model/mac-builder';
import PlatformSetup from './model/platform-setup';
import { BuildReliabilityService } from './model/orchestrator/services/reliability';

async function runMain() {
try {
Expand All @@ -14,11 +15,38 @@ async function runMain() {
Action.checkCompatibility();
Cache.verify();

// Always configure git environment for CI reliability
BuildReliabilityService.configureGitEnvironment();

const { workspace, actionFolder } = Action;

const buildParameters = await BuildParameters.create();
const baseImage = new ImageTag(buildParameters);

// Pre-build reliability checks
if (buildParameters.gitIntegrityCheck) {
core.info('Running git integrity checks...');

const isHealthy = BuildReliabilityService.checkGitIntegrity(workspace);
BuildReliabilityService.cleanStaleLockFiles(workspace);
BuildReliabilityService.validateSubmoduleBackingStores(workspace);

Comment on lines +30 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Submodule validation result is ignored in the recovery gate.

At Line 32, validateSubmoduleBackingStores returns broken paths, but that result is discarded. Recovery at Line 38 only checks checkGitIntegrity, so broken submodule backing stores won’t trigger auto-recovery.

🔧 Suggested update
-      const isHealthy = BuildReliabilityService.checkGitIntegrity(workspace);
-      BuildReliabilityService.cleanStaleLockFiles(workspace);
-      BuildReliabilityService.validateSubmoduleBackingStores(workspace);
+      BuildReliabilityService.cleanStaleLockFiles(workspace);
+      const brokenSubmodules = BuildReliabilityService.validateSubmoduleBackingStores(workspace);
+      const isHealthy =
+        BuildReliabilityService.checkGitIntegrity(workspace) && brokenSubmodules.length === 0;

Also applies to: 38-44

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/index.ts` around lines 30 - 33, The code discards the return value of
BuildReliabilityService.validateSubmoduleBackingStores which reports broken
paths, so update the logic to capture its result (e.g., const brokenSubmodules =
BuildReliabilityService.validateSubmoduleBackingStores(workspace)) and
incorporate it into the recovery gate alongside
BuildReliabilityService.checkGitIntegrity(workspace) (e.g., treat any non-empty
brokenSubmodules as unhealthy). Then adjust the recovery decision that currently
checks only isHealthy (the block around the existing recovery check) to trigger
auto-recovery when either checkGitIntegrity returns false or brokenSubmodules is
non-empty, and ensure any log messages/reference to health include the
brokenSubmodules variable for context.

if (buildParameters.cleanReservedFilenames) {
BuildReliabilityService.cleanReservedFilenames(buildParameters.projectPath);
}

if (!isHealthy && buildParameters.gitAutoRecover) {
core.info('Git corruption detected, attempting automatic recovery...');
const recovered = BuildReliabilityService.recoverCorruptedRepo(workspace);
if (!recovered) {
core.warning('Automatic recovery failed. Build may encounter issues.');
}
}
} else if (buildParameters.cleanReservedFilenames) {
// cleanReservedFilenames can run independently of gitIntegrityCheck
BuildReliabilityService.cleanReservedFilenames(buildParameters.projectPath);
}

let exitCode = -1;

if (buildParameters.providerStrategy === 'local') {
Expand All @@ -37,6 +65,13 @@ async function runMain() {
exitCode = 0;
}

// Post-build: archive and enforce retention
if (buildParameters.buildArchiveEnabled && exitCode === 0) {
core.info('Archiving build output...');
BuildReliabilityService.archiveBuildOutput(buildParameters.buildPath, buildParameters.buildArchivePath);
BuildReliabilityService.enforceRetention(buildParameters.buildArchivePath, buildParameters.buildArchiveRetention);
}

// Set output
await Output.setBuildVersion(buildParameters.buildVersion);
await Output.setAndroidVersionCode(buildParameters.androidVersionCode);
Expand Down
12 changes: 12 additions & 0 deletions src/model/build-parameters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ class BuildParameters {
public cacheUnityInstallationOnMac!: boolean;
public unityHubVersionOnMac!: string;
public dockerWorkspacePath!: string;
public gitIntegrityCheck!: boolean;
public gitAutoRecover!: boolean;
public cleanReservedFilenames!: boolean;
public buildArchiveEnabled!: boolean;
public buildArchivePath!: string;
public buildArchiveRetention!: number;

public static shouldUseRetainedWorkspaceMode(buildParameters: BuildParameters) {
return buildParameters.maxRetainedWorkspaces > 0 && Orchestrator.lockedWorkspace !== ``;
Expand Down Expand Up @@ -242,6 +248,12 @@ class BuildParameters {
cacheUnityInstallationOnMac: Input.cacheUnityInstallationOnMac,
unityHubVersionOnMac: Input.unityHubVersionOnMac,
dockerWorkspacePath: Input.dockerWorkspacePath,
gitIntegrityCheck: Input.gitIntegrityCheck,
gitAutoRecover: Input.gitAutoRecover,
cleanReservedFilenames: Input.cleanReservedFilenames,
buildArchiveEnabled: Input.buildArchiveEnabled,
buildArchivePath: Input.buildArchivePath,
buildArchiveRetention: Input.buildArchiveRetention,
};
}

Expand Down
32 changes: 32 additions & 0 deletions src/model/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,38 @@ class Input {
return Input.getInput('skipActivation')?.toLowerCase() ?? 'false';
}

static get gitIntegrityCheck(): boolean {
const input = Input.getInput('gitIntegrityCheck') ?? 'false';

return input === 'true';
}

static get gitAutoRecover(): boolean {
const input = Input.getInput('gitAutoRecover') ?? 'false';

return input === 'true';
}

static get cleanReservedFilenames(): boolean {
const input = Input.getInput('cleanReservedFilenames') ?? 'false';

return input === 'true';
}

static get buildArchiveEnabled(): boolean {
const input = Input.getInput('buildArchiveEnabled') ?? 'false';

return input === 'true';
}

static get buildArchivePath(): string {
return Input.getInput('buildArchivePath') ?? './build-archives';
}

static get buildArchiveRetention(): number {
return Number.parseInt(Input.getInput('buildArchiveRetention') ?? '30', 10);
}
Comment on lines +313 to +315
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

buildArchiveRetention can silently return NaN for invalid input.

If the user provides a non-numeric string (e.g., "abc"), Number.parseInt returns NaN, which could cause unexpected behavior in retention enforcement.

🛡️ Suggested fix with fallback
 static get buildArchiveRetention(): number {
-    return Number.parseInt(Input.getInput('buildArchiveRetention') ?? '3', 10);
+    const parsed = Number.parseInt(Input.getInput('buildArchiveRetention') ?? '3', 10);
+    return Number.isNaN(parsed) || parsed < 0 ? 3 : parsed;
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static get buildArchiveRetention(): number {
return Number.parseInt(Input.getInput('buildArchiveRetention') ?? '3', 10);
}
static get buildArchiveRetention(): number {
const parsed = Number.parseInt(Input.getInput('buildArchiveRetention') ?? '3', 10);
return Number.isNaN(parsed) || parsed < 0 ? 3 : parsed;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/model/input.ts` around lines 313 - 315, The buildArchiveRetention getter
uses Number.parseInt on Input.getInput('buildArchiveRetention') and can return
NaN for invalid strings; update the static getter buildArchiveRetention to parse
the value, validate it with Number.isNaN (and optionally ensure it's a
non-negative integer), and return the default (3) when the parsed value is NaN
or invalid so retention never becomes NaN; refer to the static get
buildArchiveRetention and Input.getInput('buildArchiveRetention') to locate and
modify the code.


public static ToEnvVarFormat(input: string) {
if (input.toUpperCase() === input) {
return input;
Expand Down
Loading
Loading