fix: clean up ~/.devsy on-disk schema drift - #888
Conversation
…ical DEVSY_HOME layout
- pathmanager.go: canonical-layout doc's "never construct a DEVSY_HOME-relative path outside this file" was an unqualified absolute contradicted by pkg/agent/workspace.go's deliberate agent-side exception; also fixed RuntimeDir's default location, which is the OS temp dir on linux, not DataDir()-adjacent. - config.go: CurrentSchemaVersion's comment claimed SaveConfig stamps the version too; only LoadConfig does — SaveConfig persists whatever is already in memory. - delete.go: SweepOrphanContentDirs cited the agent-side "skip if user-owned local folder" path as a cause of orphaned content dirs; that path never creates a content dir in the first place, so it can't leave one behind. - workspace_test.go: reworded a test comment that overclaimed the DEVSY_HOME fix closes a reachable production bug — both production callers already go through findDir, which short-circuits on DEVSY_HOME before reaching the function this test covers.
✅ Deploy Preview for images-devsy-sh canceled.
|
📝 WalkthroughWalkthroughThe PR versions on-disk configuration, centralizes provider cache paths, updates agent directory resolution, and removes stale context, machine, and workspace content directories after deletion operations. ChangesFilesystem layout and cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
✅ Deploy Preview for devsydev canceled.
|
- golines: reformat cmd/machine/delete_test.go, pkg/agent/workspace_test.go, pkg/config/config_test.go - gosec (G301): tighten test-fixture directory permissions from 0o755 to 0o750 in cmd/context/delete_test.go and cmd/machine/delete_test.go - lll: wrap the RuntimeDir line in pkg/config/pathmanager.go's canonical- layout doc comment to stay under 120 chars
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/provider/version_cache_test.go (1)
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the path manager in this test.
SaveProviderVersionCacheinitializesconfig.DefaultPathManager()with this temporaryDEVSY_HOME. The singleton remains bound to that directory aftert.Setenvrestores the environment. Reset it before setup and during cleanup.Proposed change
func TestCacheRoundtrip(t *testing.T) { + config.ResetPathManager() + t.Cleanup(config.ResetPathManager) dir := t.TempDir() t.Setenv("DEVSY_HOME", dir)🤖 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 `@pkg/provider/version_cache_test.go` around lines 34 - 38, Reset the singleton returned by config.DefaultPathManager() before setting up the temporary DEVSY_HOME and again during test cleanup, so SaveProviderVersionCache binds to the test directory and does not leak state across tests. Update the test containing the wantPath assertion and use the path-manager reset mechanism already provided by the config package.
🤖 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.
Nitpick comments:
In `@pkg/provider/version_cache_test.go`:
- Around line 34-38: Reset the singleton returned by config.DefaultPathManager()
before setting up the temporary DEVSY_HOME and again during test cleanup, so
SaveProviderVersionCache binds to the test directory and does not leak state
across tests. Update the test containing the wantPath assertion and use the
path-manager reset mechanism already provided by the config package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c625ac10-50e0-4420-8f72-ea644171073a
📒 Files selected for processing (13)
cmd/context/delete.gocmd/context/delete_test.gocmd/machine/delete.gocmd/machine/delete_test.gopkg/agent/workspace.gopkg/agent/workspace_test.gopkg/config/config.gopkg/config/config_test.gopkg/config/pathmanager.gopkg/provider/version_cache.gopkg/provider/version_cache_test.gopkg/workspace/delete.gopkg/workspace/delete_test.go
Summary
Fixes six correctness bugs and one documentation gap in devsy's on-disk state directory (
~/.devsy, akaDEVSY_HOME) that let files accumulate without cleanup and let paths drift from the centralPathManagerabstraction:PathManager.CacheDir()instead of a hand-rolled path that landed it in the wrong root.devsy machine deleteanddevsy context deletenow actually remove their on-disk directories (previously onlyprovider deleteandpro logoutdid this correctly).contents/tree, alongside the existing sweep forworkspaces/.candidateAgentDirs()honor$DEVSY_HOMEinstead of always probing the raw OS home directory.SchemaVersionintoconfig.yamland documents the canonical on-disk layout onPathManager, giving future layout changes a baseline to migrate from.Each task was implemented and reviewed independently (spec compliance + code quality), followed by a whole-branch review and a comment-accuracy pass. One whole-branch finding is worth calling out explicitly: the
candidateAgentDirs()fix (commit10d5a892) is correct and defensive, but turned out to be unreachable from both production callers today —findDiralready short-circuits on$DEVSY_HOMEbefore ever reaching that function. It hardens against a future caller that bypassesfindDir, but does not change current observable behavior.A CodeRabbit pass on the branch surfaced two findings; both were verified against the code and are false positives (details in the review thread if useful, not included here since neither required a change).
Not in scope (documented as follow-up)
$DEVSY_HOME-blindness half is fixed here).prune/gccommand exists yet to build on).Summary by CodeRabbit
New Features
Bug Fixes
Documentation