Add parse-time directory validation helper for worktree.dir #321

New issue

david · 2026-11-05T21:41:43Z

david commented

2026-11-05 21:41:43 +00:00

Summary

Add a filesystem validation helper for worktree.dir (tilde, absolute, not-inside-repo, exists-as-directory) and wire it into loadConfigFile, so a misconfigured folder fails at config-parse time as a ConfigFileError — before any git call or agent session.

Background

A preceding step in this series adds the worktree: section to the parser. ADR-022 §4 requires every dir problem to be found at config-parse time — before arg parsing finishes, before origin detection, before any git call or agent session — raised as ConfigFileError, which flows through the existing reportConfigFileError path in src/cli/services/runCli.ts → error.json with kind: config, a stderr message, exit 1.

Key constraints:

parseConfigFileYaml(contents, filePath) (in src/config/services/configFile.ts) stays a pure function — the filesystem checks live in a small helper that loadConfigFile calls after successful parsing.
loadConfigFile(cwd) is currently synchronous (readFileSync + pure parse). Keep it synchronous: use statSync/realpathSync inside the helper so every existing caller and test stays unchanged.
The helper is gated on the section being enabled (absent enabled resolves to true). An absent or disabled section must never touch the filesystem.
No writability test. fs.access with W_OK is advisory (ACLs, containers, root can all make it lie); git's own error remains the authority for a read-only folder.
--help and --list-models bypass config loading entirely (the informationalMode check in runCli), so a broken dir never blocks them.

Check order (each its own ConfigFileError):

Not tilde: dir === '~' or dir.startsWith('~/') → a dedicated, helpful message telling the operator to write the full path (e.g. /home/<user>/wt). Rationale: the shell expands ~; YAML and fs do not — path.isAbsolute('~/wt') is false, so accepting it would silently create a folder literally named ~. Tilde expansion is rejected because the home directory differs per user and per host.
Absolute: !path.isAbsolute(dir) → error. No relative-to-repo or relative-to-cwd resolution (in bare mode, cwd is the bare repo, so a relative path would mean something surprising).
Not inside the repo: resolve both paths and compare with a separator-aware prefix test:
```
const resolved = path.resolve(dir);
const root = path.resolve(repoRoot);
const inside = resolved === root || resolved.startsWith(root + path.sep);
```
Reject when inside. Do not use startsWith(root) alone — /srv/wt2 must not match /srv/wt. The check is one-directional: a dir that is a parent of the repo is allowed. Rationale: a worktree inside the working tree pollutes git status and every diff, and risks nesting one worktree in another.
Exists and is a directory: stat(resolved); a throw or !isDirectory() → error naming the path.
Symlinks: a symlinked dir that resolves inside the repo must be rejected — where both exist, compare fs.realpathSync of both paths for the prefix test. Record the choice in a code comment.

Depends on: #320

Implementation Details

New file src/config/helpers/validateWorktreeDir.ts. Note: src/config/helpers/ does not exist yet — config code currently lives in src/config/services/ and src/config/types/; this step creates the helpers directory (with its re-export).
```
export interface ValidateWorktreeDirInput {
  dir: string;
  /** Launch repo root; defaults to process.cwd(). */
  repoRoot: string;
  /** stat implementation (injectable for tests). Defaults to fs.statSync. */
  fsStat?: (path: string) => { isDirectory: () => boolean };
}
export function validateWorktreeDir(input: ValidateWorktreeDirInput): void;
```
(An async variant with Promise<...> was sketched in the plan; prefer the synchronous statSync version so loadConfigFile keeps its current synchronous signature. The injectable seam is the test contract.)
loadConfigFile(cwd): after parseConfigFileYaml succeeds, when (result.worktree.enabled ?? true) and result.worktree.dir !== undefined, call validateWorktreeDir({ dir: result.worktree.dir, repoRoot: cwd }). The helper's ConfigFileError propagates unchanged.
Export the helper from src/config/index.ts.

Acceptance Criteria

An existing directory outside the repo passes.
A missing path fails with a ConfigFileError naming the path.
A path to a regular file fails.
A relative path fails; ~ and ~/x fail with the tilde-specific message.
dir === repoRoot fails; dir inside repoRoot fails; a parent of the repo passes; sibling /srv/wt2 against root /srv/wt passes.
enabled: false with a nonexistent dir → no error, no filesystem access.
A symlinked dir that resolves inside the repo fails.
loadConfigFile surfaces the helper's ConfigFileError unchanged (verified with a temp-dir test writing a real piloop-config.yaml).
npm run lint and npm test pass.

Test Plan

New test file src/config/helpers/validateWorktreeDir.test.ts using a temp directory (created via fs.mkdtempSync, cleaned up after) covering every case above, plus a loadConfigFile test in configFile.test.ts that writes a broken config into a temp dir. Run npm test.

