feat: added functionality to delete failed, completed, all downloads from cli tui and extension

[junaid2005p]

Greptile Summary

This PR adds the ability to delete failed downloads alongside the existing completed-download cleanup, wiring the feature through the CLI (surge rm --clean-failed), HTTP API (/clear-failed), local service, and remote service.

• cmd/rm.go adds a --clean-failed flag and a corresponding else if branch, while correctly extending the empty-args guard — but two flag-combination edge cases are unguarded (see inline comments).
• internal/engine/state/state.go adds RemoveFailedDownloads, which mirrors RemoveCompletedDownloads except that the underlying DB error is not wrapped with %w, preventing callers from unwrapping the root cause.
• The DownloadService interface, local service, remote service, and HTTP routes are updated consistently; all test fixtures are updated to satisfy the new interface methods.

Confidence Score: 3/5

The CLI flag-combination logic in cmd/rm.go has two gaps that cause silent misbehaviour before the fix lands.

Both --clean --clean-failed (second flag silently ignored) and --clean-failed --purge (purge never executed, potentially leaving orphaned partial files) are defects introduced by this PR that need to be addressed before merging.

cmd/rm.go — the flag-combination guards are incomplete.

Important Files Changed

Filename	Overview
cmd/rm.go	Adds --clean-failed flag and handler; guard for empty args is correctly updated, but two logic gaps remain: --clean --clean-failed silently ignores clean_failed, and --clean-failed --purge silently ignores purge.
internal/engine/state/state.go	Adds RemoveFailedDownloads mirroring RemoveCompletedDownloads; the underlying DB error is not wrapped with %w (unlike the sibling function), which was already flagged in a previous review thread.
internal/core/interface.go	Adds ClearCompleted and ClearFailed to DownloadService interface; comment style is missing the standard Go doc format (already noted in previous thread).
internal/core/remote_service.go	Adds ClearCompleted and ClearFailed via the existing doRequest pattern; error handling and response decoding are consistent with sibling methods.
cmd/http_api.go	Registers /clear-completed and /clear-failed POST endpoints; follows the existing handler pattern correctly.
cmd/http_api_test.go	Adds stub implementations of the new interface methods to the test fixture; no logic, just satisfies the interface.
cmd/connect_test.go	Adds stub implementations of the new interface methods to the fake service; no logic.
cmd/root_lifecycle_test.go	Adds stub implementations to satisfy the updated interface; no issues.
internal/tui/delete_resilience_test.go	Adds stub implementations to satisfy the updated interface; no issues.
internal/core/local_service.go	Thin delegation to the new state helpers; straightforward and correct.

Comments Outside Diff (2)

1. cmd/rm.go, line 17-39 (link)

   --clean-failed flag is registered but never handled

   The clean-failed flag is added in init() but the RunE body never reads it. Running surge rm --clean-failed with no positional argument hits the !clean && len(args) == 0 branch and returns "provide a download ID or use --clean", silently ignoring the user's intent. The flag is effectively dead code.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: cmd/rm.go
   Line: 17-39
   
   Comment:
   **`--clean-failed` flag is registered but never handled**
   
   The `clean-failed` flag is added in `init()` but the `RunE` body never reads it. Running `surge rm --clean-failed` with no positional argument hits the `!clean && len(args) == 0` branch and returns `"provide a download ID or use --clean"`, silently ignoring the user's intent. The flag is effectively dead code.
   
   How can I resolve this? If you propose a fix, please make it concise.

2. cmd/rm.go, line 25-27 (link)

   --clean-failed guard never checked

   The early-exit condition !clean && len(args) == 0 does not account for clean_failed, so surge rm --clean-failed (no positional arg) exits with "provide a download ID or use --clean" before ever reaching the else if clean_failed branch. The flag is effectively unreachable without also supplying an ID that is then ignored.

   The condition should also allow the clean-failed flag through: !clean && !cleanFailed && len(args) == 0.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: cmd/rm.go
   Line: 25-27
   
   Comment:
   **`--clean-failed` guard never checked**
   
   The early-exit condition `!clean && len(args) == 0` does not account for `clean_failed`, so `surge rm --clean-failed` (no positional arg) exits with "provide a download ID or use --clean" before ever reaching the `else if clean_failed` branch. The flag is effectively unreachable without also supplying an ID that is then ignored.
   
   The condition should also allow the `clean-failed` flag through: `!clean && !cleanFailed && len(args) == 0`.
   
   How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (6): Last reviewed commit: "fix: clean-failed guard in rm command" | Re-trigger Greptile}

Context used:

• Rule used - What: Prioritize readable, idiomatic Go code over ... (source)

[github-actions]

Binary Size Analysis

⚠️ Size Increased

Version	Human Readable	Raw Bytes
Main	20.02 MB	20988196
PR	20.02 MB	20996388
Difference	8.00 KB	8192

[greptile-apps]

The underlying database error from db.Exec is discarded. RemoveCompletedDownloads (the sibling function) uses %w to wrap the error, but this function drops it, making it impossible for callers to inspect or unwrap the root cause.

Suggested change

	result, err := db.Exec("DELETE FROM downloads WHERE status = 'failed'")
	if err != nil {
	return 0, fmt.Errorf("failed to remove failed downloads")
	}
	result, err := db.Exec("DELETE FROM downloads WHERE status = 'failed'")
	if err != nil {
	return 0, fmt.Errorf("failed to remove failed downloads: %w", err)
	}

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/engine/state/state.go
Line: 664-667

Comment:
The underlying database error from `db.Exec` is discarded. `RemoveCompletedDownloads` (the sibling function) uses `%w` to wrap the error, but this function drops it, making it impossible for callers to inspect or unwrap the root cause.

```suggestion
	result, err := db.Exec("DELETE FROM downloads WHERE status = 'failed'")
	if err != nil {
		return 0, fmt.Errorf("failed to remove failed downloads: %w", err)
	}
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Go convention requires a space after // in single-line comments, and doc comments on exported/interface methods should follow the // MethodName ... format used elsewhere in this file.

Suggested change

	//Clear completed downloads from surge
	ClearCompleted() (int64, error)
	//Clear failed downloads from surge
	ClearFailed() (int64, error)
	// ClearCompleted removes all completed downloads and returns the count deleted.
	ClearCompleted() (int64, error)
	// ClearFailed removes all failed downloads and returns the count deleted.
	ClearFailed() (int64, error)

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/core/interface.go
Line: 54-58

Comment:
Go convention requires a space after `//` in single-line comments, and doc comments on exported/interface methods should follow the `// MethodName ...` format used elsewhere in this file.

```suggestion
	// ClearCompleted removes all completed downloads and returns the count deleted.
	ClearCompleted() (int64, error)

	// ClearFailed removes all failed downloads and returns the count deleted.
	ClearFailed() (int64, error)
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!