feat: Supporting all getters in CAS#6076
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 a per-call CAS environment (Venv) and threads it through CAS, stores, content, tree, gitstore, getters, and stacks; implements FetchSource with probe-based resolvers (HTTP/S3/GCS/Hg/Git); and migrates tests, benchmarks, and integrations to Venv-aware APIs. ChangesCAS Venv, FetchSource, and getter integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
7067d60 to
a723c95
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
a723c95 to
820e159
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
820e159 to
d6c0b51
Compare
yhakbar
force-pushed
the
feat/adding-mutable-flag-to-sources
branch
from
f7733a9 to
78f35ae
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
d6c0b51 to
68f9b6a
Compare
yhakbar
force-pushed
the
feat/adding-mutable-flag-to-sources
branch
from
78f35ae to
9bfe44a
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
68f9b6a to
e9ca395
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
e9ca395 to
a18f444
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
a18f444 to
286dd33
Compare
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/helpers/package.go (1)
312-323:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
nilafter a successful retry delete.If the retry at Line 312 succeeds, Line 323 still returns the previous failure, which can produce false cleanup failures.
💡 Suggested fix
if _, err = client.DeleteBucket(ctx, &s3.DeleteBucketInput{Bucket: aws.String(bucketName)}); err != nil { if isAWSResourceNotFoundError(err) { t.Logf("S3 bucket %s was already deleted", bucketName) return nil } t.Errorf("Failed to delete S3 bucket %s: %v", bucketName, err) return err } - return err + return nil }🤖 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 `@test/helpers/package.go` around lines 312 - 323, The cleanup logic returns a stale error after a successful retry delete; update the code around the client.DeleteBucket call so that when the DeleteBucket call succeeds you return nil instead of returning the previous err. Concretely, in the block using client.DeleteBucket (and referencing bucketName and isAWSResourceNotFoundError), ensure the final return after the if/err check returns nil on success (or clear err to nil) so successful retries do not surface as failures.internal/cas/content.go (1)
248-272:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
EnsureCopyshould not write directly to the final CAS blob path.Lines 248-268 write directly to
path; if copy fails mid-stream, a partial blob can remain and laterhasContentchecks may treat it as valid. Use temp-file + chmod + atomic rename here too.Suggested fix pattern
- f, err := v.FS.Create(path) + tempPath := path + ".tmp" + f, err := v.FS.Create(tempPath) if err != nil { - return fmt.Errorf("create file %s: %w", path, err) + return fmt.Errorf("create temp file %s: %w", tempPath, err) } ... - if err := v.FS.Chmod(path, srcInfo.Mode().Perm()&^WriteBitMask); err != nil { - return fmt.Errorf("chmod %s: %w", path, err) + if err := v.FS.Chmod(tempPath, srcInfo.Mode().Perm()&^WriteBitMask); err != nil { + return fmt.Errorf("chmod %s: %w", tempPath, err) + } + + if err := v.FS.Rename(tempPath, path); err != nil { + return fmt.Errorf("rename %s -> %s: %w", tempPath, path, err) }🤖 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 `@internal/cas/content.go` around lines 248 - 272, EnsureCopy currently creates the final CAS blob at path directly (using v.FS.Create(path)) which can leave a partial blob on failure; change the implementation to write to a temporary file (e.g., path + ".tmp" or a UUID suffix) created via v.FS.Create(tmpPath), copy from v.FS.Open(src) into the tmp file, call v.FS.Chmod(tmpPath, srcInfo.Mode().Perm()&^WriteBitMask) and close both handles (preserving error via errors.Join), then atomically rename the tmpPath to path (using the FS rename method) only after successful copy and chmod; update error messages to reference tmpPath where appropriate and ensure cleanup of tmpPath on failure.
🧹 Nitpick comments (9)
test/helpers/package.go (1)
329-415: 🏗️ Heavy lift
cleanS3Bucketis doing too much in one function.This method mixes pagination, version/delete-marker mapping, batch delete calls, and error handling in one loop. Splitting into small helpers (e.g., build identifiers, delete batch, handle pagination) would significantly reduce cognitive complexity and make cleanup failures easier to debug.
🤖 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 `@test/helpers/package.go` around lines 329 - 415, cleanS3Bucket is doing pagination, identifier mapping, batch deletes and error handling in one loop; refactor it by extracting small helpers: e.g., buildVersionIdentifiers(out *s3.ListObjectVersionsOutput) []s3types.ObjectIdentifier to map out.Versions, buildDeleteMarkerIdentifiers(out *s3.ListObjectVersionsOutput) []s3types.ObjectIdentifier to map out.DeleteMarkers, deleteObjectsBatch(ctx, client, bucketName string, objs []s3types.ObjectIdentifier) error to perform DeleteObjects and centralize the isAWSResourceNotFoundError logic, and iterate paging in cleanS3Bucket calling those helpers (preserve behavior for empty lists, truncated paging using NextKeyMarker/NextVersionIdMarker, and existing test t.Logf/require.NoError flows) so the loop is slimmer and each helper has one responsibility.internal/cas/gitstore_test.go (1)
189-210: ⚡ Quick winAdd a regression test for
Venv{FS: osFS, Git: nil}.Given the new
Venv-threaded API, add an explicit negative-path test to ensureEnsureReffails cleanly (no panic) whenGitis nil.As per coding guidelines
**/*.go: prioritize maintainability and correctness in Go code and tests.Also applies to: 212-223
🤖 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 `@internal/cas/gitstore_test.go` around lines 189 - 210, Add a regression test in gitstore_test.go that constructs a cas.Venv with an OS-backed filesystem and nil Git (e.g., Venv{FS: <OS FS>, Git: nil}) and calls cas.NewGitStore(...).EnsureRef using that venv; assert the call returns a non-nil error and does not panic (use require.NotPanics around the call and require.Error to check the returned error). Reference the Venv type and the EnsureRef method so reviewers can locate where to add the negative-path test; also add the same non-panicking EnsureRef check for the similar case mentioned around the existing ProbeCachedCommit tests (lines ~212-223).internal/cas/content.go (1)
303-377: ⚡ Quick winExtract repeated temp-file cleanup into a helper to reduce complexity.
writeContentToFilehas repeated close/remove branches across multiple error paths. Pulling cleanup into a small helper will reduce cognitive complexity and make error-path behavior easier to keep consistent.🤖 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 `@internal/cas/content.go` around lines 303 - 377, writeContentToFile repeats the same temp-file cleanup logic (closing the file and removing tempPath while logging via l.Warnf) across multiple error branches; extract that into a single helper (e.g., cleanupTempFile) and call it from each error path to reduce duplication and cognitive complexity. Implement a helper that accepts the logger, the Venv/file-system (v or v.FS), the open file handle (f) and tempPath (and returns any primary error if needed), performs a best-effort close of f (if non-nil) and removes tempPath, logging failures with l.Warnf; then replace the repeated close+remove blocks in writeContentToFile (the write/flush/close/chmod/rename error branches referencing f, tempPath, v.FS and l.Warnf) with calls to this helper and return the original wrapped error as before.internal/cas/stacks.go (1)
563-615: ⚡ Quick winRefactor
copyTreeto reduce cognitive complexity (Sonar failure)This method is currently above the configured complexity threshold and is reported as a failing static-analysis check. Split directory/symlink/regular-file handling into small helpers to unblock quality gates and improve maintainability.
🤖 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 `@internal/cas/stacks.go` around lines 563 - 615, The copyTree function is too complex; refactor by extracting the three main branches into small helper functions to reduce cognitive complexity: create handleDir(v Venv, target string) error to call v.FS.MkdirAll with DefaultDirPerms, handleSymlink(v Venv, path, src, target string, linkTarget string) error to read/resolve the symlink (using vfs.Readlink), validate that the resolved target stays inside src (using filepath.Clean and filepath.Rel and ErrSourceEscapesRepo), ensure parent dirs via v.FS.MkdirAll and create the link with vfs.Symlink, and handleRegular(v Venv, path, target string, perm fs.FileMode) error to call c.copyFileInFS(v, path, target, perm). Replace the inline branches in copyTree with calls to these helpers, passing the needed values (info, path, target, src) so the main WalkDir callback becomes a small dispatcher.internal/cas/source_test.go (1)
333-342: 💤 Low valueConsider using
v.FSinstead ofos.*for consistency with Venv abstraction.The
fakeFetcherusesos.MkdirAllandos.WriteFiledirectly. While this works becauseMakeFetchTempDircreates a real OS path, usingv.FS.MkdirAllandvfs.WriteFile(v.FS, ...)(if available) would maintain consistency with the Venv abstraction pattern used elsewhere.🤖 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 `@internal/cas/source_test.go` around lines 333 - 342, The test helper in fakeFetcher is creating files with direct os calls (os.MkdirAll, os.WriteFile) which breaks the Venv abstraction; update the loop that writes files (used after MakeFetchTempDir) to use the Venv filesystem APIs instead—call v.FS.MkdirAll for directories and use the vfs utility (e.g., vfs.WriteFile or equivalent helper) to create file contents—keeping behavior identical but routing filesystem operations through v.FS so fakeFetcher remains consistent with the Venv abstraction.internal/cas/cas.go (1)
230-243: ⚡ Quick winBranch value should be URL-encoded to avoid malformed URLs.
If
branchcontains special characters (e.g.,feature/test, spaces, or&), the raw concatenation can produce malformed or ambiguous URLs. While branch names with such characters are rare, URL-encoding the value is safer.♻️ Proposed fix
+import "net/url" + // gitURLWithRef appends ?ref=<branch> to url so a singleton // GitResolver (no per-request branch field) can recover the branch // from the URL alone. func gitURLWithRef(url, branch string) string { if branch == "" { return url } - if strings.Contains(url, "?") { - return url + "&ref=" + branch - } - - return url + "?ref=" + branch + u, err := neturl.Parse(url) + if err != nil { + // Fall back to simple concatenation if parse fails. + if strings.Contains(url, "?") { + return url + "&ref=" + branch + } + return url + "?ref=" + branch + } + + q := u.Query() + q.Set("ref", branch) + u.RawQuery = q.Encode() + + return u.String() }🤖 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 `@internal/cas/cas.go` around lines 230 - 243, The gitURLWithRef function currently concatenates branch raw into the URL, which can produce malformed URLs for branches with special characters; update gitURLWithRef to URL-encode the branch value (e.g., using net/url.QueryEscape) before appending it, preserving the existing logic for choosing "?" vs "&" so behavior is unchanged except the branch parameter becomes safely encoded.internal/getter/resolver_s3.go (1)
174-177: ⚡ Quick winExtract duplicated
"us-east-1"literal to a constant.The default region string is duplicated three times. Extracting it improves maintainability and satisfies the static analysis finding.
♻️ Proposed fix
const ( s3HostPartsPathStyle = 3 s3HostPartsVHostStyle = 4 + s3DefaultRegion = "us-east-1" // s3URLPathSegments is the count produced by splitting `/bucket/key` // on "/" with limit 3: ["", "bucket", "key"]. Used as a validation // gate before indexing. s3URLPathSegments = 3 )Then replace each occurrence:
if region == "" { - region = "us-east-1" + region = s3DefaultRegion }Also applies to: 187-190, 207-210
🤖 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 `@internal/getter/resolver_s3.go` around lines 174 - 177, Duplicate literal "us-east-1" should be extracted to a single constant; add a package-level constant (e.g., defaultS3Region = "us-east-1") in internal/getter/resolver_s3.go and replace the three occurrences where region is set (the assignments inside the S3 resolver logic around the region = ... block and the other two spots referenced in the review) with that constant; update any comments or tests if they reference the literal.internal/getter/resolver_gcs.go (1)
26-29: 💤 Low valueConsider renaming
GCSObjectinterface per Go single-method naming convention.Go convention suggests naming single-method interfaces after the method with an
-ersuffix (e.g.,AttrsProvider). However, since this interface mirrors the SDK'sObjectHandle.Attrsand the name clearly indicates its purpose, this is optional.🤖 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 `@internal/getter/resolver_gcs.go` around lines 26 - 29, Rename the single-method interface GCSObject to follow Go's single-method naming convention (e.g., AttrsProvider) to make intent clear; update all references/usages of GCSObject (including the Attrs(ctx context.Context) (*storage.ObjectAttrs, error) method signature) to the new name and run tests to ensure no remaining references break.internal/getter/casgetter.go (1)
129-178: ⚖️ Poor tradeoffDetect method slightly exceeds cognitive complexity threshold.
SonarCloud flags this at 16 vs the allowed 15. The method is reasonably clear as-is, but extracting the detector-chain loop or the local-directory handling into helper methods would reduce complexity. This is optional given the marginal overage.
🤖 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 `@internal/getter/casgetter.go` around lines 129 - 178, The Detect method exceeds the complexity threshold; extract the detector loop or the local-directory handling into a small helper to reduce cognitive complexity. Create a helper on CASGetter such as detectUsingDetectors(req *getter.Request) (bool, error) that contains the for _, detector := range g.Detectors loop and returns (true,nil) when a detector succeeds (setting req.Src and req.Copy as needed), or alternately extract the file-detector branch into handleFileDetector(detector getter.Detector, src string, req *getter.Request) (bool, error) to perform Stat, IsDir checks and set req.Copy; then simplify Detect to call the new helper and return its result, keeping existing behavior around matchGenericScheme, git prefix handling, and setting req.Forced and appendDisableArchive.
🤖 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 `@internal/cas/content.go`:
- Around line 98-101: The current materialization treats os.IsExist(err) from
vfs.Link as success, which can leave stale bytes at targetPath; in
internal/cas/content.go adjust the vfs.Link handling in the materialization path
(the block using v.FS.Stat(sourcePath) and vfs.Link(v.FS, sourcePath,
targetPath)) so that only err == nil returns success, and any os.IsExist(err)
falls through to the overwrite/remove-and-relink logic instead (i.e., do not
return on os.IsExist; instead delete or overwrite targetPath and retry linking
or let the subsequent overwrite code handle it).
In `@internal/cas/gitstore.go`:
- Around line 273-286: The code calls v.Git.WithWorkDir in ProbeCachedCommit and
inside acquire without checking v.Git for nil, which can panic and (in acquire)
leave a flock held because session.cleanup() never runs; add a nil guard: in
ProbeCachedCommit check if v.Git == nil and return early (e.g., "", false)
before any use, and in acquire validate v.Git != nil before obtaining the lock
so you never call v.Git.WithWorkDir when nil; reference Venv.Git,
ProbeCachedCommit, acquire, v.Git.WithWorkDir and ensure session.cleanup() is
only relied on after the nil-checked operations.
In `@internal/cas/stacks.go`:
- Around line 145-153: The code builds contentDir by joining cloneDir and
user-controlled subdir and then stats it, allowing path traversal (e.g.,
"//../..") to escape the repo; before calling v.FS.Stat and before any
processing in the function that uses contentDir (symbols: contentDir, cloneDir,
subdir, v.FS.Stat, cleanup), validate and normalize the joined path (use
filepath.Clean and resolve absolute paths or use filepath.Rel) and ensure the
final path is inside cloneDir (e.g., filepath.Rel(cloneDir, contentDir) returns
no leading ".."); if the check fails, call cleanup and return an error rejecting
the subdir.
In `@internal/getter/resolver_hg_test.go`:
- Around line 99-100: Replace the brittle shell invocation using
exec.CommandContext(t.Context(), "sh", "-c", "echo hello >
"+filepath.Join(repoDir, "main.tf")) with a direct file write: use os.WriteFile
(or ioutil.WriteFile) to write the desired contents ("hello\n" or "hello") to
filepath.Join(repoDir, "main.tf") using appropriate file permissions, and remove
the exec.CommandContext call and any reliance on shell quoting; keep using
t.Context() and repoDir where needed and update the other similar instance at
lines ~137-138 in the same test.
In `@internal/getter/resolver_http.go`:
- Around line 75-77: The code currently generates cache keys from raw ETag
header values even when normalizeETag(etag) returns an empty string (e.g., ETag
`""` or `W/""`), causing false cache hits; update the ETag handling in
resolver_http.go so you call normalizeETag(etag) first, reject the result if it
is empty, and only then call cas.OpaqueKey(scheme, rawURL, normalized) and
return it; apply the same empty-check logic to the other ETag-related branch(s)
around the resp.Header.Get("ETag") usage (the block at lines 89-95) so no cache
key is created for empty normalized ETags.
In `@test/cas_archive_test.go`:
- Around line 22-43: The test builds a tar.gz from the files map using an
iteration over the map (the files variable), which yields non-deterministic
entry order and breaks CAS checks; fix it by extracting the keys from files into
a slice, sort.Strings that slice, and then iterate over the sorted keys to write
entries to tw (and gz/buf as before) so the tar entry order—and thus the
resulting archive bytes and checksum—are deterministic.
In `@test/integration_cas_s3_test.go`:
- Around line 36-41: The test always sets
CreateBucketConfiguration.LocationConstraint when calling s3Client.CreateBucket
(via CreateBucketInput), which breaks for region "us-east-1"; update the test to
omit CreateBucketConfiguration for that region by conditionally constructing the
CreateBucketInput: if region == "us-east-1" pass CreateBucketConfiguration as
nil (or leave it unset), otherwise set CreateBucketConfiguration with
types.BucketLocationConstraint(region). Use the existing region variable (e.g.,
helpers.TerraformRemoteStateS3Region) and adjust the code around
s3Client.CreateBucket/CreateBucketInput to choose the appropriate payload.
---
Outside diff comments:
In `@internal/cas/content.go`:
- Around line 248-272: EnsureCopy currently creates the final CAS blob at path
directly (using v.FS.Create(path)) which can leave a partial blob on failure;
change the implementation to write to a temporary file (e.g., path + ".tmp" or a
UUID suffix) created via v.FS.Create(tmpPath), copy from v.FS.Open(src) into the
tmp file, call v.FS.Chmod(tmpPath, srcInfo.Mode().Perm()&^WriteBitMask) and
close both handles (preserving error via errors.Join), then atomically rename
the tmpPath to path (using the FS rename method) only after successful copy and
chmod; update error messages to reference tmpPath where appropriate and ensure
cleanup of tmpPath on failure.
In `@test/helpers/package.go`:
- Around line 312-323: The cleanup logic returns a stale error after a
successful retry delete; update the code around the client.DeleteBucket call so
that when the DeleteBucket call succeeds you return nil instead of returning the
previous err. Concretely, in the block using client.DeleteBucket (and
referencing bucketName and isAWSResourceNotFoundError), ensure the final return
after the if/err check returns nil on success (or clear err to nil) so
successful retries do not surface as failures.
---
Nitpick comments:
In `@internal/cas/cas.go`:
- Around line 230-243: The gitURLWithRef function currently concatenates branch
raw into the URL, which can produce malformed URLs for branches with special
characters; update gitURLWithRef to URL-encode the branch value (e.g., using
net/url.QueryEscape) before appending it, preserving the existing logic for
choosing "?" vs "&" so behavior is unchanged except the branch parameter becomes
safely encoded.
In `@internal/cas/content.go`:
- Around line 303-377: writeContentToFile repeats the same temp-file cleanup
logic (closing the file and removing tempPath while logging via l.Warnf) across
multiple error branches; extract that into a single helper (e.g.,
cleanupTempFile) and call it from each error path to reduce duplication and
cognitive complexity. Implement a helper that accepts the logger, the
Venv/file-system (v or v.FS), the open file handle (f) and tempPath (and returns
any primary error if needed), performs a best-effort close of f (if non-nil) and
removes tempPath, logging failures with l.Warnf; then replace the repeated
close+remove blocks in writeContentToFile (the write/flush/close/chmod/rename
error branches referencing f, tempPath, v.FS and l.Warnf) with calls to this
helper and return the original wrapped error as before.
In `@internal/cas/gitstore_test.go`:
- Around line 189-210: Add a regression test in gitstore_test.go that constructs
a cas.Venv with an OS-backed filesystem and nil Git (e.g., Venv{FS: <OS FS>,
Git: nil}) and calls cas.NewGitStore(...).EnsureRef using that venv; assert the
call returns a non-nil error and does not panic (use require.NotPanics around
the call and require.Error to check the returned error). Reference the Venv type
and the EnsureRef method so reviewers can locate where to add the negative-path
test; also add the same non-panicking EnsureRef check for the similar case
mentioned around the existing ProbeCachedCommit tests (lines ~212-223).
In `@internal/cas/source_test.go`:
- Around line 333-342: The test helper in fakeFetcher is creating files with
direct os calls (os.MkdirAll, os.WriteFile) which breaks the Venv abstraction;
update the loop that writes files (used after MakeFetchTempDir) to use the Venv
filesystem APIs instead—call v.FS.MkdirAll for directories and use the vfs
utility (e.g., vfs.WriteFile or equivalent helper) to create file
contents—keeping behavior identical but routing filesystem operations through
v.FS so fakeFetcher remains consistent with the Venv abstraction.
In `@internal/cas/stacks.go`:
- Around line 563-615: The copyTree function is too complex; refactor by
extracting the three main branches into small helper functions to reduce
cognitive complexity: create handleDir(v Venv, target string) error to call
v.FS.MkdirAll with DefaultDirPerms, handleSymlink(v Venv, path, src, target
string, linkTarget string) error to read/resolve the symlink (using
vfs.Readlink), validate that the resolved target stays inside src (using
filepath.Clean and filepath.Rel and ErrSourceEscapesRepo), ensure parent dirs
via v.FS.MkdirAll and create the link with vfs.Symlink, and handleRegular(v
Venv, path, target string, perm fs.FileMode) error to call c.copyFileInFS(v,
path, target, perm). Replace the inline branches in copyTree with calls to these
helpers, passing the needed values (info, path, target, src) so the main WalkDir
callback becomes a small dispatcher.
In `@internal/getter/casgetter.go`:
- Around line 129-178: The Detect method exceeds the complexity threshold;
extract the detector loop or the local-directory handling into a small helper to
reduce cognitive complexity. Create a helper on CASGetter such as
detectUsingDetectors(req *getter.Request) (bool, error) that contains the for _,
detector := range g.Detectors loop and returns (true,nil) when a detector
succeeds (setting req.Src and req.Copy as needed), or alternately extract the
file-detector branch into handleFileDetector(detector getter.Detector, src
string, req *getter.Request) (bool, error) to perform Stat, IsDir checks and set
req.Copy; then simplify Detect to call the new helper and return its result,
keeping existing behavior around matchGenericScheme, git prefix handling, and
setting req.Forced and appendDisableArchive.
In `@internal/getter/resolver_gcs.go`:
- Around line 26-29: Rename the single-method interface GCSObject to follow Go's
single-method naming convention (e.g., AttrsProvider) to make intent clear;
update all references/usages of GCSObject (including the Attrs(ctx
context.Context) (*storage.ObjectAttrs, error) method signature) to the new name
and run tests to ensure no remaining references break.
In `@internal/getter/resolver_s3.go`:
- Around line 174-177: Duplicate literal "us-east-1" should be extracted to a
single constant; add a package-level constant (e.g., defaultS3Region =
"us-east-1") in internal/getter/resolver_s3.go and replace the three occurrences
where region is set (the assignments inside the S3 resolver logic around the
region = ... block and the other two spots referenced in the review) with that
constant; update any comments or tests if they reference the literal.
In `@test/helpers/package.go`:
- Around line 329-415: cleanS3Bucket is doing pagination, identifier mapping,
batch deletes and error handling in one loop; refactor it by extracting small
helpers: e.g., buildVersionIdentifiers(out *s3.ListObjectVersionsOutput)
[]s3types.ObjectIdentifier to map out.Versions, buildDeleteMarkerIdentifiers(out
*s3.ListObjectVersionsOutput) []s3types.ObjectIdentifier to map
out.DeleteMarkers, deleteObjectsBatch(ctx, client, bucketName string, objs
[]s3types.ObjectIdentifier) error to perform DeleteObjects and centralize the
isAWSResourceNotFoundError logic, and iterate paging in cleanS3Bucket calling
those helpers (preserve behavior for empty lists, truncated paging using
NextKeyMarker/NextVersionIdMarker, and existing test t.Logf/require.NoError
flows) so the loop is slimmer and each helper has one responsibility.
🪄 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: e8aee55c-89a6-478e-975b-1101d43716a9
📒 Files selected for processing (58)
internal/cas/benchmark_test.gointernal/cas/cas.gointernal/cas/cas_test.gointernal/cas/clone_e2e_test.gointernal/cas/commitref_test.gointernal/cas/content.gointernal/cas/content_test.gointernal/cas/getter_ssh_test.gointernal/cas/getter_test.gointernal/cas/gitstore.gointernal/cas/gitstore_test.gointernal/cas/integration_test.gointernal/cas/local.gointernal/cas/local_test.gointernal/cas/materialize_test.gointernal/cas/protocol.gointernal/cas/race_test.gointernal/cas/resolver_git_test.gointernal/cas/source.gointernal/cas/source_test.gointernal/cas/stacks.gointernal/cas/stacks_local_test.gointernal/cas/stacks_test.gointernal/cas/store.gointernal/cas/store_test.gointernal/cas/tree.gointernal/cas/tree_test.gointernal/cas/venv.gointernal/getter/casgetter.gointernal/getter/casgetter_detect_test.gointernal/getter/casgetter_forced_test.gointernal/getter/casgetter_generic_test.gointernal/getter/casprotocol.gointernal/getter/defaults.gointernal/getter/options.gointernal/getter/options_test.gointernal/getter/resolver.gointernal/getter/resolver_gcs.gointernal/getter/resolver_gcs_test.gointernal/getter/resolver_hg.gointernal/getter/resolver_hg_test.gointernal/getter/resolver_http.gointernal/getter/resolver_http_test.gointernal/getter/resolver_s3.gointernal/getter/resolver_s3_test.gointernal/getter/types_test.gointernal/git/git.gointernal/runner/run/download_source.gointernal/services/catalog/module/repo.gointernal/vfs/vfs.gopkg/config/stack.gotest/cas_archive_test.gotest/helpers/package.gotest/integration_aws_test.gotest/integration_cas_gcs_test.gotest/integration_cas_rustfs_test.gotest/integration_cas_s3_test.gotest/integration_gcp_test.go
Comment on lines
+382
to
+385
| venv, err := cas.OSVenv() | ||
| if err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
Fail open on CAS environment init errors instead of aborting clone.
On Line 383, returning the OSVenv error aborts the entire clone even when non-CAS getters could still succeed. For optional CAS, prefer warning + fallback to standard client wiring.
Comment on lines
+36
to
+41
| _, err := s3Client.CreateBucket(t.Context(), &s3.CreateBucketInput{ | ||
| Bucket: aws.String(bucket), | ||
| CreateBucketConfiguration: &types.CreateBucketConfiguration{ | ||
| LocationConstraint: types.BucketLocationConstraint(region), | ||
| }, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Amazon S3 CreateBucket, is CreateBucketConfiguration.LocationConstraint required to be omitted when region is us-east-1?
💡 Result:
Yes, for Amazon S3 CreateBucket when creating a bucket in us-east-1, the CreateBucketConfiguration.LocationConstraint must be omitted (or the entire CreateBucketConfiguration if empty). It is not required and specifying it (e.g., as 'us-east-1' or empty string) will fail with an error like IllegalLocationConstraintException, as us-east-1 is not a valid value for LocationConstraint [1][2][3]. The official documentation explicitly states: "If you are creating a bucket on the US East (N. Virginia) Region (us-east-1), you do not need to specify the location" [1][4][5]. If no LocationConstraint is provided, the bucket defaults to us-east-1 [2][3]. For other regions, it must be specified with a valid regional value [6][7].
Citations:
- 1: https://docs.aws.amazon.com/AmazonS3/latest/API/API_control_CreateBucketConfiguration.html
- 2: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateBucketConfiguration.html
- 3: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateBucket.html
- 4: https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/services/s3control/model/CreateBucketConfiguration.html
- 5: https://docs.aws.amazon.com/goto/SdkForJava/s3control-2018-08-20/CreateBucketConfiguration
- 6: https://dev.to/marviecodes/aws-s3-location-constraint-error-heres-what-youre-doing-wrong-4egp
- 7: https://stackoverflow.com/questions/49174673/aws-s3api-create-bucket-bucket-make-exception
Handle S3 us-east-1 bucket creation separately.
The code always sets CreateBucketConfiguration.LocationConstraint from the region variable. For us-east-1, AWS S3 requires this field to be omitted—specifying it causes an IllegalLocationConstraintException. The test will fail if helpers.TerraformRemoteStateS3Region is us-east-1.
Suggested fix
- _, err := s3Client.CreateBucket(t.Context(), &s3.CreateBucketInput{
- Bucket: aws.String(bucket),
- CreateBucketConfiguration: &types.CreateBucketConfiguration{
- LocationConstraint: types.BucketLocationConstraint(region),
- },
- })
+ input := &s3.CreateBucketInput{Bucket: aws.String(bucket)}
+ if region != "us-east-1" {
+ input.CreateBucketConfiguration = &types.CreateBucketConfiguration{
+ LocationConstraint: types.BucketLocationConstraint(region),
+ }
+ }
+ _, err := s3Client.CreateBucket(t.Context(), input)📝 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
| _, err := s3Client.CreateBucket(t.Context(), &s3.CreateBucketInput{ | |
| Bucket: aws.String(bucket), | |
| CreateBucketConfiguration: &types.CreateBucketConfiguration{ | |
| LocationConstraint: types.BucketLocationConstraint(region), | |
| }, | |
| }) | |
| input := &s3.CreateBucketInput{Bucket: aws.String(bucket)} | |
| if region != "us-east-1" { | |
| input.CreateBucketConfiguration = &types.CreateBucketConfiguration{ | |
| LocationConstraint: types.BucketLocationConstraint(region), | |
| } | |
| } | |
| _, err := s3Client.CreateBucket(t.Context(), input) |
🤖 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 `@test/integration_cas_s3_test.go` around lines 36 - 41, The test always sets
CreateBucketConfiguration.LocationConstraint when calling s3Client.CreateBucket
(via CreateBucketInput), which breaks for region "us-east-1"; update the test to
omit CreateBucketConfiguration for that region by conditionally constructing the
CreateBucketInput: if region == "us-east-1" pass CreateBucketConfiguration as
nil (or leave it unset), otherwise set CreateBucketConfiguration with
types.BucketLocationConstraint(region). Use the existing region variable (e.g.,
helpers.TerraformRemoteStateS3Region) and adjust the code around
s3Client.CreateBucket/CreateBucketInput to choose the appropriate payload.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cas/stacks.go (1)
599-621:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not preserve absolute symlink targets when copying local sources.
The boundary check is against
src, butSymlink(v.FS, linkTarget, target)keeps an absolute target unchanged. After the copy, that link still points back to the original working tree, so CAS can read outside the temp repo instead of staying self-contained.Minimal safe fix
if info.Mode()&os.ModeSymlink != 0 { linkTarget, err := vfs.Readlink(v.FS, path) if err != nil { return err } + + if filepath.IsAbs(linkTarget) { + return fmt.Errorf("%w: absolute symlink %q -> %q", ErrSourceEscapesRepo, path, linkTarget) + } resolved := linkTarget - if !filepath.IsAbs(resolved) { - resolved = filepath.Join(filepath.Dir(path), resolved) - } + resolved = filepath.Join(filepath.Dir(path), resolved)If absolute in-repo symlinks need to stay supported, rewrite them to an equivalent target under
dstinstead of copying the original absolute path.
🧹 Nitpick comments (1)
internal/getter/resolver_s3.go (1)
178-178: ⚡ Quick winExtract duplicate
"us-east-1"literal to a constant.The default region
"us-east-1"appears three times. Extract to a package-level constant to improve maintainability.♻️ Proposed fix
Add constant at line 20 (after
s3ResolverTimeout):const s3ResolverTimeout = 10 * time.Second + +// s3DefaultRegion is the fallback when region cannot be inferred from URL. +const s3DefaultRegion = "us-east-1"Then replace usages:
if region == "" { - region = "us-east-1" + region = s3DefaultRegion }Also applies to: 191-191, 211-211
🤖 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 `@internal/getter/resolver_s3.go` at line 178, Extract the repeated "us-east-1" string into a package-level constant (e.g., defaultS3Region) placed near s3ResolverTimeout, then replace all literal occurrences in resolver_s3.go (the branches that set region around the code using region = "us-east-1" and other occurrences at the lines noted) with that constant; update any comments or usages referencing the literal to use defaultS3Region so the region value is centralized and maintainable.
🤖 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 `@internal/getter/resolver_hg.go`:
- Around line 68-80: The current probe in resolver_hg.go treats all errors from
r.Exec.Command(...).Output() as cas.ErrNoVersionMetadata, which hides caller
cancellations; modify the error handling after the Output() call to first check
the incoming context (ctx) and/or whether err is a context cancellation or
deadline error (e.g., ctx.Err() != nil or errors.Is(err,
context.Canceled/DeadlineExceeded)) and return that context error back to the
caller, and only return cas.ErrNoVersionMetadata for genuine probe misses;
ensure you still use the local hgResolverTimeout wrapper (ctx, cancel :=
context.WithTimeout(...)) but propagate parent cancellation errors instead of
collapsing them.
---
Nitpick comments:
In `@internal/getter/resolver_s3.go`:
- Line 178: Extract the repeated "us-east-1" string into a package-level
constant (e.g., defaultS3Region) placed near s3ResolverTimeout, then replace all
literal occurrences in resolver_s3.go (the branches that set region around the
code using region = "us-east-1" and other occurrences at the lines noted) with
that constant; update any comments or usages referencing the literal to use
defaultS3Region so the region value is centralized and maintainable.
🪄 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: d70fa354-435f-45b9-a64a-618e8e96319d
📒 Files selected for processing (11)
internal/cas/cas.gointernal/cas/content.gointernal/cas/content_test.gointernal/cas/getter_test.gointernal/cas/source_test.gointernal/cas/stacks.gointernal/getter/resolver_hg.gointernal/getter/resolver_hg_test.gointernal/getter/resolver_http.gointernal/getter/resolver_s3.gotest/cas_archive_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- test/cas_archive_test.go
- internal/cas/getter_test.go
- internal/getter/resolver_http.go
- internal/cas/content_test.go
- internal/cas/source_test.go
- internal/cas/cas.go
Comment on lines
+68
to
+80
| ctx, cancel := context.WithTimeout(ctx, hgResolverTimeout) | ||
| defer cancel() | ||
|
|
||
| args := []string{"identify", "--id"} | ||
| if rev != "" { | ||
| args = append(args, "-r", rev) | ||
| } | ||
|
|
||
| args = append(args, cleaned.String()) | ||
|
|
||
| out, err := r.Exec.Command(ctx, bin, args...).Output() | ||
| if err != nil { | ||
| return "", cas.ErrNoVersionMetadata |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the resolver_hg.go file
fd -t f "resolver_hg.go" -x cat -n {}Repository: gruntwork-io/terragrunt
Length of output: 3158
🏁 Script executed:
# Check the imports and error types in the file
head -30 internal/getter/resolver_hg.goRepository: gruntwork-io/terragrunt
Length of output: 1025
🏁 Script executed:
# Look at the Probe method and surrounding context handling
sed -n '45,90p' internal/getter/resolver_hg.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 1299
🏁 Script executed:
# Check how ErrNoVersionMetadata is defined and used elsewhere
rg "ErrNoVersionMetadata" -B 2 -A 2Repository: gruntwork-io/terragrunt
Length of output: 16341
Preserve caller cancellation instead of converting it to metadata-miss
At line 79, all probe failures are collapsed into cas.ErrNoVersionMetadata. If the parent context is already canceled or deadline-exceeded, this hides cancellation and can cause unnecessary fallback work. Propagate parent context errors, and only use ErrNoVersionMetadata for true probe misses.
Suggested fix
import (
"context"
+ "errors"
"fmt"
"net/url"
"strings"
"time"
@@
- ctx, cancel := context.WithTimeout(ctx, hgResolverTimeout)
+ probeCtx, cancel := context.WithTimeout(ctx, hgResolverTimeout)
defer cancel()
@@
- out, err := r.Exec.Command(ctx, bin, args...).Output()
+ out, err := r.Exec.Command(probeCtx, bin, args...).Output()
if err != nil {
+ if errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded) {
+ return "", ctx.Err()
+ }
return "", cas.ErrNoVersionMetadata
}📝 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
| ctx, cancel := context.WithTimeout(ctx, hgResolverTimeout) | |
| defer cancel() | |
| args := []string{"identify", "--id"} | |
| if rev != "" { | |
| args = append(args, "-r", rev) | |
| } | |
| args = append(args, cleaned.String()) | |
| out, err := r.Exec.Command(ctx, bin, args...).Output() | |
| if err != nil { | |
| return "", cas.ErrNoVersionMetadata | |
| probeCtx, cancel := context.WithTimeout(ctx, hgResolverTimeout) | |
| defer cancel() | |
| args := []string{"identify", "--id"} | |
| if rev != "" { | |
| args = append(args, "-r", rev) | |
| } | |
| args = append(args, cleaned.String()) | |
| out, err := r.Exec.Command(probeCtx, bin, args...).Output() | |
| if err != nil { | |
| if errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded) { | |
| return "", ctx.Err() | |
| } | |
| return "", cas.ErrNoVersionMetadata | |
| } |
🤖 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 `@internal/getter/resolver_hg.go` around lines 68 - 80, The current probe in
resolver_hg.go treats all errors from r.Exec.Command(...).Output() as
cas.ErrNoVersionMetadata, which hides caller cancellations; modify the error
handling after the Output() call to first check the incoming context (ctx)
and/or whether err is a context cancellation or deadline error (e.g., ctx.Err()
!= nil or errors.Is(err, context.Canceled/DeadlineExceeded)) and return that
context error back to the caller, and only return cas.ErrNoVersionMetadata for
genuine probe misses; ensure you still use the local hgResolverTimeout wrapper
(ctx, cancel := context.WithTimeout(...)) but propagate parent cancellation
errors instead of collapsing them.
1deea52 to
7fc8e63
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/getter/resolver_http_test.go (1)
67-93: ⚡ Quick winAdd a canceled-context probe test to lock in error semantics.
The suite covers metadata-miss/non-2xx paths, but not
ctxcancellation. Please add a case that cancels context beforeProbeand assertscontext.Canceled(notcas.ErrNoVersionMetadata), so future changes don’t regress cancellation handling.🤖 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 `@internal/getter/resolver_http_test.go` around lines 67 - 93, Add a new test (e.g., TestHTTPResolver_ReturnsContextCanceled) that constructs a cancellable context, cancels it before calling r.Probe, and asserts the returned error is context.Canceled (not cas.ErrNoVersionMetadata); specifically, use getter.NewHTTPResolver(), create ctx, call cancel(), then call r.Probe(ctx, srv.URL+"/mod.tgz") and require.ErrorIs(t, err, context.Canceled) to lock in cancellation semantics.internal/getter/resolver_s3.go (1)
176-220: ⚡ Quick winExtract
us-east-1into a constant to eliminate duplication.The literal
"us-east-1"appears three times inparseS3URL. Extracting it makes the default region explicit and easier to change.♻️ Proposed fix
const ( s3HostPartsPathStyle = 3 s3HostPartsVHostStyle = 4 // s3URLPathSegments is the count produced by splitting `/bucket/key` // on "/" with limit 3: ["", "bucket", "key"]. Used as a validation // gate before indexing. s3URLPathSegments = 3 + // s3DefaultRegion is the fallback when no region is specified. + s3DefaultRegion = "us-east-1" )Then replace the three occurrences:
- region = "us-east-1" + region = s3DefaultRegion🤖 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 `@internal/getter/resolver_s3.go` around lines 176 - 220, The function parseS3URL uses the literal "us-east-1" in three places; introduce a package-level constant (e.g. defaultAWSRegion or defaultRegion) and replace all occurrences in parseS3URL with that constant so the default region is defined once and clearly named; update any tests or callers of parseS3URL if they rely on the literal string, and ensure the new constant is exported or unexported according to existing package conventions.internal/cas/stacks.go (1)
148-160: ⚡ Quick winConsider extracting path-escape validation into a helper.
The same validation pattern (IsAbs check, Clean, filepath.Rel,
..prefix check) appears in bothProcessStackComponentandprocessLocalStackComponent. A shared helper would reduce duplication and the cognitive complexity flagged by static analysis.♻️ Proposed helper
// validateSubdir ensures subdir is relative and stays inside root after joining. // Returns the cleaned absolute path or an error if the path escapes. func validateSubdir(root, subdir string) (string, error) { if filepath.IsAbs(subdir) { return "", fmt.Errorf("%w: %q", ErrSourceEscapesRepo, subdir) } resolved := filepath.Clean(filepath.Join(root, subdir)) rel, err := filepath.Rel(root, resolved) if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return "", fmt.Errorf("%w: %q", ErrSourceEscapesRepo, subdir) } return resolved, nil }This also addresses the duplicate
"%w: %q"format string flagged by static analysis.Also applies to: 534-546
🤖 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 `@internal/cas/stacks.go` around lines 148 - 160, Both ProcessStackComponent and processLocalStackComponent duplicate path-escape validation logic; extract that into a helper (e.g., validateSubdir) which takes root and subdir and returns the cleaned absolute path or an error using ErrSourceEscapesRepo. Replace the repeated IsAbs, filepath.Clean+Join, filepath.Rel and `..` prefix checks in ProcessStackComponent and processLocalStackComponent with calls to validateSubdir(root, subdir) and use its returned path or error, eliminating the duplicated "%w: %q" formatting.internal/cas/content.go (1)
334-408: ⚖️ Poor tradeoffConsider extracting error cleanup into a helper to reduce complexity.
writeContentToFilehas high cognitive complexity (29) due to repeated error-cleanup patterns. The logic is correct but repetitive.A helper like
cleanupOnError(v Venv, l log.Logger, path string, err error) errorcould consolidate the repeated remove-and-warn pattern, though this is optional given the code's correctness.🤖 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 `@internal/cas/content.go` around lines 334 - 408, The writeContentToFile function duplicates the same "remove temp file + warn" cleanup in multiple error branches; extract that into a helper (suggest name cleanupOnError(v Venv, l log.Logger, tempPath string, originalErr error) error) that attempts v.FS.Remove(tempPath), logs any remove failures via l.Warnf and then returns the original wrapped error (so callers can return cleanupOnError(..., fmt.Errorf(...))). Replace the repeated blocks after buf.Write, buf.Flush, f.Close, v.FS.Chmod(tempPath,...), and v.FS.Rename with calls to this helper, ensuring the helper receives the exact tempPath and the error you want to wrap (e.g., fmt.Errorf("write to %s: %w", tempPath, err)).
🤖 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 `@internal/getter/resolver_http.go`:
- Around line 71-74: The client.Do(req) error is currently always converted to
cas.ErrNoVersionMetadata which hides caller cancellation/deadline; update the
error handling around the client.Do(req) call in resolver_http.go (the block
that currently returns cas.ErrNoVersionMetadata) to first check for context
cancellation/deadline (e.g., errors.Is(err, context.Canceled) or errors.Is(err,
context.DeadlineExceeded) or req.Context().Err() != nil) and return the original
context error in that case, and only map non-context errors to
cas.ErrNoVersionMetadata; keep the rest of the logic unchanged.
---
Nitpick comments:
In `@internal/cas/content.go`:
- Around line 334-408: The writeContentToFile function duplicates the same
"remove temp file + warn" cleanup in multiple error branches; extract that into
a helper (suggest name cleanupOnError(v Venv, l log.Logger, tempPath string,
originalErr error) error) that attempts v.FS.Remove(tempPath), logs any remove
failures via l.Warnf and then returns the original wrapped error (so callers can
return cleanupOnError(..., fmt.Errorf(...))). Replace the repeated blocks after
buf.Write, buf.Flush, f.Close, v.FS.Chmod(tempPath,...), and v.FS.Rename with
calls to this helper, ensuring the helper receives the exact tempPath and the
error you want to wrap (e.g., fmt.Errorf("write to %s: %w", tempPath, err)).
In `@internal/cas/stacks.go`:
- Around line 148-160: Both ProcessStackComponent and processLocalStackComponent
duplicate path-escape validation logic; extract that into a helper (e.g.,
validateSubdir) which takes root and subdir and returns the cleaned absolute
path or an error using ErrSourceEscapesRepo. Replace the repeated IsAbs,
filepath.Clean+Join, filepath.Rel and `..` prefix checks in
ProcessStackComponent and processLocalStackComponent with calls to
validateSubdir(root, subdir) and use its returned path or error, eliminating the
duplicated "%w: %q" formatting.
In `@internal/getter/resolver_http_test.go`:
- Around line 67-93: Add a new test (e.g.,
TestHTTPResolver_ReturnsContextCanceled) that constructs a cancellable context,
cancels it before calling r.Probe, and asserts the returned error is
context.Canceled (not cas.ErrNoVersionMetadata); specifically, use
getter.NewHTTPResolver(), create ctx, call cancel(), then call r.Probe(ctx,
srv.URL+"/mod.tgz") and require.ErrorIs(t, err, context.Canceled) to lock in
cancellation semantics.
In `@internal/getter/resolver_s3.go`:
- Around line 176-220: The function parseS3URL uses the literal "us-east-1" in
three places; introduce a package-level constant (e.g. defaultAWSRegion or
defaultRegion) and replace all occurrences in parseS3URL with that constant so
the default region is defined once and clearly named; update any tests or
callers of parseS3URL if they rely on the literal string, and ensure the new
constant is exported or unexported according to existing package conventions.
🪄 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: 795b5dc7-1c18-45ec-91a6-bff8881d6a83
📒 Files selected for processing (18)
internal/cas/cas.gointernal/cas/content.gointernal/cas/content_test.gointernal/cas/getter_test.gointernal/cas/resolver_git_test.gointernal/cas/source_test.gointernal/cas/stacks.gointernal/getter/casgetter.gointernal/getter/casgetter_detect_test.gointernal/getter/resolver.gointernal/getter/resolver_gcs.gointernal/getter/resolver_hg.gointernal/getter/resolver_hg_test.gointernal/getter/resolver_http.gointernal/getter/resolver_http_test.gointernal/getter/resolver_s3.gointernal/getter/resolver_s3_test.gotest/cas_archive_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/getter/resolver.go
- internal/getter/resolver_s3_test.go
- internal/cas/resolver_git_test.go
- internal/cas/getter_test.go
- internal/getter/resolver_hg_test.go
- internal/cas/source_test.go
- internal/getter/resolver_hg.go
- test/cas_archive_test.go
- internal/cas/content_test.go
Comment on lines
+71
to
+74
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return "", cas.ErrNoVersionMetadata | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate caller cancellation/deadline instead of converting it to cache-miss.
At Line 71-74, all client.Do errors are mapped to cas.ErrNoVersionMetadata. If the caller canceled ctx (or its deadline expired), this can incorrectly continue into fallback fetch/hash instead of stopping promptly.
Suggested minimal fix
resp, err := client.Do(req)
if err != nil {
+ if ctxErr := ctx.Err(); ctxErr != nil {
+ return "", ctxErr
+ }
return "", cas.ErrNoVersionMetadata
}📝 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
| resp, err := client.Do(req) | |
| if err != nil { | |
| return "", cas.ErrNoVersionMetadata | |
| } | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| if ctxErr := ctx.Err(); ctxErr != nil { | |
| return "", ctxErr | |
| } | |
| return "", cas.ErrNoVersionMetadata | |
| } |
🤖 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 `@internal/getter/resolver_http.go` around lines 71 - 74, The client.Do(req)
error is currently always converted to cas.ErrNoVersionMetadata which hides
caller cancellation/deadline; update the error handling around the
client.Do(req) call in resolver_http.go (the block that currently returns
cas.ErrNoVersionMetadata) to first check for context cancellation/deadline
(e.g., errors.Is(err, context.Canceled) or errors.Is(err,
context.DeadlineExceeded) or req.Context().Err() != nil) and return the original
context error in that case, and only map non-context errors to
cas.ErrNoVersionMetadata; keep the rest of the logic unchanged.
7fc8e63 to
0782a48
Compare
denis256
reviewed
May 13, 2026
| if filepath.IsAbs(subdir) { | ||
| cleanup() | ||
|
|
||
| return nil, fmt.Errorf("%w: %q", ErrSourceEscapesRepo, subdir) |
Member
There was a problem hiding this comment.
Should be returned ErrAbsoluteSource ?
Collaborator
Author
There was a problem hiding this comment.
The thing we're trying to protect against is escaping the repo, right? How strongly do you feel about this?
| if !vfs.IsOSFS(fs) { | ||
| // Panics when v.FS is not OS-backed; git only sees the real disk. | ||
| func (s *GitStore) ProbeCachedCommit(ctx context.Context, v Venv, url, rawRef string) (string, bool) { | ||
| if !vfs.IsOSFS(v.FS) { |
Member
There was a problem hiding this comment.
No check for nil v, if is passed without initialization will lead to nil dereference
Collaborator
Author
There was a problem hiding this comment.
It's never not initialized, though.
denis256
reviewed
May 13, 2026
|
|
||
| scheme := strings.ToLower(u.Scheme) | ||
| switch scheme { | ||
| case SchemeHTTP, SchemeHTTPS: |
Member
There was a problem hiding this comment.
Not sure how the paths to S3 buckets will be handled, like https://my-bucket.s3.amazonaws.com/path.zip
Looks like will go through HTTPS fetcher which most probably will fail
| return "", fmt.Errorf("parse hg URL %s: %w", rawURL, err) | ||
| } | ||
|
|
||
| rev := u.Query().Get("rev") |
Member
There was a problem hiding this comment.
I was thinking that through rev can be passed different random code ?rev = "tip ; echo pwned"
Collaborator
Author
There was a problem hiding this comment.
I'm not sure this can result in anything bad. We don't run the arg through a shell or parse it. We would just use it as a single argument to CommandContext, which would fail.
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
ba03711 to
f4b80ea
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
93d4e5a to
a6c3f0d
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
a6c3f0d to
cb76ad9
Compare
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
cb76ad9 to
dcd2fcf
Compare
denis256
previously approved these changes
May 21, 2026
Extends CAS to cover non-git sources reachable through go-getter (http(s), s3, gcs, hg, smb) so repeat fetches skip the network when the bytes haven't changed upstream. The new `SourceResolver` interface issues a cheap remote probe (`HEAD`, `GetObjectAttributes`, `hg identify`) to derive a tree-store cache key without downloading the bytes. On a probe hit, `FetchSource` links the cached tree directly. On a miss, or when a resolver has no usable signal, the matching `SourceFetcher` downloads, ingests through `IngestDirectory`, and the resulting tree is keyed by its content hash. Both paths converge on the same tree-link step the git path already used. Note that the existing `ls-remote` path has been pulled into this interface as well. Resolvers ship for: - http(s) (normalized `ETag`, falls back to `ErrNoVersionMetadata` when none) - s3 (`x-amz-checksum-*` cascade then `ETag`) - gcs (crc32c/md5Hash with optional generation pin, then ETag) - hg (`hg identify --id`) CASGetter wires them in via `WithDefaultGenericDispatch`, and `Detect` claims the schemes ahead of go-getter's bare protocol getters so archive=false propagates and pre-decompression stays disabled. Virtualised dependency handlers (`vfs.FS`, `*git.GitRunner`) come out of `CAS`, `Store`, `Content`, and `GitStore`. Every method now takes a `cas.Venv` that bundles the two; `cas.OSVenv()` builds the production binding. Telemetry: `cas_clone` is replaced by `cas_fetch_source`, carrying `scheme`, `url`, a `cache_hit` boolean (probe short-circuit vs network fetch), and the `branch` attribute for git sources. `cas_link` gains `force_copy` and `perm`.
yhakbar
force-pushed
the
feat/supporting-all-getters-in-cas
branch
from
dcd2fcf to
2b2f3ac
Compare
denis256
approved these changes
May 26, 2026
|
|
||
| out, err := client.HeadObject(ctx, input) | ||
| if err != nil { | ||
| return "", cas.ErrNoVersionMetadata |
Member
There was a problem hiding this comment.
client not closed in case of error
| } | ||
|
|
||
| q := u.Query() | ||
| if q.Get("archive") != "" { |
Member
There was a problem hiding this comment.
If user will set archive=false in url, it will not be possible to detect what is CAS and what is user
This was referenced May 26, 2026
8 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
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
Extends CAS to cover non-Git sources reachable through go-getter (http, s3, gcs, hg, smb) to that all go-getter sources supported by Terragrunt are also supported as CAS getters.
To do this, all of the non-Git getters needed some sort of cheap remote code probe implemented to assess whether the reference has moved remotely using a HEAD request, etc. The existing
ls-remotepath has been pulled into the same probe flow.As part of this, a
cas.Venvstruct was added to more conveniently plumb through bundles of virtual environment dependencies (incas, that'svfsandGitRunner). We should look to re-implement this pattern where necessary to define the external dependency boundaries for packages.TODOs
Read the Gruntwork contribution guidelines.
Summary by CodeRabbit