## Summary Add a filesystem validation helper for `worktree.dir` (tilde, absolute, not-inside-repo, exists-as-directory) and wire it into `loadConfigFile`, so a misconfigured folder fails at config-parse time as a `ConfigFileError` — before any git call or agent session. ## Background A preceding step in this series adds the `worktree:` section to the parser. ADR-022 §4 requires **every** `dir` problem to be found at config-parse time — before arg parsing finishes, before origin detection, before any git call or agent session — raised as `ConfigFileError`, which flows through the existing `reportConfigFileError` path in `src/cli/services/runCli.ts` → `error.json` with `kind: config`, a stderr message, exit 1. Key constraints: - `parseConfigFileYaml(contents, filePath)` (in `src/config/services/configFile.ts`) stays a **pure** function — the filesystem checks live in a small helper that `loadConfigFile` calls after successful parsing. - `loadConfigFile(cwd)` is currently synchronous (`readFileSync` + pure parse). Keep it synchronous: use `statSync`/`realpathSync` inside the helper so every existing caller and test stays unchanged. - The helper is gated on the section being enabled (absent `enabled` resolves to `true`). An absent or disabled section must never touch the filesystem. - **No writability test.** `fs.access` with `W_OK` is advisory (ACLs, containers, root can all make it lie); git's own error remains the authority for a read-only folder. - `--help` and `--list-models` bypass config loading entirely (the `informationalMode` check in `runCli`), so a broken `dir` never blocks them. Check order (each its own `ConfigFileError`): 1. **Not tilde**: `dir === '~'` or `dir.startsWith('~/')` → a dedicated, helpful message telling the operator to write the full path (e.g. `/home/<user>/wt`). Rationale: the shell expands `~`; YAML and `fs` do not — `path.isAbsolute('~/wt')` is `false`, so accepting it would silently create a folder literally named `~`. Tilde expansion is rejected because the home directory differs per user and per host. 2. **Absolute**: `!path.isAbsolute(dir)` → error. No relative-to-repo or relative-to-cwd resolution (in bare mode, cwd *is* the bare repo, so a relative path would mean something surprising). 3. **Not inside the repo**: resolve both paths and compare with a **separator-aware** prefix test: ```ts const resolved = path.resolve(dir); const root = path.resolve(repoRoot); const inside = resolved === root || resolved.startsWith(root + path.sep); ``` Reject when `inside`. Do **not** use `startsWith(root)` alone — `/srv/wt2` must not match `/srv/wt`. The check is one-directional: a `dir` that is a *parent* of the repo is allowed. Rationale: a worktree inside the working tree pollutes `git status` and every diff, and risks nesting one worktree in another. 4. **Exists and is a directory**: `stat(resolved)`; a throw or `!isDirectory()` → error naming the path. 5. **Symlinks**: a symlinked `dir` that resolves inside the repo must be rejected — where both exist, compare `fs.realpathSync` of both paths for the prefix test. Record the choice in a code comment. **Depends on:** #320 ## Implementation Details - New file `src/config/helpers/validateWorktreeDir.ts`. Note: `src/config/helpers/` does **not** exist yet — config code currently lives in `src/config/services/` and `src/config/types/`; this step creates the `helpers` directory (with its re-export). ```ts export interface ValidateWorktreeDirInput { dir: string; /** Launch repo root; defaults to process.cwd(). */ repoRoot: string; /** stat implementation (injectable for tests). Defaults to fs.statSync. */ fsStat?: (path: string) => { isDirectory: () => boolean }; } export function validateWorktreeDir(input: ValidateWorktreeDirInput): void; ``` (An async variant with `Promise<...>` was sketched in the plan; prefer the synchronous `statSync` version so `loadConfigFile` keeps its current synchronous signature. The injectable seam is the test contract.) - `loadConfigFile(cwd)`: after `parseConfigFileYaml` succeeds, when `(result.worktree.enabled ?? true)` and `result.worktree.dir !== undefined`, call `validateWorktreeDir({ dir: result.worktree.dir, repoRoot: cwd })`. The helper's `ConfigFileError` propagates unchanged. - Export the helper from `src/config/index.ts`. ## Acceptance Criteria - [ ] An existing directory outside the repo passes. - [ ] A missing path fails with a `ConfigFileError` naming the path. - [ ] A path to a regular file fails. - [ ] A relative path fails; `~` and `~/x` fail with the tilde-specific message. - [ ] `dir === repoRoot` fails; `dir` inside `repoRoot` fails; a parent of the repo passes; sibling `/srv/wt2` against root `/srv/wt` passes. - [ ] `enabled: false` with a nonexistent `dir` → no error, no filesystem access. - [ ] A symlinked `dir` that resolves inside the repo fails. - [ ] `loadConfigFile` surfaces the helper's `ConfigFileError` unchanged (verified with a temp-dir test writing a real `piloop-config.yaml`). - [ ] `npm run lint` and `npm test` pass. ## Test Plan New test file `src/config/helpers/validateWorktreeDir.test.ts` using a temp directory (created via `fs.mkdtempSync`, cleaned up after) covering every case above, plus a `loadConfigFile` test in `configFile.test.ts` that writes a broken config into a temp dir. Run `npm test`.

david added this to the Configurable worktree directory milestone

2026-11-05 21:41:43 +00:00

david added the

enhancement

ready

labels

2026-11-05 21:41:43 +00:00

david referenced this issue

2026-11-05 21:46:26 +00:00

Widen the early issue-fetch gate for worktree mode #323

david referenced this issue

2026-11-05 21:46:26 +00:00