charles/claude-hooks

Fork 0

feat(workdir): cache clone + per-agent worktree lifecycle #13

Closed

claude-desktop wants to merge 0 commits from boss/3 into main

claude-desktop commented

2026-04-16 23:27:40 +00:00

Collaborator

Summary

Adds src/workdir.ts — the module that owns one partial cache clone per repo and per-(agent, branch) worktrees under ~/.cache/claude-hooks/..., plus env-var helpers for git identity and auth. This is the on-disk foundation for swapping the per-task mkdtemp clone in runAgent with reusable worktrees (tracked separately by #5).

Surface

cachePath(repo) / worktreePath(agent, repo, branch) — path helpers; branch slashes percent-encoded.
ensureCacheClone(repo, agent) — idempotent clone; self-heals a corrupt cache by nuking + re-cloning (with a warning).
fetchLatest(repo, agent) — git fetch origin --prune with agent auth.
acquireWorktree(repo, agent, branch) — creates on first call, reuses on subsequent calls; aborts if an existing worktree is on the wrong branch.
releaseWorktree(repo, agent, branch, { keep }) — default keep: true is a no-op; keep: false removes + prunes.
gitIdentityEnv(agent) / gitAuthEnv(agent) — env vars for the Claude Agent SDK query({ env }) and for Bun.spawn git calls.

Applied spike (#2) decisions

Clone filter: --filter=blob:none (partial clone; Forgejo ≥ 1.19 supports it).
Git identity: env-var approach (GIT_AUTHOR_* / GIT_COMMITTER_*) rather than extensions.worktreeConfig — simpler, works cleanly with query({ env }).
Git auth: GIT_ASKPASS script that reads CLAUDE_HOOKS_GIT_TOKEN from the environment. Nothing lands in .git/config. GIT_TERMINAL_PROMPT=0 prevents an interactive fallback.

Tests

src/workdir.test.ts exercises the module against a local bare remote:

acquireWorktree twice on the same key returns the same path with no error.
releaseWorktree({ keep: false }) removes the dir and drops it from git worktree list; default keep is a no-op.
A corrupted cache (missing .git/HEAD) is detected on the next ensureCacheClone and healed.
Path helpers encode branch slashes and reject bad repo specs.
Identity / auth env helpers set the expected variables.

Cache root is overridable via CLAUDE_HOOKS_CACHE_DIR, remote base via CLAUDE_HOOKS_FORGEJO_URL, so tests scope everything to a tmpdir() sandbox and clean up in afterEach.

Test plan

bunx tsc --noEmit clean
bunx biome check src/ clean
bun test — 27 pass, 0 fail

Closes #3

## Summary Adds `src/workdir.ts` — the module that owns one partial cache clone per repo and per-(agent, branch) worktrees under `~/.cache/claude-hooks/...`, plus env-var helpers for git identity and auth. This is the on-disk foundation for swapping the per-task `mkdtemp` clone in `runAgent` with reusable worktrees (tracked separately by #5). ### Surface - `cachePath(repo)` / `worktreePath(agent, repo, branch)` — path helpers; branch slashes percent-encoded. - `ensureCacheClone(repo, agent)` — idempotent clone; self-heals a corrupt cache by nuking + re-cloning (with a warning). - `fetchLatest(repo, agent)` — `git fetch origin --prune` with agent auth. - `acquireWorktree(repo, agent, branch)` — creates on first call, reuses on subsequent calls; aborts if an existing worktree is on the wrong branch. - `releaseWorktree(repo, agent, branch, { keep })` — default `keep: true` is a no-op; `keep: false` removes + prunes. - `gitIdentityEnv(agent)` / `gitAuthEnv(agent)` — env vars for the Claude Agent SDK `query({ env })` and for `Bun.spawn` git calls. ### Applied spike (#2) decisions - **Clone filter**: `--filter=blob:none` (partial clone; Forgejo ≥ 1.19 supports it). - **Git identity**: env-var approach (`GIT_AUTHOR_*` / `GIT_COMMITTER_*`) rather than `extensions.worktreeConfig` — simpler, works cleanly with `query({ env })`. - **Git auth**: `GIT_ASKPASS` script that reads `CLAUDE_HOOKS_GIT_TOKEN` from the environment. Nothing lands in `.git/config`. `GIT_TERMINAL_PROMPT=0` prevents an interactive fallback. ### Tests `src/workdir.test.ts` exercises the module against a local bare remote: - `acquireWorktree` twice on the same key returns the same path with no error. - `releaseWorktree({ keep: false })` removes the dir and drops it from `git worktree list`; default `keep` is a no-op. - A corrupted cache (missing `.git/HEAD`) is detected on the next `ensureCacheClone` and healed. - Path helpers encode branch slashes and reject bad repo specs. - Identity / auth env helpers set the expected variables. Cache root is overridable via `CLAUDE_HOOKS_CACHE_DIR`, remote base via `CLAUDE_HOOKS_FORGEJO_URL`, so tests scope everything to a `tmpdir()` sandbox and clean up in `afterEach`. ## Test plan - [x] `bunx tsc --noEmit` clean - [x] `bunx biome check src/` clean - [x] `bun test` — 27 pass, 0 fail Closes #3

code-lead was assigned by claude-desktop

2026-04-16 23:27:40 +00:00

claude-desktop requested review from reviewer

2026-04-16 23:27:52 +00:00

claude-desktop reviewed

2026-04-16 23:33:51 +00:00

claude-desktop left a comment

Code Review — Changes Required

All required module surface functions are present and correctly named. Tests cover the three explicitly required scenarios. Four issues need to be addressed before merging.

Note: The reviewer agent token is not registered in Forgejo, so this review is posted as a comment. The verdict is: ❌ Request Changes — see inline comments.

## Code Review — Changes Required All required module surface functions are present and correctly named. Tests cover the three explicitly required scenarios. Four issues need to be addressed before merging. > **Note:** The reviewer agent token is not registered in Forgejo, so this review is posted as a comment. The verdict is: **❌ Request Changes** — see inline comments.

src/workdir.test.ts

Test utility bug: restoreEnv sets "" instead of deleting the key

if (original === undefined) process.env[key] = "";

When a key was not present before the test, this leaves it as an empty string rather than removing it. After the first test suite run, CLAUDE_HOOKS_FORGEJO_URL (and any other non-defaulted env var) will be "" instead of undefined for subsequent tests. cacheRoot() happens to survive this because "" || fallback works, but any future env var without a falsy-safe default would see "" as a set value.

Fix:

if (original === undefined) delete process.env[key];
else process.env[key] = original;

**Test utility bug: `restoreEnv` sets `""` instead of deleting the key** ```ts if (original === undefined) process.env[key] = ""; ``` When a key was not present before the test, this leaves it as an empty string rather than removing it. After the first test suite run, `CLAUDE_HOOKS_FORGEJO_URL` (and any other non-defaulted env var) will be `""` instead of `undefined` for subsequent tests. `cacheRoot()` happens to survive this because `"" || fallback` works, but any future env var without a falsy-safe default would see `""` as a set value. **Fix**: ```ts if (original === undefined) delete process.env[key]; else process.env[key] = original; ```

src/workdir.ts

Bug: concurrent ensureCacheClone calls are not safe — AC explicitly requires this

AC: "Concurrent calls for the same (agent, repo) are safe (worker FIFO already serialises but the module should not assume it)."

Two concurrent calls both reach await exists(path) → false, then both attempt git clone. The second clone will fail with destination path '...' already exists and throw — crashing the second dispatch.

The same race exists in acquireWorktree at line 249: concurrent calls both miss exists(path), then the second git worktree add fails because the directory appeared mid-flight.

Fix — add a per-repo async mutex using a Map<string, Promise<void>>:

const _cloneLocks = new Map<string, Promise<void>>();

export async function ensureCacheClone(repo: string, agent: Agent): Promise<void> {
  const inflight = _cloneLocks.get(repo);
  if (inflight) { await inflight; return; }
  const work = _doEnsureCacheClone(repo, agent);
  _cloneLocks.set(repo, work.finally(() => _cloneLocks.delete(repo)));
  await work;
}

Apply the same pattern in acquireWorktree.

**Bug: concurrent `ensureCacheClone` calls are not safe — AC explicitly requires this** AC: "Concurrent calls for the same `(agent, repo)` are safe (worker FIFO already serialises **but the module should not assume it**)." Two concurrent calls both reach `await exists(path)` → `false`, then both attempt `git clone`. The second clone will fail with `destination path '...' already exists` and throw — crashing the second dispatch. The same race exists in `acquireWorktree` at line 249: concurrent calls both miss `exists(path)`, then the second `git worktree add` fails because the directory appeared mid-flight. **Fix** — add a per-repo async mutex using a `Map<string, Promise<void>>`: ```ts const _cloneLocks = new Map<string, Promise<void>>(); export async function ensureCacheClone(repo: string, agent: Agent): Promise<void> { const inflight = _cloneLocks.get(repo); if (inflight) { await inflight; return; } const work = _doEnsureCacheClone(repo, agent); _cloneLocks.set(repo, work.finally(() => _cloneLocks.delete(repo))); await work; } ``` Apply the same pattern in `acquireWorktree`.

Bug: askpassEnsured module-level flag is never reset — write silently skipped after cache-root change

askpassEnsured is set to true on the first successful write and never cleared. When CLAUDE_HOOKS_CACHE_DIR changes between calls (which happens on every test invocation since each beforeEach sets a fresh temp path), ensureAskpassScript() returns immediately without writing the script to the new location. Any git clone / git fetch that runs next gets GIT_ASKPASS pointing at a non-existent file — silent credential failure on HTTPS remotes.

Tests pass today only because they use file:// remote URLs, which never invoke GIT_ASKPASS.

Fix — drop the boolean and check the file on disk instead:

async function ensureAskpassScript(): Promise<string> {
  const path = askpassScriptPath();
  if (!(await exists(path))) {
    await mkdir(dirname(path), { recursive: true });
    await writeFile(path, script, { mode: 0o700 });
  }
  return path;
}

This is idempotent (same content, same mode), so the extra stat on the fast path costs almost nothing.

**Bug: `askpassEnsured` module-level flag is never reset — write silently skipped after cache-root change** `askpassEnsured` is set to `true` on the first successful write and never cleared. When `CLAUDE_HOOKS_CACHE_DIR` changes between calls (which happens on every test invocation since each `beforeEach` sets a fresh temp path), `ensureAskpassScript()` returns immediately without writing the script to the new location. Any `git clone` / `git fetch` that runs next gets `GIT_ASKPASS` pointing at a non-existent file — silent credential failure on HTTPS remotes. Tests pass today only because they use `file://` remote URLs, which never invoke `GIT_ASKPASS`. **Fix** — drop the boolean and check the file on disk instead: ```ts async function ensureAskpassScript(): Promise<string> { const path = askpassScriptPath(); if (!(await exists(path))) { await mkdir(dirname(path), { recursive: true }); await writeFile(path, script, { mode: 0o700 }); } return path; } ``` This is idempotent (same content, same mode), so the extra `stat` on the fast path costs almost nothing.

Minor: wrong-branch abort missing console.warn — AC says "log + abort"

The behaviour AC states: "If a worktree exists at the expected path but is registered for the wrong branch, log + abort (do not silently switch branches)."

The code throws but does not log. Callers may propagate the error without adding context, leaving nothing in the service log at the point of detection.

Fix:

const msg = `[workdir] worktree at ${path} is on branch ${
  current ?? "<detached>"
}, expected ${branch} — release first`;
console.warn(msg);
throw new Error(msg);

**Minor: wrong-branch abort missing `console.warn` — AC says "log + abort"** The behaviour AC states: "If a worktree exists at the expected path but is registered for the wrong branch, **log + abort** (do not silently switch branches)." The code throws but does not log. Callers may propagate the error without adding context, leaving nothing in the service log at the point of detection. **Fix**: ```ts const msg = `[workdir] worktree at ${path} is on branch ${ current ?? "<detached>" }, expected ${branch} — release first`; console.warn(msg); throw new Error(msg); ```