Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion pkg/rag/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -574,12 +574,25 @@ func (m *Manager) readFile(path string) (string, error) {
return string(data), nil
}

// GetAbsolutePaths converts doc paths to absolute paths
// GetAbsolutePaths converts doc paths to absolute paths relative to basePath.
// If basePath is empty (e.g. for OCI/URL sources), relative paths are resolved
// against the current working directory.
func GetAbsolutePaths(basePath string, docPaths []string) []string {
var absPaths []string
for _, p := range docPaths {
if filepath.IsAbs(p) {
absPaths = append(absPaths, p)
continue
}
if basePath == "" {
slog.Debug("Resolving relative path with empty basePath, using working directory", "path", p)
abs, err := filepath.Abs(p)
if err != nil {
slog.Warn("Failed to resolve absolute path, using as-is", "path", p, "error", err)
absPaths = append(absPaths, p)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM: Error handling violates function contract

When filepath.Abs(p) fails, the code falls back to appending the original relative path unchanged:

absPaths = append(absPaths, p)

This violates the function's contract — GetAbsolutePaths promises to return absolute paths, but can silently return relative paths when filepath.Abs fails. Downstream code expecting absolute paths may encounter file-not-found errors or incorrect path comparisons.

Recommendation: Return an error instead of silently degrading to relative paths, or document this fallback behavior explicitly in the function comment and ensure all callers handle it.

} else {
absPaths = append(absPaths, abs)
}
} else {
absPaths = append(absPaths, filepath.Join(basePath, p))
}
Expand Down
33 changes: 33 additions & 0 deletions pkg/rag/manager_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package rag

import (
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestGetAbsolutePaths_WithBasePath(t *testing.T) {
result := GetAbsolutePaths("/base", []string{"relative/file.go", "/absolute/file.go"})
assert.Equal(t, []string{"/base/relative/file.go", "/absolute/file.go"}, result)
}

func TestGetAbsolutePaths_EmptyBasePath(t *testing.T) {
// When basePath is empty (OCI/URL sources), relative paths should be
// resolved against the current working directory instead of producing
// broken paths like "relative/file.go".
cwd, err := os.Getwd()
require.NoError(t, err)

result := GetAbsolutePaths("", []string{"relative/file.go", "/absolute/file.go"})

assert.Equal(t, filepath.Join(cwd, "relative", "file.go"), result[0])
assert.Equal(t, "/absolute/file.go", result[1])
}

func TestGetAbsolutePaths_NilInput(t *testing.T) {
result := GetAbsolutePaths("/base", nil)
assert.Nil(t, result)
}
29 changes: 24 additions & 5 deletions pkg/rag/strategy/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ func ResolveDatabasePath(dbCfg latest.RAGDatabaseConfig, parentDir, defaultName

// If it's a relative file path, make it absolute
if !filepath.IsAbs(dbStr) {
if parentDir == "" {
slog.Debug("Resolving relative database path with empty parentDir, using working directory", "path", dbStr)
abs, err := filepath.Abs(dbStr)
if err != nil {
return "", fmt.Errorf("failed to resolve absolute path for %q: %w", dbStr, err)
}
return abs, nil
}
return filepath.Join(parentDir, dbStr), nil
}

Expand Down Expand Up @@ -165,12 +173,23 @@ func GetParamPtr[T any](params map[string]any, key string) *T {
}
}

// makeAbsolute makes a path absolute relative to parentDir
func makeAbsolute(path, parentDir string) string {
if filepath.IsAbs(path) {
return path
// makeAbsolute makes a path absolute relative to parentDir.
// If parentDir is empty (e.g. for OCI/URL sources), the path is resolved
// against the current working directory.
func makeAbsolute(p, parentDir string) string {
if filepath.IsAbs(p) {
return p
}
if parentDir == "" {
slog.Debug("Resolving relative path with empty parentDir, using working directory", "path", p)
abs, err := filepath.Abs(p)
if err != nil {
slog.Warn("Failed to resolve absolute path, using as-is", "path", p, "error", err)
return p
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM: Error handling violates function contract

When filepath.Abs(p) fails and parentDir is empty, the function returns the original relative path unchanged:

return p

This violates the function's contract — makeAbsolute promises to make paths absolute, but can silently return relative paths when filepath.Abs fails. This is inconsistent with ResolveDatabasePath (which returns an error on failure) and could mask real problems like filesystem permission issues.

Recommendation: Either return an error (matching ResolveDatabasePath behavior) or document this fallback behavior. Consider whether callers can safely handle relative paths.

}
return abs
}
return filepath.Join(parentDir, path)
return filepath.Join(parentDir, p)
}

// EmitEvent sends an event to the events channel using non-blocking send
Expand Down
73 changes: 73 additions & 0 deletions pkg/rag/strategy/helpers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package strategy

import (
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/docker/docker-agent/pkg/config/latest"
)

// newDBConfig creates a RAGDatabaseConfig for testing via YAML unmarshaling.
func newDBConfig(t *testing.T, value string) latest.RAGDatabaseConfig {
t.Helper()
var cfg latest.RAGDatabaseConfig
err := cfg.UnmarshalYAML(func(v any) error {
p, ok := v.(*string)
if !ok {
return nil
}
*p = value
return nil
})
require.NoError(t, err)
return cfg
}

func TestMakeAbsolute_WithParentDir(t *testing.T) {
assert.Equal(t, "/parent/relative.go", makeAbsolute("relative.go", "/parent"))
assert.Equal(t, "/absolute/file.go", makeAbsolute("/absolute/file.go", "/parent"))
}

func TestMakeAbsolute_EmptyParentDir(t *testing.T) {
cwd, err := os.Getwd()
require.NoError(t, err)

result := makeAbsolute("relative.go", "")
assert.Equal(t, filepath.Join(cwd, "relative.go"), result)
}

func TestResolveDatabasePath_EmptyParentDir(t *testing.T) {
cwd, err := os.Getwd()
require.NoError(t, err)

result, err := ResolveDatabasePath(newDBConfig(t, "./my.db"), "", "default")
require.NoError(t, err)
assert.Equal(t, filepath.Join(cwd, "my.db"), result)
}

func TestResolveDatabasePath_AbsolutePathIgnoresParentDir(t *testing.T) {
result, err := ResolveDatabasePath(newDBConfig(t, "/absolute/my.db"), "/parent", "default")
require.NoError(t, err)
assert.Equal(t, "/absolute/my.db", result)
}

func TestResolveDatabasePath_RelativeWithParentDir(t *testing.T) {
result, err := ResolveDatabasePath(newDBConfig(t, "./my.db"), "/parent", "default")
require.NoError(t, err)
assert.Equal(t, "/parent/my.db", result)
}

func TestMergeDocPaths_EmptyParentDir(t *testing.T) {
cwd, err := os.Getwd()
require.NoError(t, err)

result := MergeDocPaths([]string{"shared.go"}, []string{"extra.go"}, "")
assert.Equal(t, []string{
filepath.Join(cwd, "shared.go"),
filepath.Join(cwd, "extra.go"),
}, result)
}
Loading