Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ When configured, each builder worktree (`.builders/<id>/`) becomes runnable —
}
```

- `symlinks`: globs resolve from the workspace root; matches symlink into the worktree at the same relative path. Root `.env` and `.codev/config.json` are *always* symlinked regardless. **Symlinks, not copies** — edits to main's env files reflect instantly in any running dev session.
- `symlinks`: globs resolve from the workspace root; matches symlink into the worktree at the same relative path. Root `.env` and `.codev/config.json` are *always* symlinked regardless. **Symlinks, not copies** — edits to main's env files reflect instantly in any running dev session. A directory match is silently skipped (so a glob can't mask the worktree's own source) **unless** the entry ends with a trailing slash: `".local-user-data/"` is treated as a literal path and symlinks the directory whole (shared with the parent, not branch-isolated; a dangling link is fine if the source doesn't exist yet).
- `postSpawn`: each command runs sequentially with `cwd` = worktree path. Non-zero exit aborts the spawn loud (half-built worktree stays for inspection).
- `devCommand`: the foreground command that starts your dev server. Required for `afx dev` to work.

Expand Down
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ When configured, each builder worktree (`.builders/<id>/`) becomes runnable —
}
```

- `symlinks`: globs resolve from the workspace root; matches symlink into the worktree at the same relative path. Root `.env` and `.codev/config.json` are *always* symlinked regardless. **Symlinks, not copies** — edits to main's env files reflect instantly in any running dev session.
- `symlinks`: globs resolve from the workspace root; matches symlink into the worktree at the same relative path. Root `.env` and `.codev/config.json` are *always* symlinked regardless. **Symlinks, not copies** — edits to main's env files reflect instantly in any running dev session. A directory match is silently skipped (so a glob can't mask the worktree's own source) **unless** the entry ends with a trailing slash: `".local-user-data/"` is treated as a literal path and symlinks the directory whole (shared with the parent, not branch-isolated; a dangling link is fine if the source doesn't exist yet).
- `postSpawn`: each command runs sequentially with `cwd` = worktree path. Non-zero exit aborts the spawn loud (half-built worktree stays for inspection).
- `devCommand`: the foreground command that starts your dev server. Required for `afx dev` to work.

Expand Down
201 changes: 201 additions & 0 deletions codev/plans/805-allow-directory-entries-in-wor.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
# PIR Plan: Allow directory entries in `worktree.symlinks` via trailing-slash opt-in

Issue: #805 · Area: `area/config`

## Understanding

`worktree.symlinks` in `.codev/config.json` currently accepts **file entries only**.
A directory entry (e.g. `".local-user-data"`) is silently dropped — never symlinked,
never logged.

**Root cause** — `packages/codev/src/agent-farm/commands/spawn-worktree.ts:83-91`:

```ts
for (const pattern of getWorktreeConfig(config.workspaceRoot).symlinks) {
for (const rel of globSync(pattern, { cwd: config.workspaceRoot, dot: true, nodir: true })) {
...
}
}
```

The `nodir: true` glob flag filters directory matches out before they ever reach
`symlinkSync`. That flag is **intentional**: without it, a pattern like `"apps/auth"`
would symlink the worktree's own source directory back at the parent checkout — the
builder's branch would silently edit the parent's working copy, defeating the point
of `git worktree add`. So the fix must be an **opt-in for directories**, not a blanket
relaxation of the guard.

Goal: let a trailing slash on an entry (`".local-user-data/"`) mean "this is a
directory — symlink it", while every entry without a trailing slash keeps today's
exact behaviour (and the footgun guard).

## Proposed Change

In the `worktree.symlinks` loop of `symlinkConfigFiles`, branch on whether the entry
ends with `/`:

- **No trailing slash** → unchanged. Same `globSync(pattern, { dot: true, nodir: true })`
loop, same code path. Directories that happen to match are still filtered out — the
`"apps/auth"` footgun stays guarded.
- **Trailing slash** → strip the slash and treat the remainder as a **literal relative
path** (no glob expansion). Compute `target = resolve(worktreePath, rel)` and
`srcAbs = resolve(workspaceRoot, rel)`, keep the `existsSync(target) → continue`
idempotency check, `mkdirSync(dirname(target), { recursive: true })`, then
`symlinkSync(srcAbs, target)`.

### Why literal-path, not `globSync(nodir: false)`

Acceptance requires: *"Source not existing at spawn time is non-fatal (dangling link
is acceptable — runtime tooling creates the dir)."* A glob can only ever return paths
that **already exist**, so a globbed directory entry could never produce the dangling
link the use case wants (shannon's `.local-user-data/` is created by runtime tooling,
possibly after spawn). Treating the entry as a literal path lets us create the symlink
unconditionally; if the parent dir doesn't exist yet, the link dangles until runtime
tooling materialises it — exactly the desired behaviour. It is also simpler and more
self-documenting (`foo/` = "symlink this exact directory").

