Skip to content

fix(workspace): delete cached content folder on workspace delete - #830

Merged
skevetter merged 1 commit into
mainfrom
fix/delete-removes-content-folder
Jul 31, 2026
Merged

fix(workspace): delete cached content folder on workspace delete#830
skevetter merged 1 commit into
mainfrom
fix/delete-removes-content-folder

Conversation

@skevetter

@skevetter skevetter commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • devsy workspace delete removed the container and the workspace's config/metadata folder (contexts/<ctx>/workspaces/<id>), but left the cloned repo content in contexts/<ctx>/contents/<id> untouched. That's intentional caching (so a later up --reset can reuse it, per pkg/agent/agent.go:287-288), but plain delete silently leaving a full repo checkout — potentially with embedded git credentials — on disk forever, with no cleanup path, is a bug: it violates what a command named "delete" is expected to do, and there's no equivalent of SweepOrphanWorkspaceDirs for this directory.
  • DeleteCmd.Run now also removes workspaceInfo.ContentFolder, guarded so it never touches the user's own local folder when the workspace source is LocalFolder (mirroring the existing ContentFolder == Source.LocalFolder check in up.go's prepareLocalWorkspace).
  • up --reset/--recreate behavior is unchanged — that's the existing, correct escape hatch for forcing a fresh clone on up; this PR only fixes delete not finishing the job.
  • Added TestRemoveContentFolder_* covering: devsy-managed content gets removed, a local-folder-sourced workspace's own directory is left alone, and an empty ContentFolder is a no-op.

Summary by CodeRabbit

  • Bug Fixes

    • Workspace deletion now removes associated origin and cached content folders after cleanup.
    • User-owned local source folders are preserved.
    • Missing or empty content folders no longer prevent workspace deletion.
    • Cleanup errors are handled without failing the deletion command.
  • Tests

    • Added coverage for managed-folder removal and preservation of local folders.

devsy workspace delete removed the container and the workspace's
config/metadata folder, but left the cloned repo content in
contexts/<ctx>/contents/<id> untouched — kept there intentionally so a
later `up` on the same workspace could skip re-cloning. That's a
reasonable optimization for `up --reset`, but "delete" silently leaving
a full repo checkout (with potential embedded git credentials) on disk
forever, with no cleanup path, is not.

Have delete also remove ContentFolder, unless it's the user's own
local folder mounted directly rather than a devsy-managed copy.
@netlify

netlify Bot commented Jul 31, 2026

Copy link
Copy Markdown

Deploy Preview for devsydev canceled.

Name Link
🔨 Latest commit 1a44336
🔍 Latest deploy log https://app.netlify.com/projects/devsydev/deploys/6a6c07b9bfd35f00089768bf

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Workspace deletion now removes the workspace origin and provider-managed cached content after daemon and container cleanup. Cleanup errors are logged without failing the command. User-owned local source folders and empty content paths are preserved.

Changes

Workspace deletion cleanup

Layer / File(s) Summary
Workspace folder cleanup
cmd/internal/agentworkspace/delete.go
Run removes workspace folders after daemon and container cleanup. Cleanup is best-effort, and user-owned local source folders are not removed.
Cleanup behavior tests
cmd/internal/agentworkspace/delete_test.go
Tests verify managed-folder removal, local-folder preservation, and empty content-folder handling.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: deleting the cached content folder during workspace deletion.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@netlify

netlify Bot commented Jul 31, 2026

Copy link
Copy Markdown

Deploy Preview for images-devsy-sh canceled.

Name Link
🔨 Latest commit 1a44336
🔍 Latest deploy log https://app.netlify.com/projects/images-devsy-sh/deploys/6a6c07b9f58e8f00084dee6b

1 similar comment
@netlify

netlify Bot commented Jul 31, 2026

Copy link
Copy Markdown

Deploy Preview for images-devsy-sh canceled.

