Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughRefactored CAS to use a configurable vfs.FS and functional-options constructor, moved filesystem and locking operations to the VFS layer, added an in-memory HTTP Git test server, and updated tests to use memmap VFS and the local git server while adjusting clone options and locking APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cas/store_test.go (1)
159-168:⚠️ Potential issue | 🟡 MinorThis lock-contention check is timing-sensitive.
The second goroutine starts its timer only after it gets scheduled past
<-acquired, so on a busy runner it can block correctly and still observe<50ms. An explicit handshake/select that proves the secondAcquireLockcannot finish before the first unlocks will be much less flaky.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cas/store_test.go` around lines 159 - 168, The timing-sensitive check should be replaced with an explicit handshake so the test proves the second AcquireLock is actually blocked; add a "started" channel that the second goroutine closes/sends on immediately before calling store.AcquireLock(testHash), have the main test wait for that started signal, then use a select with a short timer to assert that the AcquireLock has not returned (e.g. by waiting on a "done" channel the goroutine will send to after AcquireLock) before releasing the first lock from the acquired channel; reference store.AcquireLock, acquired, testHash and use the started/done channels to deterministically verify blocking.
🧹 Nitpick comments (1)
internal/cas/integration_test.go (1)
82-94: Consider validating the specific error type for consistency.The "clone with nonexistent branch" test (lines 63-80) validates the specific error type using
ErrorAsandErrorIs, but this test only checks that an error occurred. Based on context snippet 4 (internal/git/git.go:212-221), clone failures wrapErrGitClonein aWrappedError. Validating the error type would ensure the test fails for the right reason and maintains consistency with the adjacent test case.If this relaxation is intentional (e.g., the local test server returns different error types), a brief comment explaining the rationale would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cas/integration_test.go` around lines 82 - 94, The test "clone with invalid repository fails gracefully" only asserts an error but should validate the concrete error to ensure it failed for the expected reason; update the test that calls cas.Clone (in the t.Run block) to check that the returned error wraps ErrGitClone (and/or is a cas.WrappedError) using errors.As or errors.Is similar to the adjacent "clone with nonexistent branch" test—specifically, capture the error from c.Clone and assert errors.As(err, &cas.WrappedError{}) or errors.Is(err, cas.ErrGitClone); if the broader error type is intentionally allowed, add a short comment in the t.Run describing why the type check is relaxed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cas/content.go`:
- Around line 162-207: EnsureCopy currently mis-joins errors and closes the
wrong file handle: update the unlock defer to include the existing named error
(use errors.Join(err, lock.Unlock()) so any prior error isn't discarded) and fix
the source/destination close defer to close the source reader (replace the
second defer that calls f.Close() with one that calls r.Close(), while keeping
the earlier defer that joins f.Close() into err). Locate these changes in the
EnsureCopy function around the lock.Unlock and the two defer blocks for
f.Close/r.Close.
In `@internal/vfs/vfs.go`:
- Around line 87-90: The WriteFile wrapper currently calls afero.WriteFile (in
function WriteFile(fs FS, filename string, data []byte, perm os.FileMode)) which
relies on backend behavior and causes MemMapFs to auto-create parent dirs while
OsFs errors; modify WriteFile to ensure consistent behavior by creating parent
directories before writing: compute the parent directory of filename and call
fs.MkdirAll (or os.MkdirAll via afero if needed) with appropriate permissions,
handle and return any MkdirAll error, then call afero.WriteFile to write the
file; keep function signature WriteFile(fs FS, filename string, data []byte,
perm os.FileMode) and ensure errors from MkdirAll and WriteFile are propagated.
---
Outside diff comments:
In `@internal/cas/store_test.go`:
- Around line 159-168: The timing-sensitive check should be replaced with an
explicit handshake so the test proves the second AcquireLock is actually
blocked; add a "started" channel that the second goroutine closes/sends on
immediately before calling store.AcquireLock(testHash), have the main test wait
for that started signal, then use a select with a short timer to assert that the
AcquireLock has not returned (e.g. by waiting on a "done" channel the goroutine
will send to after AcquireLock) before releasing the first lock from the
acquired channel; reference store.AcquireLock, acquired, testHash and use the
started/done channels to deterministically verify blocking.
---
Nitpick comments:
In `@internal/cas/integration_test.go`:
- Around line 82-94: The test "clone with invalid repository fails gracefully"
only asserts an error but should validate the concrete error to ensure it failed
for the expected reason; update the test that calls cas.Clone (in the t.Run
block) to check that the returned error wraps ErrGitClone (and/or is a
cas.WrappedError) using errors.As or errors.Is similar to the adjacent "clone
with nonexistent branch" test—specifically, capture the error from c.Clone and
assert errors.As(err, &cas.WrappedError{}) or errors.Is(err, cas.ErrGitClone);
if the broader error type is intentionally allowed, add a short comment in the
t.Run describing why the type check is relaxed.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 974262a3-5a7d-4b3f-9e47-48c9a547050c
📒 Files selected for processing (22)
internal/cas/benchmark_test.gointernal/cas/cas.gointernal/cas/cas_test.gointernal/cas/content.gointernal/cas/content_test.gointernal/cas/getter.gointernal/cas/getter_ssh_test.gointernal/cas/getter_test.gointernal/cas/integration_test.gointernal/cas/local.gointernal/cas/race_test.gointernal/cas/store.gointernal/cas/store_test.gointernal/cas/testserver_test.gointernal/cas/tree.gointernal/cas/tree_test.gointernal/git/server.gointernal/git/server_test.gointernal/runner/run/download_source.gointernal/services/catalog/module/repo.gointernal/vfs/vfs.gointernal/vfs/vfs_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/vfs/vfs.go (1)
551-586: Consider reducing cognitive complexity (static analysis).Static analysis flags this function with cognitive complexity 18 vs. the allowed 15. The complexity stems from the nested error handling for
SkipDir/SkipAllsemantics.Given the comment notes this is adapted from the standard library and is temporary until
spf13/afero#571is merged, this is acceptable. However, if the afero merge is delayed, consider extracting theSkipDircheck into a helper:♻️ Optional refactor
+// shouldSkipError returns nil if the error should be skipped for a directory entry. +func shouldSkipError(err error, isDir bool) error { + if errors.Is(err, filepath.SkipDir) && isDir { + return nil + } + return err +} + func walkDir(fsys FS, path string, d fs.DirEntry, walkDirFn fs.WalkDirFunc) error { if err := walkDirFn(path, d, nil); err != nil || !d.IsDir() { - if errors.Is(err, filepath.SkipDir) && d.IsDir() { - err = nil - } - - return err + return shouldSkipError(err, d.IsDir()) } entries, err := readDirEntries(fsys, path) if err != nil { err = walkDirFn(path, d, err) if err != nil { - if errors.Is(err, filepath.SkipDir) && d.IsDir() { - err = nil - } - - return err + return shouldSkipError(err, d.IsDir()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vfs/vfs.go` around lines 551 - 586, The walkDir function's nested error handling increases cognitive complexity; extract the repeated SkipDir handling into a small helper function (e.g., handleSkipDir(err error, d fs.DirEntry) error) and replace the three identical blocks that check errors.Is(err, filepath.SkipDir) && d.IsDir() with calls to that helper; update references in walkDir where walkDirFn is invoked and where errors from recursive walkDir calls are inspected so the overall control flow remains identical but the duplicated SkipDir logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/vfs/vfs.go`:
- Around line 551-586: The walkDir function's nested error handling increases
cognitive complexity; extract the repeated SkipDir handling into a small helper
function (e.g., handleSkipDir(err error, d fs.DirEntry) error) and replace the
three identical blocks that check errors.Is(err, filepath.SkipDir) && d.IsDir()
with calls to that helper; update references in walkDir where walkDirFn is
invoked and where errors from recursive walkDir calls are inspected so the
overall control flow remains identical but the duplicated SkipDir logic is
centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83fdec72-6341-47ff-8e53-9b7695374af2
📒 Files selected for processing (2)
internal/cas/content.gointernal/vfs/vfs.go
denis256
approved these changes
Apr 16, 2026
| afero.Fs | ||
| symlinks map[string]string | ||
| locks map[string]*memLock | ||
| locksMu sync.Mutex |
Member
There was a problem hiding this comment.
Hm, no mutex to protect symlinks 🤔
Collaborator
Author
There was a problem hiding this comment.
I guess we can add one, but we don't currently test anything that tries to do concurrent symlink creation or anything.
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
2 participants
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.
Description
Adds support for using the
vfspackage incasto abstract away filesystem access.Significantly cuts down the time it takes to test the
caspackage, and makes it easier to extend functionality in the future.Added support for a local Git HTTP server in
gitthat we can use for integration with remote Git servers to reduce the overhead of cloning real repositories.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Improvements