Trade-off: glob metacharacters inside a trailing-slash entry (e.g. `packages/*/data/`)
are **not** expanded — the `*` would be taken literally. The concrete use case is a
single literal directory, and directory-glob expansion is explicitly out of scope.
The implement phase will log a warning if a trailing-slash entry contains glob
metacharacters, so a misconfiguration is visible rather than silently producing a
`*`-named link.

### Cross-platform (Windows)

On POSIX the third `symlinkSync` type argument is ignored, but on Windows a directory
symlink needs `symlinkSync(src, dest, 'dir')`. We pass `'dir'` when the source exists
and is a directory:

```ts
const isDir = existsSync(srcAbs) && statSync(srcAbs).isDirectory();
symlinkSync(srcAbs, target, isDir ? 'dir' : undefined);
```

This is correct on POSIX (type ignored) and correct on Windows for the common case
(source exists at spawn). A dangling Windows dir-symlink (source absent at spawn) is
best-effort — documented, not engineered, matching the existing POSIX-leaning code.

### Destination already occupied / idempotency

If anything already occupies the destination we **skip** it — never overwrite or merge:

- A tracked path materialised by `git worktree add`, or a dir created by a `postSpawn`
step → left untouched, builder uses it as-is.
- A previously-created, resolvable symlink → skipped (idempotent re-spawn / `afx setup`).

`existsSync` alone is insufficient because it **follows** symlinks: a *dangling*
directory symlink (a first-class supported case here — source created by runtime
tooling after spawn) reads as "absent", so a setup re-run would call `symlinkSync`
again and throw `EEXIST`. `afx setup` / "Run Worktree Setup" is explicitly idempotent,
so this path is real. Guard with an `lstatSync`-based check that detects the link
itself:

```ts
function pathOccupied(p: string): boolean {
if (existsSync(p)) return true; // real file/dir, or a resolvable symlink
try { lstatSync(p); return true; } // a dangling symlink still occupies the path
catch { return false; }
}
```

The existing file-symlink branch is unaffected (globbed file sources always exist, so
those links are never dangling), but for symmetry and safety the directory branch uses
`pathOccupied`.

### Sketch

```ts
for (const rawPattern of getWorktreeConfig(config.workspaceRoot).symlinks) {
if (rawPattern.endsWith('/')) {
const rel = rawPattern.slice(0, -1);
if (!rel) continue; // guard bare "/"
const target = resolve(worktreePath, rel);
if (pathOccupied(target)) continue; // idempotent + dangling-link safe
const srcAbs = resolve(config.workspaceRoot, rel);
mkdirSync(dirname(target), { recursive: true });
const isDir = existsSync(srcAbs) && statSync(srcAbs).isDirectory();
symlinkSync(srcAbs, target, isDir ? 'dir' : undefined);
logger.info(`Linked directory ${rel}/ from workspace root`);
} else {
for (const rel of globSync(rawPattern, { cwd: config.workspaceRoot, dot: true, nodir: true })) {
const target = resolve(worktreePath, rel);
if (existsSync(target)) continue;
mkdirSync(dirname(target), { recursive: true });
symlinkSync(resolve(config.workspaceRoot, rel), target);
logger.info(`Linked ${rel} from workspace root`);
}
}
}
```

## Files to Change

- `packages/codev/src/agent-farm/commands/spawn-worktree.ts:83-91` — branch the
symlinks loop on trailing slash (per sketch above); add a small `pathOccupied`
helper. Add `statSync` and `lstatSync` to the existing `node:fs` import on line 12.
- `packages/codev/src/agent-farm/types.ts:202-208` — extend the `symlinks` field
JSDoc to document the trailing-slash directory opt-in and the footgun rationale.
- `packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts` — add `statSync`
and `lstatSync` to the `node:fs` mock (line ~22-34, currently inherits the real ones
via `...actual`); add new `symlinkConfigFiles` test cases (see Test Plan).
- `CLAUDE.md` and `AGENTS.md` — in the "Config: the `worktree` block" section, note
that a trailing slash on a `symlinks` entry opts a directory in (one sentence, kept
in sync between the two files per their header contract).

Out of scope (per issue): `git check-ignore` auto-detection; shannon's own
`.codev/config.local.json` edit (tracked separately, one line, cross-repo follow-up).

## Risks & Alternatives Considered

