feat: check sources directory exists and is a directory#43
feat: check sources directory exists and is a directory#43feloy merged 2 commits intokortex-hub:mainfrom
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe init command's preRun now resolves the absolute sources directory and verifies it exists and is a directory, returning distinct errors for missing path, stat failures, or non-directory paths. Tests were expanded to cover these failure cases and extended E2E scenarios. Changes
Sequence Diagram(s)(omitted — change is a localized validation check and test updates, not a multi-component flow that warrants a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cmd/init_test.go (1)
206-206: Use a non-default relative path in this subtest.With
relativePath := ".", this case now overlaps heavily with"default arguments"and no longer really exercises resolution of an explicit relative source path. Using a real relative path would keep coverage on the branch this test is named after.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/init_test.go` at line 206, The test currently sets relativePath := "." which duplicates the default-arguments case; change relativePath to a real non-default relative path (e.g. "./some/relative/path" or a temp subdirectory created in the test) so the subtest actually exercises explicit relative source resolution; update any setup to create that directory (or file) and adjust expected outputs/assertions accordingly for the code paths exercised by Funcs that consume relativePath (locate the variable named relativePath in this subtest and ensure the created path is cleaned up after the test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/init_test.go`:
- Line 206: The test currently sets relativePath := "." which duplicates the
default-arguments case; change relativePath to a real non-default relative path
(e.g. "./some/relative/path" or a temp subdirectory created in the test) so the
subtest actually exercises explicit relative source resolution; update any setup
to create that directory (or file) and adjust expected outputs/assertions
accordingly for the code paths exercised by Funcs that consume relativePath
(locate the variable named relativePath in this subtest and ensure the created
path is cleaned up after the test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e7342bd-5633-4262-9859-ace667037780
📒 Files selected for processing (2)
pkg/cmd/init.gopkg/cmd/init_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cmd/init_test.go (1)
202-242:⚠️ Potential issue | 🟡 MinorUse
t.TempDir()for the relative path test to avoid polluting the working directory.The test creates
./relative/pathin the current working directory, which leaves artifacts behind and could interfere with parallel tests. Instead, create the relative path structure inside a temp directory and change the working directory for the test.As per coding guidelines: "Use
t.TempDir()for temporary directories in tests for automatic cleanup and cross-platform compatibility"🛠️ Proposed fix using t.TempDir() with os.Chdir
t.Run("relative sources directory", func(t *testing.T) { - t.Parallel() + // Cannot use t.Parallel() because we're changing the working directory tempDir := t.TempDir() - relativePath := filepath.Join(".", "relative", "path") - // Create the relative directory so it exists - if err := os.MkdirAll(relativePath, 0755); err != nil { - t.Fatalf("Failed to create relative directory: %v", err) + // Save current directory and restore after test + originalDir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current directory: %v", err) + } + defer os.Chdir(originalDir) + + // Change to temp directory + if err := os.Chdir(tempDir); err != nil { + t.Fatalf("Failed to change directory: %v", err) } + relativePath := filepath.Join(".", "relative", "path") + if err := os.MkdirAll(relativePath, 0755); err != nil { + t.Fatalf("Failed to create relative directory: %v", err) + } + c := &initCmd{}Note: This approach requires removing
t.Parallel()sinceos.Chdiraffects the entire process. Alternatively, consider testing with an absolute path that simulates the relative path scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/init_test.go` around lines 202 - 242, The test creates ./relative/path in the real working directory; switch to using t.TempDir() and change the process working directory for the test so artifacts are cleaned up: inside the "relative sources directory" test use tempRoot := t.TempDir(), create the relative structure under tempRoot (e.g. filepath.Join(tempRoot, "relative", "path")), then save cwd := os.Getwd(); defer os.Chdir(cwd); os.Chdir(tempRoot) so the relativePath := filepath.Join(".", "relative", "path") refers to the temp area; remove t.Parallel() because os.Chdir affects the whole process; keep assertions against c.sourcesDir, c.absSourcesDir (use filepath.Abs(relativePath)), and c.workspaceConfigDir (filepath.Join(relativePath, ".kortex")) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/cmd/init_test.go`:
- Around line 202-242: The test creates ./relative/path in the real working
directory; switch to using t.TempDir() and change the process working directory
for the test so artifacts are cleaned up: inside the "relative sources
directory" test use tempRoot := t.TempDir(), create the relative structure under
tempRoot (e.g. filepath.Join(tempRoot, "relative", "path")), then save cwd :=
os.Getwd(); defer os.Chdir(cwd); os.Chdir(tempRoot) so the relativePath :=
filepath.Join(".", "relative", "path") refers to the temp area; remove
t.Parallel() because os.Chdir affects the whole process; keep assertions against
c.sourcesDir, c.absSourcesDir (use filepath.Abs(relativePath)), and
c.workspaceConfigDir (filepath.Join(relativePath, ".kortex")) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4e2f37e-cf8c-4112-8edd-e1c51870a4ec
📒 Files selected for processing (2)
pkg/cmd/init.gopkg/cmd/init_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cmd/init.go
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/init_test.go`:
- Around line 202-225: The subtest t.Run("relative sources directory") mutates
the process working directory (os.Chdir) which is still unsafe because its
parent test is parallelized; either extract this case into its own top-level
Test (e.g., TestInitCmd_RelativeSourcesDirectory) that does not call
t.Parallel(), or stop mutating cwd by running the code under test with an
explicit working directory: create the same relative path inside workDir and
resolve paths via filepath.Abs or run the command with its Dir set (or pass the
absolute source path computed from workDir+relativePath) instead of calling
os.Chdir in the subtest; update the test named "relative sources directory" to
use one of these approaches and remove os.Chdir to avoid races with
TestInitCmd_E2E and other parallel tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a22bd2ea-23c8-4b20-94e1-ce088abf7352
📒 Files selected for processing (2)
pkg/cmd/init.gopkg/cmd/init_test.go
Signed-off-by: Philippe Martin <phmartin@redhat.com>
2 participants
Fixes #40