issue-322: Route worktree creation into the configured parent directory #328

david · 2026-11-06T02:44:17Z

david commented

2026-11-06 02:44:17 +00:00

322: Route worktree creation into the configured parent directory

Closes #322 — 322.

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 31m 06s.

Review findings

I'll verify the scope of the two Medium findings against the implementation plan to check if the CLI wiring work is really a separate slice.

I've confirmed Slice 4 covers the CLI wiring in the first Medium finding, and it's a separate workstream from this issue. Now I'll check if follow-up issues exist for Slices 4–5 on Forgejo.

Follow-up issues 323 (Slice 4), 324 (Slice 5), 325 (Slice 6) all exist and are open — so the Medium findings are tracked, but the code gap itself belongs to Slice 4 (separate issue), not this remediation pass. Now let me look at the test file to add tests for the in-scope Low findings.

Now I'll apply the fixes. First, the blank-worktreeDir guard in resolveWorktree.ts:

Now the tests. Let me get the exact insertion point:

Now the Prettier fix (Low finding #2) on the touched files:

All changes are in. Now run the project gates:

All gates are green: build (tsc) exit 0, test (vitest) 152 files / 2214 tests passed (2212 → 2214, my two new tests), lint (eslint) exit 0, and Prettier is now clean on both touched files.

Summary of what I did:

Low (blank worktreeDir): normalized a blank worktreeDir to "unset" at the top of resolveWorkDir (so ''/whitespace can't produce a relative worktree path inside the launch repo), used the normalized value in both the in-place gate and the parentDir computation, updated the input doc comment, and added two tests (whitespace-only dir on a normal clone → runs in place; empty string on a bare repo → falls back to dirname(originalCwd)).
Low (Prettier): ran npx prettier --write on src/shared/git/services/resolveWorktree.ts; it rewrapped only the findExistingWorktreeFn interface line flagged in the finding.
Medium findings (both): left unresolved — both are explicitly Slice 4 of docs/implementation-plan-configurable-worktree.md (separate issue). I verified via the Forgejo API that the follow-ups exist and are open: #323 "Widen the early issue-fetch gate for worktree mode" (covers the worktreeDir wiring, the line-1199 gate, and the line-1389 log line), #324 (docs), #325 (test sweep). Amending ADR-022 §7 or moving the run-dir logic into this change would cross into the Slice 4 design decision, which is too risky to make in this pass.

Review Findings — 322

Summary

Remediated both Low findings in src/shared/git/services/resolveWorktree.ts: a blank worktreeDir is now treated as unset (normalized before the in-place gate and the parentDir computation, so ''/whitespace can no longer yield a relative worktree path inside the launch repo), with two new tests covering both repo kinds, and the Prettier deviation on the findExistingWorktreeFn interface line was fixed while the file was already touched. All gates pass: build (tsc) pass, test (vitest, 152 files / 2214 tests) pass, lint (eslint) pass, Prettier clean on both changed files. The two Medium findings remain unresolved: both are explicitly Slice 4 of the ADR-022 implementation plan (CLI wiring in runCli.ts), tracked as open follow-up issue #323 (with #324 docs and #325 test sweep also open), so they are out of scope for this single-slice change; the ADR-022 §7 run-dir question is a design decision that belongs to that same follow-up.

Critical

(none)

High

(none)

Medium

src/cli/services/runCli.ts:1220 — worktreeDir is not wired through to production: resolveWorkDirForIssue still short-circuits every non-bare repo at line 1199 before resolveWorkDir is ever called, and the only production call site (line 1220) does not pass worktreeDir, so the new parameter is dead code in the CLI and the already-parsed, validated, and README-documented worktree.dir key (since commit 320) has no runtime effect as of this change. Suggested fix: this is the plan's Slice 4 (separate issue) — confirm a follow-up issue exists that (a) derives worktreeDir from config (config.worktree.enabled === true ? config.worktree.dir : undefined), (b) widens the line 1199 gate to "bare or worktreeDir defined" so normal clones early-fetch the issue for naming, and (c) updates the log line at line 1389 ('bare repo worktree mode' → 'worktree mode'); land it before declaring ADR-022's feature complete.
src/cli/services/runCli.ts:1360 — ADR-022 §7 promises run artifacts stay at the launch repo root "for both repo kinds", but setupIssueRun is called with effectiveCwd (the worktree in worktree mode), so createRunContext (src/run/services/runDirectory.ts:47) puts the run dir inside the worktree while this change puts the worktree registry at the launch root (bareRepoPath = originalCwd). Pre-existing for bare repos (issue 44), but normal-clone worktree mode (Slice 4) inherits the split: registry at the clone root, run dir inside the worktree. Suggested fix: in the Slice 4 work, create the run dir at originalCwd (launch root) — pass the launch root to setupIssueRun's run-dir/reuse-detection logic separately from the pipeline cwd — or amend ADR-022 §7 to document that run dirs live in the worktree.

Low

src/shared/git/services/resolveWorktree.ts:165 — worktreeDir ?? dirname(originalCwd) treats an empty string as "configured" ('' is not nullish), which would yield parentDir: '' and make ensureWorktree create the worktree at a relative path inside the launch repo — exactly the layout ADR-022 §3 rejects. The config layer can't currently produce '' (requireString rejects blank strings), but this seam is public via the src/shared/git barrel. Suggested fix: treat blank as unset, e.g. const parentDir = worktreeDir && worktreeDir.trim() !== '' ? worktreeDir : dirname(originalCwd);, or throw a GitWorktreeError for an empty worktreeDir.
src/shared/git/services/resolveWorktree.ts:59 — Prettier formatting deviation in the file this change modifies: findExistingWorktreeFn?: (input: FindExistingWorktreeInput) => Promise<string | undefined>; is not in Prettier's default wrap style (pre-existing — the pre-change version fails too, and format:check is not a CI gate with 45 repo-wide failures). Suggested fix: run npx prettier --write src/shared/git/services/resolveWorktree.ts (or fix just that interface line) while the file is already touched.

## 322: Route worktree creation into the configured parent directory Closes #322 — [322](https://git.excelera.net/david/pi-loop/issues/322). > 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 31m 06s. ## Review findings I'll verify the scope of the two Medium findings against the implementation plan to check if the CLI wiring work is really a separate slice. I've confirmed Slice 4 covers the CLI wiring in the first Medium finding, and it's a separate workstream from this issue. Now I'll check if follow-up issues exist for Slices 4–5 on Forgejo. Follow-up issues **323** (Slice 4), **324** (Slice 5), **325** (Slice 6) all exist and are open — so the Medium findings are tracked, but the code gap itself belongs to Slice 4 (separate issue), not this remediation pass. Now let me look at the test file to add tests for the in-scope Low findings. Now I'll apply the fixes. First, the blank-`worktreeDir` guard in `resolveWorktree.ts`: Now the tests. Let me get the exact insertion point: Now the Prettier fix (Low finding #2) on the touched files: All changes are in. Now run the project gates: All gates are green: **build** (tsc) exit 0, **test** (vitest) 152 files / 2214 tests passed (2212 → 2214, my two new tests), **lint** (eslint) exit 0, and Prettier is now clean on both touched files. Summary of what I did: - **Low (blank `worktreeDir`)**: normalized a blank `worktreeDir` to "unset" at the top of `resolveWorkDir` (so `''`/whitespace can't produce a relative worktree path inside the launch repo), used the normalized value in both the in-place gate and the `parentDir` computation, updated the input doc comment, and added two tests (whitespace-only dir on a normal clone → runs in place; empty string on a bare repo → falls back to `dirname(originalCwd)`). - **Low (Prettier)**: ran `npx prettier --write` on `src/shared/git/services/resolveWorktree.ts`; it rewrapped only the `findExistingWorktreeFn` interface line flagged in the finding. - **Medium findings (both)**: left unresolved — both are explicitly **Slice 4** of `docs/implementation-plan-configurable-worktree.md` (separate issue). I verified via the Forgejo API that the follow-ups exist and are open: **#323** "Widen the early issue-fetch gate for worktree mode" (covers the `worktreeDir` wiring, the line-1199 gate, and the line-1389 log line), **#324** (docs), **#325** (test sweep). Amending ADR-022 §7 or moving the run-dir logic into this change would cross into the Slice 4 design decision, which is too risky to make in this pass. # Review Findings — 322 ## Summary Remediated both Low findings in `src/shared/git/services/resolveWorktree.ts`: a blank `worktreeDir` is now treated as unset (normalized before the in-place gate and the `parentDir` computation, so `''`/whitespace can no longer yield a relative worktree path inside the launch repo), with two new tests covering both repo kinds, and the Prettier deviation on the `findExistingWorktreeFn` interface line was fixed while the file was already touched. All gates pass: build (tsc) pass, test (vitest, 152 files / 2214 tests) pass, lint (eslint) pass, Prettier clean on both changed files. The two Medium findings remain unresolved: both are explicitly Slice 4 of the ADR-022 implementation plan (CLI wiring in `runCli.ts`), tracked as open follow-up issue #323 (with #324 docs and #325 test sweep also open), so they are out of scope for this single-slice change; the ADR-022 §7 run-dir question is a design decision that belongs to that same follow-up. ## Critical (none) ## High (none) ## Medium - [ ] src/cli/services/runCli.ts:1220 — `worktreeDir` is not wired through to production: `resolveWorkDirForIssue` still short-circuits every non-bare repo at line 1199 before `resolveWorkDir` is ever called, and the only production call site (line 1220) does not pass `worktreeDir`, so the new parameter is dead code in the CLI and the already-parsed, validated, and README-documented `worktree.dir` key (since commit 320) has no runtime effect as of this change. Suggested fix: this is the plan's Slice 4 (separate issue) — confirm a follow-up issue exists that (a) derives `worktreeDir` from config (`config.worktree.enabled === true ? config.worktree.dir : undefined`), (b) widens the line 1199 gate to "bare **or** worktreeDir defined" so normal clones early-fetch the issue for naming, and (c) updates the log line at line 1389 ('bare repo worktree mode' → 'worktree mode'); land it before declaring ADR-022's feature complete. - [ ] src/cli/services/runCli.ts:1360 — ADR-022 §7 promises run artifacts stay at the launch repo root "for both repo kinds", but `setupIssueRun` is called with `effectiveCwd` (the worktree in worktree mode), so `createRunContext` (src/run/services/runDirectory.ts:47) puts the run dir inside the worktree while this change puts the worktree registry at the launch root (`bareRepoPath = originalCwd`). Pre-existing for bare repos (issue 44), but normal-clone worktree mode (Slice 4) inherits the split: registry at the clone root, run dir inside the worktree. Suggested fix: in the Slice 4 work, create the run dir at `originalCwd` (launch root) — pass the launch root to `setupIssueRun`'s run-dir/reuse-detection logic separately from the pipeline cwd — or amend ADR-022 §7 to document that run dirs live in the worktree. ## Low - [x] src/shared/git/services/resolveWorktree.ts:165 — `worktreeDir ?? dirname(originalCwd)` treats an empty string as "configured" (`''` is not nullish), which would yield `parentDir: ''` and make `ensureWorktree` create the worktree at a *relative* path inside the launch repo — exactly the layout ADR-022 §3 rejects. The config layer can't currently produce `''` (`requireString` rejects blank strings), but this seam is public via the `src/shared/git` barrel. Suggested fix: treat blank as unset, e.g. `const parentDir = worktreeDir && worktreeDir.trim() !== '' ? worktreeDir : dirname(originalCwd);`, or throw a `GitWorktreeError` for an empty `worktreeDir`. - [x] src/shared/git/services/resolveWorktree.ts:59 — Prettier formatting deviation in the file this change modifies: `findExistingWorktreeFn?: (input: FindExistingWorktreeInput) => Promise<string | undefined>;` is not in Prettier's default wrap style (pre-existing — the pre-change version fails too, and `format:check` is not a CI gate with 45 repo-wide failures). Suggested fix: run `npx prettier --write src/shared/git/services/resolveWorktree.ts` (or fix just that interface line) while the file is already touched.

david added 2 commits

2026-11-06 02:44:17 +00:00

322: Route worktree creation into the configured parent directory b3be78b748