fix(cli): add path-only project pruning#647
Conversation
📝 WalkthroughWalkthroughThe ChangesPath-only project pruning
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/engram/prune_paths_test.go`:
- Line 129: Add a test in the prune-paths test suite covering the --paths-only
flow when filtering produces zero candidates. Assert that the command follows
the no-candidate branch and emits “No path-named projects to prune.”, while
preserving existing happy-path and mixed-project coverage.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c034e52-44fc-4794-ad1a-9050b827f70f
📒 Files selected for processing (2)
cmd/engram/main.gocmd/engram/prune_paths_test.go
| if !has("realproject") { | ||
| t.Fatalf("project with observations must remain: %v", names) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add test coverage for the "no candidates" edge case.
As per path instructions, we must verify coverage of error paths and edge cases. The current test suite effectively covers the happy path and mixed-project scenarios, but it lacks a test for the alternate path where the --paths-only filter results in zero candidates (which triggers the "No path-named projects to prune." message added to main.go).
Please consider adding a test like the one below to ensure this edge case is fully covered.
🧪 Proposed test addition
}
+
+// TestCmdProjectsPrunePathsOnlyNoCandidates verifies that when no path-named projects exist,
+// it exits gracefully with the correct message.
+func TestCmdProjectsPrunePathsOnlyNoCandidates(t *testing.T) {
+ cfg := testConfig(t)
+
+ // Only legitimate empty project, no garbage.
+ mustSeedSession(t, cfg, "s-legit", "legit-empty")
+
+ withArgs(t, "engram", "projects", "prune", "--paths-only")
+ stdout, stderr := captureOutput(t, func() { cmdProjectsPrune(cfg) })
+
+ if stderr != "" {
+ t.Fatalf("expected no stderr, got: %q", stderr)
+ }
+ if !strings.Contains(stdout, "No path-named projects to prune.") {
+ t.Fatalf("expected no candidates message, got: %q", stdout)
+ }
+}📝 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.
| } | |
| } | |
| // TestCmdProjectsPrunePathsOnlyNoCandidates verifies that when no path-named projects exist, | |
| // it exits gracefully with the correct message. | |
| func TestCmdProjectsPrunePathsOnlyNoCandidates(t *testing.T) { | |
| cfg := testConfig(t) | |
| // Only legitimate empty project, no garbage. | |
| mustSeedSession(t, cfg, "s-legit", "legit-empty") | |
| withArgs(t, "engram", "projects", "prune", "--paths-only") | |
| stdout, stderr := captureOutput(t, func() { cmdProjectsPrune(cfg) }) | |
| if stderr != "" { | |
| t.Fatalf("expected no stderr, got: %q", stderr) | |
| } | |
| if !strings.Contains(stdout, "No path-named projects to prune.") { | |
| t.Fatalf("expected no candidates message, got: %q", stdout) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/engram/prune_paths_test.go` at line 129, Add a test in the prune-paths
test suite covering the --paths-only flow when filtering produces zero
candidates. Assert that the command follows the no-candidate branch and emits
“No path-named projects to prune.”, while preserving existing happy-path and
mixed-project coverage.
Source: Path instructions
j0k3r-dev-rgl
left a comment
j0k3r-dev-rgl
left a comment
There was a problem hiding this comment.
Thanks for keeping this cleanup slice focused. Before merging, please add regression coverage for the destructive edge cases:
- A path-named project with only soft-deleted observations, which still retain foreign-key references to its sessions. Verify that
PruneProjecthandles the failure safely and that the CLI does not report a successful prune when deletion did not complete. --paths-onlywhen no candidates match, asserting theNo path-named projects to prune.branch.
These cases matter because this command deletes persisted data, so we need to prove both the safety boundary and the user-visible result. After updating the PR, please rerun the full CI checks and we can review it again.
2 participants
🔗 Linked Issue
Closes #283
🏷️ PR Type
type:bug: Bug fixtype:feature: New featuretype:docs: Documentation onlytype:refactor: Code refactoring (no behavior change)type:chore: Maintenance, dependencies, toolingtype:breaking-change: Breaking change📝 Summary
projects prune --paths-onlyfor cleaning up legacy projects whose names are filesystem paths.📂 Changes
cmd/engram/main.go--paths-onlyflag, path-name detection, candidate filtering, and focused empty-state output.cmd/engram/prune_paths_test.go🧪 Test Plan
go test ./cmd/engram -run "Test(IsPathLikeProjectName|CmdProjectsPrunePathsOnly)" -count=1go test ./...(not rerun; this Windows host has a known unrelated package-suite failure and timeout)go test -tags e2e ./internal/server/...(not required for this CLI-only prune change)main: onlycmd/engram/main.goandcmd/engram/prune_paths_test.go🤖 Automated Checks
These run automatically and all must pass before merge:
Closes #N/Fixes #N/Resolves #Nstatus:approvedlabeltype:*labelgo test ./...passesgo test -tags e2e ./internal/server/...passes✅ Contributor Checklist
Closes #283)type:*label to this PR (requires maintainer permission)go test ./...go test -tags e2e ./internal/server/...Co-Authored-Bytrailers in commits💬 Notes for Reviewers
This PR is issue #283 bug 2 and is the second independent slice of the cleanup. The grouping safeguard remains in #445.
Chain Context
Both slices target
mainand can merge independently.Start state: legacy path-named projects require broad pruning that also selects legitimate empty projects.
End state:
--paths-onlyselects only zero-observation projects whose names contain filesystem separators.Prior dependency: none. This slice is independent from #445.
Out of scope: consolidation grouping and project write-boundary behavior.