feat(orchestrator): build reliability features — git integrity, reserved filenames, archival#808
Closed
frostebite wants to merge 6 commits intomainfrom
Closed
feat(orchestrator): build reliability features — git integrity, reserved filenames, archival#808frostebite wants to merge 6 commits intomainfrom
frostebite wants to merge 6 commits intomainfrom
Conversation
…ved filename cleanup, archival Add three optional reliability features for hardening CI pipelines: - Git corruption detection & recovery (fsck, stale lock cleanup, submodule backing store validation, auto-recovery) - Reserved filename cleanup (removes Windows device names that cause Unity asset importer infinite loops) - Build output archival with configurable retention policy All features are opt-in and fail gracefully with warnings only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 5, 2026
frostebite
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds opt-in build reliability features: pre-build Git integrity checks and optional auto-recovery, Windows reserved-filename cleanup, and post-build archival with retention. Inputs added to Changes
Sequence DiagramsequenceDiagram
participant Orch as Orchestrator
participant Service as BuildReliabilityService
participant Git as Git
participant FS as FileSystem
participant Logger as Logger
Orch->>Service: configureGitEnvironment()
Orch->>Service: (if enabled) checkGitIntegrity(repo)
Service->>Git: git fsck --no-dangling
Git-->>Service: fsck result
Service->>Logger: log integrity result
alt corruption detected
Service->>Service: cleanStaleLockFiles(repo)
Service->>FS: scan & remove .git/*.lock
FS-->>Service: removed count
Service->>Service: validateSubmoduleBackingStores(repo)
Service->>FS: read .gitmodules, verify .git/modules/*
FS-->>Service: invalid submodules list
Service->>Logger: log issues
alt auto-recover enabled
Service->>Service: recoverCorruptedRepo(repo)
Service->>Git: git fetch / clone
Git-->>Service: fetch/clone result
Service->>Logger: log recovery outcome
end
end
Orch->>Service: (on success & enabled) archiveBuildOutput(buildOut, archivePath)
Service->>FS: create archive (tar), write to archivePath
Service->>Service: enforceRetention(archivePath, days)
Service->>FS: remove old archives
Service->>Logger: log archival actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…rity, reserved filename cleanup, and build archival Adds BuildReliabilityService with the following capabilities: - checkGitIntegrity(): runs git fsck --no-dangling and parses output for corruption - cleanStaleLockFiles(): removes stale .lock files older than 10 minutes - validateSubmoduleBackingStores(): validates .git files point to valid backing stores - recoverCorruptedRepo(): orchestrates fsck, lock cleanup, re-fetch, retry fsck - cleanReservedFilenames(): removes Windows reserved filenames (con, prn, aux, nul, com1-9, lpt1-9) - archiveBuildOutput(): creates tar.gz archive of build output - enforceRetention(): deletes archives older than retention period - configureGitEnvironment(): sets GIT_TERMINAL_PROMPT=0, http.postBuffer, core.longpaths Wired into action.yml as opt-in inputs, with pre-build integrity checks and post-build archival in the main entry point. Includes 29 unit tests covering success and failure cases for all methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/model/orchestrator/services/reliability/build-reliability-service.ts (2)
38-45:LOCK_FILE_PATTERNSis defined but never used.This constant is declared but
cleanStaleLockFilesuses a simple recursive approach checkingentry.name.endsWith('.lock')instead of matching against these patterns. Either remove the unused constant or use it for more targeted cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 38 - 45, LOCK_FILE_PATTERNS is declared but unused; update cleanStaleLockFiles to use these patterns instead of the current endsWith('.lock') check by matching the relative path of each entry against LOCK_FILE_PATTERNS (e.g., with a glob/minimatch matcher) so only the intended lock file patterns are removed, or delete LOCK_FILE_PATTERNS if you prefer the simpler recursive approach; modify the logic inside cleanStaleLockFiles to compute the entry's repo-relative path and test it against each pattern in LOCK_FILE_PATTERNS (using a stable glob matcher) before unlinking.
182-189: Recovery workflow may leave repository in inconsistent state.The recovery logic removes
.gitthen runsgit init, creating an empty repository. The comment says "checkout action will complete the clone", but this assumes specific external orchestration behavior. If the checkout action expects an existing remote configuration (whichgit initdoesn't provide), the recovery may not work as intended.Consider adding documentation or logging to clarify the expected follow-up behavior, or storing remote URL before deletion to restore it after init.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 182 - 189, The recovery code removes .git then runs OrchestratorSystem.Run(`git -C "${repoPath}" init`) which creates an empty repo but does not restore remote config; update the recovery to preserve and re-add the original remote or at least log the expected remote so checkout can succeed: before deleting .git, capture the remote URL(s) (e.g., via `git -C "${repoPath}" remote get-url origin`) and after OrchestratorSystem.Run call restore them (e.g., `git -C "${repoPath}" remote add origin <url>`), and enhance OrchestratorLogger.logWarning/OrchestratorLogger.log messages to record the saved remote(s) and the action taken so callers know whether remote config was restored or must be provided by the checkout action.action.yml (2)
294-301: Consider documenting behavior whenbuildArchiveEnabled: truebutbuildArchivePathis empty.The
buildArchivePathdefaults to an empty string. If archival is enabled without specifying a path,archiveBuildOutputwill attempt to use an empty string as the archive location, which could cause unexpected behavior (writing to current directory or failing).Consider updating the description to clarify that
buildArchivePathis required whenbuildArchiveEnabledistrue, or add validation in the service.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@action.yml` around lines 294 - 301, The action currently allows buildArchiveEnabled=true with buildArchivePath empty which can cause archiveBuildOutput to try using an empty path; update the action metadata and add runtime validation: change the buildArchivePath description to state that a non-empty path is required when buildArchiveEnabled is true, and in the archival code (archiveBuildOutput) validate that when buildArchiveEnabled is true the buildArchivePath is non-empty and return/throw a clear error or skip archival if missing; reference buildArchiveEnabled, buildArchivePath and archiveBuildOutput when making these changes.
286-289:gitAutoRecoverdescription implies a dependency that isn't enforced.The description states "requires gitIntegrityCheck", but the code doesn't validate or enforce this relationship. If a user sets
gitAutoRecover: truewithoutgitIntegrityCheck: true, the auto-recovery logic may never trigger (assuming it's meant to run after a failed integrity check).Consider either:
- Documenting that
gitAutoRecoveris ignored whengitIntegrityCheckis false- Adding runtime validation in the service to warn users about this configuration
🤖 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 gitAutoRecover input declares a dependency on gitIntegrityCheck but the relationship isn't enforced; update either the action metadata or runtime config validation: (A) update the gitAutoRecover description to explicitly state "ignored unless gitIntegrityCheck is true" so users know it's dependent, and/or (B) add a runtime check in the configuration loader/validator (e.g., in the validateConfig or loadConfig routine) that detects gitAutoRecover=true while gitIntegrityCheck!=true and emits a clear warning via the service logger (or fail fast with an error if desired). Reference the input names gitAutoRecover and gitIntegrityCheck when implementing the message so the check is unambiguous.src/model/orchestrator/services/reliability/build-reliability-service.test.ts (1)
16-54: Test coverage is limited to early-exit scenarios.The tests only verify behavior when paths don't exist (returning 0 or empty arrays). Consider adding tests that exercise the actual functionality:
cleanStaleLockFiles: mock.gitdirectory with lock files, verify they're removedcleanReservedFilenames: mockAssets/with reserved names likenul.txt, verify cleanupvalidateSubmoduleBackingStores: mock.gitmoduleswith submodule entries, verify validation logicenforceRetention: mock archive directory with entries exceeding retention, verify oldest are removedcheckGitIntegrity: mockOrchestratorSystem.Runto throw, verifyfalseis returnedThe PR objectives mention "unit tests for lock cleanup, reserved filename matching, retention enforcement" which aren't fully exercised by the current test suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.test.ts` around lines 16 - 54, Add unit tests that cover non-early-exit behavior for each method: for BuildReliabilityService.cleanStaleLockFiles, mock the .git directory to exist and stub fs.readdirSync/fs.unlinkSync to simulate lock files and assert unlink was called and returned count; for cleanReservedFilenames, create a mocked Assets/ directory with names like "nul.txt" using fs.readdirSync and assert the reserved files are returned/removed as expected; for validateSubmoduleBackingStores, mock fs.existsSync and fs.readFileSync to return a .gitmodules file with submodule entries and assert validation logic outcome; for enforceRetention, stub fs.readdirSync/stat to produce files with different mtime values, call enforceRetention with retention N and assert oldest files were removed (unlink called) and return value; and for checkGitIntegrity, mock OrchestratorSystem.Run to throw and to succeed to assert false/true paths—use the existing mockFs and jest spies to control and verify fs and OrchestratorSystem behavior and avoid only testing the "path does not exist" early-exit branches.src/model/input.ts (1)
285-307: Inconsistent default value types for boolean inputs.
gitAutoRecoveruses?? 'true'(string default), while the other boolean getters use?? false(boolean default). While the=== 'true'comparison works correctly in all cases, this inconsistency can be confusing.For consistency, consider using string defaults throughout:
♻️ Suggested fix
static get gitIntegrityCheck(): boolean { - const input = Input.getInput('gitIntegrityCheck') ?? false; + const input = Input.getInput('gitIntegrityCheck') ?? 'false'; return input === 'true'; } // gitAutoRecover already correct static get cleanReservedFilenames(): boolean { - const input = Input.getInput('cleanReservedFilenames') ?? false; + const input = Input.getInput('cleanReservedFilenames') ?? 'false'; return input === 'true'; } static get buildArchiveEnabled(): boolean { - const input = Input.getInput('buildArchiveEnabled') ?? false; + const input = Input.getInput('buildArchiveEnabled') ?? 'false'; return input === 'true'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/input.ts` around lines 285 - 307, The boolean input getters are inconsistent: gitAutoRecover uses a string default ('true') while gitIntegrityCheck, cleanReservedFilenames, and buildArchiveEnabled use boolean false; normalize them to string defaults so the === 'true' comparisons are consistent. Update the getters gitIntegrityCheck, cleanReservedFilenames, and buildArchiveEnabled to use ?? 'false' (string) as the default while keeping gitAutoRecover as ?? 'true', ensuring all checks compare to the string 'true' returned from Input.getInput.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/input.ts`:
- Around line 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.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`:
- Around line 269-271: The fallback for cross-device moves currently shells out
with OrchestratorSystem.Run("cp -r ...") which is Unix-only; replace that shell
copy with a cross-platform Node API call (use fs.cpSync or fs.promises.cp where
available) to copy from the outputPath to archiveDirectory and then remove
outputPath with fs.rmSync; update the block around OrchestratorSystem.Run in
build-reliability-service.ts (the call that references outputPath and
archiveDirectory) to perform the copy using fs.cpSync/await fs.promises.cp and
retain the subsequent fs.rmSync removal.
---
Nitpick comments:
In `@action.yml`:
- Around line 294-301: The action currently allows buildArchiveEnabled=true with
buildArchivePath empty which can cause archiveBuildOutput to try using an empty
path; update the action metadata and add runtime validation: change the
buildArchivePath description to state that a non-empty path is required when
buildArchiveEnabled is true, and in the archival code (archiveBuildOutput)
validate that when buildArchiveEnabled is true the buildArchivePath is non-empty
and return/throw a clear error or skip archival if missing; reference
buildArchiveEnabled, buildArchivePath and archiveBuildOutput when making these
changes.
- Around line 286-289: The gitAutoRecover input declares a dependency on
gitIntegrityCheck but the relationship isn't enforced; update either the action
metadata or runtime config validation: (A) update the gitAutoRecover description
to explicitly state "ignored unless gitIntegrityCheck is true" so users know
it's dependent, and/or (B) add a runtime check in the configuration
loader/validator (e.g., in the validateConfig or loadConfig routine) that
detects gitAutoRecover=true while gitIntegrityCheck!=true and emits a clear
warning via the service logger (or fail fast with an error if desired).
Reference the input names gitAutoRecover and gitIntegrityCheck when implementing
the message so the check is unambiguous.
In `@src/model/input.ts`:
- Around line 285-307: The boolean input getters are inconsistent:
gitAutoRecover uses a string default ('true') while gitIntegrityCheck,
cleanReservedFilenames, and buildArchiveEnabled use boolean false; normalize
them to string defaults so the === 'true' comparisons are consistent. Update the
getters gitIntegrityCheck, cleanReservedFilenames, and buildArchiveEnabled to
use ?? 'false' (string) as the default while keeping gitAutoRecover as ??
'true', ensuring all checks compare to the string 'true' returned from
Input.getInput.
In
`@src/model/orchestrator/services/reliability/build-reliability-service.test.ts`:
- Around line 16-54: Add unit tests that cover non-early-exit behavior for each
method: for BuildReliabilityService.cleanStaleLockFiles, mock the .git directory
to exist and stub fs.readdirSync/fs.unlinkSync to simulate lock files and assert
unlink was called and returned count; for cleanReservedFilenames, create a
mocked Assets/ directory with names like "nul.txt" using fs.readdirSync and
assert the reserved files are returned/removed as expected; for
validateSubmoduleBackingStores, mock fs.existsSync and fs.readFileSync to return
a .gitmodules file with submodule entries and assert validation logic outcome;
for enforceRetention, stub fs.readdirSync/stat to produce files with different
mtime values, call enforceRetention with retention N and assert oldest files
were removed (unlink called) and return value; and for checkGitIntegrity, mock
OrchestratorSystem.Run to throw and to succeed to assert false/true paths—use
the existing mockFs and jest spies to control and verify fs and
OrchestratorSystem behavior and avoid only testing the "path does not exist"
early-exit branches.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`:
- Around line 38-45: LOCK_FILE_PATTERNS is declared but unused; update
cleanStaleLockFiles to use these patterns instead of the current
endsWith('.lock') check by matching the relative path of each entry against
LOCK_FILE_PATTERNS (e.g., with a glob/minimatch matcher) so only the intended
lock file patterns are removed, or delete LOCK_FILE_PATTERNS if you prefer the
simpler recursive approach; modify the logic inside cleanStaleLockFiles to
compute the entry's repo-relative path and test it against each pattern in
LOCK_FILE_PATTERNS (using a stable glob matcher) before unlinking.
- Around line 182-189: The recovery code removes .git then runs
OrchestratorSystem.Run(`git -C "${repoPath}" init`) which creates an empty repo
but does not restore remote config; update the recovery to preserve and re-add
the original remote or at least log the expected remote so checkout can succeed:
before deleting .git, capture the remote URL(s) (e.g., via `git -C "${repoPath}"
remote get-url origin`) and after OrchestratorSystem.Run call restore them
(e.g., `git -C "${repoPath}" remote add origin <url>`), and enhance
OrchestratorLogger.logWarning/OrchestratorLogger.log messages to record the
saved remote(s) and the action taken so callers know whether remote config was
restored or must be provided by the checkout action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c77d698-1ddc-40f3-824c-0ef2132475cf
⛔ 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 (6)
action.ymlsrc/model/build-parameters.tssrc/model/input.tssrc/model/orchestrator/services/reliability/build-reliability-service.test.tssrc/model/orchestrator/services/reliability/build-reliability-service.tssrc/model/orchestrator/services/reliability/index.ts
Comment on lines
+313
to
+315
| static get buildArchiveRetention(): number { | ||
| return Number.parseInt(Input.getInput('buildArchiveRetention') ?? '3', 10); | ||
| } |
There was a problem hiding this comment.
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.
Comment on lines
+269
to
+271
| try { | ||
| await OrchestratorSystem.Run(`cp -r "${outputPath}" "${archiveDirectory}"`, true); | ||
| fs.rmSync(outputPath, { recursive: true, force: true }); |
There was a problem hiding this comment.
Cross-device fallback uses Unix-only cp -r command.
The fallback for cross-device moves uses cp -r, which won't work on Windows. Since this is intended for self-hosted runners (which may be Windows), consider using a cross-platform approach.
♻️ Suggested cross-platform alternative
} catch {
// Cross-device move -- fall back to copy
try {
- await OrchestratorSystem.Run(`cp -r "${outputPath}" "${archiveDirectory}"`, true);
+ // Use platform-appropriate copy command
+ const isWindows = process.platform === 'win32';
+ const copyCommand = isWindows
+ ? `xcopy /E /I /Y "${outputPath}" "${archiveDirectory}"`
+ : `cp -r "${outputPath}" "${archiveDirectory}"`;
+ await OrchestratorSystem.Run(copyCommand, true);
fs.rmSync(outputPath, { recursive: true, force: true });Alternatively, use Node.js fs.cpSync (Node 16.7+) for a purely cross-platform solution:
fs.cpSync(outputPath, archiveDirectory, { recursive: 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
| try { | |
| await OrchestratorSystem.Run(`cp -r "${outputPath}" "${archiveDirectory}"`, true); | |
| fs.rmSync(outputPath, { recursive: true, force: true }); | |
| try { | |
| // Use platform-appropriate copy command | |
| const isWindows = process.platform === 'win32'; | |
| const copyCommand = isWindows | |
| ? `xcopy /E /I /Y "${outputPath}" "${archiveDirectory}"` | |
| : `cp -r "${outputPath}" "${archiveDirectory}"`; | |
| await OrchestratorSystem.Run(copyCommand, true); | |
| fs.rmSync(outputPath, { recursive: true, force: true }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`
around lines 269 - 271, The fallback for cross-device moves currently shells out
with OrchestratorSystem.Run("cp -r ...") which is Unix-only; replace that shell
copy with a cross-platform Node API call (use fs.cpSync or fs.promises.cp where
available) to copy from the outputPath to archiveDirectory and then remove
outputPath with fs.rmSync; update the block around OrchestratorSystem.Run in
build-reliability-service.ts (the call that references outputPath and
archiveDirectory) to perform the copy using fs.cpSync/await fs.promises.cp and
retain the subsequent fs.rmSync removal.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@action.yml`:
- Around line 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.
In `@src/index.ts`:
- Around line 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.
In
`@src/model/orchestrator/services/reliability/build-reliability-service.test.ts`:
- Around line 259-262: The test assigns const result =
BuildReliabilityService.recoverCorruptedRepo('/repo') but never asserts anything
about it; add an assertion for result (e.g., expect(result).toBeDefined() or
expect(result).toBeTruthy(), or the precise expected value) and if
recoverCorruptedRepo returns a Promise, await it first (await
BuildReliabilityService.recoverCorruptedRepo('/repo')) before asserting; update
the assertion near the existing callCount check so the test both verifies side
effects and the method return value.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`:
- Around line 224-255: recoverCorruptedRepo currently stops after a failed fsck;
update BuildReliabilityService.recoverCorruptedRepo to perform the documented
fresh-clone fallback when healthy is false: detect remote URL (e.g., via
execSync('git -C "<repoPath>" config --get remote.origin.url')), remove or move
the corrupted .git directory (use repoPath + '/.git' and ensure safe error
handling), perform a fresh clone into repoPath (execSync('git clone <remoteUrl>
"<repoPath>"' or clone to a temp dir and move contents), then re-run
BuildReliabilityService.checkGitIntegrity and return that result; add
appropriate core.info/core.warning logs around each step and preserve existing
cleanStaleLockFiles and fetch behavior if remote URL is missing or clone fails.
- Around line 53-57: Replace uses of execSync that build shell strings with
interpolated paths (e.g., the execSync call in build-reliability-service.ts
around the git fsck invocation and the similar calls at the other occurrences)
with safe argument-array based calls such as child_process.execFileSync or
child_process.spawnSync; specifically, stop constructing command strings like
`git -C "${repoPath}" fsck --no-dangling` and instead call execFileSync('git',
['-C', repoPath, 'fsck', '--no-dangling'], { encoding: 'utf8', timeout: 120_000,
stdio: ['pipe','pipe','pipe'] }) (and apply same pattern for the other two
occurrences), ensuring all path/arg values are passed as array elements to
prevent shell injection.
- Around line 342-365: enforceRetention currently deletes every entry under
archivePath based solely on age; restrict deletion to only managed archive
entries (e.g., match a known naming pattern or require a marker/metadata file
inside each entry) and skip any non-matching files/dirs to avoid removing
unrelated data. implement per-platform retention by grouping matching archive
entries (use entry.name and/or a metadata field inside each archive directory)
and for each platform sort by mtimeMs (newest first), keep the N most recent
per-platform, then delete only those older than retentionMs and exceeding the
per-platform keep count; update enforceRetention(archivePath: string,
retentionDays: number) to perform these checks and grouping before calling
fs.rmSync/fs.unlinkSync and incrementing removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce13de48-dfb1-4dab-bb6d-fcc88d8843c5
⛔ 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 (5)
action.ymlsrc/index.tssrc/model/input.tssrc/model/orchestrator/services/reliability/build-reliability-service.test.tssrc/model/orchestrator/services/reliability/build-reliability-service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/model/input.ts
Comment on lines
+286
to
+289
| gitAutoRecover: | ||
| description: 'Attempt automatic recovery if git corruption is detected' | ||
| required: false | ||
| default: 'false' |
There was a problem hiding this comment.
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.
Comment on lines
+30
to
+33
| const isHealthy = BuildReliabilityService.checkGitIntegrity(workspace); | ||
| BuildReliabilityService.cleanStaleLockFiles(workspace); | ||
| BuildReliabilityService.validateSubmoduleBackingStores(workspace); | ||
|
|
There was a problem hiding this comment.
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.
Comment on lines
+259
to
+262
| const result = BuildReliabilityService.recoverCorruptedRepo('/repo'); | ||
| // Should still attempt fsck after failed fetch | ||
| expect(callCount).toBeGreaterThanOrEqual(2); | ||
| }); |
There was a problem hiding this comment.
Add an assertion for result (currently unused).
At Line 259, result is never used, which can fail lint and leaves the test less explicit about the expected outcome.
✅ Suggested fix
const result = BuildReliabilityService.recoverCorruptedRepo('/repo');
// Should still attempt fsck after failed fetch
expect(callCount).toBeGreaterThanOrEqual(2);
+ expect(result).toBe(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
| const result = BuildReliabilityService.recoverCorruptedRepo('/repo'); | |
| // Should still attempt fsck after failed fetch | |
| expect(callCount).toBeGreaterThanOrEqual(2); | |
| }); | |
| const result = BuildReliabilityService.recoverCorruptedRepo('/repo'); | |
| // Should still attempt fsck after failed fetch | |
| expect(callCount).toBeGreaterThanOrEqual(2); | |
| expect(result).toBe(true); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 259-259: 'result' is assigned a value but never used.
(no-unused-vars)
[error] 260-260: Expected line before comment.
(lines-around-comment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/model/orchestrator/services/reliability/build-reliability-service.test.ts`
around lines 259 - 262, The test assigns const result =
BuildReliabilityService.recoverCorruptedRepo('/repo') but never asserts anything
about it; add an assertion for result (e.g., expect(result).toBeDefined() or
expect(result).toBeTruthy(), or the precise expected value) and if
recoverCorruptedRepo returns a Promise, await it first (await
BuildReliabilityService.recoverCorruptedRepo('/repo')) before asserting; update
the assertion near the existing callCount check so the test both verifies side
effects and the method return value.
src/model/orchestrator/services/reliability/build-reliability-service.ts
Show resolved
Hide resolved
Comment on lines
+224
to
+255
| static recoverCorruptedRepo(repoPath: string = '.'): boolean { | ||
| core.warning(`[Reliability] Attempting automatic recovery for ${repoPath}`); | ||
|
|
||
| // Step 1: Clean stale lock files that may be preventing operations | ||
| const locksRemoved = BuildReliabilityService.cleanStaleLockFiles(repoPath); | ||
| if (locksRemoved > 0) { | ||
| core.info(`[Reliability] Recovery: cleaned ${locksRemoved} lock file(s)`); | ||
| } | ||
|
|
||
| // Step 2: Re-fetch to restore missing objects | ||
| try { | ||
| core.info('[Reliability] Recovery: re-fetching from remote'); | ||
| execSync(`git -C "${repoPath}" fetch --all`, { | ||
| encoding: 'utf8', | ||
| timeout: 300_000, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
| core.info('[Reliability] Recovery: fetch completed'); | ||
| } catch (error: any) { | ||
| core.warning(`[Reliability] Recovery: fetch failed: ${error.stderr?.toString() ?? error.message}`); | ||
| } | ||
|
|
||
| // Step 3: Retry fsck | ||
| const healthy = BuildReliabilityService.checkGitIntegrity(repoPath); | ||
| if (healthy) { | ||
| core.info('[Reliability] Recovery succeeded -- repository is healthy'); | ||
| } else { | ||
| core.warning('[Reliability] Recovery failed -- repository still has integrity issues'); | ||
| } | ||
|
|
||
| return healthy; | ||
| } |
There was a problem hiding this comment.
Auto-recovery stops short of the fresh-clone fallback.
At Line 224 onward, recovery only does stale lock cleanup + git fetch + fsck retry. If corruption remains, it returns false without performing (or signaling) the .git reset/reclone fallback described for auto-recovery.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`
around lines 224 - 255, recoverCorruptedRepo currently stops after a failed
fsck; update BuildReliabilityService.recoverCorruptedRepo to perform the
documented fresh-clone fallback when healthy is false: detect remote URL (e.g.,
via execSync('git -C "<repoPath>" config --get remote.origin.url')), remove or
move the corrupted .git directory (use repoPath + '/.git' and ensure safe error
handling), perform a fresh clone into repoPath (execSync('git clone <remoteUrl>
"<repoPath>"' or clone to a temp dir and move contents), then re-run
BuildReliabilityService.checkGitIntegrity and return that result; add
appropriate core.info/core.warning logs around each step and preserve existing
cleanStaleLockFiles and fetch behavior if remote URL is missing or clone fails.
Comment on lines
+342
to
+365
| static enforceRetention(archivePath: string, retentionDays: number): number { | ||
| if (!fs.existsSync(archivePath)) { | ||
| return 0; | ||
| } | ||
|
|
||
| const now = Date.now(); | ||
| const retentionMs = retentionDays * 24 * 60 * 60 * 1000; | ||
| let removed = 0; | ||
|
|
||
| try { | ||
| const entries = fs.readdirSync(archivePath, { withFileTypes: true }); | ||
| for (const entry of entries) { | ||
| const fullPath = path.join(archivePath, entry.name); | ||
| try { | ||
| const stat = fs.statSync(fullPath); | ||
| const ageMs = now - stat.mtimeMs; | ||
|
|
||
| if (ageMs > retentionMs) { | ||
| if (entry.isDirectory()) { | ||
| fs.rmSync(fullPath, { recursive: true, force: true }); | ||
| } else { | ||
| fs.unlinkSync(fullPath); | ||
| } | ||
| removed++; |
There was a problem hiding this comment.
Retention cleanup is over-broad and can delete non-archive data.
At Line 352 onward, retention iterates every entry under archivePath and deletes by age only. This can remove unrelated files/directories if the archive path contains anything besides managed archives. It also does not implement keep-N-per-platform behavior.
🔧 Minimum safety guard
for (const entry of entries) {
+ // Only manage archives created by this service
+ if (!entry.isFile() || !entry.name.startsWith('build-') || !entry.name.endsWith('.tar.gz')) {
+ continue;
+ }
+
const fullPath = path.join(archivePath, entry.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`
around lines 342 - 365, enforceRetention currently deletes every entry under
archivePath based solely on age; restrict deletion to only managed archive
entries (e.g., match a known naming pattern or require a marker/metadata file
inside each entry) and skip any non-matching files/dirs to avoid removing
unrelated data. implement per-platform retention by grouping matching archive
entries (use entry.name and/or a metadata field inside each archive directory)
and for each platform sort by mtimeMs (newest first), keep the N most recent
per-platform, then delete only those older than retentionMs and exceeding the
per-platform keep count; update enforceRetention(archivePath: string,
retentionDays: number) to perform these checks and grouping before calling
fs.rmSync/fs.unlinkSync and incrementing removed.
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #808 +/- ##
==========================================
+ Coverage 31.25% 34.52% +3.27%
==========================================
Files 84 85 +1
Lines 4563 4814 +251
Branches 1103 1161 +58
==========================================
+ Hits 1426 1662 +236
- Misses 3137 3152 +15
🚀 New features to boost your workflow:
|
Check available disk space (cross-platform: wmic/df) before archive operations to prevent data loss on full disks. Skip archival with warning if insufficient space (10% safety margin). Clean up partial archives on tar failure. Proceed with warning when space check fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/model/orchestrator/services/reliability/build-reliability-service.ts (3)
417-419:⚠️ Potential issue | 🟠 MajorRetention policy is over-broad and does not implement keep-N-per-platform.
Current logic deletes any entry under
archivePathpurely by age, and archive naming lacks platform metadata, so per-platform retention cannot be enforced.🧭 Safer retention direction
- const archiveFile = path.join(archivePath, `build-${timestamp}.tar.gz`); + const archiveFile = path.join(archivePath, `build-${platform}-${timestamp}.tar.gz`);- for (const entry of entries) { + for (const entry of entries) { + if (!entry.isFile() || !entry.name.startsWith('build-') || !entry.name.endsWith('.tar.gz')) { + continue; + } // group by platform, sort newest-first, keep N per platform, delete overflow only }Also applies to: 446-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 417 - 419, The current retention deletes by age and filenames (timestamp) only; update the archive naming and cleanup to support keep-N-per-platform: include platform identifier in the archive filename (e.g., change how archiveFile is built using timestamp plus platform/tag from the build context) and modify the cleanup logic that scans archivePath to group archives by platform, sort by timestamp, and delete only older entries beyond the configured N per platform (instead of deleting purely by age); update functions/variables referenced (timestamp, archiveFile, archivePath and the cleanup routine around lines ~446-492) to parse platform from filenames and enforce per-platform retention.
53-57:⚠️ Potential issue | 🔴 CriticalUse argument-array process execution for git/tar commands.
These calls still build shell strings with interpolated paths. This is injection-prone and breaks on edge-case path characters.
🔒 Safer invocation pattern
- const output = execSync(`git -C "${repoPath}" fsck --no-dangling`, { + const output = execFileSync('git', ['-C', repoPath, 'fsck', '--no-dangling'], { encoding: 'utf8', timeout: 120_000, stdio: ['pipe', 'pipe', 'pipe'], }); - execSync(`git -C "${repoPath}" fetch --all`, { + execFileSync('git', ['-C', repoPath, 'fetch', '--all'], { encoding: 'utf8', timeout: 300_000, stdio: ['pipe', 'pipe', 'pipe'], }); - execSync(`tar -czf "${archiveFile}" -C "${path.dirname(sourcePath)}" "${path.basename(sourcePath)}"`, { + execFileSync('tar', ['-czf', archiveFile, '-C', path.dirname(sourcePath), path.basename(sourcePath)], { encoding: 'utf8', timeout: 600_000, stdio: ['pipe', 'pipe', 'pipe'], });#!/bin/bash # Verify interpolated shell-string execSync usage in this service file rg -nP --type=ts 'execSync\(`[^`]*\$\{[^}]+\}[^`]*`' src/model/orchestrator/services/reliability/build-reliability-service.tsAlso applies to: 236-240, 421-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 53 - 57, The execSync call that builds a shell string with an interpolated repoPath is injection-prone and fails for special-path characters; replace usages of execSync(`git -C "${repoPath}" ...`) with a safer argument-array invocation such as child_process.execFileSync or spawnSync using the command and args array (e.g., ['-C', repoPath, 'fsck', '--no-dangling']) or by passing repoPath via the cwd option, and preserve encoding/timeout/stdio options; apply the same change for the other execSync/tar invocations in this file so all execSync(`...${repoPath}...`) occurrences use argument arrays or cwd instead.
224-255:⚠️ Potential issue | 🟠 MajorRecovery flow still lacks the documented fresh-clone fallback.
Current auto-recovery stops after lock cleanup + fetch + fsck retry. If corruption persists, it returns
falsewithout attempting.gitreset/reclone.src/model/orchestrator/services/reliability/build-reliability-service.test.ts (1)
260-263:⚠️ Potential issue | 🟡 MinorAssert the recovery result in this test.
resultis currently unused; add an explicit expectation so the test validates return behavior and avoids lint failure.💚 Suggested test fix
const result = BuildReliabilityService.recoverCorruptedRepo('/repo'); // Should still attempt fsck after failed fetch expect(callCount).toBeGreaterThanOrEqual(2); + expect(result).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.test.ts` around lines 260 - 263, The test captures the return value from BuildReliabilityService.recoverCorruptedRepo('/repo') into result but never asserts it; add an explicit assertion on result (e.g., expect(result).toBeDefined() or, if the intended behavior is a boolean success flag, expect(result).toBe(true)) so the test validates the method's return and satisfies the linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`:
- Around line 498-503: In configureGitEnvironment add setting
process.env.GIT_CONFIG_NOSYSTEM = '1' alongside the existing GIT_TERMINAL_PROMPT
assignment and include a core.info log (e.g., "[Reliability] Set
GIT_CONFIG_NOSYSTEM=1") so the CI-hardening env var is applied and visible;
update the static method configureGitEnvironment to export this variable before
any git operations run.
---
Duplicate comments:
In
`@src/model/orchestrator/services/reliability/build-reliability-service.test.ts`:
- Around line 260-263: The test captures the return value from
BuildReliabilityService.recoverCorruptedRepo('/repo') into result but never
asserts it; add an explicit assertion on result (e.g.,
expect(result).toBeDefined() or, if the intended behavior is a boolean success
flag, expect(result).toBe(true)) so the test validates the method's return and
satisfies the linter.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`:
- Around line 417-419: The current retention deletes by age and filenames
(timestamp) only; update the archive naming and cleanup to support
keep-N-per-platform: include platform identifier in the archive filename (e.g.,
change how archiveFile is built using timestamp plus platform/tag from the build
context) and modify the cleanup logic that scans archivePath to group archives
by platform, sort by timestamp, and delete only older entries beyond the
configured N per platform (instead of deleting purely by age); update
functions/variables referenced (timestamp, archiveFile, archivePath and the
cleanup routine around lines ~446-492) to parse platform from filenames and
enforce per-platform retention.
- Around line 53-57: The execSync call that builds a shell string with an
interpolated repoPath is injection-prone and fails for special-path characters;
replace usages of execSync(`git -C "${repoPath}" ...`) with a safer
argument-array invocation such as child_process.execFileSync or spawnSync using
the command and args array (e.g., ['-C', repoPath, 'fsck', '--no-dangling']) or
by passing repoPath via the cwd option, and preserve encoding/timeout/stdio
options; apply the same change for the other execSync/tar invocations in this
file so all execSync(`...${repoPath}...`) occurrences use argument arrays or cwd
instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: db09fbe1-fd55-4d0b-95d5-e4d2c54962ea
📒 Files selected for processing (2)
src/model/orchestrator/services/reliability/build-reliability-service.test.tssrc/model/orchestrator/services/reliability/build-reliability-service.ts
Comment on lines
+498
to
+503
| static configureGitEnvironment(): void { | ||
| core.info('[Reliability] Configuring git environment for CI'); | ||
|
|
||
| // Prevent git from prompting for credentials (hangs in CI) | ||
| process.env.GIT_TERMINAL_PROMPT = '0'; | ||
| core.info('[Reliability] Set GIT_TERMINAL_PROMPT=0'); |
There was a problem hiding this comment.
Set GIT_CONFIG_NOSYSTEM=1 in reliability git environment setup.
The documented CI-hardening env setting is missing here, so this mitigation is currently not applied.
✅ Minimal fix
// Prevent git from prompting for credentials (hangs in CI)
process.env.GIT_TERMINAL_PROMPT = '0';
+ process.env.GIT_CONFIG_NOSYSTEM = '1';
core.info('[Reliability] Set GIT_TERMINAL_PROMPT=0');
+ core.info('[Reliability] Set GIT_CONFIG_NOSYSTEM=1');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`
around lines 498 - 503, In configureGitEnvironment add setting
process.env.GIT_CONFIG_NOSYSTEM = '1' alongside the existing GIT_TERMINAL_PROMPT
assignment and include a core.info log (e.g., "[Reliability] Set
GIT_CONFIG_NOSYSTEM=1") so the CI-hardening env var is applied and visible;
update the static method configureGitEnvironment to export this variable before
any git operations run.
7 tasks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/model/orchestrator/services/reliability/build-reliability-service.ts (6)
496-501:⚠️ Potential issue | 🟠 MajorMissing
GIT_CONFIG_NOSYSTEM=1environment variable.Per PR objectives, the CI-hardening should "set
GIT_CONFIG_NOSYSTEM=1to bypass system config issues" but this is not implemented.✅ Fix
// Prevent git from prompting for credentials (hangs in CI) process.env.GIT_TERMINAL_PROMPT = '0'; + process.env.GIT_CONFIG_NOSYSTEM = '1'; core.info('[Reliability] Set GIT_TERMINAL_PROMPT=0'); + core.info('[Reliability] Set GIT_CONFIG_NOSYSTEM=1');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 496 - 501, The configureGitEnvironment function currently sets GIT_TERMINAL_PROMPT but misses disabling system git config; update the static method configureGitEnvironment to also set process.env.GIT_CONFIG_NOSYSTEM = '1' and add a corresponding core.info('[Reliability] Set GIT_CONFIG_NOSYSTEM=1') call so CI bypasses system config issues (locate this in build-reliability-service.ts inside the configureGitEnvironment method).
246-255:⚠️ Potential issue | 🟠 MajorRecovery does not implement the fresh-clone fallback.
Per PR objectives,
gitAutoRecovershould "remove the corrupted.gitand reclone" when corruption persists. Currently, ifhealthyis false after fetch, the method just returnsfalsewithout attempting the documented.gitreset/reclone fallback. Consider implementing the full recovery path or updating documentation to reflect actual behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 246 - 255, The gitAutoRecover flow currently stops when BuildReliabilityService.checkGitIntegrity(repoPath) returns false; implement the documented fallback by removing the corrupted .git directory and performing a fresh clone of the repo into repoPath (or a temp dir then replace) when recovery fails: add logic in the method that on healthy === false deletes repoPath/.git (use safe FS ops), runs a git clone from the original remote (obtain remote URL via existing codepaths or Git config), then re-run BuildReliabilityService.checkGitIntegrity(repoPath) and return the final boolean; also emit clear logs (core.info/core.warning) before/after deletion and clone to reflect the new steps.
444-477:⚠️ Potential issue | 🟠 MajorRetention cleanup remains over-broad and lacks per-platform count.
This still deletes any entry in
archivePatholder thanretentionDays, which can remove unrelated files. Additionally, PR objectives specify "maintain a per-platform retention count (default keep 3)" but this only implements age-based cleanup, not keep-N-per-platform.🔧 Minimum safety guard
for (const entry of entries) { + // Only manage archives created by this service + if (!entry.name.startsWith('build-') || !entry.name.endsWith('.tar.gz')) { + continue; + } + const fullPath = path.join(archivePath, entry.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 444 - 477, enforceRetention currently deletes any old file under archivePath and doesn't enforce per-platform keep-N; change its behavior by adding a keepPerPlatform parameter (default 3) and only target archive entries that match the archive naming/metadata convention (parse platform via a helper like parsePlatformFromName or a regex against entry.name), then group matching entries by platform, sort each group's entries by mtimeMs (newest first) and remove entries that exceed the keepPerPlatform count for that platform (and/or are older than retentionDays), leaving unrelated files untouched; update the static method enforceRetention and use helpers parsePlatformFromName (or similar) and the existing fs.statSync/entry.isDirectory logic to identify and remove only the intended archive files.
53-57:⚠️ Potential issue | 🔴 CriticalShell command injection vulnerability persists.
The
execSynccall with interpolatedrepoPathis still vulnerable to command injection if the path contains shell metacharacters. The previous review indicated this was addressed, but the code still uses shell string interpolation instead ofexecFileSyncwith argument arrays.🔒 Secure fix using execFileSync
- const output = execSync(`git -C "${repoPath}" fsck --no-dangling`, { + const output = execFileSync('git', ['-C', repoPath, 'fsck', '--no-dangling'], { encoding: 'utf8', timeout: 120_000, stdio: ['pipe', 'pipe', 'pipe'], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 53 - 57, The execSync call that builds the shell string using repoPath is vulnerable to command injection; replace the use of execSync with execFileSync (imported from child_process) and pass the command and arguments as an array (e.g., command "git" and args ["-C", repoPath, "fsck", "--no-dangling"]) instead of interpolating repoPath into a shell string, preserving the existing options (encoding, timeout, stdio) and assigning the result to the same output variable; ensure you update the call site where execSync is used in build-reliability-service (the line creating output) and remove any need for a shell to mitigate injection.
419-423:⚠️ Potential issue | 🔴 CriticalShell command injection vulnerability in tar command.
The
tarcommand uses shell string interpolation for paths. UseexecFileSyncwith argument arrays.🔒 Secure fix
- execSync(`tar -czf "${archiveFile}" -C "${path.dirname(sourcePath)}" "${path.basename(sourcePath)}"`, { + execFileSync('tar', ['-czf', archiveFile, '-C', path.dirname(sourcePath), path.basename(sourcePath)], { encoding: 'utf8', timeout: 600_000, stdio: ['pipe', 'pipe', 'pipe'], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 419 - 423, Replace the unsafe execSync call that interpolates paths into a shell string with a call to execFileSync (from child_process) that passes "tar" as the program and an argument array built from path.dirname(sourcePath), path.basename(sourcePath) and the -C/-czf flags (e.g., ['-czf', archiveFile, '-C', path.dirname(sourcePath), path.basename(sourcePath)]), preserving the same options (encoding, timeout, stdio); update the call site where execSync is used so the command no longer goes through the shell and uses execFileSync with explicit args to avoid shell injection.
236-240:⚠️ Potential issue | 🔴 CriticalShell command injection vulnerability.
Same issue as
checkGitIntegrity— useexecFileSyncwith argument arrays.🔒 Secure fix
- execSync(`git -C "${repoPath}" fetch --all`, { + execFileSync('git', ['-C', repoPath, 'fetch', '--all'], { encoding: 'utf8', timeout: 300_000, stdio: ['pipe', 'pipe', 'pipe'], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/reliability/build-reliability-service.ts` around lines 236 - 240, The execSync call using string interpolation with repoPath in build-reliability-service.ts is vulnerable to shell injection; replace the execSync(`git -C "${repoPath}" fetch --all`, ...) call in the function that runs Git commands with execFileSync (imported from child_process) and pass the command and args as an array like ['fetch', '--all'] while setting cwd: repoPath (instead of embedding -C in the string), preserve encoding, timeout and stdio options, and ensure shell: false so the repoPath is never interpreted by a shell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`:
- Around line 60-66: The /dangling/i regex in the corruptionPatterns array is
dead because the git fsck invocation uses the --no-dangling flag; either remove
the /dangling/i entry from corruptionPatterns or remove the --no-dangling flag
so dangling object warnings are reported. Update the corruptionPatterns array
(symbol: corruptionPatterns) to drop /dangling/i if you want to keep
--no-dangling, or remove the --no-dangling flag from the git fsck call so
/dangling/i can match. Ensure only one of these changes is applied to avoid
reintroducing unreachable pattern checks.
- Around line 320-330: The Windows branch currently uses execFileSync('wmic',
...) which is deprecated; replace this with a PowerShell invocation that runs
Get-CimInstance Win32_LogicalDisk filtered by the drive (use the same
driveLetter derived from path.parse(directoryPath).root) to fetch FreeSpace,
parse the returned FreeSpace bytes and divide by (1024 * 1024) as before; update
the execFileSync call and argument list (e.g., invoke powershell.exe -NoProfile
-Command "<Get-CimInstance ... | Select-Object -ExpandProperty FreeSpace>") and
keep the same fallback of returning -1 when parsing fails, ensuring the code
references the same symbols (directoryPath, drive, driveLetter, execFileSync)
and preserves the timeout/encoding behavior.
---
Duplicate comments:
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`:
- Around line 496-501: The configureGitEnvironment function currently sets
GIT_TERMINAL_PROMPT but misses disabling system git config; update the static
method configureGitEnvironment to also set process.env.GIT_CONFIG_NOSYSTEM = '1'
and add a corresponding core.info('[Reliability] Set GIT_CONFIG_NOSYSTEM=1')
call so CI bypasses system config issues (locate this in
build-reliability-service.ts inside the configureGitEnvironment method).
- Around line 246-255: The gitAutoRecover flow currently stops when
BuildReliabilityService.checkGitIntegrity(repoPath) returns false; implement the
documented fallback by removing the corrupted .git directory and performing a
fresh clone of the repo into repoPath (or a temp dir then replace) when recovery
fails: add logic in the method that on healthy === false deletes repoPath/.git
(use safe FS ops), runs a git clone from the original remote (obtain remote URL
via existing codepaths or Git config), then re-run
BuildReliabilityService.checkGitIntegrity(repoPath) and return the final
boolean; also emit clear logs (core.info/core.warning) before/after deletion and
clone to reflect the new steps.
- Around line 444-477: enforceRetention currently deletes any old file under
archivePath and doesn't enforce per-platform keep-N; change its behavior by
adding a keepPerPlatform parameter (default 3) and only target archive entries
that match the archive naming/metadata convention (parse platform via a helper
like parsePlatformFromName or a regex against entry.name), then group matching
entries by platform, sort each group's entries by mtimeMs (newest first) and
remove entries that exceed the keepPerPlatform count for that platform (and/or
are older than retentionDays), leaving unrelated files untouched; update the
static method enforceRetention and use helpers parsePlatformFromName (or
similar) and the existing fs.statSync/entry.isDirectory logic to identify and
remove only the intended archive files.
- Around line 53-57: The execSync call that builds the shell string using
repoPath is vulnerable to command injection; replace the use of execSync with
execFileSync (imported from child_process) and pass the command and arguments as
an array (e.g., command "git" and args ["-C", repoPath, "fsck",
"--no-dangling"]) instead of interpolating repoPath into a shell string,
preserving the existing options (encoding, timeout, stdio) and assigning the
result to the same output variable; ensure you update the call site where
execSync is used in build-reliability-service (the line creating output) and
remove any need for a shell to mitigate injection.
- Around line 419-423: Replace the unsafe execSync call that interpolates paths
into a shell string with a call to execFileSync (from child_process) that passes
"tar" as the program and an argument array built from path.dirname(sourcePath),
path.basename(sourcePath) and the -C/-czf flags (e.g., ['-czf', archiveFile,
'-C', path.dirname(sourcePath), path.basename(sourcePath)]), preserving the same
options (encoding, timeout, stdio); update the call site where execSync is used
so the command no longer goes through the shell and uses execFileSync with
explicit args to avoid shell injection.
- Around line 236-240: The execSync call using string interpolation with
repoPath in build-reliability-service.ts is vulnerable to shell injection;
replace the execSync(`git -C "${repoPath}" fetch --all`, ...) call in the
function that runs Git commands with execFileSync (imported from child_process)
and pass the command and args as an array like ['fetch', '--all'] while setting
cwd: repoPath (instead of embedding -C in the string), preserve encoding,
timeout and stdio options, and ensure shell: false so the repoPath is never
interpreted by a shell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: edbaa1e3-5a29-46bf-ac65-1ddaa5e3a180
⛔ 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 (1)
src/model/orchestrator/services/reliability/build-reliability-service.ts
Comment on lines
+60
to
+66
| const corruptionPatterns = [ | ||
| /broken link/i, | ||
| /missing (blob|tree|commit|tag)/i, | ||
| /dangling/i, | ||
| /corrupt/i, | ||
| /error in /i, | ||
| ]; |
There was a problem hiding this comment.
Dead code: dangling pattern will never match.
The --no-dangling flag on line 53 suppresses dangling object warnings from git fsck, making the /dangling/i pattern at line 63 unreachable. Either remove the pattern or remove --no-dangling if you want to detect dangling objects.
♻️ Remove dead pattern
const corruptionPatterns = [
/broken link/i,
/missing (blob|tree|commit|tag)/i,
- /dangling/i,
/corrupt/i,
/error in /i,
];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`
around lines 60 - 66, The /dangling/i regex in the corruptionPatterns array is
dead because the git fsck invocation uses the --no-dangling flag; either remove
the /dangling/i entry from corruptionPatterns or remove the --no-dangling flag
so dangling object warnings are reported. Update the corruptionPatterns array
(symbol: corruptionPatterns) to drop /dangling/i if you want to keep
--no-dangling, or remove the --no-dangling flag from the git fsck call so
/dangling/i can match. Ensure only one of these changes is applied to avoid
reintroducing unreachable pattern checks.
Comment on lines
+320
to
+330
| if (process.platform === 'win32') { | ||
| const drive = path.parse(directoryPath).root; | ||
| const driveLetter = drive.replace(/[:\\\/]/g, ''); | ||
| const output = execFileSync( | ||
| 'wmic', | ||
| ['logicaldisk', 'where', `DeviceID='${driveLetter}:'`, 'get', 'FreeSpace', '/value'], | ||
| { encoding: 'utf8', timeout: 10_000 }, | ||
| ); | ||
| const match = output.match(/FreeSpace=(\d+)/); | ||
|
|
||
| return match ? Number.parseInt(match[1], 10) / (1024 * 1024) : -1; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
wmic deprecated Windows 11 Windows 21H1 removal status 2024 2025
💡 Result:
- “21H1” refers to Windows 10, version 21H1 (and Windows Server 21H1 GA channel), not Windows 11. In that release, WMIC was officially marked deprecated (WMI itself is not deprecated). [2]
- Windows 11 status (2024): WMIC was turned into a Feature on Demand (FoD) starting with Windows 11 22H2; Microsoft stated it would become disabled by default beginning with the next Windows 11 release (Insider builds after Jan 29, 2024). [3] Microsoft’s current guidance also states WMIC is disabled by default in Windows 11 23H2 and 24H2, still available as FoD. [1]
- Windows 11 status (2025): Microsoft states new installations of Windows 11 24H2 and 25H2 have WMIC removed by default (still installable as FoD), and that upgrading to Windows 11 25H2 removes WMIC if it was installed (but it can be added back as FoD). [1]
- Planned complete removal: Microsoft’s current timeline says WMIC will be fully removed in the next Windows 11 feature update (listed as 2026) and no longer available as FoD. [1]
Sources: Microsoft Support WMIC removal guidance [1], Microsoft Learn deprecated features list [2], Windows IT Pro blog “WMIC deprecation: next steps” [3].
🌐 Web query:
PowerShell Get-CimInstance Get-PSDrive disk free space alternative to wmic
💡 Result:
WMIC replacement (like wmic logicaldisk get size,freespace,caption)
CIM/WMI (recommended):
Get-CimInstance -ClassName Win32_LogicalDisk -Filter "DriveType=3" |
Select-Object DeviceID, Size, FreeSpace,
@{n='Used';e={$_.Size - $_.FreeSpace}},
@{n='FreePct';e={[math]::Round(100*$_.FreeSpace/$_.Size,2)}}Win32_LogicalDisk exposes DeviceID, Size, and FreeSpace (bytes), and DriveType=3 filters to local fixed disks. [1][2]
Single drive (e.g., C:):
Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='C:'" |
Select-Object DeviceID, Size, FreeSpace[2]
Quick alternative: Get-PSDrive (FileSystem only)
Get-PSDrive -PSProvider FileSystem |
Select-Object Name, Used, FreeThis returns Used/Free for filesystem drives (not a 1:1 match for all Win32_LogicalDisk scenarios, but handy interactively). [3]
Another modern option (Storage module): Get-Volume
Get-Volume |
Select-Object DriveLetter, Size, SizeRemaining(Useful when you want volume-centric info; not the same class as Win32_LogicalDisk.) [4]
Sources: [1] [2] [3] [4]
🌐 Web query:
wmic command Windows 11 still available deprecated timeline
💡 Result:
Yes—WMIC can still be used on Windows 11 today, but it’s in end-of-life removal. On current Windows 11 builds it may not be present by default and may require installing the WMIC Feature on Demand (FoD). [1]
Deprecation / removal timeline (Microsoft)
- 2016: WMIC deprecated (Windows Server line). [1][2]
- 2021: WMIC deprecated in Windows 10, version 21H2. [1]
- 2022: In Windows 11, version 22H2, WMIC became a Feature on Demand (FoD) (and was preinstalled/enabled by default at that time). [1][2]
- 2024: WMIC disabled by default in Windows 11, versions 23H2 and 24H2, but still available as FoD. [1][2]
- Windows 11 24H2 and 25H2 (latest installs): WMIC is removed by default (only installable as FoD). [1]
- 2025: Upgrading to Windows 11, version 25H2 removes WMIC if it was installed (but you can add it back as FoD). [1]
- 2026 (next Windows 11 feature update): WMIC is planned to be completely removed and no longer available even as FoD. [1]
What to use instead
Microsoft’s stated replacement is PowerShell WMI/CIM cmdlets (for example Get-CimInstance). [1][3]
Sources: Microsoft Support WMIC removal guidance [1], Microsoft Windows IT Pro Blog (WMIC deprecation next steps) [2], Microsoft Learn (Deprecated features) [3].
Replace wmic with PowerShell for cross-version Windows 11 compatibility.
wmic is deprecated (Windows 10 21H1) and disabled by default in Windows 11 25H2. It's planned for complete removal in 2026. Use PowerShell's Get-CimInstance instead:
Get-CimInstance Win32_LogicalDisk -Filter "DeviceID='C:'" | Select-Object FreeSpaceThis returns free space in bytes; divide by (1024 × 1024) to match the current logic.
🧰 Tools
🪛 ESLint
[error] 322-322: /[:\/]/g can be optimized to /[/:\]/g.
(unicorn/better-regex)
[error] 322-322: Unnecessary escape character: /.
(no-useless-escape)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/reliability/build-reliability-service.ts`
around lines 320 - 330, The Windows branch currently uses execFileSync('wmic',
...) which is deprecated; replace this with a PowerShell invocation that runs
Get-CimInstance Win32_LogicalDisk filtered by the drive (use the same
driveLetter derived from path.parse(directoryPath).root) to fetch FreeSpace,
parse the returned FreeSpace bytes and divide by (1024 * 1024) as before; update
the execFileSync call and argument list (e.g., invoke powershell.exe -NoProfile
-Command "<Get-CimInstance ... | Select-Object -ExpandProperty FreeSpace>") and
keep the same fallback of returning -1 when parsing fails, ensuring the code
references the same symbols (directoryPath, drive, driveLetter, execFileSync)
and preserves the timeout/encoding behavior.
This was referenced Mar 5, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-tests-mac.yml:
- 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 95f5a560-5f23-4a49-b5ab-aad18f86bda9
📒 Files selected for processing (1)
.github/workflows/build-tests-mac.yml
| buildForAllPlatformsMacOS: | ||
| name: ${{ matrix.targetPlatform }} on ${{ matrix.unityVersion }} | ||
| runs-on: macos-latest | ||
| continue-on-error: true |
There was a problem hiding this comment.
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:
- Documenting why macOS specifically needs this
- Applying consistently across all platform workflows if the intent is uniform
- 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.
The orchestrator-develop branch no longer exists. Update all fallback clone commands and test fixtures to use main instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
Closing — all orchestrator code has been extracted to the standalone Content from this PR (build reliability features — git integrity, reserved filenames, archival) is fully present in the orchestrator repo. See PR #819 for the extraction. |
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three optional reliability features that harden the build pipeline for self-hosted runners and enterprise environments (#807).
Features
gitIntegrityCheckfalsegit fsck, stale lock cleanup, submodule backing store validationgitAutoRecovertruecleanReservedFilenamesfalsebuildArchiveEnabledfalsebuildArchivePath''buildArchiveRetention3New Service
BuildReliabilityServiceatsrc/model/orchestrator/services/reliability/with methods:cleanStaleLockFiles()— Remove stale git lock files from.git/cleanReservedFilenames()— Scan and remove Windows reserved device names from Assets/validateSubmoduleBackingStores()— Verify submodule.gitfiles point to existing backing storesenforceRetention()— Post-build archival with configurable retention countconfigureGitEnvironment()— Set CI-friendly git environment variables (GIT_CONFIG_NOSYSTEM=1)All features are opt-in and fail gracefully (warnings only, never blocks the build).
Test coverage
5 unit tests covering:
.gitdirectory).gitmodules)Related
Test plan
gitIntegrityCheck: trueon a self-hosted runnercleanReservedFilenames: trueon a project with reserved namesbuildArchiveEnabled: trueto verify retention policySummary by CodeRabbit
New Features
Tests
Chores
Tracking: