From 1efd9b9c716b7c222f6a83a45c61f8353b781f47 Mon Sep 17 00:00:00 2001 From: Youngsup Oh Date: Sat, 18 Jul 2026 00:28:57 +0900 Subject: [PATCH] feat(cleaner): per-item delete strategy via DeleteMethod contract Path-only deletion couldn't express command/API reclaims (docker rmi, vendor prunes). ScanResult now carries an optional DeleteMethod (kind path/command/api + display + Run); the cleaner applies its protected/dry-run gates uniformly, then runs the method or falls back to trash/force. A method without Run is refused rather than falling back to path removal. VendorCleanup embeds the same DeleteMethod, so bulk and per-item reclaims share one execution contract. Closes #16 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_013SX3vRs78zFYA2kTRZAtg6 --- CLAUDE.md | 1 + docs/architecture.md | 26 +++++- internal/cleaner/cleaner.go | 20 +++- internal/cleaner/cleaner_test.go | 154 +++++++++++++++++++++++++++++-- internal/cli/clean.go | 4 +- internal/integration_test.go | 2 +- internal/model/types.go | 34 +++++++ internal/model/types_test.go | 50 ++++++++++ internal/scanner/scanner.go | 11 ++- internal/scanner/xcode.go | 9 +- internal/scanner/xcode_test.go | 7 +- 11 files changed, 291 insertions(+), 27 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3ea6bda..a83c0d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -51,6 +51,7 @@ Pipeline: **Scan → Classify → Filter/Sort → Output/Clean** - **AI agent friendly**: all interactions via CLI flags (`--yes`, `--json`), no interactive prompts required - **Metadata enrichment**: scanners populate `ScanResult.Label` / `Recommendation` so users can decide without decoding paths (UUID → "iPhone 17 Pro · iOS 26.3", "superseded by newer build", "runtime unavailable"). First applied in xcode; reusable for any ecosystem with opaque identifiers - **Vendor cleanup**: scanners may implement the optional `VendorCleaner` interface to register ecosystem-native cleanup commands (e.g. `xcrun simctl delete unavailable`). `devclean clean --vendor-cleanup` runs them alongside path-based deletion so vendor internal state stays consistent — natural fit for Docker (`system prune`), Homebrew (`cleanup`), Gradle, etc. +- **Deletion strategy**: `ScanResult.Delete` (`model.DeleteMethod`: kind path/command/api + display + Run closure) expresses non-path reclaims per item; nil = path removal. Cleaner applies protected/dry-run gates uniformly, then delegates to the method or falls back to trash/force. `VendorCleanup` embeds the same `DeleteMethod` — bulk (ecosystem-level) vs per-item are two uses of one execution contract ## Documentation Rules diff --git a/docs/architecture.md b/docs/architecture.md index 80baab6..67837f2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -99,6 +99,25 @@ Scanners derive these from peer comparison (DeviceSupport build ages), vendor AP Use this pattern when a single ecosystem produces directories whose names alone don't tell the user what they are. +### Deletion Strategy + +Reclaiming is not always `os.RemoveAll` on a path — Docker images or simulator devices are reclaimed through vendor commands or API calls. Each `ScanResult` carries an optional `DeleteMethod` describing how it is reclaimed: + +```go +type DeleteMethod struct { + Kind DeleteKind // "path" | "command" | "api" + Display string // what would run — dry-run / listings + Run func(ctx context.Context) error // executes the reclaim +} + +// On ScanResult: +Delete *DeleteMethod `json:"delete,omitempty"` // nil = path removal +``` + +The cleaner applies its policy gates (protected refusal, dry-run) uniformly, then executes: `Delete.Run(ctx)` when a method is attached, otherwise path removal (trash or permanent). A method with a nil `Run` is refused rather than falling back to path removal — a misconfigured item must never delete a path its method didn't intend. Trash/permanent choice only applies to path removal; command/api items follow the vendor's own recovery semantics (surfacing that in the selection UI belongs to the first ecosystem that ships such items). + +In JSON output, non-path items serialize as `"delete": {"kind": "command", "display": "..."}` (`Run` never serializes), so agents can tell strategies apart; absence of the key means path removal. + ### Vendor Cleanup Scanners may opt into an additional interface to register ecosystem-native cleanup commands: @@ -111,11 +130,12 @@ type VendorCleaner interface { type VendorCleanup struct { ID string Description string - Command string // for dry-run display - Run func(ctx context.Context) error // executes the command + model.DeleteMethod // Kind + Display (dry-run) + Run — shared execution contract } ``` +`VendorCleanup` is the ecosystem-level **bulk** counterpart of a per-item `ScanResult.Delete`: both share the `model.DeleteMethod` execution contract. Bulk actions (e.g. `brew cleanup`) register here; per-item non-path reclaims (e.g. `docker rmi `) attach a `DeleteMethod` to their `ScanResult`. + `devclean clean --vendor-cleanup` collects cleanups from selected ecosystems and runs them alongside path-based deletion. Vendor commands keep the ecosystem's internal state consistent (e.g. `xcrun simctl delete unavailable` removes simulator devices and updates CoreSimulator's database in one step). Dry-run prints the command without executing. Current implementations: @@ -173,7 +193,7 @@ Table output groups results by: **ecosystem → project → sub-package → arti - Quick select: `[a]` all, `[n]` none, `[s]` safe only, `[d]` dormant only - Protected projects hidden with explanation 3. Choose: Move to Trash / Permanently delete / Cancel -4. Execute with per-artifact status output +4. Execute with per-artifact status output — items with an attached `DeleteMethod` run it instead of path removal (see Deletion Strategy) 5. Summary: items cleaned, space freed Trash moves use `os.Rename` into the Trash dir on the home volume. When the artifact lives on a different filesystem (external drive, separate partition), `os.Rename` fails with `EXDEV`; the cleaner falls back to a recursive copy followed by removing the original. The copy recreates directories, regular files (contents + permission bits), and symlinks (as links, never followed). The original is removed only after the copy fully succeeds — a mid-copy failure leaves the original intact and discards the partial copy, so a cross-device move can never lose data. diff --git a/internal/cleaner/cleaner.go b/internal/cleaner/cleaner.go index 9ca67e8..436aaa3 100644 --- a/internal/cleaner/cleaner.go +++ b/internal/cleaner/cleaner.go @@ -1,6 +1,7 @@ package cleaner import ( + "context" "errors" "fmt" "io" @@ -29,8 +30,10 @@ func New(opts Options) *Cleaner { return &Cleaner{opts: opts} } -// Clean deletes or trashes a scan result item. -func (c *Cleaner) Clean(r model.ScanResult) error { +// Clean reclaims a scan result item: protection and dry-run gates apply to +// every strategy; execution then goes through the item's DeleteMethod when one +// is attached, or falls back to path removal (trash or permanent). +func (c *Cleaner) Clean(ctx context.Context, r model.ScanResult) error { if r.Protected { return fmt.Errorf("refusing to delete protected item: %s (%s)", r.Path, r.Reason) } @@ -39,6 +42,15 @@ func (c *Cleaner) Clean(r model.ScanResult) error { return nil } + if r.Delete != nil { + // A method without Run is a scanner bug; refuse rather than fall back + // to path removal, which could delete a path the method never meant to. + if r.Delete.Run == nil { + return fmt.Errorf("delete method for %s has no Run function", r.Path) + } + return r.Delete.Run(ctx) + } + if c.opts.Force { return os.RemoveAll(r.Path) } @@ -53,10 +65,10 @@ type CleanResult struct { } // CleanAll cleans multiple results and returns per-item outcomes. -func (c *Cleaner) CleanAll(results []model.ScanResult) []CleanResult { +func (c *Cleaner) CleanAll(ctx context.Context, results []model.ScanResult) []CleanResult { var out []CleanResult for _, r := range results { - err := c.Clean(r) + err := c.Clean(ctx, r) out = append(out, CleanResult{Item: r, Error: err}) } return out diff --git a/internal/cleaner/cleaner_test.go b/internal/cleaner/cleaner_test.go index bbf7d98..d52d5cc 100644 --- a/internal/cleaner/cleaner_test.go +++ b/internal/cleaner/cleaner_test.go @@ -1,6 +1,8 @@ package cleaner_test import ( + "context" + "errors" "os" "path/filepath" "testing" @@ -32,7 +34,7 @@ func TestForceDelete(t *testing.T) { c := cleaner.New(cleaner.Options{Force: true}) result := model.ScanResult{Path: target, Size: 100, Safety: model.SafetySafe} - err := c.Clean(result) + err := c.Clean(t.Context(), result) if err != nil { t.Fatalf("Clean error: %v", err) } @@ -50,7 +52,7 @@ func TestDryRun(t *testing.T) { c := cleaner.New(cleaner.Options{DryRun: true}) result := model.ScanResult{Path: target, Safety: model.SafetySafe} - err := c.Clean(result) + err := c.Clean(t.Context(), result) if err != nil { t.Fatalf("Clean error: %v", err) } @@ -68,7 +70,7 @@ func TestProtectedNotDeleted(t *testing.T) { c := cleaner.New(cleaner.Options{Force: true}) result := model.ScanResult{Path: target, Protected: true, Safety: model.SafetyProtected} - err := c.Clean(result) + err := c.Clean(t.Context(), result) if err == nil { t.Error("expected error when cleaning protected item") } @@ -91,7 +93,7 @@ func TestTrashDelete(t *testing.T) { c := cleaner.New(cleaner.Options{TrashDir: trashDir}) result := model.ScanResult{Path: target, Size: 100, Safety: model.SafetySafe} - err := c.Clean(result) + err := c.Clean(t.Context(), result) if err != nil { t.Fatalf("Clean error: %v", err) } @@ -120,7 +122,7 @@ func TestTrashDelete_NameConflict(t *testing.T) { c := cleaner.New(cleaner.Options{TrashDir: trashDir}) result := model.ScanResult{Path: target, Safety: model.SafetySafe} - err := c.Clean(result) + err := c.Clean(t.Context(), result) if err != nil { t.Fatalf("Clean error: %v", err) } @@ -145,7 +147,7 @@ func TestCleanAll(t *testing.T) { {Path: t2, Size: 200, Safety: model.SafetySafe}, } - cleanResults := c.CleanAll(results) + cleanResults := c.CleanAll(t.Context(), results) if len(cleanResults) != 2 { t.Fatalf("expected 2 results, got %d", len(cleanResults)) @@ -179,7 +181,7 @@ func TestCleanAllSkipsProtected(t *testing.T) { {Path: protected, Size: 200, Safety: model.SafetyProtected, Protected: true}, } - cleanResults := c.CleanAll(results) + cleanResults := c.CleanAll(t.Context(), results) // First should succeed if cleanResults[0].Error != nil { @@ -196,3 +198,141 @@ func TestCleanAllSkipsProtected(t *testing.T) { t.Error("protected dir should still exist") } } + +// deleteMethodItem builds a ScanResult whose reclaim goes through a +// DeleteMethod, backed by a real path so tests can prove path removal +// was NOT taken. +func deleteMethodItem(t *testing.T, ran *bool) model.ScanResult { + t.Helper() + dir := t.TempDir() + target := filepath.Join(dir, "vendor-managed") + mustMkdir(t, target) + return model.ScanResult{ + Path: target, + Size: 100, + Safety: model.SafetySafe, + Delete: &model.DeleteMethod{ + Kind: model.DeleteKindCommand, + Display: "vendor prune", + Run: func(context.Context) error { + *ran = true + return nil + }, + }, + } +} + +func TestDeleteMethodRun(t *testing.T) { + var ran bool + result := deleteMethodItem(t, &ran) + + c := cleaner.New(cleaner.Options{Force: true}) + if err := c.Clean(t.Context(), result); err != nil { + t.Fatalf("Clean error: %v", err) + } + + if !ran { + t.Error("expected DeleteMethod.Run to be called") + } + // Path removal must not have been taken, even with Force set. + if _, err := os.Stat(result.Path); err != nil { + t.Errorf("path must be untouched when a DeleteMethod is attached: %v", err) + } +} + +func TestDeleteMethodDryRun(t *testing.T) { + var ran bool + result := deleteMethodItem(t, &ran) + + c := cleaner.New(cleaner.Options{DryRun: true}) + if err := c.Clean(t.Context(), result); err != nil { + t.Fatalf("Clean error: %v", err) + } + + if ran { + t.Error("dry-run must not execute DeleteMethod.Run") + } +} + +func TestDeleteMethodProtected(t *testing.T) { + var ran bool + result := deleteMethodItem(t, &ran) + result.Protected = true + + c := cleaner.New(cleaner.Options{}) + if err := c.Clean(t.Context(), result); err == nil { + t.Error("expected error for protected item") + } + + if ran { + t.Error("protected item must not execute DeleteMethod.Run") + } +} + +func TestDeleteMethodRunError(t *testing.T) { + wantErr := errors.New("vendor tool failed") + result := model.ScanResult{ + Path: "/nonexistent", + Safety: model.SafetySafe, + Delete: &model.DeleteMethod{ + Kind: model.DeleteKindCommand, + Run: func(context.Context) error { return wantErr }, + }, + } + + c := cleaner.New(cleaner.Options{}) + if err := c.Clean(t.Context(), result); !errors.Is(err, wantErr) { + t.Errorf("expected Run error to propagate, got %v", err) + } +} + +func TestDeleteMethodNilRun(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "misconfigured") + mustMkdir(t, target) + result := model.ScanResult{ + Path: target, + Safety: model.SafetySafe, + Delete: &model.DeleteMethod{Kind: model.DeleteKindCommand}, + } + + c := cleaner.New(cleaner.Options{Force: true}) + if err := c.Clean(t.Context(), result); err == nil { + t.Error("expected error for DeleteMethod without Run") + } + + // No fallback to path removal for a misconfigured method. + if _, err := os.Stat(target); err != nil { + t.Errorf("path must survive a misconfigured DeleteMethod: %v", err) + } +} + +func TestCleanAllMixedStrategies(t *testing.T) { + dir := t.TempDir() + pathItem := filepath.Join(dir, "by-path") + mustMkdir(t, pathItem) + + var ran bool + methodItem := deleteMethodItem(t, &ran) + + c := cleaner.New(cleaner.Options{Force: true}) + cleanResults := c.CleanAll(t.Context(), []model.ScanResult{ + {Path: pathItem, Safety: model.SafetySafe}, + methodItem, + }) + + for _, cr := range cleanResults { + if cr.Error != nil { + t.Errorf("unexpected error for %s: %v", cr.Item.Path, cr.Error) + } + } + if _, err := os.Stat(pathItem); !os.IsNotExist(err) { + t.Error("path item should be removed via path strategy") + } + if !ran { + t.Error("method item should be reclaimed via its Run") + } + if _, err := os.Stat(methodItem.Path); err != nil { + t.Error("method item's path must be untouched") + } +} diff --git a/internal/cli/clean.go b/internal/cli/clean.go index 9e1c1eb..1980236 100644 --- a/internal/cli/clean.go +++ b/internal/cli/clean.go @@ -161,7 +161,7 @@ func newCleanCmd() *cobra.Command { relPath = rel } } - err := c.Clean(r) + err := c.Clean(cmd.Context(), r) if err != nil { failed++ fmt.Printf(" %s %s — %v\n", ui.ErrStyle.Render("✗"), relPath, err) @@ -280,7 +280,7 @@ func runVendorCleanups(ecosystems []model.Ecosystem, dryRun bool) { fmt.Printf("\n%s\n", ui.ProjectStyle.Render("Vendor cleanup:")) for _, p := range actions { header := fmt.Sprintf(" [%s] %s", p.eco, p.action.Description) - fmt.Println(ui.DimStyle.Render(" " + p.action.Command)) + fmt.Println(ui.DimStyle.Render(" " + p.action.Display)) if dryRun { fmt.Printf("%s %s\n", header, ui.DimStyle.Render("(dry-run)")) continue diff --git a/internal/integration_test.go b/internal/integration_test.go index efee531..29f2ead 100644 --- a/internal/integration_test.go +++ b/internal/integration_test.go @@ -130,7 +130,7 @@ func TestIntegration_ScanClassifyClean(t *testing.T) { // Clean (force, since TempDir won't map to Trash) c := cleaner.New(cleaner.Options{Force: true}) for _, r := range results { - if err := c.Clean(r); err != nil { + if err := c.Clean(t.Context(), r); err != nil { t.Errorf("clean error for %s: %v", r.Path, err) } } diff --git a/internal/model/types.go b/internal/model/types.go index 4946537..a9cfb30 100644 --- a/internal/model/types.go +++ b/internal/model/types.go @@ -1,6 +1,7 @@ package model import ( + "context" "fmt" "path/filepath" "sort" @@ -74,6 +75,26 @@ type ArtifactDef struct { AlwaysSafe bool `json:"always_safe"` } +// DeleteKind identifies how an artifact is reclaimed. +type DeleteKind string + +const ( + DeleteKindPath DeleteKind = "path" // filesystem removal of Path (trash or permanent) + DeleteKindCommand DeleteKind = "command" // vendor CLI command + DeleteKindAPI DeleteKind = "api" // in-process API call +) + +// DeleteMethod describes how a result is reclaimed when plain path removal +// does not apply (vendor command, API call). Display is what would run, shown +// in dry-run and listings; Run performs the reclaim and must honor ctx. Run is +// never serialized — kind and display surface in JSON so agents can tell +// strategies apart. +type DeleteMethod struct { + Kind DeleteKind `json:"kind"` + Display string `json:"display"` + Run func(ctx context.Context) error `json:"-"` +} + // InodeKey identifies a physical inode uniquely. Inode numbers are only unique // per device, so any cross-artifact dedup key must include Dev — a home scan can // span multiple filesystems (external volumes, network mounts, Docker/APFS @@ -100,12 +121,25 @@ type ScanResult struct { Recommendation string `json:"recommendation,omitempty"` // hint for the user (e.g. "old build", "unavailable runtime") LastUsedAt time.Time `json:"last_used_at,omitzero"` // when the item itself was last used, if the scanner can tell (omitzero: zero time is dropped from JSON) + // Delete overrides how this result is reclaimed. Nil means path removal + // (trash or permanent delete of Path). + Delete *DeleteMethod `json:"delete,omitempty"` + // Links maps each hard-linked inode (Nlink>1) found in this artifact to its // disk blocks, so a caller can dedup blocks shared across artifacts (e.g. // pnpm store ↔ node_modules) when computing a grand total. Not serialized. Links map[InodeKey]int64 `json:"-"` } +// DeleteStrategy returns the effective delete strategy for this result: +// the attached method's kind, or path removal when none is set. +func (r ScanResult) DeleteStrategy() DeleteKind { + if r.Delete != nil { + return r.Delete.Kind + } + return DeleteKindPath +} + // HumanSize returns a human-readable size string. func (r ScanResult) HumanSize() string { return HumanSize(r.Size) diff --git a/internal/model/types_test.go b/internal/model/types_test.go index 53b67fc..7ec0378 100644 --- a/internal/model/types_test.go +++ b/internal/model/types_test.go @@ -1,6 +1,9 @@ package model_test import ( + "context" + "encoding/json" + "strings" "testing" "time" @@ -207,3 +210,50 @@ func TestFilterResults(t *testing.T) { t.Errorf("expected 0 results, got %d", len(none)) } } + +func TestDeleteStrategy(t *testing.T) { + tests := []struct { + name string + result model.ScanResult + want model.DeleteKind + }{ + {"nil delete means path", model.ScanResult{Path: "/tmp/x"}, model.DeleteKindPath}, + {"command method", model.ScanResult{Delete: &model.DeleteMethod{Kind: model.DeleteKindCommand}}, model.DeleteKindCommand}, + {"api method", model.ScanResult{Delete: &model.DeleteMethod{Kind: model.DeleteKindAPI}}, model.DeleteKindAPI}, + } + for _, tt := range tests { + if got := tt.result.DeleteStrategy(); got != tt.want { + t.Errorf("%s: got %q, want %q", tt.name, got, tt.want) + } + } +} + +func TestScanResultJSON_DeleteMethod(t *testing.T) { + withMethod := model.ScanResult{ + Path: "/tmp/x", + Delete: &model.DeleteMethod{ + Kind: model.DeleteKindCommand, + Display: "docker rmi abc", + Run: func(context.Context) error { return nil }, + }, + } + data, err := json.Marshal(withMethod) + if err != nil { + t.Fatalf("marshal: %v", err) + } + got := string(data) + if !strings.Contains(got, `"delete":{"kind":"command","display":"docker rmi abc"}`) { + t.Errorf("delete method not serialized as kind+display: %s", got) + } + if strings.Contains(got, "Run") || strings.Contains(got, "run") { + t.Errorf("Run must not leak into JSON: %s", got) + } + + without, err := json.Marshal(model.ScanResult{Path: "/tmp/x"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if strings.Contains(string(without), `"delete"`) { + t.Errorf("nil Delete must be omitted from JSON: %s", without) + } +} diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index 2bfdc9c..7097e09 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -21,12 +21,13 @@ type Scanner interface { // VendorCleanup describes an ecosystem-native cleanup action that delegates to // an official tool (e.g. `xcrun simctl delete unavailable` for Xcode). These // run alongside path-based cleanup but use the vendor's own command so internal -// state stays consistent. +// state stays consistent. It is the ecosystem-level bulk counterpart of a +// per-item ScanResult.Delete: both share the model.DeleteMethod execution +// contract (Kind, Display for dry-run, Run to execute). type VendorCleanup struct { - ID string // stable identifier, e.g. "simctl-delete-unavailable" - Description string // user-facing summary - Command string // command that would be run, for dry-run / display - Run func(ctx context.Context) error // executes the cleanup + ID string // stable identifier, e.g. "simctl-delete-unavailable" + Description string // user-facing summary + model.DeleteMethod } // VendorCleaner is implemented by scanners that contribute vendor-native diff --git a/internal/scanner/xcode.go b/internal/scanner/xcode.go index 343ad06..73b5967 100644 --- a/internal/scanner/xcode.go +++ b/internal/scanner/xcode.go @@ -70,9 +70,12 @@ func (s *XcodeScanner) VendorCleanups() []VendorCleanup { { ID: "simctl-delete-unavailable", Description: "Delete simulator devices whose runtime was removed", - Command: "xcrun simctl delete unavailable", - Run: func(ctx context.Context) error { - return exec.CommandContext(ctx, "xcrun", "simctl", "delete", "unavailable").Run() + DeleteMethod: model.DeleteMethod{ + Kind: model.DeleteKindCommand, + Display: "xcrun simctl delete unavailable", + Run: func(ctx context.Context) error { + return exec.CommandContext(ctx, "xcrun", "simctl", "delete", "unavailable").Run() + }, }, }, } diff --git a/internal/scanner/xcode_test.go b/internal/scanner/xcode_test.go index 1438619..505a198 100644 --- a/internal/scanner/xcode_test.go +++ b/internal/scanner/xcode_test.go @@ -406,8 +406,11 @@ func TestXcodeScanner_VendorCleanups(t *testing.T) { if a.Run == nil { t.Error("Run must not be nil") } - if a.Command == "" { - t.Error("Command should be populated for display") + if a.Kind != model.DeleteKindCommand { + t.Errorf("Kind should be command, got %q", a.Kind) + } + if a.Display == "" { + t.Error("Display should be populated for dry-run") } } }