Five small fixes from #1757 and #1762: slash-safe detached write, git branch budget, honest forced removal, only deleted branches named, birth branch off the checkout

packages/agent-data/src/file-branch.test.ts

@@ -329,3 +329,19 @@ test('a detached write lands on origin from any clone without touching the persi
     await rm(solo, RETRIED_RM)
   }
 })
+
+test('a detached write works for a branch named with a slash: the throwaway checkout takes a flat name (#1762)', async () => {
+  const { bare, other, cleanup } = await initSyncedRepos()
+  try {
+    assert.deepEqual(
+      await writeFileBranchDetached(other, 'feature/store', 'slashed', async dir => {
+        await writeFile(join(dir, 'a.md'), 'a\n')
+      }),
+      { ok: true, changed: true },
+    )
+    assert.equal(await git(['show', 'feature/store:a.md'], bare), 'a\n')
+    assert.ok(!(await git(['worktree', 'list'], other)).includes('write-'))
+  } finally {
+    await cleanup()
+  }
+})

packages/agent-data/src/file-branch.ts

@@ -401,7 +401,8 @@ export async function writeFileBranchDetached(
   if (!(await hasRemote(cwd, r.git))) return { ok: false, reason: 'no-remote' }
   const { mkdtemp, rm } = await import('node:fs/promises')
   const { tmpdir } = await import('node:os')
-  const dir = await mkdtemp(join(tmpdir(), `${branch}-write-`))
+  // A slash in the branch name would name a directory under tmpdir that does not exist (#1762).
+  const dir = await mkdtemp(join(tmpdir(), `${branch.replace(/[\\/]/g, '-')}-write-`))
   // The remote's tip, or a parentless start for a branch origin does not have yet.
   const tip = async (): Promise<string> => {
     await r.git(['fetch', 'origin', branch], cwd).catch(() => {})

packages/agent-data/src/git.test.ts

@@ -34,6 +34,10 @@ const BUDGETS: { args: string[]; ms: number }[] = [
   { args: ['branch', '--list', '--merged', 'main', 'topic'], ms: GIT_READ_TIMEOUT_MS },
   { args: ['branch', '--remotes', '--contains', 'refs/heads/agent-x', '--format=%(refname:short)'], ms: GIT_READ_TIMEOUT_MS },
   { args: ['branch'], ms: GIT_READ_TIMEOUT_MS },
+  // A global option ahead of the subcommand is not one of its flags (#1757): the listing still reads.
+  { args: ['-C', '/repo', 'branch', '--list'], ms: GIT_READ_TIMEOUT_MS },
+  { args: ['-c', 'core.quotepath=off', 'branch'], ms: GIT_READ_TIMEOUT_MS },
+  { args: ['-C', '/repo', 'branch', '-D', 'agent-1'], ms: GIT_WRITE_TIMEOUT_MS },
   { args: ['branch', '-D', 'agent-1'], ms: GIT_WRITE_TIMEOUT_MS },
   { args: ['branch', '-m', 'agent-1', 'agent-cool'], ms: GIT_WRITE_TIMEOUT_MS },
   { args: ['branch', 'topic', 'abc123'], ms: GIT_WRITE_TIMEOUT_MS },

packages/agent-data/src/git.ts

@@ -54,14 +54,19 @@ const GIT_GLOBAL_VALUE_OPTIONS = new Set(['-C', '-c', '--git-dir', '--work-tree'
  * would read `git -C /repo push` as the subcommand `/repo`, costing `push` its slow budget.
  */
 function gitWords(args: string[]): string[] {
+  return gitCommand(args).filter(arg => !arg.startsWith('-'))
+}
+
+/** The subcommand and everything after it: the invocation with the leading global options dropped. */
+function gitCommand(args: string[]): string[] {
   let i = 0
   while (i < args.length) {
     const arg = args[i] ?? ''
     if (!arg.startsWith('-')) break
     // The `--opt=value` form carries its value inline; the separate form eats the next word.
     i += GIT_GLOBAL_VALUE_OPTIONS.has(arg) ? 2 : 1
   }
-  return args.slice(i).filter(arg => !arg.startsWith('-'))
+  return args.slice(i)
 }
 
 /**
@@ -81,7 +86,8 @@ export function gitTimeoutMs(args: string[]): number {
   }
   if (op === 'branch') {
     // A bare `branch` or one carrying a listing flag reads; `-D`, `-m`, or `branch <new> [start]` writes a ref.
-    const flags = args.filter(arg => arg.startsWith('-'))
+    // The subcommand's own flags: a global option ahead of it (`-C <repo>`) is not one of them.
+    const flags = gitCommand(args).filter(arg => arg.startsWith('-'))
     const reads = words.length === 1 || flags.some(flag => GIT_BRANCH_READ_FLAGS.has(flag))
     return reads && !flags.some(flag => !GIT_BRANCH_READ_FLAGS.has(flag) && !flag.startsWith('--format')) ? GIT_READ_TIMEOUT_MS : GIT_WRITE_TIMEOUT_MS
   }

packages/skill-branches/src/cli.test.ts

@@ -5,7 +5,7 @@ import { join } from 'node:path'
 import { tmpdir } from 'node:os'
 import { lstat, mkdir, mkdtemp, readFile, readlink, realpath, rm, stat, writeFile } from 'node:fs/promises'
 import { nodeGitRunner } from '@gemstack/agent-data'
-import { CLI_BIN_DIR, agentBranchName, runCli, worktreePath } from './index.js'
+import { CLI_BIN_DIR, agentBranchName, reconcileBranchLinks, runCli, worktreePath } from './index.js'
 
 // #1725: the command line is the package's functions for an agent in a shell, so every command
 // is checked against real git the way the functions are — and the contract on top of them: JSON
@@ -436,3 +436,24 @@ test('a command named like an Object property is not a command (review)', async
     await rm(repo, { recursive: true, force: true })
   }
 })
+
+test("remove <name>: a link's name reaches the checkout, and the birth branch is the checkout's own, not the name (#1757)", async () => {
+  const repo = await repoWithOrigin()
+  try {
+    await run(repo, 'create', 'a1')
+    const path = worktreePath(repo, 'a1')
+    // The agent branches away itself, so the birth branch stays behind; the reconcile links the new name.
+    await git(['checkout', '-q', '-b', 'agent-cool-name'], path)
+    await commitWork(path)
+    await reconcileBranchLinks(repo)
+    assert.equal(await readlink(join(repo, '.branches', 'agent-cool-name')), 'agent-a1')
+    const removed = await run(repo, 'remove', 'cool-name')
+    assert.equal(removed.code, 0)
+    assert.deepEqual(removed.out, { ok: true, branchesDeleted: ['agent-a1'] }, 'the birth branch is the directory\'s, not the argument\'s')
+    await assert.rejects(() => stat(path), 'the checkout is gone')
+    assert.equal(await isSymlink(join(repo, '.branches', 'agent-cool-name')), false, 'and its link')
+    assert.match(await git(['rev-parse', '--verify', 'refs/heads/agent-cool-name'], repo), /^[0-9a-f]{40}/, 'the work branch stays')
+  } finally {
+    await rm(repo, { recursive: true, force: true })
+  }
+})

packages/skill-branches/src/cli.ts

@@ -1,8 +1,8 @@
 import { parseArgs } from 'node:util'
-import { resolve } from 'node:path'
-import { stat } from 'node:fs/promises'
+import { basename, resolve } from 'node:path'
+import { realpath, stat } from 'node:fs/promises'
 import { nodeGitRunner, checkoutRoot, gitReason, type GitRunner } from '@gemstack/agent-data'
-import { agentBranchName, isSafeAgentId } from './branch-names.js'
+import { agentBranchName, agentIdFromWorktreeDir, isSafeAgentId } from './branch-names.js'
 import {
   branchPushed,
   currentBranch,
@@ -177,11 +177,17 @@ const COMMANDS: Record<string, Command> = {
 /** A refusal `remove` and `prune` can add to the reclaim rule's own: there is no such checkout. */
 type RemoveRefusal = { ok: false; reason: 'no-checkout'; agentId: string }
 
-/** One agent's checkout under the reclaim rule; a missing checkout is its own refusal. */
+/**
+ * One agent's checkout under the reclaim rule; a missing checkout is its own refusal. The
+ * argument may be the session name a rename link carries rather than the id, so the birth
+ * branch is read off the checkout's own directory, never off the argument (#1757): a link's
+ * name is the branch the agent chose, and that one is not the birth branch.
+ */
 async function reclaim(repo: string, agentId: string, mayPush: boolean, git: GitRunner): Promise<ReclaimOutcome | RemoveRefusal> {
   const path = worktreePath(repo, agentId)
   if (!(await stat(path).then(s => s.isDirectory(), () => false))) return { ok: false, reason: 'no-checkout', agentId }
-  return reclaimWorktree(repo, path, { birthBranch: agentBranchName(agentId), mayPush, git })
+  const checkout = await realpath(path)
+  return reclaimWorktree(repo, checkout, { birthBranch: agentBranchName(agentIdFromWorktreeDir(basename(checkout))), mayPush, git })
 }
 
 /** Why a checkout stayed, as one line for a person. */

packages/skill-branches/src/reclaim.test.ts

@@ -2,7 +2,7 @@ import { strict as assert } from 'node:assert'
 import { test } from 'node:test'
 import { join } from 'node:path'
 import { tmpdir } from 'node:os'
-import { mkdir, mkdtemp, readFile, realpath, rm, stat, writeFile } from 'node:fs/promises'
+import { chmod, mkdir, mkdtemp, readFile, realpath, rm, stat, writeFile } from 'node:fs/promises'
 import { nodeGitRunner } from '@gemstack/agent-data'
 import { addWorktree, agentBranchName, reclaimWorktree, type ReclaimOptions } from './index.js'
 
@@ -308,3 +308,36 @@ test('a branch renamed after its birth name was pushed is pushed under its new n
     await rm(repo, { recursive: true, force: true })
   }
 })
+
+test('a birth branch another worktree has checked out is not named as deleted (#1757)', async () => {
+  const { repo, path, branch: birth } = await repoWithDirtyWorktree()
+  const git = nodeGitRunner()
+  try {
+    await git(['push', '-q', 'origin', 'HEAD:main'], repo)
+    await git(['checkout', '-q', '-b', 'agent-cool-name'], path)
+    await commitWork(path)
+    // Someone else holds the birth branch out: git will refuse to delete it.
+    await git(['worktree', 'add', '-q', join(repo, 'elsewhere'), birth], repo)
+    assert.deepEqual(await reclaimWorktree(repo, path, ORDINARY), { ok: true }, 'the checkout goes, no branch is claimed deleted')
+    await assert.rejects(() => stat(path), 'the checkout is gone')
+    assert.match(await git(['rev-parse', '--verify', `refs/heads/${birth}`], repo), /^[0-9a-f]{40}/, 'the birth branch still exists')
+  } finally {
+    await rm(repo, { recursive: true, force: true })
+  }
+})
+
+test('a checkout git cannot remove even by force is not reported as reclaimed (#1757)', async () => {
+  const { repo, path } = await repoWithDirtyWorktree()
+  const git = nodeGitRunner()
+  try {
+    await git(['push', '-q', 'origin', 'HEAD:main'], repo)
+    await commitWork(path)
+    // No write permission on the directory: nothing inside it can be unlinked.
+    await chmod(path, 0o555)
+    await assert.rejects(() => reclaimWorktree(repo, path, ORDINARY), 'the removal that failed is said, not swallowed')
+    assert.equal((await stat(path)).isDirectory(), true, 'the checkout is still there')
+  } finally {
+    await chmod(path, 0o755).catch(() => {})
+    await rm(repo, { recursive: true, force: true })
+  }
+})

packages/skill-branches/src/reclaim.ts

@@ -126,15 +126,11 @@ export async function reclaimWorktree(repo: string, path: string, opts: ReclaimO
   await removeWorktree(repo, path, git)
   await pruneWorktrees(repo, git)
   // After the checkout: git refuses to delete a branch a worktree still has checked out.
+  // Only a branch that actually went is named: git refuses to delete a branch another worktree
+  // has checked out, and the caller must not report that one as gone.
   const deleted: string[] = []
-  if (emptyBranch) {
-    await deleteBranch(repo, branch, git)
-    deleted.push(branch)
-  }
-  if (birthBranchGoes && opts.birthBranch) {
-    await deleteBranch(repo, opts.birthBranch, git)
-    deleted.push(opts.birthBranch)
-  }
+  if (emptyBranch && (await deleteBranch(repo, branch, git))) deleted.push(branch)
+  if (birthBranchGoes && opts.birthBranch && (await deleteBranch(repo, opts.birthBranch, git))) deleted.push(opts.birthBranch)
   return deleted.length ? { ok: true, branchesDeleted: deleted } : { ok: true }
 }
 

packages/skill-branches/src/worktree.ts

@@ -1,5 +1,5 @@
 import { basename, dirname, join } from 'node:path'
-import { realpath } from 'node:fs/promises'
+import { realpath, stat } from 'node:fs/promises'
 import { nodeGitRunner, checkoutRoot, type GitRunner, BRANCHES_DIR } from '@gemstack/agent-data'
 import { AGENT_BRANCH_PREFIX, isSafeAgentId, isAgentBranch, agentBranchName, agentIdFromWorktreeDir } from './branch-names.js'
 import { DATA_BRANCH } from '@gemstack/agent-data/names'
@@ -187,19 +187,25 @@ export async function removeWorktree(repo: string, path: string, git: GitRunner
     await git(['worktree', 'remove', '--force', path], repo)
     // stderr: on a CLI run stdout carries the JSON result, and this line would corrupt it.
     console.error(`[branches] forced removal of worktree ${path} (git called it unclean)`)
-  } catch {
-    // Already removed, or never registered: nothing to do.
+  } catch (err) {
+    // Already removed, or never registered: nothing to do. A checkout still on disk is another
+    // matter (#1757): a caller told "removed" while it stands would drop its branch next.
+    if (await stat(path).then(() => true, () => false)) throw err
   }
 }
 
 /**
  * Delete a branch that holds nothing (#1650). `-D`, because "merged" in git's eyes is the wrong
  * test: the caller proved the tip is a commit the remote already has, which is the stronger fact.
  * Forgiving: the checkout is already gone by the time this runs, and a branch that would not
- * delete is a leftover name, not lost work.
+ * delete is a leftover name, not lost work. Says whether it went, so a caller names only the
+ * branches that are actually gone (#1757).
  */
-export async function deleteBranch(repo: string, branch: string, git: GitRunner = nodeGitRunner()): Promise<void> {
-  await git(['branch', '-D', branch], repo).catch(() => undefined)
+export async function deleteBranch(repo: string, branch: string, git: GitRunner = nodeGitRunner()): Promise<boolean> {
+  return git(['branch', '-D', branch], repo).then(
+    () => true,
+    () => false,
+  )
 }
 
 /**