-
Notifications
You must be signed in to change notification settings - Fork 316
fix: resolve relative paths against CWD when parentDir is empty #2221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| } | ||
|
|
||
|
|
@@ -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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM: Error handling violates function contract When return pThis violates the function's contract — Recommendation: Either return an error (matching |
||
| } | ||
| return abs | ||
| } | ||
| return filepath.Join(parentDir, path) | ||
| return filepath.Join(parentDir, p) | ||
| } | ||
|
|
||
| // EmitEvent sends an event to the events channel using non-blocking send | ||
|
|
||
| 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) | ||
| } |
There was a problem hiding this comment.
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:This violates the function's contract —
GetAbsolutePathspromises to return absolute paths, but can silently return relative paths whenfilepath.Absfails. 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.