Name Link
🔨 Latest commit 1a44336
🔍 Latest deploy log https://app.netlify.com/projects/images-devsy-sh/deploys/6a6c07b9f58e8f00084dee6b

@skevetter
skevetter marked this pull request as ready for review July 31, 2026 02:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/internal/agentworkspace/delete.go`:
- Around line 113-118: Update removeContentFolder to resolve the filesystem
identity of both ContentFolder and Source.LocalFolder before comparing them,
treating equivalent paths and symlink aliases as the same local folder. Before
calling forceRemoveAll, validate that the resolved content path remains within
the allowed Devsy content area, and skip or reject deletion when it falls
outside that boundary.
🪄 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: 14a61e0d-f703-424c-ae3b-b699bbc50146

📥 Commits

Reviewing files that changed from the base of the PR and between 2ebb3cc and 1a44336.

📒 Files selected for processing (2)
  • cmd/internal/agentworkspace/delete.go
  • cmd/internal/agentworkspace/delete_test.go

Comment on lines +113 to +118
func removeContentFolder(workspaceInfo *provider2.AgentWorkspaceInfo) error {
if workspaceInfo.ContentFolder == "" ||
workspaceInfo.ContentFolder == workspaceInfo.Workspace.Source.LocalFolder {
return nil
}
return forceRemoveAll(workspaceInfo.ContentFolder)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Trace all assignments and consumers of ContentFolder.
rg -n -C 5 --glob '*.go' 'ContentFolder\s*:|\.ContentFolder\b|func\s+WorkspaceInfo\b' .

# Find cache-root construction and path normalization logic.
rg -n -i -C 5 --glob '*.go' 'content.*(cache|folder)|cache.*content|EvalSymlinks|SameFile|filepath\.Abs' cmd pkg

Repository: devsy-org/devsy

Length of output: 50371


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
git ls-files | rg '(^cmd/internal/agentworkspace/(delete|.*workspace).*\.go$|^pkg/provider/workspace\.go$|^cmd/internal/agentworkspace/delete_test\.go$)' || true

echo
echo "== delete.go outline/source =="
ast-grep outline cmd/internal/agentworkspace/delete.go --view compact || true
sed -n '1,180p' cmd/internal/agentworkspace/delete.go | cat -n

echo
echo "== delete_test relevant source =="
sed -n '1,210p' cmd/internal/agentworkspace/delete_test.go | cat -n

echo
echo "== provider workspace relevant source =="
sed -n '130,210p' pkg/provider/workspace.go | cat -n

echo
echo "== agent resolveContentFolder full =="
sed -n '268,300p' pkg/agent/agent.go | cat -n

echo
echo "== content directory helpers =="
rg -n -C 4 --glob '*.go' 'func GetWorkspaceContentDir|GetAgentWorkspaceContentDir|AgentWorkspaceContent|contents.*workspace|workspace.*contents' pkg cmd/internal/agentworkspace

Repository: devsy-org/devsy

Length of output: 20287


Resolve filesystem ownership before deleting content folders.

removeContentFolder removes any non-empty ContentFolder that is not byte-for-byte equal to Source.LocalFolder, so equivalent paths such as /src/project/../project or symlink aliases still bypass the local-folder guard. Resolve both ContentFolder and Source.LocalFolder filesystem identity before skipping, and add a boundary check before recursive deletion so a wrong metadata value cannot delete a directory outside the Devsy content area.

🤖 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/internal/agentworkspace/delete.go` around lines 113 - 118, Update
removeContentFolder to resolve the filesystem identity of both ContentFolder and
Source.LocalFolder before comparing them, treating equivalent paths and symlink
aliases as the same local folder. Before calling forceRemoveAll, validate that
the resolved content path remains within the allowed Devsy content area, and
skip or reject deletion when it falls outside that boundary.

@skevetter
skevetter merged commit 28c008e into main Jul 31, 2026
66 checks passed
@skevetter
skevetter deleted the fix/delete-removes-content-folder branch July 31, 2026 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant