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

david · 2026-11-06T02:13:07Z

david commented

2026-11-06 02:13:07 +00:00

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

Closes #321 — 321.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 24m 01s.

Review findings

I'll start by reading the current state of the files involved in both findings.

Now let me look at the test file and check the ADR-022 reference for context on the error message format:

Let me check ADR-022 for the prescribed error message format:

The ADR doesn't prescribe the exact message format, so consistency with the configFile.ts convention (finding 1) is the right bar. Let me look at the loadConfigFile test setup and the barrel export:

I now have the full picture. Both findings are feasible and small. Let me apply the fixes:

Fix 1 (filePath prefix) — add an optional filePath field to the input, default to CONFIG_FILE_NAME, prefix all messages, and pass the real path from loadConfigFile.
Fix 2 (tilde widening) — check 1 becomes dir.startsWith('~').

Now update loadConfigFile to pass the actual file path so messages match the other parse-time errors:

Now update the tests. First the helper test — add the ~user/wt case to the tilde test and a filePath-prefix test:

Now update the loadConfigFile integration test to verify the full config path is used in the message:

Let me verify the final helper file and check for any other callers of validateWorktreeDir I might have missed:

All callers are covered. Now let me confirm the gate commands in package.json and run the gates:

Build passes. Now run the config module tests, then the full suite:

Config module is green (15 helper tests now). Full suite and lint:

Lint passes, but prettier wants a reformat in the helper. Let me apply it:

Prettier only rewrapped the string concatenation. Re-verify all gates after the reformat:

All gates green. One final full-suite run and a look at the complete working-tree diff to confirm the change is clean:

All gates pass after the fixes:

build (tsc): pass
tests: 152 files / 2206 tests pass (+1 new filePath prefix test)
lint (eslint): pass
prettier: pass on all changed files

Both Low findings are resolved in a single pass:

filePath prefix — added an optional filePath field to ValidateWorktreeDirInput (default CONFIG_FILE_NAME), prefixed all five ConfigFileError messages with ${filePath}: , and passed the real config path from loadConfigFile so the helper's errors now match the requireString/requireBoolean convention. Tests cover the default, an explicit path, and the full path through loadConfigFile.
Tilde widening — check 1 is now dir.startsWith('~'), so ~user/... gets the tilde-specific "write the full absolute path" message instead of the generic absolute-path one; the message mentions ~user, and the tilde test now includes a ~david/wt case.

No commit/stage/push was performed; the working tree carries the remediation for the pipeline to capture.

Review Findings — 321

Summary

Both Low findings fixed in a single pass. (1) ValidateWorktreeDirInput now carries an optional filePath (default CONFIG_FILE_NAME); all five helper ConfigFileError messages are prefixed ${filePath}: , and loadConfigFile passes the real config path — helper errors now match the configFile.ts validation-helper convention, verified by new tests at both the helper and loadConfigFile level. (2) The tilde check was widened to dir.startsWith('~') so ~user/... values get the dedicated tilde guidance, with a ~david/wt case added to the existing tilde test. All gates re-ran and passed: build (tsc) pass, tests pass (152 files / 2206 tests), lint pass, prettier pass. Nothing remains unresolved.

Critical

(none)

High

(none)

Medium

(none)

Low

src/config/helpers/validateWorktreeDir.ts:53 — The helper's ConfigFileError messages (lines 53, 62, 81, 92, 97) omit the filePath: prefix that every other configFile.ts validation error includes (e.g. piloop-config.yaml: "worktree.dir" must be a non-empty string... vs the helper's "worktree.dir" must exist and be a directory...), so two errors from the same config file have inconsistent formats. Suggested fix: add an optional filePath field to ValidateWorktreeDirInput (default CONFIG_FILE_NAME) and prefix each message with ${filePath}: , matching the requireString/requireBoolean convention.
src/config/helpers/validateWorktreeDir.ts:52 — The tilde check only catches ~ and ~/, so the same-family shell shorthand ~user/... (e.g. dir: ~david/wt) falls through to the generic "must be an absolute path" message instead of the tilde-specific "write the full absolute path" guidance. Suggested fix: widen check 1 to dir.startsWith('~') (safe on POSIX — any ~-prefixed path is non-absolute, so it would be rejected by check 2 anyway) and add a ~user/wt case to the existing tilde test.

