fix(cli): prevent noisy directory consolidation - #445
Conversation
4f3a806 to
053923f
Compare
|
This is linked to the approved #283 scope and looks focused on the remaining consolidate/prune bug. I added the |
|
This PR has been inactive for 30 days. Please rebase, update it, or leave a comment — otherwise it will be closed in 14 days. |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Reopening attention on this — the status:stale label is process drift, not code decay: groupSimilarProjects and cmdProjectsPrune are unchanged on main and the branch is still mergeable.
Your diagnosis is better than the issue's. #283 blamed a loose name-similarity threshold, but the actual collapse is the shared-directory union at cmd/engram/main.go:1946-1950, which unions every project sharing any directory — so one $HOME entry transitively merges the whole store. That matches Group 2 in the report containing <user-home> plus the path-named entries. Skipping directories touched by >3 distinct projects is the right lever, and leaving project.FindSimilar untouched is the right call.
One question before merge:
- The
--paths-onlyprune flag is #283 bug 2 cleanup, not bug 4. Would you split it into its own PR? It reuses existing prune machinery so it's low-risk, but this PR's title and tests are all about grouping.
Happy to merge the grouping fix as-is if you'd rather keep them together — just say so and I'll drop the request.
📝 WalkthroughWalkthroughThe consolidation grouping heuristic now ignores shared directories associated with too many distinct projects. Tests cover noisy and legitimate directory grouping, name similarity, and dry-run consolidation output. ChangesConsolidation grouping safeguard
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 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/main.go`:
- Around line 1956-1962: Apply the same maxSharedProjectsForDirMatch noise
filter to the single-project grouping logic near the unchanged matching code,
using distinctProjectCount(idxs) before accepting directory matches. Ensure
noisy directories such as $HOME cannot produce cross-project merge candidates,
and add regression coverage for single-project consolidate mode.
- Around line 1891-1898: Remove the distinctProjectCount helper and replace its
call site in the directory/project statistics flow with len(idxs). Preserve the
existing behavior, relying on the uniqueness contract of
ProjectStats.Directories and sequential dirToProjects population.
🪄 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: b5429372-7bca-4636-a5bf-734da55d45bf
📒 Files selected for processing (2)
cmd/engram/consolidate_grouping_test.gocmd/engram/main.go
| // distinctProjectCount returns the number of unique project indices in idxs. | ||
| func distinctProjectCount(idxs []int) int { | ||
| seen := make(map[int]struct{}, len(idxs)) | ||
| for _, i := range idxs { | ||
| seen[i] = struct{}{} | ||
| } | ||
| return len(seen) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Remove unnecessary distinctProjectCount helper.
Since ProjectStats.Directories contains unique directories per project (as per its contract) and dirToProjects is populated by iterating sequentially, index i is appended exactly once per project. This means idxs already contains strictly unique project indices, making len(idxs) identical to the distinct project count.
Allocating a map for every directory to count distinct projects adds unnecessary overhead, especially for noisy directories shared by thousands of projects. Consider removing distinctProjectCount entirely and replacing its usage with len(idxs).
♻️ Proposed refactor
-// distinctProjectCount returns the number of unique project indices in idxs.
-func distinctProjectCount(idxs []int) int {
- seen := make(map[int]struct{}, len(idxs))
- for _, i := range idxs {
- seen[i] = struct{}{}
- }
- return len(seen)
-}And update the call site at line 1960:
- if distinctProjectCount(idxs) > maxSharedProjectsForDirMatch {
+ if len(idxs) > maxSharedProjectsForDirMatch {📝 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.
| // distinctProjectCount returns the number of unique project indices in idxs. | |
| func distinctProjectCount(idxs []int) int { | |
| seen := make(map[int]struct{}, len(idxs)) | |
| for _, i := range idxs { | |
| seen[i] = struct{}{} | |
| } | |
| return len(seen) | |
| } |
🤖 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/main.go` around lines 1891 - 1898, Remove the distinctProjectCount
helper and replace its call site in the directory/project statistics flow with
len(idxs). Preserve the existing behavior, relying on the uniqueness contract of
ProjectStats.Directories and sequential dirToProjects population.
5d006fb to
69dba41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/engram/main.go (2)
1891-1904: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDuplicate:
distinctProjectCountstill redundant.Already flagged in a prior review round —
idxsfromdirToProjectsshould already contain unique project indices (assumingProjectStats.Directorieshas no duplicate entries per project), making the map allocation per directory unnecessary. This remains unaddressed.♻️ Proposed refactor
-// distinctProjectCount returns the number of unique project indices in idxs. -func distinctProjectCount(idxs []int) int { - seen := make(map[int]struct{}, len(idxs)) - for _, i := range idxs { - seen[i] = struct{}{} - } - return len(seen) -}And at the call site (line 1965): replace
distinctProjectCount(idxs)withlen(idxs).🤖 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/main.go` around lines 1891 - 1904, Remove the redundant distinctProjectCount helper and update its call site in the shared-directory grouping logic to use len(idxs) directly, preserving the existing unique-project counting behavior.
1961-1967: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicate: single-project mode still unprotected against noisy shared directories.
Already flagged previously as a major issue — this fix only guards the
--allgrouping path (groupSimilarProjects). The defaultprojects consolidate(single-project) path's shared-directory matching (unchanged, around lines 2075-2090) has no equivalent cap, so a canonical project whose cwd directory includes a noisy ancestor (e.g.$HOME) will still have every project sharing that directory proposed as a merge candidate. Issue#283bug 4 is only half-fixed. Also, no regression test exists for this remaining single-project-mode gap.🤖 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/main.go` around lines 1961 - 1967, The noisy shared-directory cap currently only protects groupSimilarProjects; apply equivalent distinctProjectCount and maxSharedProjectsForDirMatch filtering to the single-project projects consolidate matching path around the canonical project candidate logic. Ensure shared ancestors such as $HOME cannot produce merge candidates for unrelated projects, and add a regression test covering single-project mode.
🤖 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/consolidate_grouping_test.go`:
- Around line 22-71: Add boundary-focused tests around groupSimilarProjects
using exactly maxSharedProjectsForDirMatch distinct projects sharing one
directory and one additional case with four projects. Assert the three-project
case still forms a group and the four-project case produces no group, preserving
the existing noisy-directory behavior and detecting an off-by-one threshold
check.
---
Duplicate comments:
In `@cmd/engram/main.go`:
- Around line 1891-1904: Remove the redundant distinctProjectCount helper and
update its call site in the shared-directory grouping logic to use len(idxs)
directly, preserving the existing unique-project counting behavior.
- Around line 1961-1967: The noisy shared-directory cap currently only protects
groupSimilarProjects; apply equivalent distinctProjectCount and
maxSharedProjectsForDirMatch filtering to the single-project projects
consolidate matching path around the canonical project candidate logic. Ensure
shared ancestors such as $HOME cannot produce merge candidates for unrelated
projects, and add a regression test covering single-project mode.
🪄 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: e7eff6b7-572a-4d83-96d1-50f7d375285d
📒 Files selected for processing (2)
cmd/engram/consolidate_grouping_test.gocmd/engram/main.go
| // TestGroupSimilarProjects_NoisySharedDirNotGrouped reproduces issue #283 bug 4: | ||
| // many unrelated projects that merely share a common parent/root directory | ||
| // (e.g. $HOME or a session root) must NOT be unioned into one giant component. | ||
| // Before the fix, the shared-directory union step grouped all of them together | ||
| // and consolidate would propose merging them into a single canonical project — | ||
| // a catastrophic data-loss footgun. | ||
| func TestGroupSimilarProjects_NoisySharedDirNotGrouped(t *testing.T) { | ||
| // Names are deliberately long and mutually dissimilar (no substring overlap, | ||
| // Levenshtein distance well above the scaled threshold) so the ONLY possible | ||
| // grouping signal is the shared "/home/user" directory. | ||
| const sharedHome = "/home/user" | ||
| projects := []store.ProjectStats{ | ||
| {Name: "kubernetes", ObservationCount: 5, Directories: []string{"/home/user/kubernetes", sharedHome}}, | ||
| {Name: "photoshop", ObservationCount: 7, Directories: []string{"/home/user/photoshop", sharedHome}}, | ||
| {Name: "wireguard", ObservationCount: 3, Directories: []string{"/home/user/wireguard", sharedHome}}, | ||
| {Name: "blender", ObservationCount: 9, Directories: []string{"/home/user/blender", sharedHome}}, | ||
| {Name: "terraform", ObservationCount: 2, Directories: []string{"/home/user/terraform", sharedHome}}, | ||
| } | ||
|
|
||
| groups := groupSimilarProjects(projects) | ||
|
|
||
| // The five names are mutually dissimilar, so the ONLY thing that could group | ||
| // them is the shared "/home/user" directory. Since that directory is touched | ||
| // by more than maxSharedProjectsForDirMatch distinct projects, it must be | ||
| // treated as noise and skipped — yielding no groups at all. | ||
| if len(groups) != 0 { | ||
| t.Fatalf("expected 0 groups (noisy shared dir must be skipped), got %d: %+v", len(groups), groups) | ||
| } | ||
| } | ||
|
|
||
| // TestGroupSimilarProjects_RealSharedDirStillGroups guards against over-correcting: | ||
| // when only a small number of projects share a directory (a genuine rename signal), | ||
| // they SHOULD still be grouped. | ||
| func TestGroupSimilarProjects_RealSharedDirStillGroups(t *testing.T) { | ||
| const sharedRepo = "/repos/shared-monorepo" | ||
| projects := []store.ProjectStats{ | ||
| {Name: "webapp", ObservationCount: 12, Directories: []string{sharedRepo}}, | ||
| {Name: "legacy-portal", ObservationCount: 4, Directories: []string{sharedRepo}}, | ||
| } | ||
|
|
||
| groups := groupSimilarProjects(projects) | ||
|
|
||
| g := groupContaining(groups, "webapp") | ||
| if g == nil { | ||
| t.Fatalf("expected webapp and legacy-portal to be grouped via shared dir, got groups: %+v", groups) | ||
| } | ||
| if len(g.Names) != 2 { | ||
| t.Fatalf("expected group of 2, got %d: %+v", len(g.Names), g.Names) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add a boundary test at exactly maxSharedProjectsForDirMatch.
Existing tests only check well above (5) and well below (2) the cap of 3; there's no test for exactly 3 (should still group) vs exactly 4 (should be skipped) distinct projects sharing a directory — the precise boundary where an off-by-one in the > comparison would go unnoticed.
As per path instructions, "Verify coverage of happy path, error paths, and edge cases" for **/*_test.go files.
🤖 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/consolidate_grouping_test.go` around lines 22 - 71, Add
boundary-focused tests around groupSimilarProjects using exactly
maxSharedProjectsForDirMatch distinct projects sharing one directory and one
additional case with four projects. Assert the three-project case still forms a
group and the four-project case produces no group, preserving the existing
noisy-directory behavior and detecting an off-by-one threshold check.
Source: Path instructions
Maicololiveras
changed the title
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 consolidate --allfrom grouping unrelated projects through noisy shared directories such as$HOME.📂 Changes
cmd/engram/main.gocmd/engram/consolidate_grouping_test.go🧪 Test Plan
go test ./cmd/engram -run "Test(GroupSimilarProjects|CmdProjectsConsolidateAllDoesNotMergeNoisySharedDir)" -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 grouping change)main: onlycmd/engram/main.goandcmd/engram/consolidate_grouping_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 PRgo test ./...go test -tags e2e ./internal/server/...Co-Authored-Bytrailers in commits💬 Notes for Reviewers
This PR is limited to issue #283 bug 4. The
--paths-onlycleanup belongs to bug 2 and has moved to #647.Chain Context
Both slices target
mainand can merge independently.Start state: unrelated projects sharing a noisy ancestor directory can collapse into one consolidation group.
End state: noisy directories are ignored while legitimate grouping signals remain active.
Out of scope: path-named project cleanup, write-boundary validation, and unrelated consolidation heuristics.