feat(purge): rework index cleanup with cwd default and new flags#172
Open
Ismael wants to merge 2 commits into
Open
feat(purge): rework index cleanup with cwd default and new flags#172Ismael wants to merge 2 commits into
Ismael wants to merge 2 commits into
Conversation
Change "lumen purge" with no arguments to remove only the current
project's index (normalized to its git root) instead of wiping
everything, and add explicit modes for the broader operations:
--all Remove every index, including legacy indexes that predate
project_path metadata.
--missing Remove indexes whose recorded project folder no longer
exists; only deletes when the folder is confirmed missing.
--dry-run With --missing, preview removals without deleting.
Each removed index is logged individually, mutually exclusive flag
combinations are rejected, and the data-dir scan is shared across all
modes. README and command help updated accordingly.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR expands the ChangesPurge Command Mode Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (1)
cmd/purge_test.go (1)
268-342: ⚡ Quick winRefactor these related flag-behavior tests into a table-driven test.
These cases are a good candidate for one table-driven test (
name,flags,args,wantErr,wantErrContains,wantStderrContains,postCheck) to reduce duplication and make future mode additions safer.As per coding guidelines, "Use table-driven tests for multiple test cases in Go".
🤖 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/purge_test.go` around lines 268 - 342, Several closely related tests (TestPurge_Missing_RemovesOnlyDeletedFolders, TestPurge_Missing_AllFoldersExist_RemovesNothing, TestPurge_Missing_WithPaths_Errors, TestPurge_DryRun_Missing_DeletesNothing, TestPurge_DryRun_WithoutMissing_Errors) exercise the same purge flag behaviors and should be consolidated into a single table-driven test; replace these individual tests with one TestPurge_FlagBehavior that defines test cases with fields like name, flags ([]string), args ([]string), wantErr (bool), wantErrContains (string), wantStderrContains (string), and postCheck (func(t,*testing.T)), and for each case call resolvedTempDir, seedIndex, runPurgeCmd and assert expectations (use flagMissing and flagDryRun constants, runPurgeCmd, and verify filesystem state via os.Stat inside postCheck) to cover the same assertions as the originals while removing duplicated setup/teardown code.
🤖 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/purge_test.go`:
- Around line 308-315: Add a new unit test (or extend existing tests) to assert
that using the mutually-exclusive flags `--all` and `--missing` produces an
error: create a test similar to TestPurge_Missing_WithPaths_Errors that calls
runPurgeCmd with both flagAll and flagMissing (e.g., []string{flagAll,
flagMissing} or the equivalent string flags) and require.Error on the result,
then assert the error message mentions the conflict (e.g., contains "--all" and
"--missing"); also add the same assertion for the other location noted (the
block around the Test at lines 335-342) to cover both cases.
In `@cmd/purge.go`:
- Around line 141-145: The call to scanIndexes in purge.go may return an error
that is currently ignored, causing --all to proceed to delete the whole dataDir
without performing the promised per-index logging; update the code around
scanIndexes(dataDir) to capture the returned error (e.g., indexMap, legacy, err
:= scanIndexes(dataDir)), check err immediately, and return or propagate that
error before performing any irreversible delete operations so the command fails
safely when the pre-delete scan fails.
- Around line 235-238: The current logic treats any non-empty error from
store.ReadMetaAt as a missing metadata case and appends hashDir to legacy;
change the condition so only the “found but empty” case (err == nil && stored ==
"") appends to legacy, and handle err != nil separately: when store.ReadMetaAt
returns an error, wrap/return that error (or increment/log a distinct failure
counter) instead of classifying it as legacy; update the block around
store.ReadMetaAt(...) in cmd/purge.go to check err first and use the explicit
stored=="" branch for legacy so real DB/read failures (from ReadMetaAt) are not
silently skipped.
---
Nitpick comments:
In `@cmd/purge_test.go`:
- Around line 268-342: Several closely related tests
(TestPurge_Missing_RemovesOnlyDeletedFolders,
TestPurge_Missing_AllFoldersExist_RemovesNothing,
TestPurge_Missing_WithPaths_Errors, TestPurge_DryRun_Missing_DeletesNothing,
TestPurge_DryRun_WithoutMissing_Errors) exercise the same purge flag behaviors
and should be consolidated into a single table-driven test; replace these
individual tests with one TestPurge_FlagBehavior that defines test cases with
fields like name, flags ([]string), args ([]string), wantErr (bool),
wantErrContains (string), wantStderrContains (string), and postCheck
(func(t,*testing.T)), and for each case call resolvedTempDir, seedIndex,
runPurgeCmd and assert expectations (use flagMissing and flagDryRun constants,
runPurgeCmd, and verify filesystem state via os.Stat inside postCheck) to cover
the same assertions as the originals while removing duplicated setup/teardown
code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e66f5bca-1984-4341-b0e8-5cac3a01e823
📒 Files selected for processing (3)
README.mdcmd/purge.gocmd/purge_test.go
Add --legacy flag to remove only legacy indexes (created by older binaries without project_path metadata) plus unreadable/corrupt index directories. Legacy indexes remain usable by the system (located by path hash) but are invisible to path- and --missing-based purge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworks lumen purge to be safe-by-default and adds explicit modes for broader cleanup. Previously, lumen purge with no arguments wiped every index. Now no-args purges only the current project, and destructive/bulk operations are opt-in behind flags.
Notes
Closes #166
Summary by CodeRabbit
New Features
Documentation
Tests