- **Risk — footgun re-opened.** Mitigation: the guard only relaxes for entries that
*explicitly* end in `/`. `"apps/auth"` (no slash) still routes through the
`nodir: true` glob and is filtered out exactly as today. A test asserts a non-slash
directory entry produces no symlink.
- **Risk — `statSync` on a missing source throws.** Mitigation: short-circuit with
`existsSync(srcAbs) &&` before `statSync`, so a dangling-link case never calls
`statSync`. The `node:fs` test mock gains `statSync`/`lstatSync` so unit tests don't
hit the real fs for fake paths.
- **Risk — `EEXIST` on setup re-run over a dangling dir-symlink.** `existsSync` follows
links, so a dangling link reads as absent and `symlinkSync` would throw on re-run.
Mitigation: the `pathOccupied` helper uses `lstatSync` to detect the link itself, so
a re-run skips cleanly. (Never overwrites/merges an occupied destination either way.)
- **Risk — glob metacharacters in a trailing-slash entry.** A `*` would be taken
literally and could create a `*`-named link. Mitigation: log a warning when a
trailing-slash entry contains glob metacharacters; documented as a known limitation.
- **Alternative — `globSync(pattern, { nodir: false })` for slash entries.** Rejected:
cannot satisfy the dangling-link acceptance criterion (glob only returns existing
paths) and adds no value for the literal-directory use case.
- **Alternative — `git check-ignore` to auto-detect untracked dirs.** Rejected by the
issue as out of scope; trailing-slash opt-in is simpler and self-documenting.

## Test Plan

**Unit** (`spawn-worktree.test.ts`, `symlinkConfigFiles` describe block; mirrors the
existing mocked-fs style — no real filesystem):

1. **Directory entry symlinks the dir** — `symlinks: ['.local-user-data/']`, source
exists & is a directory → `symlinkSync('/projects/test/.local-user-data',
'/tmp/wt/.local-user-data', 'dir')`, `globSync` **not** called for that entry.
2. **Dangling link when source absent** — `symlinks: ['.local-user-data/']`,
`existsSync(srcAbs) → false` → `symlinkSync(srcAbs, target, undefined)` still called
(link created), no throw.
3. **Idempotency, real dir** — target dir already exists in worktree (`existsSync →
true`) → `symlinkSync` not called.
4. **Idempotency, dangling link** — `existsSync(target) → false` but `lstatSync(target)`
succeeds (link already present, source still absent) → `symlinkSync` not called (no
`EEXIST`).
5. **Non-slash directory entry stays filtered** — `symlinks: ['apps/auth']`, `globSync`
(nodir:true) returns `[]` → `symlinkSync` not called (footgun guard intact).
6. **File entries unaffected** — existing file-glob tests continue to pass unchanged.

**Build / typecheck**: `pnpm --filter @cluesmith/codev build` and the unit suite green.

**Manual (reviewer, at `dev-approval`)**:
- Add `".local-user-data/"` to a test `.codev/config.json` `worktree.symlinks`, spawn a
builder, confirm `<worktree>/.local-user-data → <workspaceRoot>/.local-user-data` and
that a write through the link lands at the parent path.
- Confirm a file-only config still spawns identically (no behaviour change).

**Cross-platform**: POSIX is the primary target (matches existing code). Windows
dir-symlink type is set when the source exists; not separately gated.
30 changes: 30 additions & 0 deletions codev/projects/805-allow-directory-entries-in-wor/status.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
id: '805'
title: allow-directory-entries-in-wor
protocol: pir
phase: verified
plan_phases: []
current_plan_phase: null
gates:
plan-approval:
status: approved
requested_at: '2026-05-31T03:17:44.673Z'
approved_at: '2026-05-31T03:26:18.777Z'
dev-approval:
status: approved
requested_at: '2026-05-31T03:32:15.088Z'
approved_at: '2026-05-31T03:51:39.244Z'
pr:
status: approved
requested_at: '2026-05-31T04:01:37.296Z'
approved_at: '2026-05-31T04:11:34.590Z'
iteration: 1
build_complete: false
history: []
started_at: '2026-05-31T03:14:03.261Z'
updated_at: '2026-05-31T04:11:43.163Z'
pr_history:
- phase: review
pr_number: 947
branch: builder/pir-805
created_at: '2026-05-31T03:52:55.599Z'
pr_ready_for_human: false
93 changes: 93 additions & 0 deletions codev/reviews/805-allow-directory-entries-in-wor.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# PIR Review: Allow directory entries in `worktree.symlinks` via trailing-slash opt-in

Fixes #805

## Summary

`worktree.symlinks` previously accepted file entries only — directory entries were
silently dropped because the spawn-time symlink loop globbed with `nodir: true`. This
change adds a **trailing-slash opt-in**: an entry like `".local-user-data/"` is treated
as a literal path and symlinked into each new worktree whole, so builders can share a
gitignored runtime-state directory with the parent checkout instead of re-bootstrapping
it. Entries without a trailing slash keep their exact prior behaviour, preserving the
`nodir: true` guard that stops a glob from masking the worktree's own source.