## 321: Add parse-time directory validation helper for worktree.dir Closes #321 — [321](https://git.excelera.net/david/pi-loop/issues/321). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 24m 01s. ## Review findings I'll start by reading the current state of the files involved in both findings. Now let me look at the test file and check the ADR-022 reference for context on the error message format: Let me check ADR-022 for the prescribed error message format: The ADR doesn't prescribe the exact message format, so consistency with the `configFile.ts` convention (finding 1) is the right bar. Let me look at the `loadConfigFile` test setup and the barrel export: I now have the full picture. Both findings are feasible and small. Let me apply the fixes: **Fix 1 (filePath prefix)** — add an optional `filePath` field to the input, default to `CONFIG_FILE_NAME`, prefix all messages, and pass the real path from `loadConfigFile`. **Fix 2 (tilde widening)** — check 1 becomes `dir.startsWith('~')`. Now update `loadConfigFile` to pass the actual file path so messages match the other parse-time errors: Now update the tests. First the helper test — add the `~user/wt` case to the tilde test and a `filePath`-prefix test: Now update the `loadConfigFile` integration test to verify the full config path is used in the message: Let me verify the final helper file and check for any other callers of `validateWorktreeDir` I might have missed: All callers are covered. Now let me confirm the gate commands in `package.json` and run the gates: Build passes. Now run the config module tests, then the full suite: Config module is green (15 helper tests now). Full suite and lint: Lint passes, but prettier wants a reformat in the helper. Let me apply it: Prettier only rewrapped the string concatenation. Re-verify all gates after the reformat: All gates green. One final full-suite run and a look at the complete working-tree diff to confirm the change is clean: All gates pass after the fixes: - **build** (tsc): pass - **tests**: 152 files / 2206 tests pass (+1 new `filePath` prefix test) - **lint** (eslint): pass - **prettier**: pass on all changed files Both Low findings are resolved in a single pass: 1. **filePath prefix** — added an optional `filePath` field to `ValidateWorktreeDirInput` (default `CONFIG_FILE_NAME`), prefixed all five `ConfigFileError` messages with `${filePath}: `, and passed the real config path from `loadConfigFile` so the helper's errors now match the `requireString`/`requireBoolean` convention. Tests cover the default, an explicit path, and the full path through `loadConfigFile`. 2. **Tilde widening** — check 1 is now `dir.startsWith('~')`, so `~user/...` gets the tilde-specific "write the full absolute path" message instead of the generic absolute-path one; the message mentions `~user`, and the tilde test now includes a `~david/wt` case. No commit/stage/push was performed; the working tree carries the remediation for the pipeline to capture. # Review Findings — 321 ## Summary Both Low findings fixed in a single pass. (1) `ValidateWorktreeDirInput` now carries an optional `filePath` (default `CONFIG_FILE_NAME`); all five helper `ConfigFileError` messages are prefixed `${filePath}: `, and `loadConfigFile` passes the real config path — helper errors now match the `configFile.ts` validation-helper convention, verified by new tests at both the helper and `loadConfigFile` level. (2) The tilde check was widened to `dir.startsWith('~')` so `~user/...` values get the dedicated tilde guidance, with a `~david/wt` case added to the existing tilde test. All gates re-ran and passed: build (tsc) pass, tests pass (152 files / 2206 tests), lint pass, prettier pass. Nothing remains unresolved. ## Critical - (none) ## High - (none) ## Medium - (none) ## Low - [x] src/config/helpers/validateWorktreeDir.ts:53 — The helper's `ConfigFileError` messages (lines 53, 62, 81, 92, 97) omit the `filePath:` prefix that every other `configFile.ts` validation error includes (e.g. `piloop-config.yaml: "worktree.dir" must be a non-empty string...` vs the helper's `"worktree.dir" must exist and be a directory...`), so two errors from the same config file have inconsistent formats. Suggested fix: add an optional `filePath` field to `ValidateWorktreeDirInput` (default `CONFIG_FILE_NAME`) and prefix each message with `${filePath}: `, matching the `requireString`/`requireBoolean` convention. - [x] src/config/helpers/validateWorktreeDir.ts:52 — The tilde check only catches `~` and `~/`, so the same-family shell shorthand `~user/...` (e.g. `dir: ~david/wt`) falls through to the generic "must be an absolute path" message instead of the tilde-specific "write the full absolute path" guidance. Suggested fix: widen check 1 to `dir.startsWith('~')` (safe on POSIX — any `~`-prefixed path is non-absolute, so it would be rejected by check 2 anyway) and add a `~user/wt` case to the existing tilde test.

david added 2 commits

2026-11-06 02:13:07 +00:00

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