issue-325: Full test sweep for the worktree decision table #331

david · 2026-11-06T04:26:49Z

david commented

2026-11-06 04:26:49 +00:00

325: Full test sweep for the worktree decision table

Closes #325 — 325.

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 22m 53s.

Review findings

Let me look at the makeDeps definition to confirm the detectOriginFn dependency seam:

The pattern is confirmed. Now applying the fix for finding 1 — pin detectOriginFn in the new runCli test:

Now let me verify the fix. First, check the test command and run the affected test file:

Now let me verify the fix context — check makeDeps and the gitlabOrigin fixture in the test file:

The fix is consistent — makeDeps spreads ...overrides last, so my detectOriginFn override takes effect. Now run the gates:

Targeted test passes. Now the full gates:

All 2229 tests pass. Now lint and format check on the changed file:

All gates green. Let me confirm the final working-tree diff is minimal:

All gates are green (build, 2229/2229 tests, lint, prettier on the changed file), and the diff is the minimal fix suggested. Finding 2 remains unresolved by design: it's pre-existing repo-wide Prettier drift in 42 files unrelated to this change, and its own suggested fix is to leave it untouched in this MR (separate chore) — fixing it here would bloat the diff.

Review Findings — 325

Summary

Test-only change (7 new tests) completing the ADR-022 worktree decision-table sweep: six loadConfigFile-level cases (relative dir, dir == repo root, dir inside repo, dir is a regular file, parent-of-repo accepted, prefix-sharing sibling accepted) plus one runCli-level case (bad worktree.dir → exit 1, error.json with kind: config/stage: setup, pipeline never invoked). All gates ran: build pass, test pass (152 files / 2229 tests), lint pass; prettier --check fails on 42 pre-existing files none of which are in this diff (both changed files are prettier-clean). No correctness, convention, or coverage gaps found in the diff itself.

Remediation: fixed the Low finding in runCli.test.ts (added a detectOriginFn spy passed through makeDeps plus expect(detectOriginFn).not.toHaveBeenCalled(), so the "before origin detection" comment is now actually pinned). The runCli test file re-ran green (118/118) and the full suite stays green (2229/2229); lint and prettier pass on the changed file. Left unresolved: the pre-existing repo-wide Prettier drift (42 files) — out of scope for this test-only change, and its suggested fix is to defer it to a separate chore.

Critical

(empty)

High

(empty)

Medium

(empty)

Low

src/cli/services/runCli.test.ts:2696 — The new test's comment claims the abort happens "before origin detection or any git call", but the assertions only pin runPipelineFn not being called; the default detectOriginFn in makeDeps is a plain function, so origin detection is not actually asserted (same gap as the sibling "unknown config key" test at line 2392). Suggested fix: follow the pattern at line 170 — const detectOriginFn = vi.fn(() => Promise.resolve(gitlabOrigin));, pass it in makeDeps({ argv: ['ABC-1'], runPipelineFn, detectOriginFn }), and assert expect(detectOriginFn).not.toHaveBeenCalled().
src/config/services/configFile.test.ts:426 — npm run format:check fails repo-wide on 42 files with pre-existing Prettier drift (e.g. src/model/vertexAnthropic/auth.test.ts from PR #169); no file in this diff is among them, so this is out of scope for issue 325 but will keep the format gate red for any subsequent MR. Suggested fix: leave untouched in this MR (note in the MR description as a known pre-existing gap) or handle in a separate chore running npm run format.

## 325: Full test sweep for the worktree decision table Closes #325 — [325](https://git.excelera.net/david/pi-loop/issues/325). > 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 22m 53s. ## Review findings Let me look at the `makeDeps` definition to confirm the `detectOriginFn` dependency seam: The pattern is confirmed. Now applying the fix for finding 1 — pin `detectOriginFn` in the new runCli test: Now let me verify the fix. First, check the test command and run the affected test file: Now let me verify the fix context — check `makeDeps` and the `gitlabOrigin` fixture in the test file: The fix is consistent — `makeDeps` spreads `...overrides` last, so my `detectOriginFn` override takes effect. Now run the gates: Targeted test passes. Now the full gates: All 2229 tests pass. Now lint and format check on the changed file: All gates green. Let me confirm the final working-tree diff is minimal: All gates are green (build, 2229/2229 tests, lint, prettier on the changed file), and the diff is the minimal fix suggested. Finding 2 remains unresolved by design: it's pre-existing repo-wide Prettier drift in 42 files unrelated to this change, and its own suggested fix is to leave it untouched in this MR (separate chore) — fixing it here would bloat the diff. # Review Findings — 325 ## Summary Test-only change (7 new tests) completing the ADR-022 worktree decision-table sweep: six `loadConfigFile`-level cases (relative dir, dir == repo root, dir inside repo, dir is a regular file, parent-of-repo accepted, prefix-sharing sibling accepted) plus one `runCli`-level case (bad `worktree.dir` → exit 1, `error.json` with `kind: config`/`stage: setup`, pipeline never invoked). All gates ran: build **pass**, test **pass** (152 files / 2229 tests), lint **pass**; `prettier --check` fails on 42 pre-existing files none of which are in this diff (both changed files are prettier-clean). No correctness, convention, or coverage gaps found in the diff itself. Remediation: fixed the Low finding in `runCli.test.ts` (added a `detectOriginFn` spy passed through `makeDeps` plus `expect(detectOriginFn).not.toHaveBeenCalled()`, so the "before origin detection" comment is now actually pinned). The `runCli` test file re-ran green (118/118) and the full suite stays green (2229/2229); lint and prettier pass on the changed file. Left unresolved: the pre-existing repo-wide Prettier drift (42 files) — out of scope for this test-only change, and its suggested fix is to defer it to a separate chore. ## Critical (empty) ## High (empty) ## Medium (empty) ## Low - [x] src/cli/services/runCli.test.ts:2696 — The new test's comment claims the abort happens "before origin detection or any git call", but the assertions only pin `runPipelineFn` not being called; the default `detectOriginFn` in `makeDeps` is a plain function, so origin detection is not actually asserted (same gap as the sibling "unknown config key" test at line 2392). Suggested fix: follow the pattern at line 170 — `const detectOriginFn = vi.fn(() => Promise.resolve(gitlabOrigin));`, pass it in `makeDeps({ argv: ['ABC-1'], runPipelineFn, detectOriginFn })`, and assert `expect(detectOriginFn).not.toHaveBeenCalled()`. - [ ] src/config/services/configFile.test.ts:426 — `npm run format:check` fails repo-wide on 42 files with pre-existing Prettier drift (e.g. `src/model/vertexAnthropic/auth.test.ts` from PR #169); no file in this diff is among them, so this is out of scope for issue 325 but will keep the format gate red for any subsequent MR. Suggested fix: leave untouched in this MR (note in the MR description as a known pre-existing gap) or handle in a separate chore running `npm run format`.

david added 2 commits

2026-11-06 04:26:49 +00:00

325: Full test sweep for the worktree decision table 69fcd4a6e4