## Files Changed

(vs merge-base `b4904bf9`, code + docs)

- `packages/codev/src/agent-farm/commands/spawn-worktree.ts` (+58 / -14) — branch the
symlinks loop on trailing slash; add `pathOccupied()` helper; `statSync`/`lstatSync` imports
- `packages/codev/src/agent-farm/types.ts` (+16 / -) — `symlinks` field JSDoc documents the opt-in
- `packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts` (+120 / -) — 6 new cases + `node:fs` mock additions
- `CLAUDE.md` (+1 / -1), `AGENTS.md` (+1 / -1) — one-sentence note on the trailing-slash opt-in
- `codev/plans/805-…md`, `codev/state/pir-805_thread.md` — plan + cohort thread (ship with the PR)

## Commits

- `43db7bee` [PIR #805] Symlink directory entries via trailing-slash opt-in
- `be62b59b` [PIR #805] Test directory-symlink opt-in (dir type, dangling, idempotency, footgun)
- `23662e4f` [PIR #805] Document trailing-slash directory opt-in in worktree config
- (plan commits: `d91edb11` draft, `6489bac1` revised — dangling-symlink-safe idempotency)

## Test Results

- `pnpm build`: ✓ pass
- `pnpm test` (porch `tests` check, full suite): ✓ pass (20.5s)
- `spawn-worktree.test.ts`: ✓ 77 passed (6 new)
- Manual verification: human approved the running worktree at the `dev-approval` gate.

New test cases:
1. Directory entry symlinks the dir whole, passing the `'dir'` type
2. Dangling link created when the source directory is absent (no throw)
3. Idempotency — skip when a real target dir already exists
4. Idempotency — skip when a *dangling* link already occupies the target (no `EEXIST`)
5. Non-slash directory entry stays filtered (footgun guard intact)
6. Trailing-slash entry with glob metacharacters → warn + skip

## Architecture Updates

No `arch.md` changes needed. This change extends the behaviour of one existing helper
(`symlinkConfigFiles`) within the already-documented runnable-worktree setup flow; it
introduces no new module, boundary, or pattern. The runnable-worktree config surface
(`worktree.symlinks` / `postSpawn` / `devCommand`) is already described in CLAUDE.md /
AGENTS.md, which this PR keeps in sync.

## Lessons Learned Updates

No `lessons-learned.md` entry added — the one reusable gotcha here is narrow enough to
live in this review rather than the curated lessons file (which MAINTAIN keeps lean):

> **`existsSync` follows symlinks, so it reports `false` for a dangling link** even
> though the link file occupies the path. Any "create a symlink only if absent" guard
> that relies on `existsSync` will throw `EEXIST` on a re-run once a *dangling* link
> exists. Detect the link itself with `lstatSync` (which does not follow it). This
> matters specifically because directory symlinks here are allowed to dangle (the
> source may be created by runtime tooling after spawn) and `afx setup` re-runs the
> setup idempotently.

## Things to Look At During PR Review

- **Literal-path vs glob for trailing-slash entries.** Chosen literal-path because the
acceptance criterion requires a *dangling* link when the source is absent, and a glob
only ever matches existing paths. Trade-off: glob wildcards in a directory entry are
not expanded — `packages/*/data/` would be a literal `*`. Mitigated by a warn-and-skip
when a trailing-slash entry contains glob metacharacters (`/[*?[\]{}!()]/`).
- **`pathOccupied()` / `lstatSync` guard** — the dangling-link idempotency fix (see
Lessons). Test case 4 pins it.
- **Windows dir-symlink type** — `symlinkSync(src, dest, isDir ? 'dir' : undefined)`.
POSIX ignores the type; on Windows it's set only when the source exists at spawn.
Dangling Windows dir-symlinks are best-effort, matching the existing POSIX-leaning code.
- **Footgun guard preserved** — a non-slash entry resolving to a directory is still
filtered by `nodir: true` (test case 5).

## How to Test Locally

- **View diff**: VSCode sidebar → right-click builder `pir-805` → **View Diff**
- **Run dev server**: `afx dev pir-805` (or VSCode → **Run Dev Server**)
- **What to verify** (maps to the plan's Test Plan):
- Add `".local-user-data/"` to a test repo's `.codev/config.json` `worktree.symlinks`,
spawn a builder, confirm `<worktree>/.local-user-data → <workspaceRoot>/.local-user-data`
- A write through the link lands at the parent path
- Source absent at spawn → dangling link created, non-fatal
- A file-only config spawns identically to before (no behaviour change)
Loading
Loading