fix(install): do not remove local source dirs in CleanupDiscovery#140
Merged
runkids merged 2 commits intorunkids:mainfrom Apr 22, 2026
Merged
Conversation
- Local discovery sets RepoPath to the user's directory. - the UI discover handler defers CleanupDiscovery and was deleting that tree after the response. - Skip RemoveAll for SourceTypeLocalPath so UI local installs match CLI behavior. - Adds TestCleanupDiscovery_PreservesLocalSource regression coverage.
There was a problem hiding this comment.
Code Review
This pull request prevents the accidental deletion of local user directories during the discovery cleanup process by verifying the source type before calling os.RemoveAll. It also introduces a regression test to ensure local paths remain intact. The review feedback suggests a more robust safety check by returning early if the source information is missing and using the IsGit() helper to restrict cleanup to temporary remote repositories.
- Address review: only RemoveAll discovery temp dirs when Source is non-nil and IsGit(), avoiding deletion for local paths, nil Source, or unknown types. - Add TestCleanupDiscovery_SkipsWhenSourceNil.
8d0c418 to
2a55f88
Compare
Owner
|
Thanks for the quick fix and the clear write-up! 🙌 |
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
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.
Local discovery sets RepoPath to the user's directory; the UI discover handler defers CleanupDiscovery and was deleting that tree after the response. Skip RemoveAll for SourceTypeLocalPath so UI local installs match CLI behavior.
Adds TestCleanupDiscovery_PreservesLocalSource regression coverage.
Type
proposals/only — see CONTRIBUTING.md)Linked Issue
Closes #
Checklist
make check) — for code changes