feat(orchestrator): incremental sync protocol — git delta, direct input, and storage-backed sync#799
Closed
frostebite wants to merge 4 commits intomainfrom
Closed
feat(orchestrator): incremental sync protocol — git delta, direct input, and storage-backed sync#799frostebite wants to merge 4 commits intomainfrom
frostebite wants to merge 4 commits intomainfrom
Conversation
…ut, and storage-backed sync Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new IncrementalSyncService and SyncStateManager with SyncState/SyncStrategy types, wires incremental sync inputs into orchestrator flows, and implements git-delta, direct-input, and storage-pull (rclone-backed) syncs with state persistence, workspace hashing/drift detection, and optional overlay reversion. Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant Service as IncrementalSyncService
participant Git
participant State as SyncStateFile
Orchestrator->>Service: resolveStrategy(requested, workspace, statePath)
Service->>State: loadSyncState(workspace, statePath)
State-->>Service: SyncState | undefined
Service->>Git: git fetch origin
Git-->>Service: fetched
Service->>Git: git diff --name-only last..target
Git-->>Service: changedFiles[]
alt changes detected
Service->>Git: git checkout targetReference
Git-->>Orchestrator: workspace updated
Service->>State: saveSyncState({lastSyncCommit, timestamp, workspaceHash, pendingOverlays})
else no changes
Service-->>Orchestrator: 0 changed files
end
Service-->>Orchestrator: sync result (count)
sequenceDiagram
participant Orchestrator
participant Service as IncrementalSyncService
participant Storage as rcloneRemote
participant FS as WorkspaceFS
participant State as SyncStateFile
Orchestrator->>Service: applyDirectInput(workspace, inputRef, rcloneRemote)
alt storage:// URI
Service->>Storage: rclone copy remote:path -> tmp
Storage-->>Service: archive
else local path
Service-->>Service: use local archive
end
Service->>FS: extract & overlay archive
FS-->>Service: overlay applied (paths)
Service->>State: saveSyncState({pendingOverlays + timestamp + hash})
Service-->>Orchestrator: list of applied overlay paths
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/model/orchestrator/services/sync/incremental-sync-service.ts`:
- Around line 64-67: The resolver enables 'git-delta' based only on presence of
a state but misses cases where the state has an empty baseline commit; update
the check in resolveStrategy (where requestedStrategy is compared and
IncrementalSyncService.loadSyncState is called) to verify that the loaded
state's lastSyncCommit is a non-empty string before allowing 'git-delta', and
reject/fallback to full sync if lastSyncCommit is falsy; also apply the same
guard where state is inspected around applyDirectInput (which can produce
state.lastSyncCommit === ''), ensuring both locations reference and validate
state.lastSyncCommit before choosing or applying a delta-based strategy.
- Line 31: The JSON.parse result returned as SyncState must be validated before
casting to avoid runtime crashes (e.g., accessing state.pendingOverlays.length).
In the function that currently returns JSON.parse(content) as SyncState, add a
runtime shape check for required properties and types (at minimum ensure the
root is an object, has pendingOverlays as an array, and any other required keys
of SyncState), and throw a clear, descriptive error or recreate a safe default
SyncState if validation fails; use helper validateSyncState(state) or inline
checks and replace the direct cast with the validated object so downstream code
(like accesses to pendingOverlays) is safe.
- Around line 98-99: The interpolated git refs in the commands passed to
OrchestratorSystem.Run are vulnerable to shell injection; wrap targetRef and
state.lastSyncCommit with shell-quote's quote() before building the command
strings (the two places building `git -C "${workspacePath}" diff --name-only
${state.lastSyncCommit}..${targetRef}` and the checkout command that uses
`${targetRef}`), import quote from 'shell-quote' if missing, and use the quoted
values in the string templates so the commands passed from
IncrementalSyncService (the method invoking OrchestratorSystem.Run) are safe.
- Around line 111-113: The code currently sets newState.lastSyncCommit to the
raw targetRef which may be a moving branch/tag; instead resolve the ref to the
immutable commit SHA before persisting. In the function that builds newState in
incremental-sync-service.ts (where newState is created), call the repo/git
helper that resolves refs to SHAs (e.g., a method like
resolveRef/resolveCommitSha/getCommitSha on your git client) using targetRef,
validate the returned SHA, and assign that SHA to lastSyncCommit rather than the
original targetRef; keep lastSyncTimestamp as-is.
- Around line 160-164: The tar extraction call unconditionally runs
OrchestratorSystem.Run(`tar -xf "${localArchive}" -C "${workspacePath}"`) which
is unsafe for untrusted input; change this to detect the archive type (e.g.,
.zip, .tar.gz, .tar.lz4, or plain file) and route extraction to a safe,
format-aware extractor (or use a vetted library) instead of blindly calling tar.
Before extraction, enumerate archive entries (using tar/zip library APIs or tar
-tf/unzip -l), validate and sanitize every entry path referenced by localArchive
so no entry is absolute or contains .. segments that escape workspacePath, and
reject or rewrite unsafe entries; also disallow or neutralize symlink entries so
they cannot point outside workspacePath. Replace the single
OrchestratorSystem.Run invocation with a controlled extraction function that (1)
detects format, (2) performs entry-by-entry validation against workspacePath,
(3) refuses unsafe archives, and (4) extracts using library calls or explicitly
safe command flags, referencing localArchive and workspacePath and the
extraction call site in incremental-sync-service.
- Around line 145-151: The rclone command built in the IncrementalSyncService
uses a directory-oriented `rclone copy` with `--include` which can fail if the
remote filename does not exactly match `.game-ci-input-overlay.tar`; change the
call site that constructs and runs the command (the localArchive variable and
the invocation in OrchestratorSystem.Run inside IncrementalSyncService) to use
`rclone copyto "<remote>:<remotePath>" "<localArchive>"` (or equivalent
interpolation of remote, remotePath and localArchive) so a single file is copied
directly to the intended local destination instead of relying on include
filters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1f0b5cbb-6b4f-413b-966f-4f9141b5c2e3
📒 Files selected for processing (3)
src/model/orchestrator/services/sync/incremental-sync-service.tssrc/model/orchestrator/services/sync/index.tssrc/model/orchestrator/services/sync/sync-state.ts
| try { | ||
| const content = fs.readFileSync(statePath, 'utf8'); | ||
|
|
||
| return JSON.parse(content) as SyncState; |
There was a problem hiding this comment.
Validate parsed sync-state shape before casting.
Line [31] trusts arbitrary JSON shape as SyncState. A valid-but-malformed state file can later crash reads like state.pendingOverlays.length.
Proposed hardening
- return JSON.parse(content) as SyncState;
+ const parsed = JSON.parse(content) as Partial<SyncState>;
+ if (
+ typeof parsed.lastSyncCommit !== 'string' ||
+ typeof parsed.lastSyncTimestamp !== 'string' ||
+ !Array.isArray(parsed.pendingOverlays)
+ ) {
+ throw new Error('Invalid sync state shape');
+ }
+
+ return {
+ lastSyncCommit: parsed.lastSyncCommit,
+ lastSyncTimestamp: parsed.lastSyncTimestamp,
+ workspaceHash: parsed.workspaceHash,
+ pendingOverlays: parsed.pendingOverlays,
+ };📝 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
| return JSON.parse(content) as SyncState; | |
| const parsed = JSON.parse(content) as Partial<SyncState>; | |
| if ( | |
| typeof parsed.lastSyncCommit !== 'string' || | |
| typeof parsed.lastSyncTimestamp !== 'string' || | |
| !Array.isArray(parsed.pendingOverlays) | |
| ) { | |
| throw new Error('Invalid sync state shape'); | |
| } | |
| return { | |
| lastSyncCommit: parsed.lastSyncCommit, | |
| lastSyncTimestamp: parsed.lastSyncTimestamp, | |
| workspaceHash: parsed.workspaceHash, | |
| pendingOverlays: parsed.pendingOverlays, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` at line 31,
The JSON.parse result returned as SyncState must be validated before casting to
avoid runtime crashes (e.g., accessing state.pendingOverlays.length). In the
function that currently returns JSON.parse(content) as SyncState, add a runtime
shape check for required properties and types (at minimum ensure the root is an
object, has pendingOverlays as an array, and any other required keys of
SyncState), and throw a clear, descriptive error or recreate a safe default
SyncState if validation fails; use helper validateSyncState(state) or inline
checks and replace the direct cast with the validated object so downstream code
(like accesses to pendingOverlays) is safe.
Comment on lines
+64
to
+67
| if (requestedStrategy === 'git-delta') { | ||
| const state = IncrementalSyncService.loadSyncState(workspacePath); | ||
| if (!state) { | ||
| OrchestratorLogger.log('[Sync] No sync state found, falling back to full sync'); |
There was a problem hiding this comment.
Do not enable git-delta when baseline commit is missing.
resolveStrategy only checks for state existence (Line [65]), but applyDirectInput can create state with lastSyncCommit: '' (Line [168]). That can produce an invalid/ambiguous delta range later.
Suggested guard
- if (!state) {
+ if (!state || !state.lastSyncCommit) {
OrchestratorLogger.log('[Sync] No sync state found, falling back to full sync');
return 'full';
}Also applies to: 167-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around
lines 64 - 67, The resolver enables 'git-delta' based only on presence of a
state but misses cases where the state has an empty baseline commit; update the
check in resolveStrategy (where requestedStrategy is compared and
IncrementalSyncService.loadSyncState is called) to verify that the loaded
state's lastSyncCommit is a non-empty string before allowing 'git-delta', and
reject/fallback to full sync if lastSyncCommit is falsy; also apply the same
guard where state is inspected around applyDirectInput (which can produce
state.lastSyncCommit === ''), ensuring both locations reference and validate
state.lastSyncCommit before choosing or applying a delta-based strategy.
Comment on lines
+98
to
+99
| `git -C "${workspacePath}" diff --name-only ${state.lastSyncCommit}..${targetRef}`, | ||
| true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find orchestrator-system file
echo "=== Finding orchestrator-system.ts ==="
fd -t f orchestrator-system.ts src
# Find and inspect Run method and process execution details
echo -e "\n=== Searching for OrchestratorSystem class and Run method ==="
rg -n 'class OrchestratorSystem|static\s+Run\s*\(' src --type ts -A 10
# Check process execution methods in orchestrator-system
echo -e "\n=== Checking for shell/spawn usage ==="
rg -n 'shell|execFile|spawn|exec\(' src --type ts -B 2 -A 2
# Confirm git command usage in incremental-sync-service.ts
echo -e "\n=== Git commands in incremental-sync-service.ts ==="
rg -n -C 3 'git -C' src/model/orchestrator/services/sync/incremental-sync-service.tsRepository: game-ci/unity-builder
Length of output: 8092
Unescaped git refs create command injection vulnerability.
Lines 98 and 107 interpolate targetRef and state.lastSyncCommit directly into shell commands executed via OrchestratorSystem.Run, which uses Node.js exec() and spawns a shell. Crafted refs containing shell metacharacters (;, |, &, $(), etc.) can inject arbitrary commands.
Example: if targetRef='main; rm -rf /', the checkout becomes git -C "..." checkout main; rm -rf /.
The codebase already uses shell-quote's quote() function elsewhere (src/model/orchestrator/providers/local/index.ts:79) to safely escape command strings. Apply the same pattern here to escape both targetRef and state.lastSyncCommit before interpolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around
lines 98 - 99, The interpolated git refs in the commands passed to
OrchestratorSystem.Run are vulnerable to shell injection; wrap targetRef and
state.lastSyncCommit with shell-quote's quote() before building the command
strings (the two places building `git -C "${workspacePath}" diff --name-only
${state.lastSyncCommit}..${targetRef}` and the checkout command that uses
`${targetRef}`), import quote from 'shell-quote' if missing, and use the quoted
values in the string templates so the commands passed from
IncrementalSyncService (the method invoking OrchestratorSystem.Run) are safe.
Comment on lines
+111
to
+113
| const newState: SyncState = { | ||
| lastSyncCommit: targetRef, | ||
| lastSyncTimestamp: new Date().toISOString(), |
There was a problem hiding this comment.
Persist resolved commit SHA, not the raw ref input.
Line [112] stores targetRef directly. If this is a branch/tag, future deltas can compare moving refs and incorrectly report no changes.
Suggested fix
+ const resolvedCommit = (
+ await OrchestratorSystem.Run(`git -C "${workspacePath}" rev-parse ${targetRef}`, true)
+ ).trim();
+
const newState: SyncState = {
- lastSyncCommit: targetRef,
+ lastSyncCommit: resolvedCommit,
lastSyncTimestamp: new Date().toISOString(),
pendingOverlays: state.pendingOverlays,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around
lines 111 - 113, The code currently sets newState.lastSyncCommit to the raw
targetRef which may be a moving branch/tag; instead resolve the ref to the
immutable commit SHA before persisting. In the function that builds newState in
incremental-sync-service.ts (where newState is created), call the repo/git
helper that resolves refs to SHAs (e.g., a method like
resolveRef/resolveCommitSha/getCommitSha on your git client) using targetRef,
validate the returned SHA, and assign that SHA to lastSyncCommit rather than the
original targetRef; keep lastSyncTimestamp as-is.
Comment on lines
+145
to
+151
| localArchive = path.join(workspacePath, '.game-ci-input-overlay.tar'); | ||
| OrchestratorLogger.log(`[Sync] Fetching input from storage: ${inputRef}`); | ||
|
|
||
| await OrchestratorSystem.Run( | ||
| `rclone copy "${remote}:${remotePath}" "${path.dirname(localArchive)}" --include "${path.basename(localArchive)}"`, | ||
| true, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
rclone copy --include filter behavior with single file
💡 Result:
rclone copy is directory-oriented: it copies the contents of source:path and is not meant for a single file—for that, use rclone copyto. [1]
What --include matches
- Filters are matched against the path relative to the source root of the command. [2]
--includeadds include rules and then implicitly appends an--exclude **(exclude everything else). [2]- Pattern anchoring:
file.jpgmatchesfile.jpgandany/dir/file.jpg/file.jpgmatches onlyfile.jpgat the source root (not in subdirs). [2]
Practical “single file” patterns
Best (actual single file copy):
rclone copyto /src/path/file.txt remote:dest/file.txt(use copyto for single files). [1]
If you must use copy (copy from the parent directory and include one file):
rclone copy /src/path remote:dest --include "/file.txt"/file.txt ensures you only match that file at the source root, and the implied --exclude ** drops everything else. [2]
If the file may be anywhere under the source dir:
rclone copy /src/path remote:dest --include "file.txt"This can match subdir/file.txt too. [2]
Sources:
[1] rclone copy docs (note about single files / copyto) https://rclone.org/commands/rclone_copy/
[2] rclone Filtering docs (pattern semantics, implied exclude) https://rclone.org/filtering/
🌐 Web query:
rclone copyto command how to copy remote file to exact local filename
💡 Result:
Use copyto and put the full destination path including the filename:
rclone copyto remote:/path/to/remote/file.ext /path/to/local/exact-name.extExample (download and rename while copying):
rclone copyto gdrive:Reports/2026-03.csv ./latest.csvNotes:
copytois the rclone command intended for copying a single file to an exact destination filename (unlikecopy, which treats the destination as a directory when you give it one). [1]- Paths are
remote:path/to/filefor a configured remote, and normal filesystem paths for local destinations (quote paths with spaces; on Windows, local paths may useC:\...). [2] - Renaming is done by specifying a different destination filename:
rclone copyto <source oldname> <dest newname>. [3]
Sources: [1] https://rclone.org/commands/rclone_copy/ [2] https://rclone.org/docs/ [3] https://docs.rc.uab.edu/data_management/transfer/rclone/
🌐 Web query:
rclone copy vs rclone copyto difference
💡 Result:
-
rclone copy src: dst:: Copies the contents of a directory tree fromsrcinto the destination directorydst, skipping files that are already identical. It does not copy the top-level source directory name itself (it behaves like there’s an implicit trailing/). Use it for “copy this folder’s contents into that folder”. The destination is treated as a directory (and when copying a single file withcopy, the destination must be a directory). [1], [3] -
rclone copyto src:path dst:path: Copiessrcto the exact destination path you specify. Ifsrcis a file, you can set/rename the filename on the destination and it will overwrite an existing destination file (unless prevented by flags). Ifsrcis a directory, it behaves the same asrclone copy. [2]
Use rclone copyto for single file copy to avoid filter mismatch.
The current rclone copy with --include filter will fail if the source file has any name other than .game-ci-input-overlay.tar. rclone copy is directory-oriented and relies on the filename filter matching exactly. Use rclone copyto instead, which is designed to copy a single remote file to an exact local destination:
Suggested fix
- await OrchestratorSystem.Run(
- `rclone copy "${remote}:${remotePath}" "${path.dirname(localArchive)}" --include "${path.basename(localArchive)}"`,
- true,
- );
+ await OrchestratorSystem.Run(
+ `rclone copyto "${remote}:${remotePath}" "${localArchive}"`,
+ 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
| localArchive = path.join(workspacePath, '.game-ci-input-overlay.tar'); | |
| OrchestratorLogger.log(`[Sync] Fetching input from storage: ${inputRef}`); | |
| await OrchestratorSystem.Run( | |
| `rclone copy "${remote}:${remotePath}" "${path.dirname(localArchive)}" --include "${path.basename(localArchive)}"`, | |
| true, | |
| ); | |
| localArchive = path.join(workspacePath, '.game-ci-input-overlay.tar'); | |
| OrchestratorLogger.log(`[Sync] Fetching input from storage: ${inputRef}`); | |
| await OrchestratorSystem.Run( | |
| `rclone copyto "${remote}:${remotePath}" "${localArchive}"`, | |
| true, | |
| ); |
🧰 Tools
🪛 ESLint
[error] 149-149: Replace localArchive with ⏎··········localArchive,⏎········
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around
lines 145 - 151, The rclone command built in the IncrementalSyncService uses a
directory-oriented `rclone copy` with `--include` which can fail if the remote
filename does not exactly match `.game-ci-input-overlay.tar`; change the call
site that constructs and runs the command (the localArchive variable and the
invocation in OrchestratorSystem.Run inside IncrementalSyncService) to use
`rclone copyto "<remote>:<remotePath>" "<localArchive>"` (or equivalent
interpolation of remote, remotePath and localArchive) so a single file is copied
directly to the intended local destination instead of relying on include
filters.
Comment on lines
+160
to
+164
| // Extract overlay | ||
| await OrchestratorSystem.Run( | ||
| `tar -xf "${localArchive}" -C "${workspacePath}"`, | ||
| true, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, verify currently supported extraction paths in this service
echo "=== Checking for extraction-related patterns ==="
rg -n 'tar -xf|unzip|lz4|storage://|raw' src/model/orchestrator/services/sync/incremental-sync-service.ts
# Check for archive entry validation/sanitization patterns
echo ""
echo "=== Checking for path validation patterns ==="
rg -n 'tar -t|path\.resolve|startsWith\(|\.\./|absolute|symlink|hardlink' src/model/orchestrator/services/sync/incremental-sync-service.tsRepository: game-ci/unity-builder
Length of output: 775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the file content to understand the broader context
echo "=== Full file context around lines 160-164 ==="
cat -n src/model/orchestrator/services/sync/incremental-sync-service.ts | sed -n '140,180p'Repository: game-ci/unity-builder
Length of output: 1797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how the archive is obtained and if format is determined
echo "=== Search for archive variable and format detection ==="
rg -n 'localArchive|archive|\.zip|\.tar|\.lz4|format' src/model/orchestrator/services/sync/incremental-sync-service.ts | head -30Repository: game-ci/unity-builder
Length of output: 1016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full function signature
echo "=== Function signature and start ==="
sed -n '120,145p' src/model/orchestrator/services/sync/incremental-sync-service.ts
# Search for callers of applyDirectInputOverlay
echo ""
echo "=== Callers of applyDirectInputOverlay ==="
rg -n 'applyDirectInputOverlay' --type tsRepository: game-ci/unity-builder
Length of output: 1215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any archive format detection elsewhere in the codebase
echo "=== Archive format detection patterns ==="
rg -n '\.zip|\.tar\.gz|\.tar\.lz4|mime-type|file-type|magic' src/model/orchestrator/services/sync/ --type ts
# Check for tar entry path validation
echo ""
echo "=== Tar entry validation patterns ==="
rg -n 'validatePath|sanitize|normali|escap' src/model/orchestrator/services/ --type ts | head -20Repository: game-ci/unity-builder
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for callers using the correct function name
echo "=== Callers of applyDirectInput ==="
rg -n 'applyDirectInput' --type ts
# Check for any archive format detection in related files
echo ""
echo "=== Files in the sync directory ==="
fd . src/model/orchestrator/services/sync/ --type f
# Look for input validation at call sites
echo ""
echo "=== Check for archive validation in overlay methods ==="
rg -n 'overlay|archive' src/model/orchestrator/services/sync/incremental-sync-service.ts | head -20Repository: game-ci/unity-builder
Length of output: 1315
🏁 Script executed:
#!/bin/bash
set -euo pipeflag
# Check if applyDirectInput is exported or called from index.ts
echo "=== Check index.ts for exports/calls ==="
cat -n src/model/orchestrator/services/sync/index.ts
# Search more broadly for any usage patterns
echo ""
echo "=== Search entire codebase for applyDirectInput usage ==="
rg -r 'applyDirectInput' --type ts 2>/dev/null || echo "Not called elsewhere"
# Check what arguments are passed to OrchestratorSystem.Run for tar
echo ""
echo "=== Verify tar extraction with -xf and -C behavior ==="
echo "Note: -xf extracts without following symlinks (-h flag would be needed)"
echo "Checking if there are any other tar operations that might validate entries..."
rg -n 'tar ' src/model/orchestrator/services/ --type tsRepository: game-ci/unity-builder
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if applyDirectInput is exported or called from index.ts
echo "=== Check index.ts for exports/calls ==="
cat -n src/model/orchestrator/services/sync/index.ts
# Search more broadly for any usage patterns
echo ""
echo "=== Search entire codebase for applyDirectInput usage ==="
rg -r 'applyDirectInput' --type ts 2>/dev/null || echo "Not called elsewhere"
# Check what arguments are passed to OrchestratorSystem.Run for tar
echo ""
echo "=== Verify tar extraction with -xf and -C behavior ==="
echo "Note: -xf extracts without following symlinks (-h flag would be needed)"
echo "Checking if there are any other tar operations that might validate entries..."
rg -n 'tar ' src/model/orchestrator/services/ --type tsRepository: game-ci/unity-builder
Length of output: 918
Archive extraction lacks format detection and path safety validation for untrusted input.
Line 162 unconditionally uses tar -xf regardless of actual archive format. The function accepts arbitrary user-provided archive paths (inputRef) but does not:
- Detect or handle multiple archive formats (
.zip,.tar.gz,.tar.lz4, raw files) - Validate tar entry paths before extraction, allowing directory traversal via entries like
../../../etc/passwd - Prevent symlink extraction that could escape the workspace
Since this is exported as a public API, these gaps create directory traversal and privilege escalation risks when processing untrusted archives.
🧰 Tools
🪛 ESLint
[error] 161-164: Replace ⏎······tar·-xf·"${localArchive}"·-C·"${workspacePath}",⏎······true,⏎···· with ``tar·-xf·"${localArchive}"·-C·"${workspacePath}",·true
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around
lines 160 - 164, The tar extraction call unconditionally runs
OrchestratorSystem.Run(`tar -xf "${localArchive}" -C "${workspacePath}"`) which
is unsafe for untrusted input; change this to detect the archive type (e.g.,
.zip, .tar.gz, .tar.lz4, or plain file) and route extraction to a safe,
format-aware extractor (or use a vetted library) instead of blindly calling tar.
Before extraction, enumerate archive entries (using tar/zip library APIs or tar
-tf/unzip -l), validate and sanitize every entry path referenced by localArchive
so no entry is absolute or contains .. segments that escape workspacePath, and
reject or rewrite unsafe entries; also disallow or neutralize symlink entries so
they cannot point outside workspacePath. Replace the single
OrchestratorSystem.Run invocation with a controlled extraction function that (1)
detects format, (2) performs entry-by-entry validation against workspacePath,
(3) refuses unsafe archives, and (4) extracts using library calls or explicitly
safe command flags, referencing localArchive and workspacePath and the
extraction call site in incremental-sync-service.
frostebite
added
enhancement
…te management, and tests (#799) - Add storage-pull strategy: rclone-based sync from remote storage with overlay and clean modes, URI parsing (storage://remote:bucket/path), transfer parallelism, and automatic rclone availability checking - Add SyncStateManager: persistent state load/save with configurable paths, workspace hash calculation via SHA-256 of key project files, and drift detection for external modification awareness - Add action.yml inputs: syncStrategy, syncInputRef, syncStorageRemote, syncRevertAfter, syncStatePath with sensible defaults - Wire sync into Input (5 getters), BuildParameters (5 fields), index.ts (local build path), and RemoteClient (orchestrator path) with post-job overlay revert when syncRevertAfter is true - Add 42 unit tests covering all strategies, URI parsing, state management, hash calculation, drift detection, error handling, and edge cases (missing rclone, invalid URIs, absent state, empty diffs) - Add root:true to eslintrc to prevent plugin resolution conflicts Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/model/orchestrator/services/sync/sync-state-manager.ts (1)
43-47:⚠️ Potential issue | 🟠 MajorValidate loaded JSON before treating it as
SyncState.Line [46] casts arbitrary parsed JSON directly; valid-but-malformed state can break later reads (e.g.,
pendingOverlays.length).Proposed hardening
export class SyncStateManager { + private static isSyncState(value: unknown): value is SyncState { + if (!value || typeof value !== 'object') { + return false; + } + + const candidate = value as Partial<SyncState>; + return ( + typeof candidate.lastSyncCommit === 'string' && + typeof candidate.lastSyncTimestamp === 'string' && + (candidate.workspaceHash === undefined || typeof candidate.workspaceHash === 'string') && + Array.isArray(candidate.pendingOverlays) + ); + } + static loadState(workspacePath: string, statePath?: string): SyncState | undefined { @@ - return JSON.parse(content) as SyncState; + const parsed = JSON.parse(content); + if (!SyncStateManager.isSyncState(parsed)) { + throw new Error('Invalid sync state shape'); + } + + return parsed;src/model/orchestrator/services/sync/incremental-sync-service.ts (4)
41-47:⚠️ Potential issue | 🟠 MajorRequire a non-empty baseline commit before selecting
git-delta.
stateexistence alone is not sufficient. State created by overlay paths can carrylastSyncCommit: '', which yields an invalid delta range.Suggested guard
- if (!state) { + if (!state || !state.lastSyncCommit?.trim()) { OrchestratorLogger.log('[Sync] No sync state found, falling back to full sync'); return 'full'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around lines 41 - 47, The guard in IncrementalSyncService that chooses 'git-delta' only checks for state existence; change it to require a non-empty baseline commit by loading the state via SyncStateManager.loadState(workspacePath, statePath) and verifying state.lastSyncCommit is a non-empty string (e.g., truthy/trimmed) before accepting 'git-delta'; if lastSyncCommit is missing/empty, call OrchestratorLogger.log('[Sync] No sync state found, falling back to full sync') (or similar) and return 'full' so invalid overlay-created states with lastSyncCommit: '' do not produce an invalid delta range.
89-91:⚠️ Potential issue | 🟠 MajorPersist immutable commit SHA, not raw input ref.
Saving
targetReferencedirectly means branches/tags can move, making subsequent deltas unstable and potentially incorrect.Suggested fix
+ const resolvedCommit = (await OrchestratorSystem.Run(`git -C "${workspacePath}" rev-parse HEAD`)).trim(); + const newState: SyncState = { - lastSyncCommit: targetReference, + lastSyncCommit: resolvedCommit, lastSyncTimestamp: new Date().toISOString(), workspaceHash: SyncStateManager.calculateWorkspaceHash(workspacePath), pendingOverlays: state.pendingOverlays, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around lines 89 - 91, Currently newState.lastSyncCommit is set to the raw targetReference (branch/tag/ref) which can move; instead resolve that ref to the immutable commit SHA before persisting. Locate the code around newState creation in incremental-sync-service.ts (symbols: newState, SyncState, lastSyncCommit, targetReference) and replace saving targetReference with the commit SHA returned from the git provider (e.g., call the repo client method that resolves a ref to a commit SHA or use rev-parse/GET commit API), then store that SHA in lastSyncCommit; ensure any subsequent uses expect a commit SHA and update tests/comments accordingly.
76-77:⚠️ Potential issue | 🔴 CriticalSanitize git refs before interpolating shell commands.
state.lastSyncCommitandtargetReferenceare inserted directly into exec-backed shell strings, enabling command injection via crafted refs.Also applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around lines 76 - 77, The git ref values state.lastSyncCommit and targetReference are interpolated directly into shell strings (e.g., `git -C "${workspacePath}" diff --name-only ${state.lastSyncCommit}..${targetReference}`) which allows command injection; fix by either (A) switching to a safe child_process API that accepts args arrays (execFile/spawn) and pass refs as separate arguments instead of building a shell command string, or (B) strictly validating/sanitizing the refs before use (allow only commit SHA pattern /^[0-9a-f]{7,40}$/i or a safe ref-name character set) and escaping any input; update the code paths that build git commands (references to state.lastSyncCommit, targetReference and workspacePath in IncrementalSyncService or the method executing git) to use one of these safe approaches for both occurrences noted.
138-140:⚠️ Potential issue | 🔴 CriticalArchive extraction is unsafe for untrusted overlay input.
Line [139] extracts user-provided archive content directly into workspace without entry path/symlink validation, which opens traversal and workspace-escape risks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around lines 138 - 140, The tar extraction call using OrchestratorSystem.Run with localArchive into workspacePath is unsafe for untrusted overlays; change the flow to first extract the archive into a controlled temporary directory, then iterate and validate every entry (reject absolute paths, reject any path containing .. that would escape workspacePath, and disallow or normalize symlinks and device nodes) before moving files into workspacePath; implement the validation step in the incremental-sync-service logic that handles localArchive/workspacePath and only perform the final move/copy of validated entries into workspacePath, failing the operation and logging an error if any unsafe entries are found.
🧹 Nitpick comments (4)
src/index.ts (2)
122-124: Handle unknown strategies explicitly (fail or fallback), don’t only warn.Warning-only behavior can silently ignore misconfiguration and proceed with unintended workspace state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 122 - 124, The switch default currently only calls core.warning(`[Sync] Unknown sync strategy: ${resolvedStrategy}`) which can silently continue; update the default branch in the switch that checks resolvedStrategy to explicitly fail or revert to a safe fallback: either throw a descriptive Error (e.g., throw new Error(`Unknown sync strategy: ${resolvedStrategy}`)) or call core.setFailed and return/exit so execution stops; ensure the change is made in the same switch that references resolvedStrategy and core.warning so the function exits early on unknown values instead of proceeding.
30-34: Drive post-build revert off the resolved strategy, not raw input.If resolution falls back to
full, the current flow still treats the run as incremental for revert checks.Also applies to: 47-54, 76-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 30 - 34, The code uses the raw input buildParameters.syncStrategy (variable syncStrategy) for post-build revert decisions, which can be wrong when resolution falls back to 'full'; change the flow so revert checks use the resolved strategy: add a resolvedSyncStrategy variable and populate it by either calling a new helper (e.g., resolveSyncStrategy(buildParameters)) or by changing applySyncStrategy to return the resolved strategy and assigning its result to resolvedSyncStrategy, then replace usages of syncStrategy in the logging and revert logic (references: buildParameters, syncStrategy, applySyncStrategy) with resolvedSyncStrategy throughout the affected blocks.src/model/orchestrator/services/sync/incremental-sync.test.ts (1)
62-80: Add a regression test forgit-deltawith empty baseline commit.Current coverage checks only state existence; include a case with
lastSyncCommit: ''to ensure strategy resolves tofull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/sync/incremental-sync.test.ts` around lines 62 - 80, Add a regression test covering the case where a SyncState exists but has an empty baseline commit so that IncrementalSyncService.resolveStrategy('git-delta', workspacePath) falls back to 'full'; update the test file to add a new it(...) that mocks fs.existsSync to true and fs.readFileSync to return a SyncState JSON with lastSyncCommit: '' (and valid lastSyncTimestamp/pendingOverlays) and assert the result is 'full' to ensure resolveStrategy treats empty lastSyncCommit as no valid baseline.src/model/build-parameters.ts (1)
109-113: Use a strict union type forsyncStrategyin build parameters.Typing this as
stringforces unsafe casts downstream and weakens compile-time validation.Proposed refactor
- public syncStrategy!: string; + public syncStrategy!: 'full' | 'git-delta' | 'direct-input' | 'storage-pull';Also applies to: 250-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/build-parameters.ts` around lines 109 - 113, Replace the loose string type on syncStrategy with a strict union of allowed literal values (e.g. type SyncStrategy = 'none' | 'pull' | 'push' — use the actual valid strategies your code expects) and update the BuildParameters class property signature from "public syncStrategy!: string;" to "public syncStrategy!: SyncStrategy;". Export the SyncStrategy type so callers and downstream code (including any places doing unsafe casts) can reference it, and update all usages and assertions (including the other occurrences noted around the second block of properties) to use this union instead of string or unchecked casts.
🤖 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 256-260: The syncRevertAfter getter on class Input currently
compares the raw string exactly to 'true', causing values like 'TRUE' or 'True'
to be treated as false; change the comparison in static get syncRevertAfter() to
normalize the value (e.g., call .toLowerCase() on the result of
Input.getInput('syncRevertAfter') ?? 'true') and then compare to 'true' so the
check is case-insensitive while preserving the existing default behavior.
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts`:
- Around line 72-73: The OrchestratorSystem.Run invocations for critical sync
steps (e.g., the git fetch call shown and other calls around the sync flow at
the same spots) are being invoked with suppressError=true which lets failures
pass silently; change those calls (including the invocation of
OrchestratorSystem.Run at the shown fetch, the copy/cleanup runs referenced
around lines 183-204, 287-290, and 304-311) to not suppress errors (remove or
set the suppressError argument to false) so that failures surface and prevent
updating sync state; ensure any surrounding logic handles the thrown errors
appropriately (propagate or catch and log) so failed fetch/copy/cleanup aborts
the sync.
---
Duplicate comments:
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts`:
- Around line 41-47: The guard in IncrementalSyncService that chooses
'git-delta' only checks for state existence; change it to require a non-empty
baseline commit by loading the state via
SyncStateManager.loadState(workspacePath, statePath) and verifying
state.lastSyncCommit is a non-empty string (e.g., truthy/trimmed) before
accepting 'git-delta'; if lastSyncCommit is missing/empty, call
OrchestratorLogger.log('[Sync] No sync state found, falling back to full sync')
(or similar) and return 'full' so invalid overlay-created states with
lastSyncCommit: '' do not produce an invalid delta range.
- Around line 89-91: Currently newState.lastSyncCommit is set to the raw
targetReference (branch/tag/ref) which can move; instead resolve that ref to the
immutable commit SHA before persisting. Locate the code around newState creation
in incremental-sync-service.ts (symbols: newState, SyncState, lastSyncCommit,
targetReference) and replace saving targetReference with the commit SHA returned
from the git provider (e.g., call the repo client method that resolves a ref to
a commit SHA or use rev-parse/GET commit API), then store that SHA in
lastSyncCommit; ensure any subsequent uses expect a commit SHA and update
tests/comments accordingly.
- Around line 76-77: The git ref values state.lastSyncCommit and targetReference
are interpolated directly into shell strings (e.g., `git -C "${workspacePath}"
diff --name-only ${state.lastSyncCommit}..${targetReference}`) which allows
command injection; fix by either (A) switching to a safe child_process API that
accepts args arrays (execFile/spawn) and pass refs as separate arguments instead
of building a shell command string, or (B) strictly validating/sanitizing the
refs before use (allow only commit SHA pattern /^[0-9a-f]{7,40}$/i or a safe
ref-name character set) and escaping any input; update the code paths that build
git commands (references to state.lastSyncCommit, targetReference and
workspacePath in IncrementalSyncService or the method executing git) to use one
of these safe approaches for both occurrences noted.
- Around line 138-140: The tar extraction call using OrchestratorSystem.Run with
localArchive into workspacePath is unsafe for untrusted overlays; change the
flow to first extract the archive into a controlled temporary directory, then
iterate and validate every entry (reject absolute paths, reject any path
containing .. that would escape workspacePath, and disallow or normalize
symlinks and device nodes) before moving files into workspacePath; implement the
validation step in the incremental-sync-service logic that handles
localArchive/workspacePath and only perform the final move/copy of validated
entries into workspacePath, failing the operation and logging an error if any
unsafe entries are found.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 122-124: The switch default currently only calls
core.warning(`[Sync] Unknown sync strategy: ${resolvedStrategy}`) which can
silently continue; update the default branch in the switch that checks
resolvedStrategy to explicitly fail or revert to a safe fallback: either throw a
descriptive Error (e.g., throw new Error(`Unknown sync strategy:
${resolvedStrategy}`)) or call core.setFailed and return/exit so execution
stops; ensure the change is made in the same switch that references
resolvedStrategy and core.warning so the function exits early on unknown values
instead of proceeding.
- Around line 30-34: The code uses the raw input buildParameters.syncStrategy
(variable syncStrategy) for post-build revert decisions, which can be wrong when
resolution falls back to 'full'; change the flow so revert checks use the
resolved strategy: add a resolvedSyncStrategy variable and populate it by either
calling a new helper (e.g., resolveSyncStrategy(buildParameters)) or by changing
applySyncStrategy to return the resolved strategy and assigning its result to
resolvedSyncStrategy, then replace usages of syncStrategy in the logging and
revert logic (references: buildParameters, syncStrategy, applySyncStrategy) with
resolvedSyncStrategy throughout the affected blocks.
In `@src/model/build-parameters.ts`:
- Around line 109-113: Replace the loose string type on syncStrategy with a
strict union of allowed literal values (e.g. type SyncStrategy = 'none' | 'pull'
| 'push' — use the actual valid strategies your code expects) and update the
BuildParameters class property signature from "public syncStrategy!: string;" to
"public syncStrategy!: SyncStrategy;". Export the SyncStrategy type so callers
and downstream code (including any places doing unsafe casts) can reference it,
and update all usages and assertions (including the other occurrences noted
around the second block of properties) to use this union instead of string or
unchecked casts.
In `@src/model/orchestrator/services/sync/incremental-sync.test.ts`:
- Around line 62-80: Add a regression test covering the case where a SyncState
exists but has an empty baseline commit so that
IncrementalSyncService.resolveStrategy('git-delta', workspacePath) falls back to
'full'; update the test file to add a new it(...) that mocks fs.existsSync to
true and fs.readFileSync to return a SyncState JSON with lastSyncCommit: '' (and
valid lastSyncTimestamp/pendingOverlays) and assert the result is 'full' to
ensure resolveStrategy treats empty lastSyncCommit as no valid baseline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6411a15a-f1ae-4436-beea-855c3d99e1c8
⛔ 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 (10)
.eslintrc.jsonaction.ymlsrc/index.tssrc/model/build-parameters.tssrc/model/input.tssrc/model/orchestrator/remote-client/index.tssrc/model/orchestrator/services/sync/incremental-sync-service.tssrc/model/orchestrator/services/sync/incremental-sync.test.tssrc/model/orchestrator/services/sync/index.tssrc/model/orchestrator/services/sync/sync-state-manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/model/orchestrator/services/sync/index.ts
Comment on lines
+256
to
+260
| static get syncRevertAfter(): boolean { | ||
| const input = Input.getInput('syncRevertAfter') ?? 'true'; | ||
|
|
||
| return input === 'true'; | ||
| } |
There was a problem hiding this comment.
Parse syncRevertAfter case-insensitively.
Values like TRUE / True currently resolve to false.
Proposed fix
static get syncRevertAfter(): boolean {
const input = Input.getInput('syncRevertAfter') ?? 'true';
- return input === 'true';
+ return input.toLowerCase() === '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
| static get syncRevertAfter(): boolean { | |
| const input = Input.getInput('syncRevertAfter') ?? 'true'; | |
| return input === 'true'; | |
| } | |
| static get syncRevertAfter(): boolean { | |
| const input = Input.getInput('syncRevertAfter') ?? 'true'; | |
| return input.toLowerCase() === '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 256 - 260, The syncRevertAfter getter on
class Input currently compares the raw string exactly to 'true', causing values
like 'TRUE' or 'True' to be treated as false; change the comparison in static
get syncRevertAfter() to normalize the value (e.g., call .toLowerCase() on the
result of Input.getInput('syncRevertAfter') ?? 'true') and then compare to
'true' so the check is case-insensitive while preserving the existing default
behavior.
Comment on lines
+72
to
+73
| await OrchestratorSystem.Run(`git -C "${workspacePath}" fetch origin`, true); | ||
|
|
There was a problem hiding this comment.
Do not suppress failures for required sync commands.
Critical steps currently run with suppressError=true, so failed fetch/copy/cleanup can be treated as success and still persist updated sync state.
Suggested fix pattern
- await OrchestratorSystem.Run(`git -C "${workspacePath}" fetch origin`, true);
+ await OrchestratorSystem.Run(`git -C "${workspacePath}" fetch origin`);
@@
- await OrchestratorSystem.Run('rclone version', true, true);
+ await OrchestratorSystem.Run('rclone version', false, true);
@@
- await OrchestratorSystem.Run(`rclone copy "${rcloneSource}" "${workspacePath}" --transfers 8 --checkers 16`, true);
+ await OrchestratorSystem.Run(`rclone copy "${rcloneSource}" "${workspacePath}" --transfers 8 --checkers 16`);
@@
- await OrchestratorSystem.Run(`git -C "${workspacePath}" checkout -- .`, true);
- await OrchestratorSystem.Run(`git -C "${workspacePath}" clean -fd`, true);
+ await OrchestratorSystem.Run(`git -C "${workspacePath}" checkout -- .`);
+ await OrchestratorSystem.Run(`git -C "${workspacePath}" clean -fd`);Also applies to: 183-204, 287-290, 304-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/sync/incremental-sync-service.ts` around
lines 72 - 73, The OrchestratorSystem.Run invocations for critical sync steps
(e.g., the git fetch call shown and other calls around the sync flow at the same
spots) are being invoked with suppressError=true which lets failures pass
silently; change those calls (including the invocation of OrchestratorSystem.Run
at the shown fetch, the copy/cleanup runs referenced around lines 183-204,
287-290, and 304-311) to not suppress errors (remove or set the suppressError
argument to false) so that failures surface and prevent updating sync state;
ensure any surrounding logic handles the thrown errors appropriately (propagate
or catch and log) so failed fetch/copy/cleanup aborts the sync.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #799 +/- ##
==========================================
+ Coverage 31.25% 33.17% +1.92%
==========================================
Files 84 87 +3
Lines 4563 4772 +209
Branches 1103 1151 +48
==========================================
+ Hits 1426 1583 +157
- Misses 3137 3189 +52
🚀 New features to boost your workflow:
|
This was referenced Mar 5, 2026
Co-Authored-By: Claude Opus 4.6 <[email protected]>
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: Remove the job-level continue-on-error and instead add
continue-on-error: true to the build step (the step that runs the matrix macOS
build/test commands, e.g., the "build" step); then change the artifact upload
step's condition to run regardless of build success by setting its if to
always() (or if: failure() || success()) so artifacts are uploaded even on
failure. Ensure you reference the build step name and the upload step name in
the workflow when making these changes and remove the top-level
continue-on-error key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 32e42d85-8ed8-4949-8864-a977312bdf3f
📒 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.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/build-tests-mac.ymlRepository: game-ci/unity-builder
Length of output: 3713
🏁 Script executed:
cat -n .github/workflows/build-tests-ubuntu.yml | head -200Repository: game-ci/unity-builder
Length of output: 10434
🏁 Script executed:
sed -n '200,220p' .github/workflows/build-tests-ubuntu.ymlRepository: game-ci/unity-builder
Length of output: 467
Move continue-on-error to step level and guard the upload step.
At Line 15, job-level continue-on-error: true allows the workflow run to pass even when a matrix leg fails. It also does not make the later upload step run after a failed build, because steps still default to if: success() unless explicitly overridden. Move continue-on-error to the build step and give the upload step an explicit condition so artifacts are still collected on failure.
Suggested change
buildForAllPlatformsMacOS:
name: ${{ matrix.targetPlatform }} on ${{ matrix.unityVersion }}
runs-on: macos-latest
- continue-on-error: true
strategy:
fail-fast: false
@@
- uses: ./
+ continue-on-error: true
env:
UNITY_EMAIL: ${{ secrets.UNITY_EMAIL }}
UNITY_PASSWORD: ${{ secrets.UNITY_PASSWORD }}
UNITY_SERIAL: ${{ secrets.UNITY_SERIAL }}
UNITY_LICENSE: ${{ secrets.UNITY_LICENSE }}
@@
- uses: actions/upload-artifact@v4
+ if: ${{ !cancelled() }}
with:
name: Build ${{ matrix.targetPlatform }} on MacOS (${{ matrix.unityVersion }})${{ matrix.buildProfile && ' With Build Profile' || '' }}
path: build
retention-days: 14🤖 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, Remove the job-level
continue-on-error and instead add continue-on-error: true to the build step (the
step that runs the matrix macOS build/test commands, e.g., the "build" step);
then change the artifact upload step's condition to run regardless of build
success by setting its if to always() (or if: failure() || success()) so
artifacts are uploaded even on failure. Ensure you reference the build step name
and the upload step name in the workflow when making these changes and remove
the top-level continue-on-error key.
The orchestrator-develop branch no longer exists. Update all fallback clone commands and test fixtures to use main instead. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
|
Closing — all orchestrator code has been extracted to the standalone Content from this PR (incremental sync protocol — git delta, direct input, storage-backed sync) 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
Fully implements the incremental sync protocol — cacheless workspace updates via git delta, direct input, or rclone-backed storage. Instead of traditional cache restore, delivers only what changed to the build environment.
Implementation
full(traditional clone),git-delta(fetch + selective checkout since last sync commit),direct-input(tar extraction with overlay management),storage-pull(rclone-backed content retrieval from any of 70+ backends). Automatic fallback to full sync on failure.New action.yml inputs (5)
syncStrategyfullfull,git-delta,direct-input,storage-pullsyncInputRef''syncStorageRemote''syncRevertAfter''syncStatePath.game-ci/sync-state.jsonKey design decisions
Usage
Test coverage
42 unit tests covering:
Related
Documentation
14-incremental-sync-protocol.mdxTest plan
tsc --noEmit— no type errorsTracking:
Summary by CodeRabbit
New Features
Chores
Tests