Land PR #41's content into main (was stranded), fix repeated deprecation warning

[frostebite]

Supersedes #41. Found during a full review pass of this session's PRs: #41 was stacked on #40's branch (remove-canonical-overlay-cache) and merged into that branch — but #40's branch was squash-merged into main first (22:49:59), 12 seconds before #41 merged into it (22:50:12). So #41's actual payload (default cacheMode → move-directory, EXDEV/EPERM copy fallback, localCacheMode→cacheMode rename) never reached main at all — it landed on a branch that immediately became orphaned relative to main.

This PR cherry-picks #41's squash commit onto current main so the content actually lands, plus one more fix found in the same review pass:

• Deprecation warning fired on every read, not once. config.cacheMode's core.warning(...) call had no memoization and is read from 2+ live call sites (restoreMode, saveMode) — so a workflow using only the deprecated localCacheMode input got the warning logged at least twice per run, contradicting the intended "warn once" behavior. Fixed with a module-level flag.

Verified: tsc --noEmit clean, full test suite passes (915/916, 1 pre-existing skip) excluding 3 flaky subprocess-spawn CLI-integration tests that time out in this sandbox regardless of this change (confirmed independently reproducible on a clean run with no diff).

Closes #41 (superseded — its content is fully included here, plus the warning fix).

Summary by CodeRabbit

• New Features

  • Added the cacheMode configuration option for selecting distributed cache behavior.
  • Directory-based cache restore and saving are now the default.
  • Tar-based caching remains available as an explicit option.
  • Cache operations can now fall back to copying when direct moves are unavailable.

• Bug Fixes

  • Improved cache and workspace handling across filesystems and permission-restricted environments.

• Deprecation

  • localCacheMode is retained as a deprecated alias for cacheMode, with a one-time warning.
  • Removed support for the canonical-overlay mode.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds canonical cacheMode configuration, keeps localCacheMode as a deprecated alias, defaults directory caching to move-directory, and adds copy fallback for EXDEV and EPERM rename failures.

Changes

Cache mode and filesystem fallback

Layer / File(s)	Summary
Cache mode configuration and resolution action.yml, src/plugin-lifecycle.ts	The action exposes cacheMode and retains localCacheMode as a deprecated alias. The resolved default is move-directory, with one-time deprecation warnings for the alias.
Local cache mode behavior src/model/orchestrator/services/cache/local-cache-service.ts, src/model/orchestrator/services/cache/local-cache-service.test.ts	Local cache restore and save default to directory moves. Tar mode remains explicit. Tests cover rename operations and EXDEV copy fallback.
Workspace and separate-cache fallback src/model/orchestrator/services/cache/child-workspace-service.ts	Workspace and separate cache operations use renameOrCopy, which copies recursively and removes the source when rename returns EXDEV or EPERM.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: 🟠 High · up to ea94a

The cache fallback can leave incomplete data in place after a copy failure, potentially causing later restores to use corrupted or partial workspace data. The PR is not merge-ready until the destination replacement and failure cleanup are made safe.

Sequence Diagram(s)

sequenceDiagram
  participant PluginLifecycle
  participant LocalCacheService
  participant FileSystem
  PluginLifecycle->>LocalCacheService: pass resolved cacheMode
  LocalCacheService->>FileSystem: restore or save directory
  FileSystem-->>LocalCacheService: rename succeeds or returns EXDEV/EPERM
  LocalCacheService->>FileSystem: copy recursively and remove source

Loading

Possibly related PRs

• game-ci/orchestrator#22: Updates the same cache-mode handling and configuration paths.
• game-ci/orchestrator#40: Modifies local cache mode handling and canonical-overlay behavior.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately identifies landing PR #41 and fixing the repeated deprecation warning.
Linked Issues check	✅ Passed	The changes implement the linked issue objectives for cache defaults, fallback behavior, input migration, warnings, and tests.
Out of Scope Changes check	✅ Passed	All changed files directly support the linked caching objectives and contain no unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch land-pr41-move-directory-default

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/model/orchestrator/services/cache/local-cache-service.test.ts (1)

218-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add EPERM fallback test cases.

renameOrCopy handles both EXDEV and EPERM. The tests only force EXDEV. Add EPERM cases so the permission-error fallback remains covered.

• src/model/orchestrator/services/cache/local-cache-service.test.ts#L218-L240: Add an EPERM restore fallback case that verifies copy and source cleanup.
• src/model/orchestrator/services/cache/local-cache-service.test.ts#L297-L320: Add an EPERM save fallback case that verifies copy and source cleanup.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/model/orchestrator/services/cache/local-cache-service.test.ts` around
lines 218 - 240, Add EPERM fallback coverage in local-cache-service.test.ts at
lines 218-240 and 297-320: add restore and save cases that make renameSync throw
an error with code EPERM, then verify the operation succeeds via cpSync and
removes the source with rmSync, matching the existing EXDEV assertions and
setup.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/model/orchestrator/services/cache/child-workspace-service.ts`:
- Line 86: Update ChildWorkspaceService.renameOrCopy and all affected
save/restore call sites to stage copies in a unique sibling temporary path,
rename the completed temporary path to destination only after fs.cpSync
succeeds, and remove the temporary path on failure. Delete source only after
destination replacement completes, preserving destination integrity across EXDEV
and EPERM fallback paths; add failure tests covering both fallbacks.

---

Nitpick comments:
In `@src/model/orchestrator/services/cache/local-cache-service.test.ts`:
- Around line 218-240: Add EPERM fallback coverage in
local-cache-service.test.ts at lines 218-240 and 297-320: add restore and save
cases that make renameSync throw an error with code EPERM, then verify the
operation succeeds via cpSync and removes the source with rmSync, matching the
existing EXDEV assertions and setup.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ea71151d-c071-4d1d-8b6f-801663dcb25e

📥 Commits

Reviewing files that changed from the base of the PR and between 983fe3c and ea94a9a.

📒 Files selected for processing (5)

• action.yml
• src/model/orchestrator/services/cache/child-workspace-service.ts
• src/model/orchestrator/services/cache/local-cache-service.test.ts
• src/model/orchestrator/services/cache/local-cache-service.ts
• src/plugin-lifecycle.ts

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Stage the copy before replacing the destination.

If fs.cpSync fails after it writes target entries, Line 433 throws without removing the partial destination. The save paths have already removed the prior cache. A later restore accepts the nonempty partial cache and can use incomplete workspace or library data.

Copy to a unique sibling temporary path. Rename that path to destination only after the copy succeeds. Remove the temporary path if the copy fails. Delete source only after the destination is complete. Add failure tests for both EXDEV and EPERM fallback paths.

Also applies to: 154-154, 197-197, 249-249, 421-435

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/model/orchestrator/services/cache/child-workspace-service.ts` at line 86,
Update ChildWorkspaceService.renameOrCopy and all affected save/restore call
sites to stage copies in a unique sibling temporary path, rename the completed
temporary path to destination only after fs.cpSync succeeds, and remove the
temporary path on failure. Delete source only after destination replacement
completes, preserving destination integrity across EXDEV and EPERM fallback
paths; add failure tests covering both fallbacks.