mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(workspaces): close a retained local task directory as archived, not cleanup_failed (#20248)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A task runs in an execution workspace. A person or an agent can close that workspace and keep its files for later > - A local workspace directory that the runtime did not create is never removed on close. Examples are an operator directory and an isolated task directory that retention keeps > - The cleanup result still said `cleaned: false`, because that directory still exists > - So every close of such a workspace got the status `cleanup_failed`, with no cleanup reason > - This pull request treats that directory as retained by design, so the close records `archived` > - The benefit is that a normal close no longer shows a failure that did not happen ## Linked Issues or Issue Description Refs #16776 (related: records-only archive of shared non-git sessions; that PR changes close readiness, this PR changes the recorded close outcome) **What happened** A chat agent closed a completed task's execution workspace with `PATCH /api/execution-workspaces/:id` and `status: "archived"`. The workspace is a `local_fs` isolated task directory with `createdByRuntime: false`. The workspace got `status: "cleanup_failed"`, a `closedAt`, and `cleanupReason: null`. The files stayed, as intended. The agent then had to tell the person that cleanup failed. **Expected behavior** The close records `status: "archived"`. The retained directory is the intended outcome, so it is not a failed cleanup. A runtime-created directory is still removed. **Steps to reproduce** 1. Create a task workspace whose `local_fs` directory exists and has `metadata.createdByRuntime: false` (for example an isolated task directory). 2. Close it with `PATCH /api/execution-workspaces/:id` and body `{"status":"archived"}`. 3. Read the workspace. Its status is `cleanup_failed` and `cleanupReason` is `null`. ## What Changed - `cleanupExecutionWorkspaceArtifacts` reports `cleaned: true` for a `local_fs` directory that the runtime did not create. It never removes that directory, so nothing is left to clean. - Added `server/src/__tests__/workspace-cleanup-retained-local-dir.test.ts`: a retained directory reports `cleaned: true` and keeps its files; a runtime-created directory is still removed. ## Verification - `pnpm exec vitest run server/src/__tests__/workspace-cleanup-retained-local-dir.test.ts` fails before the change (`cleaned: false`) and passes after it. - `server/src/__tests__/workspace-runtime.test.ts` (179), `execution-workspaces-routes.test.ts` (17), `execution-workspaces-service.test.ts` (85) and `workspace-delivery-cleanup.integration.test.ts` (19) pass. - End to end: the chat eval `workspaces.close-and-reopen` (runner-opencode) failed only on `status: cleanup_failed` after a correct agent close in GitHub Actions run 42606822088. This fix is cherry-picked onto the eval branch for a rerun. ## Risks - Low risk. Only `local_fs` workspaces with `createdByRuntime` not `true` change. Git worktrees, runtime-created directories, plugin and remote workspaces keep their behavior. - The terminal reaper uses the same cleanup function. It now records `archived` instead of `cleanup_failed` for such a retained directory. Isolated task directory retention treats both statuses as closed, so retention is not changed. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code execution in Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
7adb78d811
commit
b3bf12a5db
2 files changed
+38
No files matched your search
@@ -0,0 +1,33 @@
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { cleanupExecutionWorkspaceArtifacts } from "../services/workspace-runtime.ts";
|
||||
|
||||
const dirs: string[] = [];
|
||||
afterEach(async () => { await Promise.all(dirs.splice(0).map(dir => fs.rm(dir, { recursive: true, force: true }))); });
|
||||
|
||||
async function localWorkspace(createdByRuntime: boolean) {
|
||||
const root = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-retained-dir-")); dirs.push(root);
|
||||
const cwd = path.join(root, "task-directory");
|
||||
await fs.mkdir(cwd); await fs.writeFile(path.join(cwd, "website-fixture.txt"), "saved\n");
|
||||
return { cwd, workspace: { id: "workspace-1", companyId: "company-1", cwd, providerType: "local_fs", providerRef: null,
|
||||
branchName: null, repoUrl: null, baseRef: null, projectId: null, projectWorkspaceId: null, sourceIssueId: "issue-1",
|
||||
metadata: { createdByRuntime } } };
|
||||
}
|
||||
|
||||
describe("closing a local workspace directory", () => {
|
||||
it("reports a retained directory the runtime did not create as cleaned and keeps its files", async () => {
|
||||
const { cwd, workspace } = await localWorkspace(false);
|
||||
const cleanup = await cleanupExecutionWorkspaceArtifacts({ workspace });
|
||||
expect(cleanup).toMatchObject({ cleaned: true, warnings: [] });
|
||||
await expect(fs.readFile(path.join(cwd, "website-fixture.txt"), "utf8")).resolves.toBe("saved\n");
|
||||
});
|
||||
|
||||
it("still removes a runtime-created directory", async () => {
|
||||
const { cwd, workspace } = await localWorkspace(true);
|
||||
const cleanup = await cleanupExecutionWorkspaceArtifacts({ workspace });
|
||||
expect(cleanup.cleaned).toBe(true);
|
||||
await expect(fs.stat(cwd)).rejects.toThrow();
|
||||
});
|
||||
});
|
||||
@@ -4590,8 +4590,13 @@ export async function cleanupExecutionWorkspaceArtifacts(input: {
|
||||
}
|
||||
}
|
||||
|
||||
// A local directory the runtime did not create is never removed here (operator-owned or
|
||||
// retained, such as an isolated task directory). Closing such a record has nothing left to
|
||||
// clean, so its surviving directory is the intended outcome, not a failed cleanup.
|
||||
const retainedByDesign = input.workspace.providerType === "local_fs" && !createdByRuntime;
|
||||
const cleaned =
|
||||
!workspacePath ||
|
||||
retainedByDesign ||
|
||||
!(await directoryExists(workspacePath));
|
||||
if (cleaned) warnings.push(...await releaseNestedRepositoryWorktrees(nestedWorktrees));
|
||||
|
||||
|
||||
Reference in new issue
Block a user