From 086249a386822d9e8f88ade1cde23da86e6cbe14 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Sat, 31 Oct 2026 17:57:51 +0800 Subject: [PATCH] fix(cases): never delete another clone's files after a failed direct clone A direct clone into an empty folder emptied the folder after any failure, which deleted a concurrent attempt's finished clone when git had written nothing. Clones into one folder are now mutually exclusive in-process, the cleanup is skipped when git refused the folder, never ran, or the mount is unresponsive, and a name linked while the clone ran is a 409 that keeps the folder. Documents path for POST /api/cases/clone. --- CLAUDE.md | 2 +- docs/api-reference.md | 2 + docs/wiki/Core-Concepts.md | 10 ++--- src/git-clone.ts | 45 ++++++++++++++------ src/web/routes/case-routes.ts | 8 ++-- src/web/schemas.ts | 2 +- test/routes/case-clone-custom-folder.test.ts | 32 +++++++++++++- 7 files changed, 78 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 343f6cd6..fff3794d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -244,7 +244,7 @@ The terminal provider scans `terminalLinkLine()`, which extends `terminalPathLin **Ultracode / workflow-run visualization** (opt-in, default OFF): the Workflow tool writes a completion artifact only at run *end*, so live in-flight runs exist solely as transcript dirs. `workflow-run-watcher.ts` therefore synthesizes ACTIVE runs from transcripts until the completion artifact appears and supersedes them. It is **STANDALONE** and deliberately never imports or touches `subagent-watcher.ts`, despite reading the same tree. Two independent toggles: `showUltracodeAgents` (docked panel) and `ultracodeFloatingWindows` (floating windows); the watcher starts if **either** is on. → [architecture-invariants#ultracode-and-workflow-run-visualization](docs/architecture-invariants.md#ultracode-and-workflow-run-visualization) -**Clone a repository as a case** (issue #236, Add Case → **Clone Repo**): `POST /api/cases/clone` clones synchronously into the caller's case space (bounded by `GIT_CLONE_TIMEOUT_MS`, no job store); `POST /api/cases/clone-preflight` checks anonymous cloneability and lists refs. Core in `src/git-clone.ts`. ⚠️ **The URL is a code-execution surface**: refuse every `::` form and a leading `-`, spawn only argv arrays with `--` before operands. ⚠️ Stay non-interactive (`gitNonInteractiveEnv()`) or the open request hangs; never collect credentials, refuse `user:password@` URLs. ⚠️ Timeout kills the process GROUP, remove the destination only if this attempt created it, and repo contents win over scaffolding (existing `CLAUDE.md` kept, hooks merged, repo `.claude/settings*` warned about). ⚠️ A `path` is the folder cloned INTO (no case-name subfolder) and registers a linked case: admin-only in multi-user mode, judged by `prepareNewCasePath()`, empty or new only (`intoEmptyDir` clones straight into an empty one and never removes it). → [architecture-invariants#clone-a-repository-as-a-case](docs/architecture-invariants.md#clone-a-repository-as-a-case) +**Clone a repository as a case** (issue #236, Add Case → **Clone Repo**): `POST /api/cases/clone` clones synchronously into the caller's case space (bounded by `GIT_CLONE_TIMEOUT_MS`, no job store); `POST /api/cases/clone-preflight` checks anonymous cloneability and lists refs. Core in `src/git-clone.ts`. ⚠️ **The URL is a code-execution surface**: refuse every `::` form and a leading `-`, spawn only argv arrays with `--` before operands. ⚠️ Stay non-interactive (`gitNonInteractiveEnv()`) or the open request hangs; never collect credentials, refuse `user:password@` URLs. ⚠️ Timeout kills the process GROUP, remove the destination only if this attempt created it, and repo contents win over scaffolding (existing `CLAUDE.md` kept, hooks merged, repo `.claude/settings*` warned about). ⚠️ A `path` is the folder cloned INTO (no case-name subfolder) and registers a linked case: admin-only in multi-user mode, judged by `prepareNewCasePath()`, empty or new only (`intoEmptyDir` clones straight into an empty one and never removes it; one direct clone per folder at a time, and its cleanup is skipped when git wrote nothing or the mount is unresponsive). → [architecture-invariants#clone-a-repository-as-a-case](docs/architecture-invariants.md#clone-a-repository-as-a-case) **Git worktrees of a case** (case picker → **Git worktrees…**; `src/git-worktrees.ts`, `routes/worktree-routes.ts`): a local repository case keeps worktrees at `/.claude/worktrees/` on `worktree-`, Claude Code's own `--worktree` layout. ⚠️ Only that managed path is ever created or removed, every component `lstat`ed and realpath-checked (a symlinked `.claude` is refused). ⚠️ git never runs for a Docker or remote case, nor at or inside a Docker workspace (`worktree add` runs hooks and filters on the host). ⚠️ Remove is `git worktree remove` WITHOUT `--force`, refused while a live session is inside; branch delete is `-d`, never `-D`. ⚠️ Sessions start through the existing create paths (quick-start `worktree`, or `POST /api/sessions` `workingDir`), never a new spawn path. → [architecture-invariants#git-worktrees-of-a-case](docs/architecture-invariants.md#git-worktrees-of-a-case) diff --git a/docs/api-reference.md b/docs/api-reference.md index 4a0cb6b7..70f9d3a7 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1315,6 +1315,8 @@ The target is judged before anything is written: Admin only in multi-user mode (`403`), like `POST /api/cases/link`: it writes outside the cases directory and into the shared, ownerless registry. If anything fails after the first write, what this call created is removed (the whole folder if it created it, otherwise only the scaffold inside the empty folder you picked) and the response is `500`. +`POST /api/cases/clone` takes the same optional `path`, with `{ name, repository, ref?, shallow?, description?, path? }`. Without `path` the repository is cloned to `/` as always. With `path` the repository is cloned **into that exact folder** (no `` subfolder is added) and registered as a linked case, judged by the same rules as above: `400` for an invalid or blocked path (or a symlink or file at the target), `404` for a missing parent, `422` for an unreachable parent, and `409 ALREADY_EXISTS` for a folder that has files, a name already in use or a folder that is already a case. Admin only in multi-user mode (`403`). An existing empty folder is cloned into directly and is kept, emptied again, if the clone fails; a second clone into the same folder while one is running is refused with `409`. If the clone succeeds but the registry write fails (or the name was linked while git ran, which is `409`), the cloned folder is kept and the error names it: `500 OPERATION_FAILED`, then use Link Existing. + ## Git status `GET /api/sessions/:id/git-status` is what the bottom-bar Git indicator and its panel read (Settings → Header & Panels → Bottom bar, per-device, default off). It reports what the session's workspace has not committed or pushed. **Read-only and offline:** it never fetches, pulls, commits or writes (it runs `git status` with `--no-optional-locks`, so it does not even refresh the index), which is why `behind` is as of the last `git fetch` (the panel's Fetch button is a separate route, below). The session is resolved like every session route (ownership via `findSessionOrFail`; another user's session is `404`). A repository whose root is, or is inside, a Docker case workspace is dropped (from the walk-up, the scan below a folder, and the diff route): a container can write there, and a repository's own clean filter or signature program would run on the host. When a branch's upstream does not exist on the remote (deleted and pruned, or never pushed, as after cloning an empty repository and committing), `upstreamGone` is `true` and the unpushed list falls back to commits on no remote-tracking ref at all. diff --git a/docs/wiki/Core-Concepts.md b/docs/wiki/Core-Concepts.md index fa849ba4..1b2428a0 100644 --- a/docs/wiki/Core-Concepts.md +++ b/docs/wiki/Core-Concepts.md @@ -22,11 +22,11 @@ Codeman-side configuration: Three ways to get one, all under **New or link a case…** at the bottom of the case picker (the case dropdown in the bottom toolbar; on a phone, the case sheet's **Create New Case**): -| How | Result | -| ----------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| **Create New** | A fresh `~/codeman-cases/` with a scaffolded `CLAUDE.md`, or with **Create in a custom folder**, a new folder inside a parent you choose, scaffolded the same way and registered in place like a linked case. | -| **Clone Repo** | A repo cloned into `~/codeman-cases/` (or straight into an empty folder you choose) and registered as a case. Private repos need this machine's own git credentials (see below). | -| **Link Existing** | An existing folder anywhere on disk, registered in place. Nothing is copied or moved. | +| How | Result | +| ----------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Create New** | A fresh `~/codeman-cases/` with a scaffolded `CLAUDE.md`, or with **Create in a custom folder**, a new folder inside a parent you choose, scaffolded the same way and registered in place like a linked case. | +| **Clone Repo** | A repo cloned into `~/codeman-cases/` (or straight into a folder you choose, which must be empty or not exist yet) and registered as a case. Private repos need this machine's own git credentials (see below). | +| **Link Existing** | An existing folder anywhere on disk, registered in place. Nothing is copied or moved. | Linked cases keep living where they are. Deleting a case in Codeman removes the registration, and for a linked case that is all it removes. diff --git a/src/git-clone.ts b/src/git-clone.ts index 249e4234..14d206a8 100644 --- a/src/git-clone.ts +++ b/src/git-clone.ts @@ -53,6 +53,7 @@ import { existsSync } from 'node:fs'; import { lstat, readdir, rename, rm } from 'node:fs/promises'; import { basename, dirname, join } from 'node:path'; import { EXEC_TIMEOUT_MS } from './config/exec-timeout.js'; +import { probePathKind } from './utils/bounded-path-probe.js'; // ─── Tunables ──────────────────────────────────────────────────────────────── @@ -852,6 +853,16 @@ export async function probeGitRemote( }; } +/** Destinations with a direct (`intoEmptyDir`) clone in flight. */ +const directClones = new Set(); + +function destinationExists(): CloneResult { + return { + ok: false, + failure: { code: 'DESTINATION_EXISTS', message: 'The destination directory already exists.', stderr: '' }, + }; +} + /** True for a real (not symlinked) directory with nothing in it. Any read error counts as not empty. */ async function isEmptyDirectory(path: string): Promise { try { @@ -896,19 +907,29 @@ export async function cloneRepository(opts: CloneOptions): Promise }; } if (existsSync(opts.destination)) { - if (!opts.intoEmptyDir || !(await isEmptyDirectory(opts.destination))) { - return { - ok: false, - failure: { code: 'DESTINATION_EXISTS', message: 'The destination directory already exists.', stderr: '' }, - }; + // One direct clone per folder at a time: once two gits have written into the same folder their + // files cannot be told apart, so the second caller is refused before it spawns anything. Taken + // before the emptiness check (which can precede a long wait for a git slot). + if (!opts.intoEmptyDir || directClones.has(opts.destination)) return destinationExists(); + directClones.add(opts.destination); + try { + if (!(await isEmptyDirectory(opts.destination))) return destinationExists(); + // The folder is the user's own, so there is no temporary sibling to rename into place: git fills it. + const direct = await runGit(buildCloneArgs(opts), opts.timeoutMs ?? GIT_CLONE_TIMEOUT_MS, MAX_STDERR_BYTES); + if (direct.code === 0 && !direct.spawnError) return { ok: true, stderr: sanitizeGitOutput(direct.stderr) }; + const failure = classifyGitFailure(direct.stderr, direct.timedOut, direct.spawnError); + // The folder was empty and is ours alone, so what is in it is this attempt's partial tree, but only + // if git wrote anything: not when it refused the folder or never ran (slot timeout, full queue). + // A mount that stopped answering is left alone too, so a libuv worker is not held on it. + if (failure.code !== 'DESTINATION_EXISTS' && !direct.spawnError) { + if ((await probePathKind(opts.destination, { pastCap: true })) === 'directory') { + await emptyDirectory(opts.destination); + } + } + return { ok: false, failure }; + } finally { + directClones.delete(opts.destination); } - // The folder is the user's own, so there is no temporary sibling to rename into place: git fills it. - const direct = await runGit(buildCloneArgs(opts), opts.timeoutMs ?? GIT_CLONE_TIMEOUT_MS, MAX_STDERR_BYTES); - if (direct.code === 0 && !direct.spawnError) return { ok: true, stderr: sanitizeGitOutput(direct.stderr) }; - // It was empty when the clone began, so whatever is in it now is this attempt's partial tree. The - // folder itself stays. - await emptyDirectory(opts.destination); - return { ok: false, failure: classifyGitFailure(direct.stderr, direct.timedOut, direct.spawnError) }; } // Sibling of the destination (same filesystem), so the rename is atomic. diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index cb7f9389..e676a6db 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -859,14 +859,16 @@ export function registerCaseRoutes( // POST /api/cases/link the write is not serialized, so this only narrows the window. try { const fresh = await readLinkedCases(); - if (fresh[name]) throw new Error(`Case "${name}" was linked while the clone ran`); + if (fresh[name]) + throw Object.assign(new Error(`Case "${name}" was linked while the clone ran`), { conflict: true }); fresh[name] = casePath; await fs.writeFile(LINKED_CASES_FILE, JSON.stringify(fresh, null, 2)); } catch (err) { // The clone itself succeeded: keep the folder (the user waited for it) and say where it is. - reply.code(500); + const conflict = (err as { conflict?: boolean }).conflict; + reply.code(conflict ? 409 : 500); return createErrorResponse( - ApiErrorCode.OPERATION_FAILED, + conflict ? ApiErrorCode.ALREADY_EXISTS : ApiErrorCode.OPERATION_FAILED, `Cloned into ${casePath} but could not register the case (${getErrorMessage(err)}). Use Link Existing to add it.` ); } diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 13a8b038..60aaab4e 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -786,7 +786,7 @@ export const CloneCaseSchema = z.object({ shallow: z.boolean().optional(), description: z.string().max(1000).optional(), /** - * Clone into this exact folder (parent + case name) instead of under the cases directory, and + * Clone into this exact folder (the folder itself, no case-name subfolder is added) instead of under the cases directory, and * register it as a linked case. Absolute or starting with `~`; only length-bounded here, since * `prepareNewCasePath()` (web/case-path.ts) judges it and produces the user-facing reason. */ diff --git a/test/routes/case-clone-custom-folder.test.ts b/test/routes/case-clone-custom-folder.test.ts index a5c1ac49..f5a2aa0f 100644 --- a/test/routes/case-clone-custom-folder.test.ts +++ b/test/routes/case-clone-custom-folder.test.ts @@ -24,7 +24,7 @@ import { join } from 'node:path'; import { createRouteTestHarness } from './_route-test-utils.js'; import { registerCaseRoutes } from '../../src/web/routes/case-routes.js'; import { dataPath } from '../../src/config/instance.js'; -import { isGitAvailable } from '../../src/git-clone.js'; +import { cloneRepository, isGitAvailable } from '../../src/git-clone.js'; import { isSandboxedHome, safeRmHomeTree } from '../mocks/index.js'; const LINKED = () => dataPath('linked-cases.json'); @@ -142,6 +142,36 @@ describe.skipIf(!isGitAvailable())('POST /api/cases/clone with a custom folder', expect(linked()).toEqual({}); }); + it('two clones into one empty folder at once: exactly one wins and its files survive', async () => { + const target = join(workDir, 'raced'); + mkdirSync(target); + const results = await Promise.all([ + cloneRepository({ repository: origin, destination: target, intoEmptyDir: true }), + cloneRepository({ repository: origin, destination: target, intoEmptyDir: true }), + ]); + expect(results.filter((r) => r.ok)).toHaveLength(1); + const loser = results.find((r) => !r.ok); + expect(loser && !loser.ok && loser.failure.code).toBe('DESTINATION_EXISTS'); + expect(existsSync(join(target, 'README.md'))).toBe(true); + expect(existsSync(join(target, '.git'))).toBe(true); + }); + + it('a name linked while the clone ran is a 409 that keeps the folder and names it', async () => { + const { app } = await createRouteTestHarness(registerCaseRoutes); + const [a, b] = await Promise.all([ + clone(app, { name: 'twin', repository: origin, path: join(workDir, 'twin-a') }), + clone(app, { name: 'twin', repository: origin, path: join(workDir, 'twin-b') }), + ]); + const loserDir = a.json().success ? join(workDir, 'twin-b') : join(workDir, 'twin-a'); + const loser = a.json().success ? b : a; + expect([a.json().success, b.json().success].filter(Boolean)).toHaveLength(1); + expect(loser.statusCode).toBe(409); + expect(loser.json().errorCode).toBe('ALREADY_EXISTS'); + expect(loser.json().error).toContain(loserDir); + expect(existsSync(join(loserDir, 'README.md'))).toBe(true); + expect(Object.keys(linked())).toEqual(['twin']); + }); + it('refuses a blocked tree such as ~/.ssh', async () => { const { app } = await createRouteTestHarness(registerCaseRoutes); mkdirSync(join(homedir(), '.ssh'), { recursive: true });