feat(worker): refresh snapshot workspace instead of recloning on reuse#1047
feat(worker): refresh snapshot workspace instead of recloning on reuse#1047
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Well-structured change with solid test coverage and comprehensive graceful fallback handling. One correctness concern with the project-ID prefix matching in findSnapshotWorkspaceDir that could cause a snapshot directory to be matched to the wrong project.
Code Issues
Should Fix
-
src/agents/shared/repository.ts:33 (
findSnapshotWorkspaceDir) —startsWith(prefix)whereprefix = "cascade-${projectId}-"can produce false positives when one project ID is a prefix of another.Example: If
projectId = "foo", the prefixcascade-foo-will match bothcascade-foo-1711234567890(correct — projectfoo) andcascade-foo-bar-1711234567890(wrong — projectfoo-bar). The first match wins viaArray.find(), so the wrong directory could be returned depending on filesystem ordering.Since
createTempDirinrepo.tsalways producescascade-${projectId}-${Date.now()}whereDate.now()is purely numeric, tighten the match to verify the suffix is all digits:const match = entries.find((e) => { if (!e.startsWith(prefix)) return false; const suffix = e.slice(prefix.length); return /^\d+$/.test(suffix); });
This ensures only directories with a numeric timestamp suffix (the
Date.now()part) after the project ID are matched, preventing cross-project collisions.
Everything else looks good:
- The explicit env flag approach (
CASCADE_SNAPSHOT_REUSE) is cleaner than filesystem heuristics - Graceful fallback to clone when no snapshot dir is found is well-handled
- Non-zero git exit tolerance is consistent with the existing
setup.shfailure handling - The
maybeWarmTsCacheextraction is a sensible refactor - Router-side detection (
workerImage !== routerConfig.workerImage) correctly identifies reuse - Test coverage is thorough — all paths exercised including error/fallback cases
🕵️ claude-code · claude-opus-4-6 · run details
…timestamp suffix Prevents false-positive matches when one project ID is a prefix of another (e.g. project "foo" matching directory "cascade-foo-bar-<timestamp>"). Now verifies the suffix after the project-ID prefix is all digits, matching only directories created by createTempDir (cascade-<projectId>-<Date.now()>). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — well-structured feature with clean separation between router-side detection and worker-side workspace refresh. The explicit env flag approach, graceful fallback to clone, and comprehensive test coverage make this solid.
Verified:
- Snapshot reuse detection logic in
container-manager.tscorrectly gates onsnapshotEnabled && workerImage !== routerConfig.workerImage findSnapshotWorkspaceDircorrectly matchescascade-<projectId>-<digits>to prevent cross-project prefix collisions (e.g., projectIdfoowon't matchcascade-foo-bar-*)refreshSnapshotWorkspacefetch → reset → checkout sequence is correct; non-zero exits are logged as warnings and tolerated, consistent with the existingsetup.shfailure handling pattern- Cold-start path is completely unaffected — the
CASCADE_SNAPSHOT_REUSEguard combined with theproject.repocheck ensures no code-path change for non-snapshot runs maybeWarmTsCacheextraction is a clean refactor that keeps behavior identical on both paths- Path construction via template literals is consistent with the existing
repo.tspattern (not usingpath.join) - All 7 CI checks pass
- Test coverage is thorough: 21 new tests covering happy paths, edge cases, error tolerance, and fallback behavior
🕵️ claude-code · claude-opus-4-6 · run details
Summary
CASCADE_SNAPSHOT_REUSE=trueenv flag injected by the router into worker containers that start from a reused snapshot image, so worker code can take a snapshot-specific path without affecting cold startsfindSnapshotWorkspaceDir()to locate the baked-in workspace directory within the container image (by scanning/workspace/cascade-<projectId>-*)refreshSnapshotWorkspace()to update an existing checkout viagit fetch origin+git reset --hard origin/<branch>+git checkout <branch>instead of cloning from scratchsetupRepository()now detectsCASCADE_SNAPSHOT_REUSE=trueand takes the refresh path when a snapshot workspace is found; falls back transparently to the clone path when no directory is foundCASCADE_SNAPSHOT_REUSE→ no code-path change)Key decisions
CASCADE_SNAPSHOT_REUSE=trueis set only when the router selects a snapshot image (workerImage !== routerConfig.workerImage), making the intent unambiguous rather than guessing from filesystem statesetup.shfailure handling patternmaybeWarmTsCachehelper extracted: KeepssetupRepositorywithin biome's cognitive-complexity limit (15)Test plan
findSnapshotWorkspaceDir— 5 new tests covering match, first-match, no-match, read-error, and custom workspace basesetupRepositorysnapshot-reuse path — 12 new tests covering: snapshot dir used, skip clone, git commands issued, prBranch respected, baseBranch fallback, no setup.sh, TS cache warming, fallback to clone when no dir, flag-absent cold start, no repo cold start, fetch non-zero continues, reset non-zero continuesbuildWorkerEnvWithProjectId— 4 new tests confirmingCASCADE_SNAPSHOT_REUSE=trueis injected only whensnapshotReuse=trueCard
https://trello.com/c/v88fCsqv/558-as-a-worker-i-want-to-refresh-a-reused-snapshot-workspace-instead-of-recloning-it-so-that-subsequent-runs-skip-redundant-setup
🕵️ claude-code · claude-sonnet-4-6 · run details