Merged
Conversation
Benchstat (Other)Base: ✅ 4 improvement(s)
Full benchstat output |
Benchstat (RLS)Base: 📊 1 minor regression(s) (all within 5% threshold)
✅ 2 improvement(s)
Full benchstat output |
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:
WalkthroughAdds a pluggable blob/artifact storage system: filesystem abstractions, multiple backend adapters (local, S3, GCS, Azure, SFTP, SMB), inline DB storage with optional compression, blob-store wiring into context/connection layers, logging wrappers, and unit + e2e tests with testcontainers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Context
participant Provider as BlobStoreProvider
participant Conn as ConnectionHydrator
participant FS as FilesystemRW
participant Storage
participant DB
Client->>Context: Blobs()
Context->>Context: read artifacts.connection property
alt no external connection
Context-->>Client: InlineStore (DB-backed)
else external connection
Context->>Provider: BlobStoreProvider(ctx, connURL)
Provider->>Conn: Hydrate connection by URL
Conn-->>Provider: models.Connection
Provider->>FS: GetFSForConnection(ctx, conn)
FS->>Storage: communicate with backend service (S3/GCS/Azure/SFTP/SMB/Local)
Provider->>DB: supply DB to BlobStore constructor
Provider-->>Context: Logged BlobStore wrapping FS+DB
Context-->>Client: BlobStore
end
sequenceDiagram
participant Caller
participant SaveArtifact
participant Hasher as SHA256
participant MIME as MIMEWriter
participant FS as FilesystemRW
participant Storage
participant DB
Caller->>SaveArtifact: SaveArtifact(ctx, fs, artifactModel, data)
SaveArtifact->>Hasher: stream via TeeReader (compute checksum)
SaveArtifact->>MIME: buffer prefix for detection
SaveArtifact->>FS: fs.Write(ctx, path, streamedReader)
FS->>Storage: store bytes (upload)
SaveArtifact->>MIME: Detect() -> MIME type
Hasher-->>SaveArtifact: checksum hex
SaveArtifact->>DB: db.Create(artifactModel with metadata)
DB-->>SaveArtifact: success / error
SaveArtifact-->>Caller: result / error
Possibly related PRs
🚥 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 unit tests (beta)
✨ Simplify code
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 17
🧹 Nitpick comments (5)
connection/opensearch.go (1)
138-140: Consider adding a minimum TLS version to the custom transport configuration.At Line 139, the custom
tls.Configsets onlyInsecureSkipVerifywithout specifyingMinVersion. While Go defaults to TLS 1.2 (the secure minimum), explicitly settingMinVersionstrengthens the baseline crypto posture. Set it totls.VersionTLS12for standard security, ortls.VersionTLS13if your OpenSearch deployment requires it.Suggested hardening diff (TLS 1.2)
tr = &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/opensearch.go` around lines 138 - 140, The custom transport creation uses tls.Config with only InsecureSkipVerify; update the tls.Config in the http.Transport initialization (variable tr / struct http.Transport) to explicitly set MinVersion to tls.VersionTLS12 (or tls.VersionTLS13 if your OpenSearch requires TLS1.3) so the code becomes tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12}; keep the existing InsecureSkipVerify value and ensure you import/use the tls constant names as shown.artifact/fs/gcs.go (1)
69-86: Memory concern:io.ReadAllbuffers entire artifact before upload.Unlike the S3 backend (see
artifact/fs/s3.goline 132) which streams data directly viaBody: data, this implementation buffers the entire artifact in memory. For large files, this could cause OOM issues.Consider streaming directly to the GCS writer:
♻️ Proposed streaming implementation
func (t *gcsFS) Write(ctx gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { obj := t.Client.Bucket(t.Bucket).Object(path) - content, err := io.ReadAll(data) - if err != nil { - return nil, err - } - writer := obj.NewWriter(ctx) - if _, err := writer.Write(content); err != nil { + if _, err := io.Copy(writer, data); err != nil { + _ = writer.Close() return nil, err } if err := writer.Close(); err != nil { return nil, err } return t.Stat(path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/gcs.go` around lines 69 - 86, The Write method in gcs.go (func (t *gcsFS) Write) currently uses io.ReadAll to buffer the entire reader into memory; replace that with streaming by writing directly to the GCS writer returned by obj.NewWriter(ctx) (e.g., use io.Copy(writer, data) and handle errors from the copy and Close calls), ensure writer.Close() is always called on error paths, and then return t.Stat(path) after a successful close; this avoids buffering large files in memory and matches the streaming approach used in the S3 backend.artifact/fs/s3.go (1)
45-95: Minor:maxObjectslimit counts all listed objects, not just glob-matched ones.When using glob patterns,
numObjectsFetched(line 86) increments by the totalKeyCountincluding non-matching objects. This means the limit may be reached before collectingmaxObjectsmatching results.This is likely acceptable given the high default limit (500k), but worth documenting if the behavior is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/s3.go` around lines 45 - 95, ReadDir currently increments numObjectsFetched by resp.KeyCount which counts all listed objects (matched or not), so when using glob patterns the t.maxObjects cap can be hit before collecting maxObjects matches; modify the loop in ReadDir to increment numObjectsFetched only when a matching object is appended to output (i.e., move the increment to after output = append(output, fileInfo) or track a separate matchedCount variable) and use that matched count to enforce the t.maxObjects cutoff; alternatively, if the current behavior is intentional, update the ReadDir/S3 FS documentation to explicitly state that maxObjects limits listed objects rather than matched results.tests/artifacts_test.go (1)
209-230: Minor: Describe block title doesn't match the method being tested.The describe block is titled
"context.Artifacts()"but the test callsDefaultContext.Blobs(). Consider updating the title to match.Proposed fix
- Describe("context.Artifacts()", func() { + Describe("context.Blobs()", func() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/artifacts_test.go` around lines 209 - 230, The Describe block title is wrong: it says "context.Artifacts()" but the test calls DefaultContext.Blobs(); update the Describe string to match the method being tested (e.g., "context.Blobs()" or "DefaultContext.Blobs()") so the block title reflects DefaultContext.Blobs() and keeps test intent clear.artifact/inline.go (1)
35-59: Consider:Writecreates duplicates if called with the same path.Each
Writecall creates a new artifact record viadb.Create. If the same path is written multiple times, multiple records will exist, andReadwill only return the first one found (non-deterministic without ordering). Consider using upsert semantics or documenting this as intentional append-only behavior.Option: Use upsert to replace existing artifact
- if err := s.db.Create(&artifact).Error; err != nil { + if err := s.db.Where("path = ?", path).Assign(&artifact).FirstOrCreate(&artifact).Error; err != nil { return nil, fmt.Errorf("saving inline artifact %s: %w", path, err) }Alternatively, if append-only is intentional, add ordering to
Read:- if err := s.db.Where("path = ?", path).First(&artifact).Error; err != nil { + if err := s.db.Where("path = ?", path).Order("created_at DESC").First(&artifact).Error; err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/inline.go` around lines 35 - 59, InlineStore.Write currently always calls s.db.Create which inserts a new models.Artifact record for the same path, leading to duplicates; change Write to perform an upsert instead (e.g., use GORM's OnConflict clause keyed on the Artifact.Path or find-and-update via s.db.Where("path = ?", path).Assign(...).FirstOrCreate) so writing the same path replaces the previous record, and keep the returned inlineFileInfo populated from the saved artifact; if you intentionally want append-only instead, leave Write as-is but modify the Read logic to explicitly order results (e.g., by CreatedAt DESC) so the most recent artifact is deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@artifact/clients/aws/fileinfo.go`:
- Around line 27-41: S3FileInfo.Mode currently always returns 0644 which
contradicts S3FileInfo.IsDir for keys ending in "/"—update Mode() so it checks
IsDir() and sets the directory bit (fs.ModeDir) for directories (e.g., return
fs.FileMode(0755)|fs.ModeDir for dirs, fs.FileMode(0644) for files); adjust
S3FileInfo.Mode implementation to call S3FileInfo.IsDir() (which uses
Name()/FullPath()) and include the fs.ModeDir flag when appropriate so
Mode().IsDir() matches IsDir().
In `@artifact/clients/sftp/sftp.go`:
- Around line 9-15: The SSH client configuration currently sets HostKeyCallback
to ssh.InsecureIgnoreHostKey(), which disables host verification; replace this
with a proper host-key check by using either ssh.FixedHostKey(...) with the
server's parsed public key or by building a HostKeyCallback from a known_hosts
file (e.g., using knownhosts.New(knownHostsPath)) and assign that to
ssh.ClientConfig.HostKeyCallback; update the code around the config variable and
ssh.ClientConfig instantiation to parse/load the trusted host key(s) and pass
the resulting callback instead of ssh.InsecureIgnoreHostKey().
- Around line 22-24: When sftp.NewClient(conn) returns an error the SSH
connection variable conn is left open; update the function that calls
sftp.NewClient (the block referencing conn and sftp.NewClient) to close the
underlying SSH connection on error (call conn.Close() before returning the
error) so the resource is not leaked, or use a deferred closure pattern only
after confirming client creation succeeded; ensure you reference the existing
conn variable and the sftp.NewClient call when making this change.
In `@artifact/clients/smb/smb.go`:
- Around line 36-43: d.Dial(conn) and s.Mount(share) error paths leak resources:
if d.Dial(conn) fails close conn before returning; if s.Mount(share) fails close
the newly-created session (e.g. s.Close() or s.Logoff()) and also close the
underlying conn, and clear smb.Session so the struct doesn't hold a dead
session/connection; update the error returns in the d.Dial(conn) and
s.Mount(share) branches to perform these cleanups (referencing SMBSession, conn,
d.Dial, s, and s.Mount).
- Around line 19-21: Replace the manual server + ":" + port concatenation and
plain net.Dial with a host/port join and a dial that enforces a timeout: use
net.JoinHostPort(server, port) to produce a correct host:port (handles IPv6
brackets) and call net.DialTimeout (or context with net.Dialer.Timeout) instead
of net.Dial with a reasonable timeout (e.g., 5s) when creating the SMB
connection (the code around the server variable and the net.Dial call in
smb.go).
- Around line 51-60: The Close method currently closes Conn before calling
Logoff and Umount which breaks the SMB channel; change SMBSession.Close to
perform teardown in LIFO order: if s.Share != nil call s.Umount(), then if
s.Session != nil call s.Logoff(), and finally if s.Conn != nil call
s.Conn.Close(); preserve existing nil checks and error swallowing/handling but
reorder the calls to s.Umount(), s.Logoff(), then s.Conn.Close() so
Umount/Logoff execute while the connection is still open.
In `@artifact/fs/local.go`:
- Around line 36-44: The ReadDir implementation can be exploited via path
traversal because filepath.Join alone doesn't enforce the path staying within
t.base; resolve the absolute paths for t.base and the target (e.g., using
filepath.Abs or filepath.Rel) inside localFS.ReadDir (and similarly in
localFS.Stat, localFS.Read, localFS.Write) and reject requests where the
resolved target is outside the base (e.g., filepath.Rel(baseAbs, targetAbs)
yields a path starting with ".." or the targetAbs does not have baseAbs as a
prefix); return a clear error when the check fails and only call
os.ReadDir/os.Stat/os.ReadFile/os.WriteFile after the validation.
- Around line 90-108: The Write method on localFS opens a file (f) with
os.Create but never closes it, leaking file descriptors; modify localFS.Write to
call f.Close() explicitly after io.Copy and before calling t.Stat(path), while
also keeping a defer f.Close() immediately after creating f as a safety net for
early returns, and ensure any error returned by f.Close() is checked/propagated
(i.e., if close fails, return that error instead of or in addition to prior
errors) so write/close failures aren't swallowed.
In `@artifact/fs/save.go`:
- Around line 20-31: MIMEWriter.Write currently returns (0, nil) or (rem, nil)
when it writes fewer bytes than requested, which violates the io.Writer
contract; update MIMEWriter.Write to return a non-nil error whenever n < len(bb)
(e.g., return io.ErrShortWrite) so callers can detect short writes:
specifically, in the branch where len(t.buffer) > t.Max return 0,
io.ErrShortWrite, and when rem < len(bb) return rem, io.ErrShortWrite after
appending bb[:rem] to t.buffer; keep successful full-write behavior unchanged.
In `@artifact/fs/smb.go`:
- Around line 48-59: In smbFS.Write, the file returned by s.Create(path)
(variable f) is never closed, causing potential handle leaks on errors or
success; fix by ensuring f.Close() is called: immediately after successful
Create, register a defer that closes f and captures/returns any close error if
no prior error occurred, and then proceed with io.Copy and f.Stat; reference the
smbFS.Write function, the s.Create call, the f variable, io.Copy(f, data) and
f.Stat() when applying the change.
- Around line 79-97: ReadDirGlob is setting SMBFileInfo.Base to the original
glob pattern (name) instead of the non-glob directory prefix (base), causing
incorrect FullPath() results; change the SMBFileInfo construction in ReadDirGlob
to use Base: base (the variable returned by doublestar.SplitPattern) instead of
Base: name so the returned artifact.FileInfo entries match ReadDir behavior and
produce correct full paths.
- Around line 18-20: smbFS is missing a Close() implementation required by the
FilesystemRW interface; add a pointer-receiver Close method on smbFS that
satisfies io.Closer by delegating to the embedded smb.SMBSession's Close (or
returning nil if session is nil). Locate the smbFS type and implement func (s
*smbFS) Close() error { if s.SMBSession == nil { return nil } return
s.SMBSession.Close() } so smbFS can be used as FilesystemRW.
In `@artifact/fs/ssh.go`:
- Around line 17-20: sshFS is missing the io.Closer implementation required by
the FilesystemRW interface; add a pointer-receiver Close() method on sshFS that
satisfies io.Closer by closing the embedded *sftp.Client (call the client's
Close() and return its error) so sshFS implements FilesystemRW (ensure the
method name is Close and signature is Close() error and refers to the embedded
sftp.Client).
- Around line 92-108: In sshFS.Write the file handle returned by s.Create is
never closed; add a deferred close right after successful creation of f (in
function sshFS.Write, after s.Create) to ensure the file is closed even if
io.Copy or f.Stat fails, and propagate any Close() error (wrap it with
fmt.Errorf) so close failures aren't swallowed—i.e., set defer func() { if cerr
:= f.Close(); cerr != nil { if err != nil { err = fmt.Errorf("%w; close error:
%v", err, cerr) } else { err = fmt.Errorf("close error: %w", cerr) } } }() and
return the possibly-wrapped err after io.Copy and f.Stat.
In `@connection/blob.go`:
- Around line 85-90: The SFTP branch (models.ConnectionTypeSFTP) can build an
invalid host string when c.Properties["port"] is empty; modify the logic after
url.Parse(c.URL) to determine a port fallback (use c.Properties["port"] if set,
otherwise use parsedURL.Port() if present, otherwise default to "22") and then
call artifactFS.NewSSHFS with fmt.Sprintf("%s:%s", parsedURL.Hostname(),
chosenPort) while still passing c.Username and c.Password; ensure you handle the
case where parsedURL.Host already includes a port by preferring that value.
In `@context/blobs.go`:
- Around line 16-18: The current branch that checks connURL == "" ||
BlobStoreProvider == nil silently returns artifact.NewInlineStore(k.DB()) when a
connection URL is configured but BlobStoreProvider is nil; change this so that
if connURL != "" and BlobStoreProvider == nil you return an explicit error (e.g.
fmt.Errorf("artifacts.connection configured but BlobStoreProvider is nil"))
instead of creating an inline store; update the return to nil, error and ensure
the function returns that error path (adjust callers/tests if needed); locate
this logic around the connURL / BlobStoreProvider check and replace the
artifact.NewInlineStore(k.DB()) fallback with the explicit error return.
In `@schema/artifacts.hcl`:
- Around line 50-54: Add a CHECK constraint on the column "compression_type" in
the artifacts.hcl schema so only the supported values 'gzip', 'zstd', or 'none'
can be persisted; update the column definition for compression_type to include a
constraint (e.g., CHECK(compression_type IN (...))) so invalid text values are
rejected at the schema level and the comment remains accurate.
---
Nitpick comments:
In `@artifact/fs/gcs.go`:
- Around line 69-86: The Write method in gcs.go (func (t *gcsFS) Write)
currently uses io.ReadAll to buffer the entire reader into memory; replace that
with streaming by writing directly to the GCS writer returned by
obj.NewWriter(ctx) (e.g., use io.Copy(writer, data) and handle errors from the
copy and Close calls), ensure writer.Close() is always called on error paths,
and then return t.Stat(path) after a successful close; this avoids buffering
large files in memory and matches the streaming approach used in the S3 backend.
In `@artifact/fs/s3.go`:
- Around line 45-95: ReadDir currently increments numObjectsFetched by
resp.KeyCount which counts all listed objects (matched or not), so when using
glob patterns the t.maxObjects cap can be hit before collecting maxObjects
matches; modify the loop in ReadDir to increment numObjectsFetched only when a
matching object is appended to output (i.e., move the increment to after output
= append(output, fileInfo) or track a separate matchedCount variable) and use
that matched count to enforce the t.maxObjects cutoff; alternatively, if the
current behavior is intentional, update the ReadDir/S3 FS documentation to
explicitly state that maxObjects limits listed objects rather than matched
results.
In `@artifact/inline.go`:
- Around line 35-59: InlineStore.Write currently always calls s.db.Create which
inserts a new models.Artifact record for the same path, leading to duplicates;
change Write to perform an upsert instead (e.g., use GORM's OnConflict clause
keyed on the Artifact.Path or find-and-update via s.db.Where("path = ?",
path).Assign(...).FirstOrCreate) so writing the same path replaces the previous
record, and keep the returned inlineFileInfo populated from the saved artifact;
if you intentionally want append-only instead, leave Write as-is but modify the
Read logic to explicitly order results (e.g., by CreatedAt DESC) so the most
recent artifact is deterministic.
In `@connection/opensearch.go`:
- Around line 138-140: The custom transport creation uses tls.Config with only
InsecureSkipVerify; update the tls.Config in the http.Transport initialization
(variable tr / struct http.Transport) to explicitly set MinVersion to
tls.VersionTLS12 (or tls.VersionTLS13 if your OpenSearch requires TLS1.3) so the
code becomes tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12};
keep the existing InsecureSkipVerify value and ensure you import/use the tls
constant names as shown.
In `@tests/artifacts_test.go`:
- Around line 209-230: The Describe block title is wrong: it says
"context.Artifacts()" but the test calls DefaultContext.Blobs(); update the
Describe string to match the method being tested (e.g., "context.Blobs()" or
"DefaultContext.Blobs()") so the block title reflects DefaultContext.Blobs() and
keeps test intent clear.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db8cbc89-9faa-48d8-9cdb-800228ae320e
📒 Files selected for processing (31)
artifact/clients/aws/doc.goartifact/clients/aws/fileinfo.goartifact/clients/gcp/fileinfo.goartifact/clients/sftp/sftp.goartifact/clients/smb/smb.goartifact/fs.goartifact/fs/gcs.goartifact/fs/local.goartifact/fs/s3.goartifact/fs/save.goartifact/fs/smb.goartifact/fs/ssh.goartifact/inline.goconnection/aws.goconnection/blob.goconnection/cnrm.goconnection/eks.goconnection/gcs.goconnection/gke.goconnection/http.goconnection/kubernetes.goconnection/opensearch.goconnection/prometheus.gocontext/blobs.gocontext/context.gogo.modlogs/opensearch/search.gomodels/artifacts.gomodels/artifacts_test.goschema/artifacts.hcltests/artifacts_test.go
Comment on lines
+27
to
+41
| func (obj S3FileInfo) Mode() fs.FileMode { | ||
| return fs.FileMode(0644) | ||
| } | ||
|
|
||
| func (obj S3FileInfo) ModTime() time.Time { | ||
| return lo.FromPtr(obj.Object.LastModified) | ||
| } | ||
|
|
||
| func (obj S3FileInfo) FullPath() string { | ||
| return *obj.Object.Key | ||
| } | ||
|
|
||
| func (obj S3FileInfo) IsDir() bool { | ||
| return strings.HasSuffix(obj.Name(), "/") | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l artifact/clients/aws/fileinfo.goRepository: flanksource/duty
Length of output: 95
🏁 Script executed:
cat -n artifact/clients/aws/fileinfo.goRepository: flanksource/duty
Length of output: 1128
🏁 Script executed:
# Search for any imports and interface implementations in the file
head -30 artifact/clients/aws/fileinfo.goRepository: flanksource/duty
Length of output: 507
🏁 Script executed:
# Check if S3FileInfo implements fs.FileInfo interface
rg -A 2 "type S3FileInfo" artifact/clients/aws/fileinfo.goRepository: flanksource/duty
Length of output: 107
🏁 Script executed:
# Check Name() method implementation
rg -A 3 "func \(obj S3FileInfo\) Name\(\)" artifact/clients/aws/fileinfo.goRepository: flanksource/duty
Length of output: 124
🏁 Script executed:
# Search for any usage of S3FileInfo Mode() or IsDir()
rg "Mode\(\)|IsDir\(\)" artifact/clients/aws/ -B 2 -A 2Repository: flanksource/duty
Length of output: 551
🏁 Script executed:
# Search for usage of S3FileInfo in the codebase
rg "S3FileInfo" --type go -B 2 -A 2Repository: flanksource/duty
Length of output: 2109
🏁 Script executed:
# Search for patterns checking Mode().IsDir() in the codebase
rg "Mode\(\)\.IsDir|\.IsDir\(\)" --type go -B 2 -A 2 | head -50Repository: flanksource/duty
Length of output: 625
🏁 Script executed:
# Check if there are any tests for S3FileInfo
fd -e test.go artifact/clients/awsRepository: flanksource/duty
Length of output: 445
🏁 Script executed:
# Search for tests in artifact/clients/aws directory
fd . artifact/clients/aws --type fRepository: flanksource/duty
Length of output: 120
🏁 Script executed:
# Check artifact/fs/s3.go to see how S3FileInfo is used
sed -n '1,50p' artifact/fs/s3.goRepository: flanksource/duty
Length of output: 1049
🏁 Script executed:
# Look for more context on how S3FileInfo is used
rg -B 5 -A 10 "S3FileInfo{" artifact/fs/s3.goRepository: flanksource/duty
Length of output: 145
🏁 Script executed:
# Check if Mode() is used directly with S3FileInfo anywhere
rg "fileInfo.*Mode|\.Mode\(\)" artifact/ --type go -B 2 -A 2Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
# Search for S3FileInfo usage with proper escaping
rg "S3FileInfo\{" artifact/ --type go -B 5 -A 5Repository: flanksource/duty
Length of output: 915
🏁 Script executed:
# Look at the full s3.go to understand usage context
wc -l artifact/fs/s3.goRepository: flanksource/duty
Length of output: 81
🏁 Script executed:
# Get more of s3.go to see how S3FileInfo is used
sed -n '40,120p' artifact/fs/s3.goRepository: flanksource/duty
Length of output: 1848
🏁 Script executed:
# Check if there's any code that relies on Mode().IsDir() matching IsDir()
rg "Mode\(\).*IsDir|IsDir.*Mode\(\)" --type goRepository: flanksource/duty
Length of output: 42
🏁 Script executed:
# Check the artifact.FileInfo type to confirm it extends fs.FileInfo
rg "interface.*FileInfo|type.*FileInfo" artifact/clients/aws/ artifact/fs/ --type go -B 2 -A 5Repository: flanksource/duty
Length of output: 1229
🏁 Script executed:
# Look at artifact package to understand FileInfo interface
fd . artifact/ -name "*.go" -type f | head -20Repository: flanksource/duty
Length of output: 290
🏁 Script executed:
# Search for artifact.FileInfo definition
rg "type.*FileInfo|interface.*FileInfo" artifact/ --type go -B 2 -A 5 | head -40Repository: flanksource/duty
Length of output: 1223
🏁 Script executed:
# Check if IsDir() or Mode() are actually used on S3FileInfo in the codebase
rg "fileInfo.*IsDir|fileInfo.*Mode" artifact/ --type go -B 3 -A 3Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
# Look at how ReadDir output is used
rg "ReadDir" artifact/ --type go -B 2 -A 5 | head -60Repository: flanksource/duty
Length of output: 2556
Mode() is inconsistent with IsDir() for directory keys.
For keys ending in /, IsDir() is true but Mode() does not include fs.ModeDir, violating the fs.FileInfo interface contract where Mode().IsDir() should be consistent with IsDir().
Suggested fix
func (obj S3FileInfo) Mode() fs.FileMode {
- return fs.FileMode(0644)
+ if obj.IsDir() {
+ return fs.ModeDir | 0o755
+ }
+ return 0o644
}📝 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
| func (obj S3FileInfo) Mode() fs.FileMode { | |
| return fs.FileMode(0644) | |
| } | |
| func (obj S3FileInfo) ModTime() time.Time { | |
| return lo.FromPtr(obj.Object.LastModified) | |
| } | |
| func (obj S3FileInfo) FullPath() string { | |
| return *obj.Object.Key | |
| } | |
| func (obj S3FileInfo) IsDir() bool { | |
| return strings.HasSuffix(obj.Name(), "/") | |
| } | |
| func (obj S3FileInfo) Mode() fs.FileMode { | |
| if obj.IsDir() { | |
| return fs.ModeDir | 0o755 | |
| } | |
| return 0o644 | |
| } | |
| func (obj S3FileInfo) ModTime() time.Time { | |
| return lo.FromPtr(obj.Object.LastModified) | |
| } | |
| func (obj S3FileInfo) FullPath() string { | |
| return *obj.Object.Key | |
| } | |
| func (obj S3FileInfo) IsDir() bool { | |
| return strings.HasSuffix(obj.Name(), "/") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/clients/aws/fileinfo.go` around lines 27 - 41, S3FileInfo.Mode
currently always returns 0644 which contradicts S3FileInfo.IsDir for keys ending
in "/"—update Mode() so it checks IsDir() and sets the directory bit
(fs.ModeDir) for directories (e.g., return fs.FileMode(0755)|fs.ModeDir for
dirs, fs.FileMode(0644) for files); adjust S3FileInfo.Mode implementation to
call S3FileInfo.IsDir() (which uses Name()/FullPath()) and include the
fs.ModeDir flag when appropriate so Mode().IsDir() matches IsDir().
Comment on lines
+9
to
+15
| config := &ssh.ClientConfig{ | ||
| User: user, | ||
| Auth: []ssh.AuthMethod{ | ||
| ssh.Password(password), | ||
| }, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant lines
if [ -f "artifact/clients/sftp/sftp.go" ]; then
echo "=== Lines 1-30 of artifact/clients/sftp/sftp.go ==="
head -30 artifact/clients/sftp/sftp.go | cat -n
else
echo "File not found, searching for sftp.go files..."
find . -name "sftp.go" -type f
fiRepository: flanksource/duty
Length of output: 787
🏁 Script executed:
# Also check total lines in the file to understand context
wc -l artifact/clients/sftp/sftp.go 2>/dev/null || echo "File not accessible"Repository: flanksource/duty
Length of output: 92
🏁 Script executed:
# Search for any host key validation or certificate pinning elsewhere in the codebase
rg "HostKeyCallback|known_hosts|InsecureIgnoreHostKey" --type go -B 2 -A 2Repository: flanksource/duty
Length of output: 547
Do not disable SSH host key verification.
ssh.InsecureIgnoreHostKey() disables host authenticity verification and enables man-in-the-middle attacks. Use ssh.FixedHostKey() to pin a known host key, or implement a proper host key callback (e.g., reading from a known_hosts file).
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 14-14: Use of insecure HostKeyCallback implementation
Configuring SSH ClientConfig with insecure HostKeyCallback implementation from this source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/clients/sftp/sftp.go` around lines 9 - 15, The SSH client
configuration currently sets HostKeyCallback to ssh.InsecureIgnoreHostKey(),
which disables host verification; replace this with a proper host-key check by
using either ssh.FixedHostKey(...) with the server's parsed public key or by
building a HostKeyCallback from a known_hosts file (e.g., using
knownhosts.New(knownHostsPath)) and assign that to
ssh.ClientConfig.HostKeyCallback; update the code around the config variable and
ssh.ClientConfig instantiation to parse/load the trusted host key(s) and pass
the resulting callback instead of ssh.InsecureIgnoreHostKey().
Comment on lines
+19
to
+21
| server = server + ":" + port | ||
| conn, err := net.Dial("tcp", server) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n artifact/clients/smb/smb.go | head -40Repository: flanksource/duty
Length of output: 1031
🏁 Script executed:
# Check the broader context around the SMB client
rg -B5 -A10 'net.Dial' artifact/clients/smb/smb.goRepository: flanksource/duty
Length of output: 428
🏁 Script executed:
# Check for any timeout handling in the file
rg -i 'timeout|context' artifact/clients/smb/smb.goRepository: flanksource/duty
Length of output: 42
🏁 Script executed:
# Verify IPv6 formatting issue with string concatenation
python3 << 'EOF'
# IPv6 address example
ipv6_addr = "2001:db8::1"
port = "445"
# What the current code does (wrong)
wrong = ipv6_addr + ":" + port
print(f"Current approach (WRONG): {wrong}")
print(f" Issue: Missing brackets, format is invalid for IPv6")
# What net.JoinHostPort does (correct)
# In Go, net.JoinHostPort adds brackets for IPv6
correct = f"[{ipv6_addr}]:{port}"
print(f"net.JoinHostPort result (CORRECT): {correct}")
# IPv4 example to show it works for both
ipv4_addr = "192.168.1.1"
ipv4_wrong = ipv4_addr + ":" + port
print(f"\nIPv4 with current approach: {ipv4_wrong} (happens to work)")
ipv4_correct = f"{ipv4_addr}:{port}"
print(f"IPv4 with JoinHostPort: {ipv4_correct} (also works)")
EOFRepository: flanksource/duty
Length of output: 324
🏁 Script executed:
# Check Go stdlib documentation references for best practices
rg -i 'joinhostport|dialtimeout' --type go | head -20Repository: flanksource/duty
Length of output: 42
Use net.JoinHostPort and a dial timeout for SMB connect.
String concatenation with ":" breaks IPv6 host formatting—IPv6 addresses require brackets (e.g., [2001:db8::1]:445), which string concatenation does not provide. Additionally, net.Dial without an explicit timeout can cause indefinite blocking if the network becomes unresponsive.
Suggested fix
- server = server + ":" + port
- conn, err := net.Dial("tcp", server)
+ addr := net.JoinHostPort(server, port)
+ conn, err := net.DialTimeout("tcp", addr, 10*time.Second)📝 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
| server = server + ":" + port | |
| conn, err := net.Dial("tcp", server) | |
| if err != nil { | |
| addr := net.JoinHostPort(server, port) | |
| conn, err := net.DialTimeout("tcp", addr, 10*time.Second) | |
| if err != nil { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/clients/smb/smb.go` around lines 19 - 21, Replace the manual server
+ ":" + port concatenation and plain net.Dial with a host/port join and a dial
that enforces a timeout: use net.JoinHostPort(server, port) to produce a correct
host:port (handles IPv6 brackets) and call net.DialTimeout (or context with
net.Dialer.Timeout) instead of net.Dial with a reasonable timeout (e.g., 5s)
when creating the SMB connection (the code around the server variable and the
net.Dial call in smb.go).
Comment on lines
+92
to
+108
| func (s *sshFS) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { | ||
| dir := filepath.Dir(path) | ||
| if err := s.MkdirAll(dir); err != nil { | ||
| return nil, fmt.Errorf("error creating directory: %w", err) | ||
| } | ||
|
|
||
| f, err := s.Create(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating file: %w", err) | ||
| } | ||
|
|
||
| if _, err = io.Copy(f, data); err != nil { | ||
| return nil, fmt.Errorf("error writing to file: %w", err) | ||
| } | ||
|
|
||
| return f.Stat() | ||
| } |
Contributor
There was a problem hiding this comment.
File handle not explicitly closed after write.
The created file f is never closed. If io.Copy or f.Stat() fails, the file handle leaks. This can exhaust file descriptors on the remote SFTP server under load.
Proposed fix
func (s *sshFS) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) {
dir := filepath.Dir(path)
if err := s.MkdirAll(dir); err != nil {
return nil, fmt.Errorf("error creating directory: %w", err)
}
f, err := s.Create(path)
if err != nil {
return nil, fmt.Errorf("error creating file: %w", err)
}
+ defer f.Close()
if _, err = io.Copy(f, data); err != nil {
return nil, fmt.Errorf("error writing to file: %w", err)
}
return f.Stat()
}📝 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
| func (s *sshFS) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { | |
| dir := filepath.Dir(path) | |
| if err := s.MkdirAll(dir); err != nil { | |
| return nil, fmt.Errorf("error creating directory: %w", err) | |
| } | |
| f, err := s.Create(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("error creating file: %w", err) | |
| } | |
| if _, err = io.Copy(f, data); err != nil { | |
| return nil, fmt.Errorf("error writing to file: %w", err) | |
| } | |
| return f.Stat() | |
| } | |
| func (s *sshFS) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { | |
| dir := filepath.Dir(path) | |
| if err := s.MkdirAll(dir); err != nil { | |
| return nil, fmt.Errorf("error creating directory: %w", err) | |
| } | |
| f, err := s.Create(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("error creating file: %w", err) | |
| } | |
| defer f.Close() | |
| if _, err = io.Copy(f, data); err != nil { | |
| return nil, fmt.Errorf("error writing to file: %w", err) | |
| } | |
| return f.Stat() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/fs/ssh.go` around lines 92 - 108, In sshFS.Write the file handle
returned by s.Create is never closed; add a deferred close right after
successful creation of f (in function sshFS.Write, after s.Create) to ensure the
file is closed even if io.Copy or f.Stat fails, and propagate any Close() error
(wrap it with fmt.Errorf) so close failures aren't swallowed—i.e., set defer
func() { if cerr := f.Close(); cerr != nil { if err != nil { err =
fmt.Errorf("%w; close error: %v", err, cerr) } else { err = fmt.Errorf("close
error: %w", cerr) } } }() and return the possibly-wrapped err after io.Copy and
f.Stat.
Comment on lines
+85
to
+90
| case models.ConnectionTypeSFTP: | ||
| parsedURL, err := url.Parse(c.URL) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, c.Properties["port"]), c.Username, c.Password) |
Contributor
There was a problem hiding this comment.
Missing default port for SFTP connections.
If c.Properties["port"] is empty, the host string becomes "hostname:" which is invalid. SFTP typically uses port 22 by default.
🔧 Proposed fix
case models.ConnectionTypeSFTP:
parsedURL, err := url.Parse(c.URL)
if err != nil {
return nil, err
}
- return artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, c.Properties["port"]), c.Username, c.Password)
+ port := c.Properties["port"]
+ if port == "" {
+ port = "22"
+ }
+ return artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, port), c.Username, c.Password)📝 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
| case models.ConnectionTypeSFTP: | |
| parsedURL, err := url.Parse(c.URL) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, c.Properties["port"]), c.Username, c.Password) | |
| case models.ConnectionTypeSFTP: | |
| parsedURL, err := url.Parse(c.URL) | |
| if err != nil { | |
| return nil, err | |
| } | |
| port := c.Properties["port"] | |
| if port == "" { | |
| port = "22" | |
| } | |
| return artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, port), c.Username, c.Password) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/blob.go` around lines 85 - 90, The SFTP branch
(models.ConnectionTypeSFTP) can build an invalid host string when
c.Properties["port"] is empty; modify the logic after url.Parse(c.URL) to
determine a port fallback (use c.Properties["port"] if set, otherwise use
parsedURL.Port() if present, otherwise default to "22") and then call
artifactFS.NewSSHFS with fmt.Sprintf("%s:%s", parsedURL.Hostname(), chosenPort)
while still passing c.Username and c.Password; ensure you handle the case where
parsedURL.Host already includes a port by preferring that value.
Comment on lines
+50
to
+54
| column "compression_type" { | ||
| null = true | ||
| type = text | ||
| comment = "compression algorithm applied to content: gzip, zstd, or none" | ||
| } |
Contributor
There was a problem hiding this comment.
Constrain compression_type to supported values at schema level.
The column comment defines allowed values, but the schema accepts arbitrary text. Add a CHECK constraint to prevent invalid persisted states.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schema/artifacts.hcl` around lines 50 - 54, Add a CHECK constraint on the
column "compression_type" in the artifacts.hcl schema so only the supported
values 'gzip', 'zstd', or 'none' can be persisted; update the column definition
for compression_type to include a constraint (e.g., CHECK(compression_type IN
(...))) so invalid text values are rejected at the schema level and the comment
remains accurate.
moshloop
force-pushed
the
pr/inline-artifacts
branch
from
fd6ab3f to
5d0cbc7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
artifact/clients/sftp/sftp.go (1)
8-15:⚠️ Potential issue | 🔴 CriticalCritical:
ssh.InsecureIgnoreHostKey()disables host verification.This enables man-in-the-middle attacks. Consider accepting a host key or known_hosts path as a parameter, or at minimum document this security trade-off clearly.
Suggested approach
// Option 1: Accept a known host key func SSHConnect(host, user, password string, hostKey ssh.PublicKey) (*sftp.Client, error) { config := &ssh.ClientConfig{ User: user, Auth: []ssh.AuthMethod{ssh.Password(password)}, HostKeyCallback: ssh.FixedHostKey(hostKey), } // ... } // Option 2: Use known_hosts file import "golang.org/x/crypto/ssh/knownhosts" func SSHConnect(host, user, password, knownHostsPath string) (*sftp.Client, error) { hostKeyCallback, err := knownhosts.New(knownHostsPath) if err != nil { return nil, err } config := &ssh.ClientConfig{ User: user, Auth: []ssh.AuthMethod{ssh.Password(password)}, HostKeyCallback: hostKeyCallback, } // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/clients/sftp/sftp.go` around lines 8 - 15, The SSHConnect function currently uses ssh.InsecureIgnoreHostKey(), which disables host verification; replace this by accepting and using a real host verification mechanism: update SSHConnect to take either a hostKey (ssh.PublicKey) and use ssh.FixedHostKey(hostKey) or take a knownHostsPath and create a callback via knownhosts.New(knownHostsPath), then assign that to ssh.ClientConfig.HostKeyCallback instead of ssh.InsecureIgnoreHostKey(); if you must retain an opt-in insecure mode, make it explicit via a parameter (e.g., allowInsecure bool) and document the security trade-off in the function comment.
🧹 Nitpick comments (6)
artifact/fs/gcs.go (1)
32-34: Consider accepting context parameter forReadDirandStat.
ReadDirandStatusecontext.TODO()whileReadandWriteaccept a context parameter. This inconsistency prevents callers from controlling timeouts or cancellation for listing/stat operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/gcs.go` around lines 32 - 34, ReadDir and Stat currently use gocontext.TODO() causing inconsistent cancellation behavior; change their signatures to accept a context parameter (e.g., ReadDir(ctx context.Context, name string) and Stat(ctx context.Context, name string)) and thread that ctx into the gcs client calls (replace gocontext.TODO() with the passed ctx) so callers can control timeouts/cancellation; update any callers and tests that invoke gcsFS.ReadDir/Stat and mirror the same context pattern used by Read and Write.artifact/fs/azure.go (2)
102-105:SaveArtifactInlineis not Azure-specific; consider relocating.This helper function is generic and doesn't use any Azure-specific code. It belongs in the parent
artifactpackage orartifact/fspackage's common utilities file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/azure.go` around lines 102 - 105, SaveArtifactInline is a generic helper (uses artifact.FilesystemRW) and should be moved out of the Azure-specific file: extract the function SaveArtifactInline from artifact/fs/azure.go and place it into a common utilities file in the artifact package or artifact/fs (e.g., artifact/fs/util.go), update its package declaration and imports accordingly, then update all call sites to import the new package location (or use the same package if moved within artifact/fs) and remove the Azure-specific copy from azure.go so there is a single canonical implementation.
60-60: Use passed context instead ofgocontext.TODO().
ReadDirshould accept and propagate a context parameter for cancellation support, or at minimum usegocontext.Background()with a comment explaining why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/azure.go` at line 60, The call to pager.NextPage currently uses gocontext.TODO(); update the surrounding function (e.g., ReadDir) to accept a context.Context parameter and pass that context into pager.NextPage(ctx) so cancellations propagate, or if changing the signature is impossible, use gocontext.Background() with a clear comment explaining why and why TODO is inappropriate; locate the pager.NextPage(gocontext.TODO()) invocation in azure.go and replace it to use the function's ctx (or Background() + comment) and propagate the ctx through any callers as needed.models/artifacts.go (1)
112-119: Minor: Variable nameprefixis misleading for exact-match comparison.The loop variable is named
prefixbut the comparison uses exact equality (ct == prefix), not prefix matching. Consider renaming tosupportedorallowedfor clarity.func isImageContentType(ct string) bool { - for _, prefix := range []string{"image/png", "image/jpeg", "image/gif", "image/webp", "image/svg"} { - if ct == prefix { + for _, supported := range []string{"image/png", "image/jpeg", "image/gif", "image/webp", "image/svg"} { + if ct == supported { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/artifacts.go` around lines 112 - 119, In isImageContentType(ct string) the loop variable name "prefix" is misleading because the code performs exact equality checks; rename the loop variable to something like "supported" or "allowed" (or change the slice to a map and iterate keys) so the intent matches the implementation, e.g., update the range loop over the slice of content types and replace "prefix" with "supported" in the comparison and any related references.tests/e2e-blobs/containers.go (1)
96-96: Global environment mutation may affect parallel tests.
os.Setenv("STORAGE_EMULATOR_HOST", ...)modifies the process-wide environment. If tests run in parallel or other code checks this env var, it could cause interference.Consider documenting this side effect or scoping it if the GCS client supports explicit endpoint configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-blobs/containers.go` at line 96, The test currently sets the process-wide env var via os.Setenv("STORAGE_EMULATOR_HOST", ...) which can leak into parallel tests; replace this with a scoped approach such as using testing.T's t.Setenv(...) inside the test to automatically restore the previous value, or avoid mutating env at all by configuring the GCS client directly (use storage.NewClient(ctx, option.WithEndpoint(emulatorHost), ...) or the equivalent client constructor) so you no longer need os.Setenv; also add a short comment documenting the previous side-effect if you keep any global change.artifact/inline.go (1)
16-112: Add doc comments for exportedInlineStoreAPI.Exported type/methods are missing Go doc comments, which hurts discoverability and lint hygiene.
As per coding guidelines, "Comments about functions are considered good practice in Go even if they are self-explanatory".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/inline.go` around lines 16 - 112, Add Go doc comments for the exported InlineStore API: add a package-level comment where appropriate and a leading sentence comment for the type InlineStore and each exported method/function (NewInlineStore, WithCompression, WithMaxSize, Write, Read, ReadDir, Stat, Close). Ensure each comment starts with the exact symbol name (e.g., "InlineStore ..." or "NewInlineStore ..."), briefly describes purpose/behavior and any notable parameters/return semantics, and follows Go conventions (complete sentence, period). Place comments immediately above the corresponding declarations so linters and godoc pick them up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@artifact/fs/gcs.go`:
- Around line 69-85: In gcsFS.Write, avoid buffering with io.ReadAll and ensure
the writer is always closed: replace reading the entire reader into memory with
streaming via io.Copy from the provided data to obj.NewWriter(ctx) so large
files don't OOM; create the writer once, defer a close that handles and
preserves any write error (call writer.Close() in a deferred function and if
Close returns an error and no prior error, return that; if there was a write
error, still attempt Close and return the original write error or wrap both).
Target the gcsFS.Write function, obj.NewWriter, writer.Write and writer.Close
for these changes.
In `@artifact/inline.go`:
- Around line 36-37: The Write and Read methods currently ignore the supplied
gocontext.Context (they use a blank identifier), so GORM calls don't honor
cancellations or deadlines; update signatures to name the context parameter
(e.g., ctx gocontext.Context) and pass it into all GORM operations via
db.WithContext(ctx) or tx.WithContext(ctx) used in InlineStore.Write and
InlineStore.Read (and any internal calls like Create, Save, First, Where,
Delete) so the database calls observe the provided context.
- Around line 36-48: InlineStore.Write currently calls io.ReadAll on the
incoming reader unbounded, which can OOM before artifact.SetContent enforces
maxSize; to fix, wrap the incoming data reader with an io.LimitReader (or an
io.LimitedReader) using s.maxSize+1 (or an appropriate uncompressed byte limit)
and read from that limited reader so you can detect and return a clear "payload
too large" error if the limit is exceeded before calling artifact.SetContent;
update InlineStore.Write to use the limited reader, still pass the resulting raw
bytes into artifact.SetContent (which will continue to enforce compressed size),
and reference InlineStore.Write, artifact.SetContent, and the
s.maxSize/s.compression fields when making the change.
- Around line 78-83: The current ReadDir logic builds SQL LIKE patterns by only
replacing '*' with '%', which fails to escape literal '%' and '_' and
backslashes; add a helper function (e.g., toLikePattern) that iterates runes,
converts '*'→'%', and escapes '%', '_' and '\\' by prefixing with '\\', then
appends a trailing '%' if the pattern contains no '%' and use
toLikePattern(name) in place of strings.ReplaceAll(name, "*", "%") before
calling s.db.Where("path LIKE ?", pattern) in the ReadDir/inline listing path.
In `@connection/blob.go`:
- Around line 99-108: The Azure connection code for models.ConnectionTypeAzure
hardcodes DefaultEndpointsProtocol=https while using c.URL for BlobEndpoint,
which can mismatch for HTTP endpoints (e.g., Azurite); update the code that
calls azblob.NewClientFromConnectionString to derive the protocol from c.URL (or
accept a configurable protocol property) and build the connection string using
that protocol (not always "https"), then pass the client to
artifactFS.NewAzureBlobFS with the existing container value so HTTP endpoints
work correctly.
In `@go.mod`:
- Line 101: Update the grpc module version in go.mod to a patched release to
address the authorization bypass (GHSA-p77j-4mvh-x3m3): replace the existing
module line "google.golang.org/grpc v1.79.1" with "google.golang.org/grpc
v1.79.3" (or v1.80.0) and run go mod tidy to refresh go.sum; verify builds and
tests pass after updating references to the grpc module.
- Line 123: Update the module requirement for github.com/docker/docker to
v29.3.1 or later to address the reported vulnerabilities: change the go.mod
entry for "github.com/docker/docker" to at least v29.3.1, then run the module
update commands (e.g., go get github.com/docker/docker@v29.3.1 and go mod tidy)
to refresh go.sum and ensure dependency graph integrity; finally run tests/build
to verify no regressions.
In `@tests/e2e-blobs/containers.go`:
- Around line 82-84: The calls to container.Host and container.MappedPort are
ignoring errors which can produce a malformed emulatorHost; update the code
around Host, MappedPort and emulatorHost to check and handle both returned
errors (e.g., if err != nil) instead of discarding them—log or fail the test
(t.Fatalf or return the error) with a clear message including the underlying
error and avoid constructing emulatorHost when either call fails so downstream
logic won't receive a bad address.
---
Duplicate comments:
In `@artifact/clients/sftp/sftp.go`:
- Around line 8-15: The SSHConnect function currently uses
ssh.InsecureIgnoreHostKey(), which disables host verification; replace this by
accepting and using a real host verification mechanism: update SSHConnect to
take either a hostKey (ssh.PublicKey) and use ssh.FixedHostKey(hostKey) or take
a knownHostsPath and create a callback via knownhosts.New(knownHostsPath), then
assign that to ssh.ClientConfig.HostKeyCallback instead of
ssh.InsecureIgnoreHostKey(); if you must retain an opt-in insecure mode, make it
explicit via a parameter (e.g., allowInsecure bool) and document the security
trade-off in the function comment.
---
Nitpick comments:
In `@artifact/fs/azure.go`:
- Around line 102-105: SaveArtifactInline is a generic helper (uses
artifact.FilesystemRW) and should be moved out of the Azure-specific file:
extract the function SaveArtifactInline from artifact/fs/azure.go and place it
into a common utilities file in the artifact package or artifact/fs (e.g.,
artifact/fs/util.go), update its package declaration and imports accordingly,
then update all call sites to import the new package location (or use the same
package if moved within artifact/fs) and remove the Azure-specific copy from
azure.go so there is a single canonical implementation.
- Line 60: The call to pager.NextPage currently uses gocontext.TODO(); update
the surrounding function (e.g., ReadDir) to accept a context.Context parameter
and pass that context into pager.NextPage(ctx) so cancellations propagate, or if
changing the signature is impossible, use gocontext.Background() with a clear
comment explaining why and why TODO is inappropriate; locate the
pager.NextPage(gocontext.TODO()) invocation in azure.go and replace it to use
the function's ctx (or Background() + comment) and propagate the ctx through any
callers as needed.
In `@artifact/fs/gcs.go`:
- Around line 32-34: ReadDir and Stat currently use gocontext.TODO() causing
inconsistent cancellation behavior; change their signatures to accept a context
parameter (e.g., ReadDir(ctx context.Context, name string) and Stat(ctx
context.Context, name string)) and thread that ctx into the gcs client calls
(replace gocontext.TODO() with the passed ctx) so callers can control
timeouts/cancellation; update any callers and tests that invoke
gcsFS.ReadDir/Stat and mirror the same context pattern used by Read and Write.
In `@artifact/inline.go`:
- Around line 16-112: Add Go doc comments for the exported InlineStore API: add
a package-level comment where appropriate and a leading sentence comment for the
type InlineStore and each exported method/function (NewInlineStore,
WithCompression, WithMaxSize, Write, Read, ReadDir, Stat, Close). Ensure each
comment starts with the exact symbol name (e.g., "InlineStore ..." or
"NewInlineStore ..."), briefly describes purpose/behavior and any notable
parameters/return semantics, and follows Go conventions (complete sentence,
period). Place comments immediately above the corresponding declarations so
linters and godoc pick them up.
In `@models/artifacts.go`:
- Around line 112-119: In isImageContentType(ct string) the loop variable name
"prefix" is misleading because the code performs exact equality checks; rename
the loop variable to something like "supported" or "allowed" (or change the
slice to a map and iterate keys) so the intent matches the implementation, e.g.,
update the range loop over the slice of content types and replace "prefix" with
"supported" in the comparison and any related references.
In `@tests/e2e-blobs/containers.go`:
- Line 96: The test currently sets the process-wide env var via
os.Setenv("STORAGE_EMULATOR_HOST", ...) which can leak into parallel tests;
replace this with a scoped approach such as using testing.T's t.Setenv(...)
inside the test to automatically restore the previous value, or avoid mutating
env at all by configuring the GCS client directly (use storage.NewClient(ctx,
option.WithEndpoint(emulatorHost), ...) or the equivalent client constructor) so
you no longer need os.Setenv; also add a short comment documenting the previous
side-effect if you keep any global change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52c6d1a6-d464-43be-8849-237ba5b9067c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.github/workflows/test.yamlMakefileartifact/clients/aws/doc.goartifact/clients/aws/fileinfo.goartifact/clients/azure/fileinfo.goartifact/clients/gcp/fileinfo.goartifact/clients/sftp/sftp.goartifact/clients/smb/smb.goartifact/fs.goartifact/fs/azure.goartifact/fs/gcs.goartifact/fs/local.goartifact/fs/s3.goartifact/fs/save.goartifact/fs/smb.goartifact/fs/ssh.goartifact/inline.goconnection/aws.goconnection/blob.gocontext/blobs.gogo.modmodels/artifacts.gomodels/artifacts_test.goschema/artifacts.hcltests/artifacts_test.gotests/e2e-blobs/blobs_test.gotests/e2e-blobs/containers.gotests/e2e-blobs/sftp-configuration.jsontests/e2e-blobs/suite_test.go
✅ Files skipped from review due to trivial changes (9)
- artifact/clients/aws/doc.go
- tests/e2e-blobs/sftp-configuration.json
- schema/artifacts.hcl
- artifact/clients/aws/fileinfo.go
- tests/artifacts_test.go
- models/artifacts_test.go
- artifact/fs/local.go
- artifact/fs/s3.go
- artifact/clients/gcp/fileinfo.go
🚧 Files skipped from review as they are similar to previous changes (3)
- connection/aws.go
- context/blobs.go
- artifact/fs.go
Comment on lines
+69
to
+85
| func (t *gcsFS) Write(ctx gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { | ||
| obj := t.Client.Bucket(t.Bucket).Object(path) | ||
|
|
||
| content, err := io.ReadAll(data) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| writer := obj.NewWriter(ctx) | ||
| if _, err := writer.Write(content); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := writer.Close(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return t.Stat(path) |
Contributor
There was a problem hiding this comment.
Writer not closed on write failure; consider streaming instead of buffering.
Two issues:
- If
writer.Writefails at line 78, the writer is never closed, potentially leaving corrupt state. io.ReadAllloads the entire file into memory, which can cause OOM for large artifacts.
Suggested fix for cleanup
func (t *gcsFS) Write(ctx gocontext.Context, path string, data io.Reader) (os.FileInfo, error) {
obj := t.Client.Bucket(t.Bucket).Object(path)
-
- content, err := io.ReadAll(data)
- if err != nil {
- return nil, err
- }
-
writer := obj.NewWriter(ctx)
- if _, err := writer.Write(content); err != nil {
+ if _, err := io.Copy(writer, data); err != nil {
+ _ = writer.Close()
return nil, err
}
if err := writer.Close(); err != nil {
return nil, err
}
return t.Stat(path)
}📝 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
| func (t *gcsFS) Write(ctx gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { | |
| obj := t.Client.Bucket(t.Bucket).Object(path) | |
| content, err := io.ReadAll(data) | |
| if err != nil { | |
| return nil, err | |
| } | |
| writer := obj.NewWriter(ctx) | |
| if _, err := writer.Write(content); err != nil { | |
| return nil, err | |
| } | |
| if err := writer.Close(); err != nil { | |
| return nil, err | |
| } | |
| return t.Stat(path) | |
| func (t *gcsFS) Write(ctx gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { | |
| obj := t.Client.Bucket(t.Bucket).Object(path) | |
| writer := obj.NewWriter(ctx) | |
| if _, err := io.Copy(writer, data); err != nil { | |
| _ = writer.Close() | |
| return nil, err | |
| } | |
| if err := writer.Close(); err != nil { | |
| return nil, err | |
| } | |
| return t.Stat(path) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/fs/gcs.go` around lines 69 - 85, In gcsFS.Write, avoid buffering
with io.ReadAll and ensure the writer is always closed: replace reading the
entire reader into memory with streaming via io.Copy from the provided data to
obj.NewWriter(ctx) so large files don't OOM; create the writer once, defer a
close that handles and preserves any write error (call writer.Close() in a
deferred function and if Close returns an error and no prior error, return that;
if there was a write error, still attempt Close and return the original write
error or wrap both). Target the gcsFS.Write function, obj.NewWriter,
writer.Write and writer.Close for these changes.
Comment on lines
+36
to
+37
| func (s *InlineStore) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { | ||
| raw, err := io.ReadAll(data) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "inline.go" -type f | head -20Repository: flanksource/duty
Length of output: 80
🏁 Script executed:
wc -l ./artifact/inline.goRepository: flanksource/duty
Length of output: 84
🏁 Script executed:
cat -n ./artifact/inline.goRepository: flanksource/duty
Length of output: 4192
Propagate context.Context into GORM calls.
Write and Read methods ignore the provided context, so cancellations and deadlines won't stop database operations.
Proposed fix
-func (s *InlineStore) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) {
+func (s *InlineStore) Write(ctx gocontext.Context, path string, data io.Reader) (os.FileInfo, error) {
@@
- if err := s.db.Create(&artifact).Error; err != nil {
+ if err := s.db.WithContext(ctx).Create(&artifact).Error; err != nil {
return nil, fmt.Errorf("saving inline artifact %s: %w", path, err)
}
@@
-func (s *InlineStore) Read(_ gocontext.Context, path string) (io.ReadCloser, error) {
+func (s *InlineStore) Read(ctx gocontext.Context, path string) (io.ReadCloser, error) {
var artifact models.Artifact
- if err := s.db.Where("path = ?", path).First(&artifact).Error; err != nil {
+ if err := s.db.WithContext(ctx).Where("path = ?", path).First(&artifact).Error; err != nil {
return nil, fmt.Errorf("finding inline artifact %s: %w", path, err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/inline.go` around lines 36 - 37, The Write and Read methods
currently ignore the supplied gocontext.Context (they use a blank identifier),
so GORM calls don't honor cancellations or deadlines; update signatures to name
the context parameter (e.g., ctx gocontext.Context) and pass it into all GORM
operations via db.WithContext(ctx) or tx.WithContext(ctx) used in
InlineStore.Write and InlineStore.Read (and any internal calls like Create,
Save, First, Where, Delete) so the database calls observe the provided context.
Comment on lines
+36
to
+48
| func (s *InlineStore) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { | ||
| raw, err := io.ReadAll(data) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("reading artifact data: %w", err) | ||
| } | ||
|
|
||
| artifact := models.Artifact{ | ||
| Path: path, | ||
| Filename: path, | ||
| } | ||
| if err := artifact.SetContent(raw, s.compression, s.maxSize); err != nil { | ||
| return nil, fmt.Errorf("setting inline content for %s: %w", path, err) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the structure and find artifact/inline.go
find . -name "inline.go" -type f | head -20Repository: flanksource/duty
Length of output: 80
🏁 Script executed:
# Let's check if artifact/inline.go exists and read it
git ls-files | grep -E "(inline|artifact)" | head -20Repository: flanksource/duty
Length of output: 526
🏁 Script executed:
# Get the full content of inline.go to understand context
fd inline.go --type fRepository: flanksource/duty
Length of output: 78
🏁 Script executed:
# Read the full inline.go file
cat -n artifact/inline.goRepository: flanksource/duty
Length of output: 4192
🏁 Script executed:
# Also read models/artifacts.go to understand SetContent
wc -l models/artifacts.goRepository: flanksource/duty
Length of output: 83
🏁 Script executed:
# Read the models/artifacts.go file, focusing on SetContent
cat -n models/artifacts.goRepository: flanksource/duty
Length of output: 8087
🏁 Script executed:
# Also check if there are any tests for InlineStore
cat -n tests/artifacts_test.goRepository: flanksource/duty
Length of output: 8856
🏁 Script executed:
# Verify the context usage in DB operations
rg "s\.db\." artifact/inline.goRepository: flanksource/duty
Length of output: 350
🏁 Script executed:
# Double-check SetContent behavior and when validation occurs
sed -n '150,170p' models/artifacts.goRepository: flanksource/duty
Length of output: 830
Bound the incoming stream before io.ReadAll to prevent unbounded memory allocation.
Write reads the entire stream into memory without size bounds. The maxSize check in SetContent only validates the compressed size after compression, allowing large uncompressed payloads to spike memory usage before validation fails. A 1GB stream could exhaust memory before hitting the size limit.
Proposed fix
func (s *InlineStore) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) {
- raw, err := io.ReadAll(data)
+ if s.maxSize <= 0 {
+ return nil, fmt.Errorf("invalid maxSize %d", s.maxSize)
+ }
+ raw, err := io.ReadAll(io.LimitReader(data, int64(s.maxSize)+1))
if err != nil {
return nil, fmt.Errorf("reading artifact data: %w", err)
}
+ if len(raw) > s.maxSize {
+ return nil, fmt.Errorf("inline artifact %s exceeds max size %d bytes", path, s.maxSize)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/inline.go` around lines 36 - 48, InlineStore.Write currently calls
io.ReadAll on the incoming reader unbounded, which can OOM before
artifact.SetContent enforces maxSize; to fix, wrap the incoming data reader with
an io.LimitReader (or an io.LimitedReader) using s.maxSize+1 (or an appropriate
uncompressed byte limit) and read from that limited reader so you can detect and
return a clear "payload too large" error if the limit is exceeded before calling
artifact.SetContent; update InlineStore.Write to use the limited reader, still
pass the resulting raw bytes into artifact.SetContent (which will continue to
enforce compressed size), and reference InlineStore.Write, artifact.SetContent,
and the s.maxSize/s.compression fields when making the change.
Comment on lines
+78
to
+83
| pattern := strings.ReplaceAll(name, "*", "%") | ||
| if !strings.Contains(pattern, "%") { | ||
| pattern += "%" | ||
| } | ||
| if err := s.db.Where("path LIKE ?", pattern).Find(&artifacts).Error; err != nil { | ||
| return nil, fmt.Errorf("listing inline artifacts under %s: %w", name, err) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -name "inline.go" -path "*/artifact/*" 2>/dev/null | head -20Repository: flanksource/duty
Length of output: 115
🏁 Script executed:
cd /root && git ls-files | grep -i "artifact.*inline\|inline.*artifact"Repository: flanksource/duty
Length of output: 115
🏁 Script executed:
cd /root && rg "artifact/inline" --type go -l 2>/dev/null | head -5Repository: flanksource/duty
Length of output: 115
🏁 Script executed:
cd /root && find . -type f -name "inline.go" 2>/dev/nullRepository: flanksource/duty
Length of output: 115
🏁 Script executed:
cd /root && git ls-files "*.go" | grep -E "inline|artifact"Repository: flanksource/duty
Length of output: 115
🏁 Script executed:
git ls-files "*.go" | grep -i inlineRepository: flanksource/duty
Length of output: 78
🏁 Script executed:
git ls-files | grep artifactRepository: flanksource/duty
Length of output: 584
🏁 Script executed:
cat -n artifact/inline.go | sed -n '70,90p'Repository: flanksource/duty
Length of output: 850
🏁 Script executed:
git ls-files "*.go" | xargs grep -l "db.Where.*LIKE" | head -10Repository: flanksource/duty
Length of output: 78
🏁 Script executed:
rg "gorm|database|sqlite|postgres|mysql" --type go artifact/inline.go -A 2 -B 2Repository: flanksource/duty
Length of output: 323
🏁 Script executed:
cat artifact/inline.go | head -30Repository: flanksource/duty
Length of output: 534
🏁 Script executed:
rg "LIKE\s+\?" --type go -B 2 -A 2Repository: flanksource/duty
Length of output: 6916
🏁 Script executed:
git ls-files "*.go" | xargs grep -l "main\|init" | xargs grep -l "gorm\|database" | head -5Repository: flanksource/duty
Length of output: 178
🏁 Script executed:
rg "db\.Where.*\%" --type go | head -20Repository: flanksource/duty
Length of output: 150
🏁 Script executed:
rg "ReadDir" --type go -B 3 -A 5Repository: flanksource/duty
Length of output: 8704
🏁 Script executed:
cat tests/artifacts_test.go | head -100Repository: flanksource/duty
Length of output: 3039
🏁 Script executed:
rg "should list artifacts via ReadDir" -A 20 tests/artifacts_test.goRepository: flanksource/duty
Length of output: 848
🏁 Script executed:
rg "ReadDir.*_\|ReadDir.*%" --type go tests/Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
cat tests/artifacts_test.go | grep -A 30 "should list artifacts via ReadDir"Repository: flanksource/duty
Length of output: 1160
🏁 Script executed:
rg "ESCAPE" --type goRepository: flanksource/duty
Length of output: 42
🏁 Script executed:
cat artifact/inline.go | sed -n '1,100p'Repository: flanksource/duty
Length of output: 2523
Escape SQL LIKE metacharacters to prevent unintended wildcard matches.
The ReadDir pattern only escapes * to % but doesn't handle literal _ or % in filenames. If a path contains these characters (e.g., file_name.txt or file%test.txt), they become SQL wildcards and cause incorrect results. For example, searching for /dir/ would match /dir/file_name.txt as well as /dir/filexname.txt (any single character substitution).
Proposed fix
- pattern := strings.ReplaceAll(name, "*", "%")
- if !strings.Contains(pattern, "%") {
- pattern += "%"
- }
- if err := s.db.Where("path LIKE ?", pattern).Find(&artifacts).Error; err != nil {
+ pattern := toLikePattern(name)
+ if err := s.db.Where("path LIKE ? ESCAPE '\\'", pattern).Find(&artifacts).Error; err != nil {
return nil, fmt.Errorf("listing inline artifacts under %s: %w", name, err)
}// helper (outside this block)
func toLikePattern(name string) string {
var b strings.Builder
for _, r := range name {
switch r {
case '*':
b.WriteByte('%')
case '%', '_', '\\':
b.WriteByte('\\')
b.WriteRune(r)
default:
b.WriteRune(r)
}
}
p := b.String()
if !strings.Contains(p, "%") {
p += "%"
}
return p
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/inline.go` around lines 78 - 83, The current ReadDir logic builds
SQL LIKE patterns by only replacing '*' with '%', which fails to escape literal
'%' and '_' and backslashes; add a helper function (e.g., toLikePattern) that
iterates runes, converts '*'→'%', and escapes '%', '_' and '\\' by prefixing
with '\\', then appends a trailing '%' if the pattern contains no '%' and use
toLikePattern(name) in place of strings.ReplaceAll(name, "*", "%") before
calling s.db.Where("path LIKE ?", pattern) in the ReadDir/inline listing path.
Comment on lines
+99
to
+108
| case models.ConnectionTypeAzure: | ||
| endpoint := c.URL | ||
| container := c.Properties["container"] | ||
| client, err := azblob.NewClientFromConnectionString( | ||
| fmt.Sprintf("DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s;BlobEndpoint=%s", | ||
| c.Username, c.Password, endpoint), nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating Azure Blob client: %w", err) | ||
| } | ||
| return artifactFS.NewAzureBlobFS(client, container), nil |
Contributor
There was a problem hiding this comment.
Azure connection string hardcodes HTTPS protocol.
The connection string uses DefaultEndpointsProtocol=https but the BlobEndpoint is taken from c.URL which might be an HTTP endpoint (e.g., Azurite emulator at http://localhost:10000). This could cause protocol mismatches.
Consider extracting the protocol from c.URL or making it configurable:
+ parsedURL, err := url.Parse(endpoint)
+ if err != nil {
+ return nil, fmt.Errorf("parsing Azure endpoint URL: %w", err)
+ }
+ protocol := parsedURL.Scheme
+ if protocol == "" {
+ protocol = "https"
+ }
client, err := azblob.NewClientFromConnectionString(
- fmt.Sprintf("DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s;BlobEndpoint=%s",
+ fmt.Sprintf("DefaultEndpointsProtocol=%s;AccountName=%s;AccountKey=%s;BlobEndpoint=%s",
+ protocol, c.Username, c.Password, endpoint), nil)
- c.Username, c.Password, endpoint), nil)📝 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
| case models.ConnectionTypeAzure: | |
| endpoint := c.URL | |
| container := c.Properties["container"] | |
| client, err := azblob.NewClientFromConnectionString( | |
| fmt.Sprintf("DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s;BlobEndpoint=%s", | |
| c.Username, c.Password, endpoint), nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("creating Azure Blob client: %w", err) | |
| } | |
| return artifactFS.NewAzureBlobFS(client, container), nil | |
| case models.ConnectionTypeAzure: | |
| endpoint := c.URL | |
| container := c.Properties["container"] | |
| parsedURL, err := url.Parse(endpoint) | |
| if err != nil { | |
| return nil, fmt.Errorf("parsing Azure endpoint URL: %w", err) | |
| } | |
| protocol := parsedURL.Scheme | |
| if protocol == "" { | |
| protocol = "https" | |
| } | |
| client, err := azblob.NewClientFromConnectionString( | |
| fmt.Sprintf("DefaultEndpointsProtocol=%s;AccountName=%s;AccountKey=%s;BlobEndpoint=%s", | |
| protocol, c.Username, c.Password, endpoint), nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("creating Azure Blob client: %w", err) | |
| } | |
| return artifactFS.NewAzureBlobFS(client, container), nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/blob.go` around lines 99 - 108, The Azure connection code for
models.ConnectionTypeAzure hardcodes DefaultEndpointsProtocol=https while using
c.URL for BlobEndpoint, which can mismatch for HTTP endpoints (e.g., Azurite);
update the code that calls azblob.NewClientFromConnectionString to derive the
protocol from c.URL (or accept a configurable protocol property) and build the
connection string using that protocol (not always "https"), then pass the client
to artifactFS.NewAzureBlobFS with the existing container value so HTTP endpoints
work correctly.
| gonum.org/v1/gonum v0.17.0 | ||
| google.golang.org/api v0.262.0 | ||
| google.golang.org/grpc v1.78.0 | ||
| google.golang.org/grpc v1.79.1 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of google.golang.org/grpc and what version fixes GHSA-p77j-4mvh-x3m3?
💡 Result:
The latest version of google.golang.org/grpc is v1.80.0, released on 2026-04-01. GHSA-p77j-4mvh-x3m3 is fixed in v1.79.3 and later versions.
Citations:
- 1: https://pkg.go.dev/google.golang.org/grpc
- 2: https://pkg.go.dev/google.golang.org/grpc?tab=versions
- 3: https://github.com/grpc/grpc-go/releases/tag/v1.80.0
- 4: https://github.com/grpc/grpc-go
- 5: https://github.com/grpc/grpc-go/releases/tag/v1.79.3
- 6: GHSA-p77j-4mvh-x3m3
Upgrade google.golang.org/grpc to v1.79.3 or later to fix critical authorization bypass vulnerability.
Version v1.79.1 is vulnerable to GHSA-p77j-4mvh-x3m3, which allows unauthorized access to gRPC endpoints due to missing leading slash validation in :path header handling. The fix is available in v1.79.3 and v1.80.0 (latest). Upgrade immediately.
🧰 Tools
🪛 OSV Scanner (2.3.5)
[CRITICAL] 101-101: google.golang.org/grpc 1.79.1: Authorization bypass in gRPC-Go via missing leading slash in :path in google.golang.org/grpc
(GO-2026-4762)
[CRITICAL] 101-101: google.golang.org/grpc 1.79.1: gRPC-Go has an authorization bypass via missing leading slash in :path
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 101, Update the grpc module version in go.mod to a patched
release to address the authorization bypass (GHSA-p77j-4mvh-x3m3): replace the
existing module line "google.golang.org/grpc v1.79.1" with
"google.golang.org/grpc v1.79.3" (or v1.80.0) and run go mod tidy to refresh
go.sum; verify builds and tests pass after updating references to the grpc
module.
| github.com/containerd/log v0.1.0 // indirect | ||
| github.com/containerd/platforms v0.2.1 // indirect | ||
| github.com/cpuguy83/dockercfg v0.3.2 // indirect | ||
| github.com/docker/docker v28.5.2+incompatible // indirect |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What version of github.com/docker/docker fixes GHSA-pxq6-2prw-chj9 and GHSA-x744-4wpc-v9h2?
💡 Result:
The version of github.com/docker/docker (Moby Docker Engine) that fixes both GHSA-pxq6-2prw-chj9 (CVE-2026-33997, off-by-one error in plugin privilege validation) and GHSA-x744-4wpc-v9h2 (CVE-2026-34040, AuthZ plugin bypass with oversized request body) is v29.3.1 and later versions. Both advisories explicitly state affected versions are < 29.3.1 and patched versions are 29.3.1. The v29.3.1 release notes confirm fixes for both: CVE-2026-34040 (GHSA-x744-4wpc-v9h2) and CVE-2026-33997 (GHSA-pxq6-2prw-chj9).
Citations:
- 1: GHSA-pxq6-2prw-chj9
- 2: https://osv.dev/vulnerability/GHSA-pxq6-2prw-chj9
- 3: [Bug]: Vulnerability found in moby (CVE-2026-34040) testcontainers/testcontainers-go#3614
- 4: GHSA-x744-4wpc-v9h2
- 5: https://advisories.gitlab.com/pkg/golang/github.com/docker/docker/CVE-2026-34040/
- 6: https://osv.dev/vulnerability/GHSA-x744-4wpc-v9h2
- 7: x/vulndb: potential Go vuln in github.com/moby/moby/v2: GHSA-pxq6-2prw-chj9 golang/vulndb#4883
- 8: https://github.com/moby/moby/releases/tag/docker-v29.3.1
- 9: https://docs.docker.com/engine/release-notes/29/
Update github.com/docker/docker to v29.3.1 or later to fix HIGH severity vulnerabilities.
The current version v28.5.2 is affected by GHSA-pxq6-2prw-chj9 (off-by-one error in plugin privilege validation) and GHSA-x744-4wpc-v9h2 (AuthZ plugin bypass). Both are fixed in v29.3.1+.
🧰 Tools
🪛 OSV Scanner (2.3.5)
[HIGH] 123-123: github.com/docker/docker 28.5.2+incompatible: Moby has an Off-by-one error in its plugin privilege validation in github.com/docker/docker
(GO-2026-4883)
[HIGH] 123-123: github.com/docker/docker 28.5.2+incompatible: Moby has AuthZ plugin bypass when provided oversized request bodies in github.com/docker/docker
(GO-2026-4887)
[HIGH] 123-123: github.com/docker/docker 28.5.2+incompatible: Moby has an Off-by-one error in its plugin privilege validation
[HIGH] 123-123: github.com/docker/docker 28.5.2+incompatible: Moby has AuthZ plugin bypass when provided oversized request bodies
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 123, Update the module requirement for
github.com/docker/docker to v29.3.1 or later to address the reported
vulnerabilities: change the go.mod entry for "github.com/docker/docker" to at
least v29.3.1, then run the module update commands (e.g., go get
github.com/docker/docker@v29.3.1 and go mod tidy) to refresh go.sum and ensure
dependency graph integrity; finally run tests/build to verify no regressions.
Comment on lines
+82
to
+84
| host, _ := container.Host(ctx) | ||
| port, _ := container.MappedPort(ctx, "8083") | ||
| emulatorHost := fmt.Sprintf("%s:%s", host, port.Port()) |
Contributor
There was a problem hiding this comment.
Errors from Host() and MappedPort() are silently ignored.
If these calls fail, emulatorHost will be malformed, leading to confusing downstream errors.
🔧 Proposed fix
- host, _ := container.Host(ctx)
- port, _ := container.MappedPort(ctx, "8083")
+ host, err := container.Host(ctx)
+ if err != nil {
+ return nil, container, fmt.Errorf("getting host: %w", err)
+ }
+ port, err := container.MappedPort(ctx, "8083")
+ if err != nil {
+ return nil, container, fmt.Errorf("getting mapped port: %w", err)
+ }📝 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
| host, _ := container.Host(ctx) | |
| port, _ := container.MappedPort(ctx, "8083") | |
| emulatorHost := fmt.Sprintf("%s:%s", host, port.Port()) | |
| host, err := container.Host(ctx) | |
| if err != nil { | |
| return nil, container, fmt.Errorf("getting host: %w", err) | |
| } | |
| port, err := container.MappedPort(ctx, "8083") | |
| if err != nil { | |
| return nil, container, fmt.Errorf("getting mapped port: %w", err) | |
| } | |
| emulatorHost := fmt.Sprintf("%s:%s", host, port.Port()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e-blobs/containers.go` around lines 82 - 84, The calls to
container.Host and container.MappedPort are ignoring errors which can produce a
malformed emulatorHost; update the code around Host, MappedPort and emulatorHost
to check and handle both returned errors (e.g., if err != nil) instead of
discarding them—log or fail the test (t.Fatalf or return the error) with a clear
message including the underlying error and avoid constructing emulatorHost when
either call fails so downstream logic won't receive a bad address.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
artifact/clients/sftp/sftp.go (1)
17-17:⚠️ Potential issue | 🔴 CriticalRe-enable SSH host key verification.
ssh.InsecureIgnoreHostKey()at Line 17 allows MITM attacks. Please switch to a trusted callback (knownhosts.New(...)orssh.FixedHostKey(...)).Suggested fix
import ( + "os" "github.com/pkg/sftp" "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/knownhosts" ) @@ func SSHConnect(host, user, password string) (*sftp.Client, error) { + knownHostsPath := os.Getenv("SSH_KNOWN_HOSTS") + hostKeyCallback, err := knownhosts.New(knownHostsPath) + if err != nil { + return nil, err + } + config := &ssh.ClientConfig{ User: user, Auth: []ssh.AuthMethod{ ssh.Password(password), }, - HostKeyCallback: ssh.InsecureIgnoreHostKey(), //nolint:gosec + HostKeyCallback: hostKeyCallback, } - conn, err := ssh.Dial("tcp", host, config) + conn, err := ssh.Dial("tcp", host, config)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/clients/sftp/sftp.go` at line 17, The SSH client configuration currently uses ssh.InsecureIgnoreHostKey() for HostKeyCallback in sftp.go (the HostKeyCallback assignment), which disables host key verification; replace this with a proper verification callback such as knownhosts.New(...) pointing at the user's known_hosts file or ssh.FixedHostKey(...) using the server's public key. Update the code where the ssh.ClientConfig (or equivalent struct) is built—replace HostKeyCallback: ssh.InsecureIgnoreHostKey() with HostKeyCallback: knownhosts.New(pathToKnownHosts) (handle the returned error) or HostKeyCallback: ssh.FixedHostKey(parsedPubKey) after loading/parsing the server key, and ensure error handling and tests are updated accordingly.artifact/fs/local.go (1)
15-29:⚠️ Potential issue | 🔴 CriticalPath traversal protections are still bypassable in list/glob flows.
Two issues remain:
safePathat Line 25 uses a raw prefix check (HasPrefix), which is bypassable by sibling paths (e.g.,/tmp/base2).ReadDir/ReadDirGlob(Line 57 and Line 78) don’t route throughsafePath, so traversal input can still escape base.Suggested fix
func (t *localFS) safePath(path string) (string, error) { full := filepath.Join(t.base, path) abs, err := filepath.Abs(full) @@ baseAbs, err := filepath.Abs(t.base) @@ - if !strings.HasPrefix(abs, baseAbs) { + rel, err := filepath.Rel(baseAbs, abs) + if err != nil { + return "", fmt.Errorf("resolving relative path: %w", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return "", fmt.Errorf("path %q escapes base directory", path) } - return full, nil + return abs, nil } @@ func (t *localFS) ReadDir(name string) ([]artifact.FileInfo, error) { @@ - path := filepath.Join(t.base, name) + path, err := t.safePath(name) + if err != nil { + return nil, err + } files, err := os.ReadDir(path) @@ func (t *localFS) ReadDirGlob(name string) ([]artifact.FileInfo, error) { - base, pattern := doublestar.SplitPattern(filepath.Join(t.base, name)) + baseInput, pattern := doublestar.SplitPattern(name) + base, err := t.safePath(baseInput) + if err != nil { + return nil, err + } matches, err := doublestar.Glob(os.DirFS(base), pattern)Also applies to: 52-59, 77-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/local.go` around lines 15 - 29, safePath currently uses a raw HasPrefix check and returns the non-absolute full path, and ReadDir/ReadDirGlob bypass safePath entirely; fix by making safePath compute and return the absolute resolved path (use filepath.Abs on both and then use filepath.Rel or ensure abs == baseAbs or strings.HasPrefix(abs, baseAbs+string(os.PathSeparator)) to prevent sibling escapes) and update ReadDir and ReadDirGlob to call safePath for their input paths and use the returned absolute path for filesystem operations so all listing/globbing flows are validated against the base before use.artifact/inline.go (3)
68-71:⚠️ Potential issue | 🟠 MajorPropagate
context.Contextinto GORM query inRead.Line 68 discards
ctx, so Line 70 query does not honor cancellation/deadline.Proposed fix
-func (s *InlineStore) Read(_ gocontext.Context, path string) (io.ReadCloser, error) { +func (s *InlineStore) Read(ctx gocontext.Context, path string) (io.ReadCloser, error) { var artifact models.Artifact - if err := s.db.Where("path = ?", path).First(&artifact).Error; err != nil { + if err := s.db.WithContext(ctx).Where("path = ?", path).First(&artifact).Error; err != nil { return nil, fmt.Errorf("finding inline artifact %s: %w", path, err) }#!/bin/bash # Verify context is currently ignored in InlineStore.Read and no WithContext is used there rg -n 'func \(s \*InlineStore\) Read\(_ gocontext\.Context|s\.db\.Where\("path = \?", path\)' artifact/inline.go -n -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/inline.go` around lines 68 - 71, The Read method on InlineStore currently ignores the passed ctx (signature uses _ gocontext.Context) so the GORM query via s.db.Where("path = ?", path).First(&artifact) won't observe cancellation or deadlines; change the signature to accept ctx (e.g., ctx gocontext.Context) and propagate it into the DB call by using s.db.WithContext(ctx).Where(...).First(&artifact) (and similarly for any other DB calls in InlineStore) so the query honors the context.
84-89:⚠️ Potential issue | 🟡 MinorEscape SQL LIKE metacharacters in path patterns.
Current pattern handling only converts
*to%. Literal%and_in paths are treated as wildcards and can return incorrect rows.Proposed fix
- pattern := strings.ReplaceAll(name, "*", "%") - if !strings.Contains(pattern, "%") { - pattern += "%" - } - if err := s.db.Where("path LIKE ?", pattern).Find(&artifacts).Error; err != nil { + pattern := toLikePattern(name) + if err := s.db.Where("path LIKE ? ESCAPE '\\\\'", pattern).Find(&artifacts).Error; err != nil { return nil, fmt.Errorf("listing inline artifacts under %s: %w", name, err) }func toLikePattern(name string) string { var b strings.Builder for _, r := range name { switch r { case '*': b.WriteByte('%') case '%', '_', '\\': b.WriteByte('\\') b.WriteRune(r) default: b.WriteRune(r) } } p := b.String() if !strings.Contains(p, "%") { p += "%" } return p }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/inline.go` around lines 84 - 89, The current pattern builder converts '*' to '%' but doesn't escape literal '%', '_' or '\' which makes SQL LIKE treat them as wildcards; implement a helper like toLikePattern(name string) that iterates runes, converts '*' to '%', escapes '%', '_' and '\' by prefixing with '\' and appends a trailing '%' if no '%' is present, then replace the current pattern construction and the s.db.Where call (the variable pattern and the query s.db.Where("path LIKE ?", pattern)) to use toLikePattern(name) so the DB query searches literal paths correctly.
36-40:⚠️ Potential issue | 🟠 MajorBound input size before
io.ReadAllto prevent memory blowups.Line 37 reads the entire stream with no cap, so oversized payloads can exhaust memory before rejection.
Proposed fix
func (s *InlineStore) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { - raw, err := io.ReadAll(data) + if s.maxSize <= 0 { + return nil, fmt.Errorf("invalid maxSize %d", s.maxSize) + } + raw, err := io.ReadAll(io.LimitReader(data, int64(s.maxSize)+1)) if err != nil { return nil, fmt.Errorf("reading artifact data: %w", err) } + if len(raw) > s.maxSize { + return nil, fmt.Errorf("inline artifact %s exceeds max size %d bytes", path, s.maxSize) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/inline.go` around lines 36 - 40, The InlineStore.Write implementation currently uses io.ReadAll(data) in InlineStore.Write which can OOM on large inputs; change it to read with a bounded reader (e.g., wrap data with io.LimitReader using a configured maxBytes constant) and detect when the input exceeds that limit (return an error like "artifact too large"); alternatively stream into a temp file if larger-than-memory artifacts must be supported. Update InlineStore.Write to use the bounded reader and return a clear error when the limit is hit.artifact/fs/ssh.go (1)
106-113:⚠️ Potential issue | 🟠 Major
Writestill swallows file close errors.Line 106 defers
f.Close()but its error is ignored. A failed close can invalidate a reported successful write.Proposed fix
-func (s *sshFS) Write(_ gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { +func (s *sshFS) Write(_ gocontext.Context, path string, data io.Reader) (info os.FileInfo, err error) { dir := filepath.Dir(path) if err := s.MkdirAll(dir); err != nil { return nil, fmt.Errorf("error creating directory: %w", err) } f, err := s.Create(path) if err != nil { return nil, fmt.Errorf("error creating file: %w", err) } - defer f.Close() + defer func() { + if cerr := f.Close(); cerr != nil { + if err != nil { + err = fmt.Errorf("%w; close error: %v", err, cerr) + } else { + err = fmt.Errorf("close error: %w", cerr) + } + } + }() if _, err = io.Copy(f, data); err != nil { return nil, fmt.Errorf("error writing to file: %w", err) } - return f.Stat() + info, err = s.Stat(path) + return info, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/ssh.go` around lines 106 - 113, The Write function currently defers f.Close() and ignores its error; update Write (in artifact/fs/ssh.go) to capture Close() errors and return them if present: store the result of f.Close() (e.g., closeErr) in the deferred function or use a named error return so that after io.Copy succeeds you still check and return any close error (combining or preferring the close error when no earlier error occurred), ensuring you don't swallow errors from f.Close() when returning f.Stat() or a nil result.connection/blob.go (2)
145-157:⚠️ Potential issue | 🟠 MajorAzure connection string protocol is hardcoded and can mismatch endpoint scheme.
DefaultEndpointsProtocol=httpsis fixed at Line 145, butBlobEndpointis dynamic (c.URL). HTTP endpoints can break when these diverge.Proposed fix
case models.ConnectionTypeAzure: container := c.Properties["container"] - connStr := fmt.Sprintf("DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s;BlobEndpoint=%s", - c.Username, c.Password, c.URL) + parsedURL, err := url.Parse(c.URL) + if err != nil { + return nil, "", fmt.Errorf("parsing Azure endpoint URL: %w", err) + } + protocol := parsedURL.Scheme + if protocol == "" { + protocol = "https" + } + connStr := fmt.Sprintf("DefaultEndpointsProtocol=%s;AccountName=%s;AccountKey=%s;BlobEndpoint=%s", + protocol, c.Username, c.Password, c.URL) var opts *azblob.ClientOptions#!/bin/bash # Verify hardcoded protocol usage in Azure connection string assembly rg -n 'DefaultEndpointsProtocol=https|BlobEndpoint=%s' connection/blob.go -n -C2
124-134:⚠️ Potential issue | 🟠 MajorSFTP host/port assembly can generate invalid
host:port:port.Line 133 uses
parsedURL.Host(which may already include:port) and appends another port. For URLs likesftp://example.com:2222, this can becomeexample.com:2222:22.Proposed fix
case models.ConnectionTypeSFTP: parsedURL, err := url.Parse(c.URL) if err != nil { return nil, "", err } port := c.Properties["port"] if port == "" { - port = "22" + if parsedURL.Port() != "" { + port = parsedURL.Port() + } else { + port = "22" + } } - fs, err := artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, port), c.Username, c.Password) + host := parsedURL.Hostname() + if host == "" { + host = parsedURL.Host + } + fs, err := artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", host, port), c.Username, c.Password) return fs, "sftp", err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/blob.go` around lines 124 - 134, The code concatenates parsedURL.Host with a port string which can produce "host:port:port" when parsedURL.Host already includes a port; update the SFTP case in connection/blob.go to use parsedURL.Hostname() (not parsedURL.Host) and derive the port by preferring c.Properties["port"], then parsedURL.Port(), then defaulting to "22", then call artifactFS.NewSSHFS with fmt.Sprintf("%s:%s", hostname, port). Use parsedURL.Hostname(), parsedURL.Port(), c.Properties["port"], and artifactFS.NewSSHFS to locate and change the logic.
🧹 Nitpick comments (1)
tests/e2e-blobs/blobs_test.go (1)
53-62: Prefer shared dummy fixtures instead of bespoke test files.Consider sourcing these cases from
tests/fixtures/dummy/all.goto keep test data consistent across suites and reduce drift.Based on learnings: Applies to tests/**/*_test.go : Use resources from the dummy dataset in tests/fixtures/dummy/all.go in tests before creating new test resources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-blobs/blobs_test.go` around lines 53 - 62, Replace the bespoke testFiles slice with the shared dummy fixtures from tests/fixtures/dummy/all.go: remove the local var testFiles and import the dummy fixtures package (e.g., tests/fixtures/dummy or fixtures/dummy) and reference the exported variable (e.g., DummyFiles or DummyFixtures) instead when creating test resources; ensure the test uses the package's exported names and shapes (map/slice of {name,content}) and adapt any iteration code to the shared structure so the test reuses the canonical dataset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@artifact/blob_store.go`:
- Around line 31-33: In blobStore.Write, validate the artifact pointer before
setting its fields: check that the parameter a (type *models.Artifact) is not
nil at the top of the function and return a descriptive error if it is,
preventing a panic when later assigning fields; apply the same nil-check and
early-return pattern to the other block that assigns into a (the section
referenced by lines 52-63) so any assignment to a.* is guarded.
In `@artifact/clients/sftp/sftp.go`:
- Around line 12-20: The ssh.ClientConfig used in sftp.go (variable config)
lacks a TCP dial timeout, so ssh.Dial("tcp", host, config) can hang; add a
Timeout field to the ssh.ClientConfig (e.g., 10s) to bound the TCP connection
phase and import time; update the config construction in the function that
creates the SSH connection (where config is defined and ssh.Dial is called) to
set config.Timeout = time.Duration(10)*time.Second (or another suitable
constant) so the TCP dial will fail fast on network faults.
In `@artifact/fs/save.go`:
- Around line 43-45: The SaveArtifact function currently dereferences
data.Content and the artifact pointer a without checks; add nil-guards at the
start of SaveArtifact to return a clear error if a == nil or data.Content == nil
before the defer and any use (reference symbols: SaveArtifact, data.Content, a,
models.Artifact, artifact.FilesystemRW), and ensure the defer that closes
data.Content is only set after validating it is non-nil; also validate any
fields of a that are later accessed (used around the block referencing a at
lines ~61-66) and return descriptive errors instead of allowing a panic.
In `@artifact/fs/ssh.go`:
- Around line 37-40: The Getwd() error path leaks the SFTP client; when
client.Getwd() returns an error you must close the created client before
returning. Update the error branch around the call to client.Getwd()
(referencing the client variable and the Getwd() call) to call client.Close()
(or defer and cancel appropriately) prior to returning the fmt.Errorf, ensuring
the SFTP connection is released even on failure.
In `@artifact/logged.go`:
- Line 44: The log call using fmt.Sprintf in the v.Infof invocation should not
use fmt.Sprintf("%s", time.Since(start)); update the v.Infof call that includes
l.formatOp("Read", artifactID.String()), data.Pretty().String(),
clicky.Text(...).String() to pass time.Since(start).String() directly (remove
fmt.Sprintf) so the duration is converted via its String() method and the S1025
lint error is resolved.
---
Duplicate comments:
In `@artifact/clients/sftp/sftp.go`:
- Line 17: The SSH client configuration currently uses
ssh.InsecureIgnoreHostKey() for HostKeyCallback in sftp.go (the HostKeyCallback
assignment), which disables host key verification; replace this with a proper
verification callback such as knownhosts.New(...) pointing at the user's
known_hosts file or ssh.FixedHostKey(...) using the server's public key. Update
the code where the ssh.ClientConfig (or equivalent struct) is built—replace
HostKeyCallback: ssh.InsecureIgnoreHostKey() with HostKeyCallback:
knownhosts.New(pathToKnownHosts) (handle the returned error) or HostKeyCallback:
ssh.FixedHostKey(parsedPubKey) after loading/parsing the server key, and ensure
error handling and tests are updated accordingly.
In `@artifact/fs/local.go`:
- Around line 15-29: safePath currently uses a raw HasPrefix check and returns
the non-absolute full path, and ReadDir/ReadDirGlob bypass safePath entirely;
fix by making safePath compute and return the absolute resolved path (use
filepath.Abs on both and then use filepath.Rel or ensure abs == baseAbs or
strings.HasPrefix(abs, baseAbs+string(os.PathSeparator)) to prevent sibling
escapes) and update ReadDir and ReadDirGlob to call safePath for their input
paths and use the returned absolute path for filesystem operations so all
listing/globbing flows are validated against the base before use.
In `@artifact/fs/ssh.go`:
- Around line 106-113: The Write function currently defers f.Close() and ignores
its error; update Write (in artifact/fs/ssh.go) to capture Close() errors and
return them if present: store the result of f.Close() (e.g., closeErr) in the
deferred function or use a named error return so that after io.Copy succeeds you
still check and return any close error (combining or preferring the close error
when no earlier error occurred), ensuring you don't swallow errors from
f.Close() when returning f.Stat() or a nil result.
In `@artifact/inline.go`:
- Around line 68-71: The Read method on InlineStore currently ignores the passed
ctx (signature uses _ gocontext.Context) so the GORM query via s.db.Where("path
= ?", path).First(&artifact) won't observe cancellation or deadlines; change the
signature to accept ctx (e.g., ctx gocontext.Context) and propagate it into the
DB call by using s.db.WithContext(ctx).Where(...).First(&artifact) (and
similarly for any other DB calls in InlineStore) so the query honors the
context.
- Around line 84-89: The current pattern builder converts '*' to '%' but doesn't
escape literal '%', '_' or '\' which makes SQL LIKE treat them as wildcards;
implement a helper like toLikePattern(name string) that iterates runes, converts
'*' to '%', escapes '%', '_' and '\' by prefixing with '\' and appends a
trailing '%' if no '%' is present, then replace the current pattern construction
and the s.db.Where call (the variable pattern and the query s.db.Where("path
LIKE ?", pattern)) to use toLikePattern(name) so the DB query searches literal
paths correctly.
- Around line 36-40: The InlineStore.Write implementation currently uses
io.ReadAll(data) in InlineStore.Write which can OOM on large inputs; change it
to read with a bounded reader (e.g., wrap data with io.LimitReader using a
configured maxBytes constant) and detect when the input exceeds that limit
(return an error like "artifact too large"); alternatively stream into a temp
file if larger-than-memory artifacts must be supported. Update InlineStore.Write
to use the bounded reader and return a clear error when the limit is hit.
In `@connection/blob.go`:
- Around line 124-134: The code concatenates parsedURL.Host with a port string
which can produce "host:port:port" when parsedURL.Host already includes a port;
update the SFTP case in connection/blob.go to use parsedURL.Hostname() (not
parsedURL.Host) and derive the port by preferring c.Properties["port"], then
parsedURL.Port(), then defaulting to "22", then call artifactFS.NewSSHFS with
fmt.Sprintf("%s:%s", hostname, port). Use parsedURL.Hostname(),
parsedURL.Port(), c.Properties["port"], and artifactFS.NewSSHFS to locate and
change the logic.
---
Nitpick comments:
In `@tests/e2e-blobs/blobs_test.go`:
- Around line 53-62: Replace the bespoke testFiles slice with the shared dummy
fixtures from tests/fixtures/dummy/all.go: remove the local var testFiles and
import the dummy fixtures package (e.g., tests/fixtures/dummy or fixtures/dummy)
and reference the exported variable (e.g., DummyFiles or DummyFixtures) instead
when creating test resources; ensure the test uses the package's exported names
and shapes (map/slice of {name,content}) and adapt any iteration code to the
shared structure so the test reuses the canonical dataset.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78feca81-6af4-4c6d-87c8-be8b02014e3e
📒 Files selected for processing (16)
artifact/blob_store.goartifact/clients/aws/fileinfo.goartifact/clients/sftp/sftp.goartifact/clients/smb/smb.goartifact/data.goartifact/fs/local.goartifact/fs/save.goartifact/fs/smb.goartifact/fs/ssh.goartifact/inline.goartifact/logged.goconnection/blob.gocontext/blobs.gotests/artifacts_test.gotests/e2e-blobs/blobs_test.gotests/e2e-blobs/suite_test.go
✅ Files skipped from review due to trivial changes (3)
- artifact/clients/aws/fileinfo.go
- tests/artifacts_test.go
- tests/e2e-blobs/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- artifact/clients/smb/smb.go
Comment on lines
+37
to
+40
| wd, err := client.Getwd() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get working directory: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Close SFTP client when Getwd() fails to avoid connection leak.
If client.Getwd() fails, the created client is not closed before returning.
Proposed fix
wd, err := client.Getwd()
if err != nil {
+ _ = client.Close()
return nil, fmt.Errorf("failed to get working directory: %w", err)
}📝 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
| wd, err := client.Getwd() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get working directory: %w", err) | |
| } | |
| wd, err := client.Getwd() | |
| if err != nil { | |
| _ = client.Close() | |
| return nil, fmt.Errorf("failed to get working directory: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/fs/ssh.go` around lines 37 - 40, The Getwd() error path leaks the
SFTP client; when client.Getwd() returns an error you must close the created
client before returning. Update the error branch around the call to
client.Getwd() (referencing the client variable and the Getwd() call) to call
client.Close() (or defer and cancel appropriately) prior to returning the
fmt.Errorf, ensuring the SFTP connection is released even on failure.
b836080 to
1e6d3d4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
artifact/fs/gcs.go (1)
69-83:⚠️ Potential issue | 🟠 MajorStream uploads instead of buffering the whole artifact.
blobStore.Writealready passes a streaming reader with no size cap, but this backend doesio.ReadAllbefore uploading. Large artifacts will spike memory, andwriter.Close()is skipped ifwriter.Writefails. Stream withio.Copyand close the writer on every path.cloud.google.com/go/storage Writer Close required to finalize upload and recommended streaming upload pattern from an io.ReaderSuggested fix
func (t *gcsFS) Write(ctx gocontext.Context, path string, data io.Reader) (os.FileInfo, error) { obj := t.Client.Bucket(t.Bucket).Object(path) - - content, err := io.ReadAll(data) - if err != nil { - return nil, err - } writer := obj.NewWriter(ctx) - if _, err := writer.Write(content); err != nil { + if _, err := io.Copy(writer, data); err != nil { + _ = writer.Close() return nil, err } if err := writer.Close(); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/gcs.go` around lines 69 - 83, The gcsFS.Write implementation currently buffers the entire reader with io.ReadAll and may skip writer.Close on write errors; change it to stream directly from the provided io.Reader using io.Copy into the writer returned by obj.NewWriter(ctx) (use io.Copy(writer, data)), ensure writer.Close() is always invoked (use defer-style handling that captures Close() error and returns the original write/copy error if present), and propagate any Close() error when no earlier error occurred; update references in gcsFS.Write to use obj.NewWriter, io.Copy, writer.Close and handle both copy and close errors correctly.artifact/fs/save.go (1)
43-45:⚠️ Potential issue | 🔴 CriticalGuard
aanddata.Contentbefore dereferencing them.
defer data.Content.Close()and the later field writes will panic on nil inputs. This helper should return a clear error instead of crashing the caller.Suggested fix
func SaveArtifact(ctx context.Context, fs artifact.FilesystemRW, a *models.Artifact, data Artifact) error { + if a == nil { + return fmt.Errorf("artifact model is nil") + } + if data.Content == nil { + return fmt.Errorf("artifact content is nil") + } defer func() { _ = data.Content.Close() }()Also applies to: 61-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/save.go` around lines 43 - 45, SaveArtifact currently defers data.Content.Close() and writes to a's fields without checking for nil, which will panic on nil inputs; update SaveArtifact to validate inputs up front: return a descriptive error if a is nil or if data or data.Content is nil, only call defer data.Content.Close() after confirming data.Content != nil, and guard all subsequent dereferences/field writes on a (e.g., where SaveArtifact updates a.Path/Size/ContentType/etc.) so they only run when a is non-nil.connection/blob.go (2)
143-161:⚠️ Potential issue | 🟡 MinorAzure connection string hardcodes HTTPS protocol.
c.URLcould be an HTTP endpoint (e.g., Azurite emulator athttp://localhost:10000), causing a protocol mismatch withDefaultEndpointsProtocol=https.Proposed fix
case models.ConnectionTypeAzure: container := c.Properties["container"] + parsedURL, err := url.Parse(c.URL) + if err != nil { + return nil, "", fmt.Errorf("parsing Azure endpoint URL: %w", err) + } + protocol := parsedURL.Scheme + if protocol == "" { + protocol = "https" + } - connStr := fmt.Sprintf("DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s;BlobEndpoint=%s", + connStr := fmt.Sprintf("DefaultEndpointsProtocol=%s;AccountName=%s;AccountKey=%s;BlobEndpoint=%s", + protocol, c.Username, c.Password, c.URL) - c.Username, c.Password, c.URL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/blob.go` around lines 143 - 161, The Azure blob connection string currently hardcodes DefaultEndpointsProtocol=https which breaks HTTP endpoints like Azurite; update the ConnectionTypeAzure branch (the code that builds connStr before calling azblob.NewClientFromConnectionString and the return using artifactFS.NewAzureBlobFS) to derive the protocol from c.URL (or parse c.URL's scheme) and set DefaultEndpointsProtocol to "http" when the URL scheme is "http" (otherwise "https"), ensure BlobEndpoint remains c.URL (without duplicating scheme), and then call azblob.NewClientFromConnectionString with the corrected connStr so HTTP endpoints (e.g., Azurite) work correctly.
124-134:⚠️ Potential issue | 🟠 MajorSFTP host string may include duplicate port.
parsedURL.Hostalready includes the port when present in the URL (e.g.,sftp://host:2222/pathyieldsHost = "host:2222"). Appendingportcreates an invalid address like"host:2222:22".Use
parsedURL.Hostname()instead, or check if the URL already contains a port:Proposed fix
case models.ConnectionTypeSFTP: parsedURL, err := url.Parse(c.URL) if err != nil { return nil, "", err } - port := c.Properties["port"] - if port == "" { - port = "22" + port := parsedURL.Port() + if port == "" { + port = c.Properties["port"] + } + if port == "" { + port = "22" } - fs, err := artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, port), c.Username, c.Password) + fs, err := artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Hostname(), port), c.Username, c.Password) return fs, "sftp", err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/blob.go` around lines 124 - 134, The SFTP host string currently uses parsedURL.Host which can already include a port and leads to duplicated ports when combined with the `port` variable; in the SFTP case (case models.ConnectionTypeSFTP) update the address construction used for artifactFS.NewSSHFS so it uses parsedURL.Hostname() (or prefer parsedURL.Port() if present) together with the resolved port fallback logic (properties["port"] default "22") to produce a single host:port string for NewSSHFS; adjust the logic around parsedURL, port, and the call to artifactFS.NewSSHFS accordingly.artifact/blob_store.go (1)
31-68:⚠️ Potential issue | 🔴 CriticalValidate
artifactpointer before field assignment.The
a *models.Artifactparameter is not validated fornilbefore assigning fields at lines 54-62. Passingnilwill cause a panic.Proposed fix
func (s *blobStore) Write(data Data, a *models.Artifact) (*models.Artifact, error) { + if a == nil { + return nil, fmt.Errorf("artifact model is nil") + } if data.Content == nil { return nil, fmt.Errorf("artifact data content is nil") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/blob_store.go` around lines 31 - 68, The Write method (blobStore.Write) assigns fields on the input artifact pointer a without validating it; update Write to check if the artifact pointer a is nil early (after successful write and mime detection but before any a.Field assignments or InlineArtifact calls) and return a descriptive error (e.g., "artifact is nil") instead of panicking; ensure the nil check covers the InlineArtifact(info) handling and subsequent assignments to a.Path, a.Filename, a.Size, a.ContentType, a.Checksum so callers get a clear error when passing nil.
🧹 Nitpick comments (3)
artifact/fs/azure.go (3)
64-70: Consider defensive nil checks on blob properties.The code dereferences
blob.Name,blob.Properties.ContentLength, andblob.Properties.LastModifiedwithout nil checks. While Azure SDK typically guarantees these fields for listed blobs, defensive coding would prevent panics from unexpected API responses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/azure.go` around lines 64 - 70, The loop over resp.Segment.BlobItems dereferences blob.Name, blob.Properties.ContentLength and blob.Properties.LastModified without nil checks; update the iteration in the code that builds azureUtil.BlobFileInfo (the block referencing blob, resp.Segment.BlobItems and constructing BlobFileInfo with BlobName/BlobSize/LastMod) to defensively handle nils: skip or continue if blob or blob.Name is nil (since name is required), and for blob.Properties, check for nil before accessing ContentLength/LastModified and fall back to safe zero values (e.g., 0 for BlobSize and time.Time{} for LastMod) when those properties are absent, ensuring no direct dereference of pointers without guards.
35-46: Write buffers entire content in memory.
io.ReadAllloads the full blob into memory before uploading. For large artifacts, this could cause memory pressure. Consider usingUploadStreamfor streaming uploads if large files are expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/azure.go` around lines 35 - 46, The Write method (azureBlobFS.Write) currently reads the entire io.Reader into memory with io.ReadAll and calls t.client.UploadBuffer; change it to stream the upload instead by using the SDK streaming upload API (e.g., t.client.UploadStream or equivalent) so you pass the original io.Reader directly and avoid buffering the whole file; update the error handling to reflect the UploadStream call and keep the final return of t.Stat(path) unchanged.
75-91:Statdownloads entire blob body unnecessarily.Using
DownloadStreamto get metadata is inefficient because it initiates a full download. The Azure SDK providesclient.NewContainerClient(container).NewBlobClient(name).GetProperties()for metadata-only requests, similar to how the S3 implementation usesHeadObject(seeartifact/fs/s3.go:97-114).Proposed optimization
func (t *azureBlobFS) Stat(name string) (os.FileInfo, error) { - resp, err := t.client.DownloadStream(gocontext.TODO(), t.container, name, nil) + blobClient := t.client.NewContainerClient(t.container).NewBlobClient(name) + props, err := blobClient.GetProperties(gocontext.TODO(), nil) if err != nil { return nil, fmt.Errorf("stat blob %s: %w", name, err) } - _ = resp.Body.Close() size := int64(0) - if resp.ContentLength != nil { - size = *resp.ContentLength + if props.ContentLength != nil { + size = *props.ContentLength } return azureUtil.BlobFileInfo{ BlobName: name, BlobSize: size, + LastMod: *props.LastModified, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/azure.go` around lines 75 - 91, The Stat method currently calls t.client.DownloadStream which triggers a full blob download; change it to call the metadata-only API instead by creating a blob client and using GetProperties (e.g. t.client.NewContainerClient(t.container).NewBlobClient(name).GetProperties(...)) to retrieve headers/properties, extract content length from the response (resp.ContentLength or resp.ContentLength pointer) and construct the azureUtil.BlobFileInfo as before, and wrap/return errors similarly—remove the DownloadStream call and the unnecessary resp.Body.Close() usage and ensure error messages still include the blob name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@artifact/fs/save.go`:
- Around line 52-66: The inline-store save currently writes the file via
fs.Write but never copies the stored payload into the DB model, so only metadata
is persisted; update the save routine (the function that calls fs.Write in
artifact/fs/save.go) to populate the artifact model's Content and
CompressionType by calling artifact.InlineArtifact(info) (or otherwise
extracting the inline payload from info) before calling ctx.DB().Create(a), and
ensure you still set Path, Filename, Size, ContentType and Checksum as shown.
In `@artifact/fs/ssh.go`:
- Line 9: The remote SFTP path construction is using filepath (which emits
OS-specific backslashes on Windows) causing broken remote paths; update the code
that builds remote paths—specifically replace filepath.Join calls used for
remote uploads (e.g., in functions/methods that call filepath.Join at the
upload/remote path logic) with path.Join and replace filepath.Dir (the instance
used to compute remote parent directories) with path.Dir so all SFTP/remote path
handling uses the POSIX-style path package instead of filepath. Ensure imports
are adjusted to import "path" where these remote path helpers live.
In `@models/artifacts.go`:
- Around line 95-106: The code inserts a.Filename directly into raw HTML
(img.html and img.staticHTML), enabling attribute/HTML injection; update the
formatting to escape the filename before embedding by calling an HTML-escaping
utility (e.g., html.EscapeString or template.HTMLEscapeString) on a.Filename and
use that escaped value in both artifactImage.html and artifactImage.staticHTML
generation (references: artifactImage, a.Filename, img.html, img.staticHTML,
a.GetContent, a.ContentType).
In `@tests/e2e-blobs/blobs_test.go`:
- Around line 29-50: getBackends currently drops errors from
artifactFS.NewSSHFS/NewSMBFS and returns a slice omitting failed backends which
lets suites call Skip(); change getBackends to initialize a map/slice of
testBackend records that include an error field (e.g., extend testBackend with
Err), attempt to create every backend exactly once (including
NewS3FS/NewGCSFS/NewAzureBlobFS/NewLocalFS/NewSSHFS/NewSMBFS), store either the
logged FS or the initialization error in that record, and return all records;
then update the test setup that consumes getBackends to fail the test run when a
backend has Err unless that backend is explicitly marked optional, ensuring
BeforeAll rebuilds/cleans every backend but only closes the chosen fs.
---
Duplicate comments:
In `@artifact/blob_store.go`:
- Around line 31-68: The Write method (blobStore.Write) assigns fields on the
input artifact pointer a without validating it; update Write to check if the
artifact pointer a is nil early (after successful write and mime detection but
before any a.Field assignments or InlineArtifact calls) and return a descriptive
error (e.g., "artifact is nil") instead of panicking; ensure the nil check
covers the InlineArtifact(info) handling and subsequent assignments to a.Path,
a.Filename, a.Size, a.ContentType, a.Checksum so callers get a clear error when
passing nil.
In `@artifact/fs/gcs.go`:
- Around line 69-83: The gcsFS.Write implementation currently buffers the entire
reader with io.ReadAll and may skip writer.Close on write errors; change it to
stream directly from the provided io.Reader using io.Copy into the writer
returned by obj.NewWriter(ctx) (use io.Copy(writer, data)), ensure
writer.Close() is always invoked (use defer-style handling that captures Close()
error and returns the original write/copy error if present), and propagate any
Close() error when no earlier error occurred; update references in gcsFS.Write
to use obj.NewWriter, io.Copy, writer.Close and handle both copy and close
errors correctly.
In `@artifact/fs/save.go`:
- Around line 43-45: SaveArtifact currently defers data.Content.Close() and
writes to a's fields without checking for nil, which will panic on nil inputs;
update SaveArtifact to validate inputs up front: return a descriptive error if a
is nil or if data or data.Content is nil, only call defer data.Content.Close()
after confirming data.Content != nil, and guard all subsequent
dereferences/field writes on a (e.g., where SaveArtifact updates
a.Path/Size/ContentType/etc.) so they only run when a is non-nil.
In `@connection/blob.go`:
- Around line 143-161: The Azure blob connection string currently hardcodes
DefaultEndpointsProtocol=https which breaks HTTP endpoints like Azurite; update
the ConnectionTypeAzure branch (the code that builds connStr before calling
azblob.NewClientFromConnectionString and the return using
artifactFS.NewAzureBlobFS) to derive the protocol from c.URL (or parse c.URL's
scheme) and set DefaultEndpointsProtocol to "http" when the URL scheme is "http"
(otherwise "https"), ensure BlobEndpoint remains c.URL (without duplicating
scheme), and then call azblob.NewClientFromConnectionString with the corrected
connStr so HTTP endpoints (e.g., Azurite) work correctly.
- Around line 124-134: The SFTP host string currently uses parsedURL.Host which
can already include a port and leads to duplicated ports when combined with the
`port` variable; in the SFTP case (case models.ConnectionTypeSFTP) update the
address construction used for artifactFS.NewSSHFS so it uses
parsedURL.Hostname() (or prefer parsedURL.Port() if present) together with the
resolved port fallback logic (properties["port"] default "22") to produce a
single host:port string for NewSSHFS; adjust the logic around parsedURL, port,
and the call to artifactFS.NewSSHFS accordingly.
---
Nitpick comments:
In `@artifact/fs/azure.go`:
- Around line 64-70: The loop over resp.Segment.BlobItems dereferences
blob.Name, blob.Properties.ContentLength and blob.Properties.LastModified
without nil checks; update the iteration in the code that builds
azureUtil.BlobFileInfo (the block referencing blob, resp.Segment.BlobItems and
constructing BlobFileInfo with BlobName/BlobSize/LastMod) to defensively handle
nils: skip or continue if blob or blob.Name is nil (since name is required), and
for blob.Properties, check for nil before accessing ContentLength/LastModified
and fall back to safe zero values (e.g., 0 for BlobSize and time.Time{} for
LastMod) when those properties are absent, ensuring no direct dereference of
pointers without guards.
- Around line 35-46: The Write method (azureBlobFS.Write) currently reads the
entire io.Reader into memory with io.ReadAll and calls t.client.UploadBuffer;
change it to stream the upload instead by using the SDK streaming upload API
(e.g., t.client.UploadStream or equivalent) so you pass the original io.Reader
directly and avoid buffering the whole file; update the error handling to
reflect the UploadStream call and keep the final return of t.Stat(path)
unchanged.
- Around line 75-91: The Stat method currently calls t.client.DownloadStream
which triggers a full blob download; change it to call the metadata-only API
instead by creating a blob client and using GetProperties (e.g.
t.client.NewContainerClient(t.container).NewBlobClient(name).GetProperties(...))
to retrieve headers/properties, extract content length from the response
(resp.ContentLength or resp.ContentLength pointer) and construct the
azureUtil.BlobFileInfo as before, and wrap/return errors similarly—remove the
DownloadStream call and the unnecessary resp.Body.Close() usage and ensure error
messages still include the blob name.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53f2060a-ddea-4e06-b470-f9c614bdf356
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
.github/workflows/test.yamlMakefileartifact/blob_store.goartifact/clients/aws/doc.goartifact/clients/aws/fileinfo.goartifact/clients/azure/fileinfo.goartifact/clients/gcp/fileinfo.goartifact/clients/sftp/sftp.goartifact/clients/smb/smb.goartifact/data.goartifact/fs.goartifact/fs/azure.goartifact/fs/gcs.goartifact/fs/local.goartifact/fs/s3.goartifact/fs/save.goartifact/fs/smb.goartifact/fs/ssh.goartifact/inline.goartifact/logged.goconnection/aws.goconnection/blob.goconnection/gcp.goconnection/gcs.gocontext/blobs.gogo.modlogs/gcpcloudlogging/cloud_logging.gomodels/artifacts.gomodels/artifacts_test.goschema/artifacts.hcltests/artifacts_test.gotests/e2e-blobs/blobs_test.gotests/e2e-blobs/containers.gotests/e2e-blobs/sftp-configuration.jsontests/e2e-blobs/suite_test.go
✅ Files skipped from review due to trivial changes (19)
- artifact/clients/aws/doc.go
- logs/gcpcloudlogging/cloud_logging.go
- connection/gcp.go
- tests/e2e-blobs/sftp-configuration.json
- schema/artifacts.hcl
- artifact/clients/sftp/sftp.go
- artifact/fs.go
- tests/e2e-blobs/suite_test.go
- models/artifacts_test.go
- connection/gcs.go
- artifact/clients/smb/smb.go
- artifact/clients/gcp/fileinfo.go
- artifact/clients/aws/fileinfo.go
- artifact/clients/azure/fileinfo.go
- tests/e2e-blobs/containers.go
- artifact/fs/local.go
- tests/artifacts_test.go
- artifact/fs/s3.go
- artifact/inline.go
🚧 Files skipped from review as they are similar to previous changes (4)
- connection/aws.go
- .github/workflows/test.yaml
- artifact/fs/smb.go
- go.mod
| "io" | ||
| "io/fs" | ||
| "os" | ||
| "path/filepath" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go stdlib guidance on using path vs path/filepath for slash-separated remote paths such as SFTP paths
💡 Result:
Use the path package from the Go standard library for manipulating slash-separated remote paths such as SFTP paths. The path package is specifically designed for paths separated by forward slashes (/), like those in URLs or protocol-specific paths (e.g., SFTP), regardless of the host operating system. The filepath package is for local operating system paths, which use OS-specific separators (e.g., \ on Windows). Official guidance from pkg.go.dev/path: "Package path implements utility routines for manipulating slash-separated paths. The path package should only be used for paths separated by forward slashes, such as the paths in URLs." From pkg.go.dev/path/filepath: "To process paths such as URLs that always use forward slashes regardless of the operating system, see the path package." SFTP paths are slash-separated per the protocol (similar to Unix-style paths over SSH), so path is appropriate. Popular SFTP libraries like github.com/pkg/sftp internally handle path normalization using path.Clean and conversions like filepath.FromSlash/ToSlash for protocol compliance on different hosts.
Citations:
- 1: https://pkg.go.dev/path/filepath/
- 2: https://pkg.go.dev/path/filepath@go1.19.1
- 3: https://pkg.go.dev/path/filepath
- 4: https://pkg.go.dev/path/filepath@go1.26.0
- 5: https://pkg.go.dev/path
- 6: https://pkg.go.dev/path/
- 7: https://stackoverflow.com/questions/39181790/whats-the-difference-between-path-path-filepath-packages-in-go
- 8: Windows issues with current directory and drive changes pkg/sftp#444
- 9: [bugfix] Windows paths don’t work with Server pkg/sftp#445
🏁 Script executed:
#!/bin/bash
# Check the artifact/fs/ssh.go file structure
wc -l artifact/fs/ssh.go
echo "=== First 100 lines ==="
head -100 artifact/fs/ssh.goRepository: flanksource/duty
Length of output: 2145
🏁 Script executed:
#!/bin/bash
# Check the remaining lines and verify the specific usages
echo "=== Lines 50-70 (ReadDir method) ==="
sed -n '50,70p' artifact/fs/ssh.go
echo "=== Lines 90-110 (Write method) ==="
sed -n '90,110p' artifact/fs/ssh.goRepository: flanksource/duty
Length of output: 1134
Use path, not filepath, for remote SFTP paths.
The code uses filepath.Join() (lines 61, 63) and filepath.Dir() (line 97) to build paths sent to the remote SFTP server. On Windows, filepath produces backslashes, which breaks SFTP operations. The path package is designed for slash-separated remote paths and should be used instead. Replace filepath.Join with path.Join and filepath.Dir with path.Dir for remote path handling.
Suggested fix
import (
gocontext "context"
"fmt"
"io"
"io/fs"
"os"
- "path/filepath"
+ pathpkg "path"
"strings"
@@
for _, file := range files {
base := name
if !strings.HasPrefix(name, "/") {
- base = filepath.Join(t.wd, name)
+ base = pathpkg.Join(t.wd, name)
}
- output = append(output, &sshFileInfo{FileInfo: file, fullpath: filepath.Join(base, file.Name())})
+ output = append(output, &sshFileInfo{FileInfo: file, fullpath: pathpkg.Join(base, file.Name())})
}
@@
- dir := filepath.Dir(path)
+ dir := pathpkg.Dir(path)📝 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
| "path/filepath" | |
| import ( | |
| gocontext "context" | |
| "fmt" | |
| "io" | |
| "io/fs" | |
| "os" | |
| pathpkg "path" | |
| "strings" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/fs/ssh.go` at line 9, The remote SFTP path construction is using
filepath (which emits OS-specific backslashes on Windows) causing broken remote
paths; update the code that builds remote paths—specifically replace
filepath.Join calls used for remote uploads (e.g., in functions/methods that
call filepath.Join at the upload/remote path logic) with path.Join and replace
filepath.Dir (the instance used to compute remote parent directories) with
path.Dir so all SFTP/remote path handling uses the POSIX-style path package
instead of filepath. Ensure imports are adjusted to import "path" where these
remote path helpers live.
Comment on lines
+95
to
+106
| url := fmt.Sprintf("/artifacts/download/%s", a.ID) | ||
| img := artifactImage{ | ||
| html: fmt.Sprintf( | ||
| `<img src="%s" alt="%s" style="max-width:100%%;border-radius:4px" loading="lazy" />`, | ||
| url, a.Filename, | ||
| ), | ||
| } | ||
| if data, err := a.GetContent(); err == nil && len(data) > 0 { | ||
| b64 := base64.StdEncoding.EncodeToString(data) | ||
| img.staticHTML = fmt.Sprintf( | ||
| `<img src="data:%s;base64,%s" alt="%s" style="max-width:100%%;border-radius:4px" />`, | ||
| a.ContentType, b64, a.Filename, |
Contributor
There was a problem hiding this comment.
Escape Filename before embedding it in raw HTML.
RowDetail() interpolates a.Filename directly into the <img> tag. A crafted artifact name can break the alt attribute and inject HTML/JS into the detail pane.
Suggested fix
import (
"bytes"
"compress/gzip"
"crypto/sha256"
"encoding/base64"
"fmt"
+ "html"
"io"
"time"
@@
func (a Artifact) RowDetail() api.Textable {
if a.ContentType == "" || !isImageContentType(a.ContentType) {
return nil
}
url := fmt.Sprintf("/artifacts/download/%s", a.ID)
+ safeFilename := html.EscapeString(a.Filename)
img := artifactImage{
html: fmt.Sprintf(
`<img src="%s" alt="%s" style="max-width:100%%;border-radius:4px" loading="lazy" />`,
- url, a.Filename,
+ url, safeFilename,
),
}
if data, err := a.GetContent(); err == nil && len(data) > 0 {
b64 := base64.StdEncoding.EncodeToString(data)
img.staticHTML = fmt.Sprintf(
`<img src="data:%s;base64,%s" alt="%s" style="max-width:100%%;border-radius:4px" />`,
- a.ContentType, b64, a.Filename,
+ a.ContentType, b64, safeFilename,
)
}📝 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
| url := fmt.Sprintf("/artifacts/download/%s", a.ID) | |
| img := artifactImage{ | |
| html: fmt.Sprintf( | |
| `<img src="%s" alt="%s" style="max-width:100%%;border-radius:4px" loading="lazy" />`, | |
| url, a.Filename, | |
| ), | |
| } | |
| if data, err := a.GetContent(); err == nil && len(data) > 0 { | |
| b64 := base64.StdEncoding.EncodeToString(data) | |
| img.staticHTML = fmt.Sprintf( | |
| `<img src="data:%s;base64,%s" alt="%s" style="max-width:100%%;border-radius:4px" />`, | |
| a.ContentType, b64, a.Filename, | |
| url := fmt.Sprintf("/artifacts/download/%s", a.ID) | |
| safeFilename := html.EscapeString(a.Filename) | |
| img := artifactImage{ | |
| html: fmt.Sprintf( | |
| `<img src="%s" alt="%s" style="max-width:100%%;border-radius:4px" loading="lazy" />`, | |
| url, safeFilename, | |
| ), | |
| } | |
| if data, err := a.GetContent(); err == nil && len(data) > 0 { | |
| b64 := base64.StdEncoding.EncodeToString(data) | |
| img.staticHTML = fmt.Sprintf( | |
| `<img src="data:%s;base64,%s" alt="%s" style="max-width:100%%;border-radius:4px" />`, | |
| a.ContentType, b64, safeFilename, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/artifacts.go` around lines 95 - 106, The code inserts a.Filename
directly into raw HTML (img.html and img.staticHTML), enabling attribute/HTML
injection; update the formatting to escape the filename before embedding by
calling an HTML-escaping utility (e.g., html.EscapeString or
template.HTMLEscapeString) on a.Filename and use that escaped value in both
artifactImage.html and artifactImage.staticHTML generation (references:
artifactImage, a.Filename, img.html, img.staticHTML, a.GetContent,
a.ContentType).
Comment on lines
+29
to
+50
| func getBackends() []testBackend { | ||
| backends := []testBackend{ | ||
| {"s3", logged("s3", artifactFS.NewS3FS(s3Client, "test"))}, | ||
| {"gcs", logged("gcs", artifactFS.NewGCSFS(gcsClient, "test"))}, | ||
| {"azure", logged("azure", artifactFS.NewAzureBlobFS(azureClient, "test"))}, | ||
| {"local", logged("local", artifactFS.NewLocalFS(GinkgoT().TempDir()))}, | ||
| } | ||
|
|
||
| sshfs, err := artifactFS.NewSSHFS(sftpHost, "foo", "pass") | ||
| if err == nil { | ||
| backends = append(backends, testBackend{"sftp", logged("sftp", sshfs)}) | ||
| } | ||
|
|
||
| smbfs, err := artifactFS.NewSMBFS(smbHost, smbPort, "users", types.Authentication{ | ||
| Username: types.EnvVar{ValueStatic: "foo"}, | ||
| Password: types.EnvVar{ValueStatic: "pass"}, | ||
| }) | ||
| if err == nil { | ||
| backends = append(backends, testBackend{"smb", logged("smb", smbfs)}) | ||
| } | ||
|
|
||
| return backends |
Contributor
There was a problem hiding this comment.
Backend init failures are currently turned into green skips.
Each BeforeAll rebuilds every backend, but only the selected fs is closed. More importantly, NewSSHFS/NewSMBFS errors are dropped, so the matching suites just Skip(). With the dedicated e2e-blobs job in .github/workflows/test.yaml:102-124, that means CI can stay green without actually covering a broken backend. Build the backend map once, keep the init error per backend, and fail unless a backend is explicitly optional.
Also applies to: 70-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e-blobs/blobs_test.go` around lines 29 - 50, getBackends currently
drops errors from artifactFS.NewSSHFS/NewSMBFS and returns a slice omitting
failed backends which lets suites call Skip(); change getBackends to initialize
a map/slice of testBackend records that include an error field (e.g., extend
testBackend with Err), attempt to create every backend exactly once (including
NewS3FS/NewGCSFS/NewAzureBlobFS/NewLocalFS/NewSSHFS/NewSMBFS), store either the
logged FS or the initialization error in that record, and return all records;
then update the test setup that consumes getBackends to fail the test run when a
backend has Err unless that backend is explicitly marked optional, ensuring
BeforeAll rebuilds/cleans every backend but only closes the chosen fs.
1e6d3d4 to
9e28319
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
tests/e2e-blobs/containers.go (1)
96-96: Global environment modification may cause test pollution.
os.Setenv("STORAGE_EMULATOR_HOST", ...)modifies process-wide state. If tests run in parallel or the test suite doesn't clean up, this could affect other tests. Consider usingt.Setenv()(if a*testing.Tis available) or documenting that cleanup is expected elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e-blobs/containers.go` at line 96, The test currently calls os.Setenv("STORAGE_EMULATOR_HOST", emulatorHost) which mutates global process state; change this to use testing.T's environment helper (t.Setenv("STORAGE_EMULATOR_HOST", emulatorHost)) where a *testing.T is available, or if not accessible, capture the previous value and restore it after the test (prev := os.Getenv("STORAGE_EMULATOR_HOST"); os.Setenv(...); defer func(){ os.Setenv("STORAGE_EMULATOR_HOST", prev) }()), updating the call site that currently invokes os.Setenv to one of these two safe alternatives to avoid cross-test pollution..github/workflows/test.yaml (1)
102-124: Newe2e-blobsjob looks good overall.The job structure is consistent with the existing
e2ejob. Minor inconsistency: line 109 hascache: falsewithout the explanatory comment present in other jobs (e.g., line 46:# Using custom cache action below for .bin directory).✏️ Suggested fix for consistency
- name: Install Go uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 with: go-version: 1.25.x - cache: false + cache: false # Using custom cache action below for .bin directory - name: Checkout code🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yaml around lines 102 - 124, In the e2e-blobs GitHub Actions job, add the same explanatory comment next to the actions/setup-go step's cache: false entry as used in the other job to keep consistency; locate the e2e-blobs job and update the actions/setup-go step (the line with cache: false) to include a short comment such as why custom caching is used (mirroring the comment near the other job's setup-go) so the intent matches across jobs.context/blobs.go (1)
19-25: Repeated log message on everyBlobs()call.
blobsLogger.Infof("Initializing inline blob store")is called each timeBlobs()is invoked when no connection is configured. If called frequently, this will spam logs. Consider caching the store or reducing log verbosity.Suggested approach
Consider caching the blob store instance on the context or using
Debuglevel instead ofInfo:- blobsLogger.Infof("Initializing inline blob store") + blobsLogger.Debugf("Initializing inline blob store")Or cache at the application level if the store should be reused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@context/blobs.go` around lines 19 - 25, The Blobs() method on Context repeatedly logs "Initializing inline blob store" via blobsLogger.Infof and recreates the inline store on every call when artifacts.connection is empty; change Blobs() to cache and reuse a single inline blob store instance on the Context (store created with artifact.NewBlobStore(artifact.NewInlineStore(...), k.DB(), "inline") and wrapped with artifact.NewLoggedBlobStore) so subsequent calls return the same object without reinitializing or logging, or alternatively lower the log call to Debug level (blobsLogger.Debugf) if caching is not desired; implement the cache as a Context field or memoized value checked before creating the store.artifact/data.go (1)
51-54:mimeWriter.Maxdefaults to zero if not set.The
Maxfield has no default value. If a caller creates&mimeWriter{}without settingMax, the buffer will never be filled (sincelen(t.buffer) < 0is always false). Ensure callers always initializeMaxor provide a constructor.Consider adding a constructor
func newMimeWriter() *mimeWriter { return &mimeWriter{Max: maxBytesForMimeDetection} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/data.go` around lines 51 - 54, The mimeWriter type's Max field defaults to zero causing its buffer logic to never trigger; add a constructor (e.g., newMimeWriter) that returns &mimeWriter{Max: maxBytesForMimeDetection} (or set a sensible default within the constructor) and update callers to use newMimeWriter() instead of &mimeWriter{}; alternatively enforce Max is positive in methods on mimeWriter (e.g., in Write or whatever method uses Max) by setting a fallback default when Max <= 0 to ensure proper buffer filling.models/artifacts.go (1)
137-146:formatBytesis duplicated inartifact/data.go.The same function exists in both files with identical implementation. Consider extracting to a shared utility package to avoid duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/artifacts.go` around lines 137 - 146, The function formatBytes is duplicated (in models/artifacts.go and artifact/data.go); extract it into a single shared utility (e.g., a new package util with an exported FormatBytes function) and remove the duplicate implementations. Replace local calls to formatBytes in both artifacts.go and data.go with util.FormatBytes (or the chosen package name), update imports, and run tests to ensure no remaining duplicates or unresolved references.artifact/fs/s3.go (1)
64-64:ReadDirandStatusecontext.TODO()instead of accepting context.Unlike
ReadandWritewhich accept a context parameter, these methods usegocontext.TODO(). This prevents proper cancellation and timeout propagation.♻️ Signature changes for consistency
-func (t *s3FS) ReadDir(pattern string) ([]artifact.FileInfo, error) { +func (t *s3FS) ReadDir(ctx gocontext.Context, pattern string) ([]artifact.FileInfo, error) { // ... - resp, err := t.Client.ListObjectsV2(gocontext.TODO(), req) + resp, err := t.Client.ListObjectsV2(ctx, req) -func (t *s3FS) Stat(path string) (fs.FileInfo, error) { +func (t *s3FS) Stat(ctx gocontext.Context, path string) (fs.FileInfo, error) { - headObject, err := t.Client.HeadObject(gocontext.TODO(), &s3.HeadObjectInput{ + headObject, err := t.Client.HeadObject(ctx, &s3.HeadObjectInput{This would require updating the
FilesystemRWinterface inartifact/fs.goas well.Also applies to: 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/s3.go` at line 64, ReadDir and Stat currently call gocontext.TODO() instead of taking a context; change their signatures to accept ctx context.Context (matching Read/Write) and update the FilesystemRW interface in artifact/fs.go accordingly, then replace gocontext.TODO() in methods like (t *S3Filesystem).ReadDir and (t *S3Filesystem).Stat with the passed ctx when calling t.Client.ListObjectsV2 and any other SDK calls, and update all call sites to forward their context to these methods so cancellation/timeouts propagate correctly.artifact/fs/azure.go (2)
102-105:SaveArtifactInlinehelper seems misplaced.This function is a generic helper that writes bytes to any
FilesystemRW, not Azure-specific. Consider moving it to theartifactpackage or a shared utilities file for better cohesion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/azure.go` around lines 102 - 105, SaveArtifactInline is a generic helper that operates on artifact.FilesystemRW and should be moved out of the Azure-specific file; remove the SaveArtifactInline implementation from artifact/fs/azure.go and relocate it into the artifact package (or a shared utilities file) keeping the same function signature SaveArtifactInline(ctx gocontext.Context, fs artifact.FilesystemRW, path string, data []byte) (os.FileInfo, error), update imports where it is referenced, and ensure callers use the new package path so the function continues to call fs.Write(ctx, path, bytes.NewReader(data)) against artifact.FilesystemRW.
75-91: ReplaceDownloadStreamwithGetPropertiesfor metadata-only request.
DownloadStreamfetches the entire blob body. UseGetProperties(on BlobClient) for a metadata-only HEAD request to retrieve the content length without unnecessary data transfer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact/fs/azure.go` around lines 75 - 91, In azureBlobFS.Stat replace the data-download call to t.client.DownloadStream(...) with a metadata-only HEAD call using the BlobClient GetProperties method (e.g., obtain the blob client for name and call GetProperties(ctx, nil)), remove the resp.Body.Close() usage, and keep the same error wrapping and use resp.ContentLength from the GetProperties response to populate azureUtil.BlobFileInfo.BlobSize; ensure the call site remains in the azureBlobFS.Stat function and update imports/context usage as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@artifact/blob_store.go`:
- Line 43: The Write and Read methods currently access s.db.Statement.Context
which can be nil; update these methods to accept a context.Context parameter
(e.g., func (s *BlobStore) Write(ctx context.Context, ... ) and Read(ctx
context.Context, ...)) and use s.db.WithContext(ctx) when calling the filesystem
operations and any queries, or alternatively call s.db = s.db.WithContext(ctx)
locally before using s.db.Statement; replace direct uses of
s.db.Statement.Context with the provided ctx (or the WithContext-bound DB) to
ensure a non-nil context is used for the fs.Write/fs.Read and any DB calls.
In `@artifact/clients/aws/fileinfo.go`:
- Around line 38-40: FullPath() currently dereferences obj.Object.Key without a
nil check which can panic; update S3FileInfo.FullPath to mirror the nil-safe
behavior used in Name(): check that obj.Object and obj.Object.Key are non-nil
and return an empty string (or appropriate default) when nil, otherwise return
the string value of *obj.Object.Key, ensuring consistency with Name()'s guarding
logic.
In `@artifact/fs/azure.go`:
- Around line 87-90: Stat currently returns a BlobFileInfo with BlobName and
BlobSize but leaves LastMod zero; mirror ReadDir by fetching the blob's
properties in Stat (use the same blob properties retrieval used in ReadDir,
e.g., the blob client's GetProperties/GetPropertiesResponse or
Properties().LastModified) and populate BlobFileInfo.LastMod with that property
(convert to time.Time as done in ReadDir). Ensure you handle a missing/empty
LastModified by falling back safely to zero value and keep the BlobFileInfo
return shape consistent with ReadDir.
- Around line 64-70: The loop that builds azureUtil.BlobFileInfo dereferences
pointer fields (blob.Name, blob.Properties.ContentLength,
blob.Properties.LastModified) without nil checks; update the code in
artifact/fs/azure.go where you append azureUtil.BlobFileInfo to first
defensively check for nil (blob and blob.Properties) and for each pointer field
before dereferencing, using safe defaults (empty string, 0, zero time) or
skipping the blob if appropriate, so BlobName, BlobSize and LastMod are
populated only from non-nil pointers; reference the blob iteration and the
azureUtil.BlobFileInfo construction to locate and implement these checks.
In `@artifact/fs/local.go`:
- Around line 52-75: ReadDir currently constructs the directory path with
filepath.Join(t.base, name) which bypasses path traversal checks; replace that
with the same path-sanitization used elsewhere by calling t.safePath(name) (or
the equivalent safePath method on localFS), handle any error it returns before
calling os.ReadDir, and then use the sanitized path for building fullPath (e.g.,
filepath.Join(safePath, match.Name())) and subsequent os.Stat calls so all
filesystem accesses go through the safePath validation in the ReadDir function.
In `@artifact/fs/s3.go`:
- Around line 106-113: The Stat implementation is setting s3Types.Object.Key to
filepath.Base(path) which makes awsUtil.S3FileInfo.FullPath() return only the
filename; change Stat to set Key to the full S3 key (the original path variable
or a cleaned/normalized full key) when constructing the awsUtil.S3FileInfo in
the return, and if a base-name is needed implement Name() to return
filepath.Base(obj.Object.Key) instead of storing only the base; update the
return in the Stat function (the block constructing awsUtil.S3FileInfo /
s3Types.Object) to use the full key for Key so FullPath() satisfies the
interface contract.
In `@connection/blob.go`:
- Around line 124-134: The SFTP branch builds the SSH address using
parsedURL.Host which can already include a port, causing addresses like
host:2222:22; update the logic in the models.ConnectionTypeSFTP case to extract
hostname and port separately using parsedURL.Hostname() and parsedURL.Port(): if
parsedURL.Port() is non-empty use it, else use c.Properties["port"] (default
"22"), then call artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Hostname(),
port), c.Username, c.Password) and return fs, "sftp", err (keep c.URL,
parsedURL, c.Properties, artifactFS.NewSSHFS, c.Username, c.Password identifiers
to locate the code).
In `@models/artifacts.go`:
- Around line 183-200: The schema allows "zstd" compression but compress() (and
likely decompress()) only supports "gzip" and "none"; add zstd handling to both
functions by implementing a zstd branch for compression and decompression using
a zstd encoder/decoder (e.g., from github.com/klauspost/compress/zstd) so
compress(data, "zstd") returns zstd-compressed bytes and decompress handles
"zstd" similarly, or alternatively update the schema to remove "zstd"—make
changes in the compress and decompress functions to add the "zstd" case and
import the zstd package referenced by those functions.
In `@tests/e2e-blobs/containers.go`:
- Around line 88-94: The call to http.NewRequestWithContext is ignoring its
error, which can leave updateReq nil and cause a panic when passed to
http.DefaultClient.Do; modify the code around updateReq (the
http.NewRequestWithContext call) to capture the returned error, check if err !=
nil and handle it (fail the test or return the error) before calling
http.DefaultClient.Do, and only call Do when updateReq is non-nil; keep the
existing resp.Body.Close behavior after a successful Do.
---
Nitpick comments:
In @.github/workflows/test.yaml:
- Around line 102-124: In the e2e-blobs GitHub Actions job, add the same
explanatory comment next to the actions/setup-go step's cache: false entry as
used in the other job to keep consistency; locate the e2e-blobs job and update
the actions/setup-go step (the line with cache: false) to include a short
comment such as why custom caching is used (mirroring the comment near the other
job's setup-go) so the intent matches across jobs.
In `@artifact/data.go`:
- Around line 51-54: The mimeWriter type's Max field defaults to zero causing
its buffer logic to never trigger; add a constructor (e.g., newMimeWriter) that
returns &mimeWriter{Max: maxBytesForMimeDetection} (or set a sensible default
within the constructor) and update callers to use newMimeWriter() instead of
&mimeWriter{}; alternatively enforce Max is positive in methods on mimeWriter
(e.g., in Write or whatever method uses Max) by setting a fallback default when
Max <= 0 to ensure proper buffer filling.
In `@artifact/fs/azure.go`:
- Around line 102-105: SaveArtifactInline is a generic helper that operates on
artifact.FilesystemRW and should be moved out of the Azure-specific file; remove
the SaveArtifactInline implementation from artifact/fs/azure.go and relocate it
into the artifact package (or a shared utilities file) keeping the same function
signature SaveArtifactInline(ctx gocontext.Context, fs artifact.FilesystemRW,
path string, data []byte) (os.FileInfo, error), update imports where it is
referenced, and ensure callers use the new package path so the function
continues to call fs.Write(ctx, path, bytes.NewReader(data)) against
artifact.FilesystemRW.
- Around line 75-91: In azureBlobFS.Stat replace the data-download call to
t.client.DownloadStream(...) with a metadata-only HEAD call using the BlobClient
GetProperties method (e.g., obtain the blob client for name and call
GetProperties(ctx, nil)), remove the resp.Body.Close() usage, and keep the same
error wrapping and use resp.ContentLength from the GetProperties response to
populate azureUtil.BlobFileInfo.BlobSize; ensure the call site remains in the
azureBlobFS.Stat function and update imports/context usage as needed.
In `@artifact/fs/s3.go`:
- Line 64: ReadDir and Stat currently call gocontext.TODO() instead of taking a
context; change their signatures to accept ctx context.Context (matching
Read/Write) and update the FilesystemRW interface in artifact/fs.go accordingly,
then replace gocontext.TODO() in methods like (t *S3Filesystem).ReadDir and (t
*S3Filesystem).Stat with the passed ctx when calling t.Client.ListObjectsV2 and
any other SDK calls, and update all call sites to forward their context to these
methods so cancellation/timeouts propagate correctly.
In `@context/blobs.go`:
- Around line 19-25: The Blobs() method on Context repeatedly logs "Initializing
inline blob store" via blobsLogger.Infof and recreates the inline store on every
call when artifacts.connection is empty; change Blobs() to cache and reuse a
single inline blob store instance on the Context (store created with
artifact.NewBlobStore(artifact.NewInlineStore(...), k.DB(), "inline") and
wrapped with artifact.NewLoggedBlobStore) so subsequent calls return the same
object without reinitializing or logging, or alternatively lower the log call to
Debug level (blobsLogger.Debugf) if caching is not desired; implement the cache
as a Context field or memoized value checked before creating the store.
In `@models/artifacts.go`:
- Around line 137-146: The function formatBytes is duplicated (in
models/artifacts.go and artifact/data.go); extract it into a single shared
utility (e.g., a new package util with an exported FormatBytes function) and
remove the duplicate implementations. Replace local calls to formatBytes in both
artifacts.go and data.go with util.FormatBytes (or the chosen package name),
update imports, and run tests to ensure no remaining duplicates or unresolved
references.
In `@tests/e2e-blobs/containers.go`:
- Line 96: The test currently calls os.Setenv("STORAGE_EMULATOR_HOST",
emulatorHost) which mutates global process state; change this to use testing.T's
environment helper (t.Setenv("STORAGE_EMULATOR_HOST", emulatorHost)) where a
*testing.T is available, or if not accessible, capture the previous value and
restore it after the test (prev := os.Getenv("STORAGE_EMULATOR_HOST");
os.Setenv(...); defer func(){ os.Setenv("STORAGE_EMULATOR_HOST", prev) }()),
updating the call site that currently invokes os.Setenv to one of these two safe
alternatives to avoid cross-test pollution.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54a59801-5054-4feb-9518-6ef95ac8200b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.github/workflows/test.yamlMakefileartifact/blob_store.goartifact/clients/aws/doc.goartifact/clients/aws/fileinfo.goartifact/clients/azure/fileinfo.goartifact/clients/gcp/fileinfo.goartifact/clients/sftp/sftp.goartifact/clients/smb/smb.goartifact/data.goartifact/fs.goartifact/fs/azure.goartifact/fs/gcs.goartifact/fs/local.goartifact/fs/s3.goartifact/fs/save.goartifact/fs/smb.goartifact/fs/ssh.goartifact/inline.goartifact/logged.goconnection/aws.goconnection/blob.goconnection/gcp.goconnection/gcs.goconnection/gke.gocontext/blobs.gogo.modlogs/gcpcloudlogging/cloud_logging.gomodels/artifacts.gomodels/artifacts_test.goschema/artifacts.hcltests/artifacts_test.gotests/e2e-blobs/blobs_test.gotests/e2e-blobs/containers.gotests/e2e-blobs/sftp-configuration.jsontests/e2e-blobs/suite_test.go
✅ Files skipped from review due to trivial changes (14)
- connection/gke.go
- connection/gcs.go
- connection/gcp.go
- logs/gcpcloudlogging/cloud_logging.go
- connection/aws.go
- tests/e2e-blobs/sftp-configuration.json
- artifact/fs.go
- artifact/clients/gcp/fileinfo.go
- artifact/clients/azure/fileinfo.go
- models/artifacts_test.go
- artifact/clients/smb/smb.go
- artifact/clients/sftp/sftp.go
- artifact/fs/smb.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (8)
- artifact/clients/aws/doc.go
- schema/artifacts.hcl
- Makefile
- tests/e2e-blobs/suite_test.go
- tests/artifacts_test.go
- artifact/logged.go
- tests/e2e-blobs/blobs_test.go
- artifact/fs/gcs.go
| mw := &mimeWriter{Max: maxBytesForMimeDetection} | ||
| fileReader := io.TeeReader(mimeReader, mw) | ||
|
|
||
| info, err := s.fs.Write(s.db.Statement.Context, data.Filename, fileReader) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how blobStore is instantiated and whether db already has a statement context set
rg -n -A5 "NewBlobStore\(" --type=goRepository: flanksource/duty
Length of output: 2010
🏁 Script executed:
cat -n artifact/blob_store.goRepository: flanksource/duty
Length of output: 3442
🏁 Script executed:
fd -t f "\.go$" | xargs rg -l "gorm.Open|\.DB\(\)" | head -5Repository: flanksource/duty
Length of output: 143
🏁 Script executed:
rg -n "func.*\(\).*\*gorm\.DB" --type=go | head -20Repository: flanksource/duty
Length of output: 235
🏁 Script executed:
sed -n '400,430p' context/context.goRepository: flanksource/duty
Length of output: 615
🏁 Script executed:
rg "db\.Statement\.Context" --type=go -C2Repository: flanksource/duty
Length of output: 661
🏁 Script executed:
rg "Statement\." --type=go | head -20Repository: flanksource/duty
Length of output: 1155
🏁 Script executed:
rg "type FilesystemRW" -A10 --type=goRepository: flanksource/duty
Length of output: 434
🏁 Script executed:
web_search
How to get context from gorm.DB instance without executing queryRepository: flanksource/duty
Length of output: 152
🌐 Web query:
GORM db.Statement.Context nil when fresh *gorm.DB created
💡 Result:
Yes, when you create a fresh *gorm.DB instance using gorm.Open, the db.Statement.Context field is nil by default. This is the expected behavior for a newly initialized DB connection, as no context has been explicitly set yet. To use a context with database operations, you must explicitly set it using db.WithContext(ctx) or db.Session(&gorm.Session{Context: ctx}). This creates a new DB session with the context propagated to its Statement. In hooks/callbacks (BeforeCreate, etc.), you can access the context via tx.Statement.Context, but only after a context has been set on the session. This design supports goroutine safety and method chaining - fresh DB instances are reusable until a chain/finisher method initializes the Statement, at which point you need Session/WithContext to create safe new instances.
Citations:
- 1: https://gorm.io/docs/context.html
- 2: Reuse context inside Gorm plugin "fails" go-gorm/gorm#7502
- 3: https://gorm.io/hi_IN/docs/session.html
- 4: https://www.mintlify.com/go-gorm/gorm/api/db
- 5: https://gorm.io/hi_IN/docs/method_chaining.html
🏁 Script executed:
rg "\.WithContext\(" --type=go -B2 -A2 | head -40Repository: flanksource/duty
Length of output: 1758
🏁 Script executed:
rg "db\.(WithContext|Statement)" --type=go -B3 -A3 | grep -A3 -B3 "func"Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
rg "func.*\(s \*\w+Store\)" --type=go -A15 | grep -E "(func|\.db\.|\.DB)" | head -30Repository: flanksource/duty
Length of output: 1990
🏁 Script executed:
rg "Statement\.Context" --type=go -B5 | grep -E "(Before|After|callback)" -A3 -B3 | head -20Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
rg "v\.WithContext" --type=go -B5 -A10Repository: flanksource/duty
Length of output: 679
🏁 Script executed:
# Check if there's any test coverage that shows the actual behavior
rg "blobStore\)" --type=go -A10 | grep -E "(Write|Read)" -B5 -A5 | head -50Repository: flanksource/duty
Length of output: 1077
🏁 Script executed:
# Check the test files to see if blobStore tests show the actual runtime behavior
cat tests/artifacts_test.go | grep -A30 "NewBlobStore"Repository: flanksource/duty
Length of output: 2023
🏁 Script executed:
# Check if there are any recent issues or PRs related to this
git log --oneline --all artifact/blob_store.go | head -10Repository: flanksource/duty
Length of output: 150
Unsafe access to s.db.Statement.Context—context may be nil.
s.db.Statement is only initialized after a query executes. On line 43 in the Write method, no query has run yet, so Statement.Context will be nil. On line 91 in the Read method, while a query runs at line 73, Statement is not reliably available on the receiver s.db after the query completes.
Accept context.Context as a parameter to Write() and Read(), or use s.db.WithContext(ctx) to safely bind a context before these operations.
Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/blob_store.go` at line 43, The Write and Read methods currently
access s.db.Statement.Context which can be nil; update these methods to accept a
context.Context parameter (e.g., func (s *BlobStore) Write(ctx context.Context,
... ) and Read(ctx context.Context, ...)) and use s.db.WithContext(ctx) when
calling the filesystem operations and any queries, or alternatively call s.db =
s.db.WithContext(ctx) locally before using s.db.Statement; replace direct uses
of s.db.Statement.Context with the provided ctx (or the WithContext-bound DB) to
ensure a non-nil context is used for the fs.Write/fs.Read and any DB calls.
Comment on lines
+38
to
+40
| func (obj S3FileInfo) FullPath() string { | ||
| return *obj.Object.Key | ||
| } |
Contributor
There was a problem hiding this comment.
Missing nil check in FullPath() can cause panic.
Name() guards against nil Key, but FullPath() dereferences obj.Object.Key directly. If Key is nil, this will panic.
Suggested fix
func (obj S3FileInfo) FullPath() string {
+ if obj.Object.Key == nil {
+ return ""
+ }
return *obj.Object.Key
}📝 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
| func (obj S3FileInfo) FullPath() string { | |
| return *obj.Object.Key | |
| } | |
| func (obj S3FileInfo) FullPath() string { | |
| if obj.Object.Key == nil { | |
| return "" | |
| } | |
| return *obj.Object.Key | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/clients/aws/fileinfo.go` around lines 38 - 40, FullPath() currently
dereferences obj.Object.Key without a nil check which can panic; update
S3FileInfo.FullPath to mirror the nil-safe behavior used in Name(): check that
obj.Object and obj.Object.Key are non-nil and return an empty string (or
appropriate default) when nil, otherwise return the string value of
*obj.Object.Key, ensuring consistency with Name()'s guarding logic.
Comment on lines
+64
to
+70
| for _, blob := range resp.Segment.BlobItems { | ||
| output = append(output, azureUtil.BlobFileInfo{ | ||
| BlobName: *blob.Name, | ||
| BlobSize: *blob.Properties.ContentLength, | ||
| LastMod: *blob.Properties.LastModified, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Nil pointer dereference risk on blob properties.
blob.Name, blob.Properties.ContentLength, and blob.Properties.LastModified are pointer types that may be nil for certain blob types or incomplete responses.
🛡️ Proposed defensive fix
for _, blob := range resp.Segment.BlobItems {
+ if blob.Name == nil || blob.Properties == nil {
+ continue
+ }
+ size := int64(0)
+ if blob.Properties.ContentLength != nil {
+ size = *blob.Properties.ContentLength
+ }
+ var lastMod time.Time
+ if blob.Properties.LastModified != nil {
+ lastMod = *blob.Properties.LastModified
+ }
output = append(output, azureUtil.BlobFileInfo{
- BlobName: *blob.Name,
- BlobSize: *blob.Properties.ContentLength,
- LastMod: *blob.Properties.LastModified,
+ BlobName: *blob.Name,
+ BlobSize: size,
+ LastMod: lastMod,
})
}📝 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
| for _, blob := range resp.Segment.BlobItems { | |
| output = append(output, azureUtil.BlobFileInfo{ | |
| BlobName: *blob.Name, | |
| BlobSize: *blob.Properties.ContentLength, | |
| LastMod: *blob.Properties.LastModified, | |
| }) | |
| } | |
| for _, blob := range resp.Segment.BlobItems { | |
| if blob.Name == nil || blob.Properties == nil { | |
| continue | |
| } | |
| size := int64(0) | |
| if blob.Properties.ContentLength != nil { | |
| size = *blob.Properties.ContentLength | |
| } | |
| var lastMod time.Time | |
| if blob.Properties.LastModified != nil { | |
| lastMod = *blob.Properties.LastModified | |
| } | |
| output = append(output, azureUtil.BlobFileInfo{ | |
| BlobName: *blob.Name, | |
| BlobSize: size, | |
| LastMod: lastMod, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/fs/azure.go` around lines 64 - 70, The loop that builds
azureUtil.BlobFileInfo dereferences pointer fields (blob.Name,
blob.Properties.ContentLength, blob.Properties.LastModified) without nil checks;
update the code in artifact/fs/azure.go where you append azureUtil.BlobFileInfo
to first defensively check for nil (blob and blob.Properties) and for each
pointer field before dereferencing, using safe defaults (empty string, 0, zero
time) or skipping the blob if appropriate, so BlobName, BlobSize and LastMod are
populated only from non-nil pointers; reference the blob iteration and the
azureUtil.BlobFileInfo construction to locate and implement these checks.
Comment on lines
+87
to
+90
| return azureUtil.BlobFileInfo{ | ||
| BlobName: name, | ||
| BlobSize: size, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Stat omits LastMod field, inconsistent with ReadDir.
ReadDir populates LastMod from blob properties, but Stat leaves it as the zero value. This inconsistency may cause issues for callers relying on modification time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/fs/azure.go` around lines 87 - 90, Stat currently returns a
BlobFileInfo with BlobName and BlobSize but leaves LastMod zero; mirror ReadDir
by fetching the blob's properties in Stat (use the same blob properties
retrieval used in ReadDir, e.g., the blob client's
GetProperties/GetPropertiesResponse or Properties().LastModified) and populate
BlobFileInfo.LastMod with that property (convert to time.Time as done in
ReadDir). Ensure you handle a missing/empty LastModified by falling back safely
to zero value and keep the BlobFileInfo return shape consistent with ReadDir.
Comment on lines
+52
to
+75
| func (t *localFS) ReadDir(name string) ([]artifact.FileInfo, error) { | ||
| if strings.Contains(name, "*") { | ||
| return t.ReadDirGlob(name) | ||
| } | ||
|
|
||
| path := filepath.Join(t.base, name) | ||
| files, err := os.ReadDir(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| output := make([]artifact.FileInfo, 0, len(files)) | ||
| for _, match := range files { | ||
| fullPath := filepath.Join(path, match.Name()) | ||
| info, err := os.Stat(fullPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| output = append(output, localFileInfo{FileInfo: info, fullpath: fullPath}) | ||
| } | ||
|
|
||
| return output, nil | ||
| } |
Contributor
There was a problem hiding this comment.
ReadDir bypasses path traversal protection.
safePath() is correctly used in Stat, Read, and Write, but ReadDir uses filepath.Join(t.base, name) directly without validation. An attacker could call ReadDir("../../../etc") to list arbitrary directories.
Suggested fix
func (t *localFS) ReadDir(name string) ([]artifact.FileInfo, error) {
if strings.Contains(name, "*") {
return t.ReadDirGlob(name)
}
- path := filepath.Join(t.base, name)
+ path, err := t.safePath(name)
+ if err != nil {
+ return nil, err
+ }
files, err := os.ReadDir(path)📝 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
| func (t *localFS) ReadDir(name string) ([]artifact.FileInfo, error) { | |
| if strings.Contains(name, "*") { | |
| return t.ReadDirGlob(name) | |
| } | |
| path := filepath.Join(t.base, name) | |
| files, err := os.ReadDir(path) | |
| if err != nil { | |
| return nil, err | |
| } | |
| output := make([]artifact.FileInfo, 0, len(files)) | |
| for _, match := range files { | |
| fullPath := filepath.Join(path, match.Name()) | |
| info, err := os.Stat(fullPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| output = append(output, localFileInfo{FileInfo: info, fullpath: fullPath}) | |
| } | |
| return output, nil | |
| } | |
| func (t *localFS) ReadDir(name string) ([]artifact.FileInfo, error) { | |
| if strings.Contains(name, "*") { | |
| return t.ReadDirGlob(name) | |
| } | |
| path, err := t.safePath(name) | |
| if err != nil { | |
| return nil, err | |
| } | |
| files, err := os.ReadDir(path) | |
| if err != nil { | |
| return nil, err | |
| } | |
| output := make([]artifact.FileInfo, 0, len(files)) | |
| for _, match := range files { | |
| fullPath := filepath.Join(path, match.Name()) | |
| info, err := os.Stat(fullPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| output = append(output, localFileInfo{FileInfo: info, fullpath: fullPath}) | |
| } | |
| return output, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/fs/local.go` around lines 52 - 75, ReadDir currently constructs the
directory path with filepath.Join(t.base, name) which bypasses path traversal
checks; replace that with the same path-sanitization used elsewhere by calling
t.safePath(name) (or the equivalent safePath method on localFS), handle any
error it returns before calling os.ReadDir, and then use the sanitized path for
building fullPath (e.g., filepath.Join(safePath, match.Name())) and subsequent
os.Stat calls so all filesystem accesses go through the safePath validation in
the ReadDir function.
Comment on lines
+106
to
+113
| return &awsUtil.S3FileInfo{ | ||
| Object: s3Types.Object{ | ||
| Key: utils.Ptr(filepath.Base(path)), | ||
| Size: headObject.ContentLength, | ||
| LastModified: headObject.LastModified, | ||
| ETag: headObject.ETag, | ||
| }, | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Stat returns incorrect FullPath() - breaks interface contract.
Setting Key to filepath.Base(path) means S3FileInfo.FullPath() returns only the filename, not the full S3 key. Per artifact/clients/aws/fileinfo.go:38-40, FullPath() returns *obj.Object.Key. Callers expecting the full path will get incorrect results.
🐛 Proposed fix
return &awsUtil.S3FileInfo{
Object: s3Types.Object{
- Key: utils.Ptr(filepath.Base(path)),
+ Key: utils.Ptr(path),
Size: headObject.ContentLength,
LastModified: headObject.LastModified,
ETag: headObject.ETag,
},
}, nilNote: If Name() (which typically returns filepath.Base) is needed separately, consider storing the full key and computing the base name in the Name() method implementation.
📝 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 &awsUtil.S3FileInfo{ | |
| Object: s3Types.Object{ | |
| Key: utils.Ptr(filepath.Base(path)), | |
| Size: headObject.ContentLength, | |
| LastModified: headObject.LastModified, | |
| ETag: headObject.ETag, | |
| }, | |
| }, nil | |
| return &awsUtil.S3FileInfo{ | |
| Object: s3Types.Object{ | |
| Key: utils.Ptr(path), | |
| Size: headObject.ContentLength, | |
| LastModified: headObject.LastModified, | |
| ETag: headObject.ETag, | |
| }, | |
| }, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@artifact/fs/s3.go` around lines 106 - 113, The Stat implementation is setting
s3Types.Object.Key to filepath.Base(path) which makes
awsUtil.S3FileInfo.FullPath() return only the filename; change Stat to set Key
to the full S3 key (the original path variable or a cleaned/normalized full key)
when constructing the awsUtil.S3FileInfo in the return, and if a base-name is
needed implement Name() to return filepath.Base(obj.Object.Key) instead of
storing only the base; update the return in the Stat function (the block
constructing awsUtil.S3FileInfo / s3Types.Object) to use the full key for Key so
FullPath() satisfies the interface contract.
Comment on lines
+124
to
+134
| case models.ConnectionTypeSFTP: | ||
| parsedURL, err := url.Parse(c.URL) | ||
| if err != nil { | ||
| return nil, "", err | ||
| } | ||
| port := c.Properties["port"] | ||
| if port == "" { | ||
| port = "22" | ||
| } | ||
| fs, err := artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, port), c.Username, c.Password) | ||
| return fs, "sftp", err |
Contributor
There was a problem hiding this comment.
SFTP host may contain port, causing double-port in address.
If c.URL is sftp://host:2222/path, then parsedURL.Host is host:2222. Appending :22 (or the configured port) produces host:2222:22.
🔧 Proposed fix using Hostname()
case models.ConnectionTypeSFTP:
parsedURL, err := url.Parse(c.URL)
if err != nil {
return nil, "", err
}
port := c.Properties["port"]
if port == "" {
- port = "22"
+ if parsedURL.Port() != "" {
+ port = parsedURL.Port()
+ } else {
+ port = "22"
+ }
}
- fs, err := artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Host, port), c.Username, c.Password)
+ fs, err := artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Hostname(), port), c.Username, c.Password)
return fs, "sftp", err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/blob.go` around lines 124 - 134, The SFTP branch builds the SSH
address using parsedURL.Host which can already include a port, causing addresses
like host:2222:22; update the logic in the models.ConnectionTypeSFTP case to
extract hostname and port separately using parsedURL.Hostname() and
parsedURL.Port(): if parsedURL.Port() is non-empty use it, else use
c.Properties["port"] (default "22"), then call
artifactFS.NewSSHFS(fmt.Sprintf("%s:%s", parsedURL.Hostname(), port),
c.Username, c.Password) and return fs, "sftp", err (keep c.URL, parsedURL,
c.Properties, artifactFS.NewSSHFS, c.Username, c.Password identifiers to locate
the code).
Comment on lines
+183
to
+200
| func compress(data []byte, compressionType string) ([]byte, error) { | ||
| switch compressionType { | ||
| case "gzip": | ||
| var buf bytes.Buffer | ||
| w := gzip.NewWriter(&buf) | ||
| if _, err := w.Write(data); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := w.Close(); err != nil { | ||
| return nil, err | ||
| } | ||
| return buf.Bytes(), nil | ||
| case "", "none": | ||
| return data, nil | ||
| default: | ||
| return nil, fmt.Errorf("unsupported compression type: %s", compressionType) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check schema comment about compression types
rg -n "compression" schema/artifacts.hcl -B 2 -A 2
# Check if zstd is used anywhere in the codebase
rg -n "zstd" --type goRepository: flanksource/duty
Length of output: 356
Schema mentions zstd compression but code doesn't support it.
The schema documentation at schema/artifacts.hcl:53 states "compression algorithm applied to content: gzip, zstd, or none", but the compress() function only handles "gzip", "", and "none". Either implement zstd support in both compress() and decompress() functions, or update the schema documentation to match the supported algorithms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/artifacts.go` around lines 183 - 200, The schema allows "zstd"
compression but compress() (and likely decompress()) only supports "gzip" and
"none"; add zstd handling to both functions by implementing a zstd branch for
compression and decompression using a zstd encoder/decoder (e.g., from
github.com/klauspost/compress/zstd) so compress(data, "zstd") returns
zstd-compressed bytes and decompress handles "zstd" similarly, or alternatively
update the schema to remove "zstd"—make changes in the compress and decompress
functions to add the "zstd" case and import the zstd package referenced by those
functions.
Comment on lines
+88
to
+94
| updateReq, _ := http.NewRequestWithContext(ctx, http.MethodPut, | ||
| emulatorURL+"/_internal/config", | ||
| strings.NewReader(fmt.Sprintf(`{"externalUrl": "%s"}`, emulatorURL))) | ||
| updateReq.Header.Set("Content-Type", "application/json") | ||
| if resp, err := http.DefaultClient.Do(updateReq); err == nil { | ||
| _ = resp.Body.Close() | ||
| } |
Contributor
There was a problem hiding this comment.
Error from NewRequestWithContext is ignored, risking nil pointer dereference.
If http.NewRequestWithContext fails, updateReq will be nil, and calling http.DefaultClient.Do(nil) on line 92 will panic.
🔧 Proposed fix
- updateReq, _ := http.NewRequestWithContext(ctx, http.MethodPut,
+ updateReq, err := http.NewRequestWithContext(ctx, http.MethodPut,
emulatorURL+"/_internal/config",
strings.NewReader(fmt.Sprintf(`{"externalUrl": "%s"}`, emulatorURL)))
- updateReq.Header.Set("Content-Type", "application/json")
- if resp, err := http.DefaultClient.Do(updateReq); err == nil {
- _ = resp.Body.Close()
+ if err == nil {
+ updateReq.Header.Set("Content-Type", "application/json")
+ if resp, err := http.DefaultClient.Do(updateReq); err == nil {
+ _ = resp.Body.Close()
+ }
}📝 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
| updateReq, _ := http.NewRequestWithContext(ctx, http.MethodPut, | |
| emulatorURL+"/_internal/config", | |
| strings.NewReader(fmt.Sprintf(`{"externalUrl": "%s"}`, emulatorURL))) | |
| updateReq.Header.Set("Content-Type", "application/json") | |
| if resp, err := http.DefaultClient.Do(updateReq); err == nil { | |
| _ = resp.Body.Close() | |
| } | |
| updateReq, err := http.NewRequestWithContext(ctx, http.MethodPut, | |
| emulatorURL+"/_internal/config", | |
| strings.NewReader(fmt.Sprintf(`{"externalUrl": "%s"}`, emulatorURL))) | |
| if err == nil { | |
| updateReq.Header.Set("Content-Type", "application/json") | |
| if resp, err := http.DefaultClient.Do(updateReq); err == nil { | |
| _ = resp.Body.Close() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e-blobs/containers.go` around lines 88 - 94, The call to
http.NewRequestWithContext is ignoring its error, which can leave updateReq nil
and cause a panic when passed to http.DefaultClient.Do; modify the code around
updateReq (the http.NewRequestWithContext call) to capture the returned error,
check if err != nil and handle it (fail the test or return the error) before
calling http.DefaultClient.Do, and only call Do when updateReq is non-nil; keep
the existing resp.Body.Close behavior after a successful Do.
…rnal backends Introduces a pluggable artifact storage system supporting multiple backends (S3, GCS, SMB, SFTP, local filesystem) and inline database storage. Includes compression support (gzip), filesystem abstraction interfaces, and database schema for storing artifacts either inline or in external cloud/object storage systems. External connections can be configured via connection URL properties.
moshloop
force-pushed
the
pr/inline-artifacts
branch
from
9e28319 to
7f2ce8b
Compare
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.
Summary by CodeRabbit
New Features
Tests
Chores