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

david · 2026-11-06T03:33:12Z

david commented

2026-11-06 03:33:12 +00:00

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

Closes #323 — 323.

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 48m 52s.

Review findings

Now let me look at the implementation plan, the test file areas, and the README batch-mode section.

Let me verify the batch mode code path to confirm the behavior the test pins, and check the exact lines of the plan's Slice 4 step 6.

Now let me verify the actual code paths — what git calls the real resolveWorkDir/ensureWorktree/setupIssueRun make (needed for the mock hardening), and where run dirs and the friction aggregate actually live.

Before amending ADR-022 §7, let me verify where the friction aggregate actually lives (launch root vs worktree) so the amended wording is accurate.

Let me trace the rest of runSingleIssueMode after the pipeline call to enumerate any other git calls that would hit the mocked runner.

I've confirmed that there are no other git calls via runGitFn on these test paths. Next, I'll check whether there are any cross-references to the ADR §7 heading or other wording I'm about to change.

No cross-references to worry about. Now applying all fixes — starting with the test file (Medium test-comment rewrite + Low mock hardening):

Now the implementation plan (Slice 4 step 6 + Slice 6 "Batch and resume"):

Now the README batch-mode note:

All edits applied. Now let me confirm the gate commands from package.json and run them:

Build and lint pass. Quick targeted check of the touched test file first, then the full suite:

Targeted file passes (117/117). Now the full suite:

All green. Final format check on the changed files:

Let me check whether those warnings are pre-existing or introduced by my edits:

Both files already had prettier warnings as of the committed state — but let me verify my edited regions specifically don't add new formatting issues:

The prettier diff hunks only cover pre-existing issues (table alignment, *italics* style) — none of them touch the regions I edited. Finally, let's do a last review of the full working tree diff:

All changes are in place and all gates pass. The working-tree diff is complete and minimal:

Medium (batch-mode worktree config undocumented): chose the "decide and record" branch of the suggested fix — the code-side branch (routing batch per-issue resolution through resolveWorkDirForIssue) is new feature code beyond a one-pass remediation, so I recorded the pinned limitation everywhere it was mis-stated: plan Slice 4 step 6 + Slice 6, a new Consequence in ADR-022, a note in the README batch-mode section, and a rewritten test comment that no longer mis-cites step 6.
Low (ADR-022 §7): amended to state the registry stays at the launch root while per-run artifacts (and the friction aggregate, verified in runPipeline.ts:489-490 via repoRoot = runContext.repoRoot) live inside the worktree, keeping the chicken-and-egg rationale.
Low (mock silently swallows git calls): makeRepoGitRunner now throws on unrecognized args, with an explicit no-op handler for worktree add (the one previously swallowed call, verified by tracing the real gate path: rev-parse ×2, symbolic-ref, worktree add).

Gates re-run and green: npm run build ✓, npm run lint ✓, npm test ✓ (152 files, 2222/2222). Prettier: the test file and README are clean; the two doc files carry pre-existing warnings (verified present at the committed state via stash), and my edited regions add no new formatting issues.

Review Findings — 323

Summary

All three findings fixed in a single pass. Medium: chose the "decide and record" branch of the suggested fix — batch/scan mode's in-place behavior is the real, working code (runBatchMode/runScanMode → makeBatchGitSeam, never through resolveWorkDirForIssue), so the limitation is now recorded instead of routing batch through the worktree (new code, follow-up): the plan's Slice 4 step 6 and Slice 6 "Batch and resume" are amended to match the as-built behavior, ADR-022 gains a "Batch and scan mode are out of scope" Consequence, the README batch-mode section notes that worktree: has no effect on batch runs, and the test comment at line 2688 now describes the pinned limitation instead of mis-citing step 6. Low (ADR-022 §7): amended — the worktree registry stays at the launch root, while per-run artifacts and the aggregated friction live inside the worktree (verified in code: setupIssueRun(effectiveCwd) → createRunContext and aggregateDir = join(repoRoot, '.pi-loop', 'friction') with repoRoot = runContext.repoRoot), keeping the registry chicken-and-egg rationale. Low (mock): makeRepoGitRunner now throws on unrecognized git args, with an explicit no-op handler for the previously swallowed worktree add call. Gates re-run: build ✓, lint ✓, tests ✓ (152 files, 2222/2222); prettier clean on the edited regions (two doc files carry pre-existing warnings unrelated to these edits).

Critical

(empty)

High

(empty)

Medium

src/cli/services/runCli.test.ts:2688 — The new "batch mode ignores worktree config" test pins that batch/scan runs never apply the worktree section, and its comment cites plan Slice 4 step 6 as the authority ("Regression (step 6)") — but step 6 actually reads the opposite: "confirm the per-issue resolution path routes through the same function, so batch runs inherit the behavior without new code" (and Slice 6 repeats "Batch mode inherits the widened gate"). The code is consistent with the test (runBatchMode → makeBatchGitSeam at src/cli/services/runCli.ts:497,698 indeed never routes through resolveWorkDirForIssue), but the plan's premise was factually wrong and the deviation is recorded nowhere: plan Slice 4 step 6, Slice 6, ADR-022, and the README all stay silent, so an operator who enables worktree: {enabled: true, dir: ...} gets per-issue isolation for a single ref yet batch runs silently mutate the launch checkout in place for every issue. Suggested fix: decide and record — either route batch per-issue resolution through the same worktree function (new code, likely a follow-up issue), or amend docs/implementation-plan-configurable-worktree.md (Slice 4 step 6 + Slice 6 "Batch and resume"), add a Consequence to ADR-022, and note in the README batch-mode section that worktree config does not apply to batch runs; in all cases rewrite the test comment (line 2688–2690) to describe the pinned limitation instead of citing step 6.

Low

docs/adr/022-configurable-worktree-dir.md:143 — §7 states "Run artifacts, the worktree registry and the aggregated friction stay under the repo pi-loop was launched from (originalCwd) for both repo kinds", but the per-run run dir (and therefore issue-context.json, run.log, error.json, *.diff) is created inside the resolved worktree via setupIssueRun(effectiveCwd) → createRunContext(...); the new tests now pin this for both repo kinds (src/cli/services/runCli.test.ts:2531,2566). Pre-existing for bare-repo runs (not introduced by this diff), but the ADR wording now contradicts pinned behavior, and only the registry actually lives at the launch root. Suggested fix: amend §7 to state that the worktree registry stays at the launch root while per-run artifacts live in the worktree (whose tracked .gitignore covers .pi-loop/), keeping the registry chicken-and-egg rationale.
src/cli/services/runCli.test.ts:2467 — makeRepoGitRunner returns '' for any git args it doesn't explicitly handle, so a future git call added to the worktree-resolution path (e.g., in ensureWorktree/resolveDefaultBranch) would be silently swallowed instead of failing the tests. Suggested fix: throw (or record and assert on) unrecognized args in the mock so new git calls surface immediately.

Gates

Gate	Command	Result
Build	`npm run build`	pass
Lint	`npm run lint`	pass
Tests	`npm test`	pass — 2222/2222 (152 files), incl. 8 new worktree-gate tests
Format	`npx prettier --check` on diff files	pass for both changed files (42 pre-existing warnings in unrelated files; parent commit's `runCli.ts` was dirtier than the diff's)

## 323: Widen the early issue-fetch gate for worktree mode Closes #323 — [323](https://git.excelera.net/david/pi-loop/issues/323). > 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 48m 52s. ## Review findings Now let me look at the implementation plan, the test file areas, and the README batch-mode section. Let me verify the batch mode code path to confirm the behavior the test pins, and check the exact lines of the plan's Slice 4 step 6. Now let me verify the actual code paths — what git calls the real `resolveWorkDir`/`ensureWorktree`/`setupIssueRun` make (needed for the mock hardening), and where run dirs and the friction aggregate actually live. Before amending ADR-022 §7, let me verify where the friction aggregate actually lives (launch root vs worktree) so the amended wording is accurate. Let me trace the rest of `runSingleIssueMode` after the pipeline call to enumerate any other git calls that would hit the mocked runner. I've confirmed that there are no other git calls via `runGitFn` on these test paths. Next, I'll check whether there are any cross-references to the ADR §7 heading or other wording I'm about to change. No cross-references to worry about. Now applying all fixes — starting with the test file (Medium test-comment rewrite + Low mock hardening): Now the implementation plan (Slice 4 step 6 + Slice 6 "Batch and resume"): Now the README batch-mode note: All edits applied. Now let me confirm the gate commands from package.json and run them: Build and lint pass. Quick targeted check of the touched test file first, then the full suite: Targeted file passes (117/117). Now the full suite: All green. Final format check on the changed files: Let me check whether those warnings are pre-existing or introduced by my edits: Both files already had prettier warnings as of the committed state — but let me verify my edited regions specifically don't add new formatting issues: The prettier diff hunks only cover pre-existing issues (table alignment, `*italics*` style) — none of them touch the regions I edited. Finally, let's do a last review of the full working tree diff: All changes are in place and all gates pass. The working-tree diff is complete and minimal: 1. **Medium (batch-mode worktree config undocumented):** chose the "decide and record" branch of the suggested fix — the code-side branch (routing batch per-issue resolution through `resolveWorkDirForIssue`) is new feature code beyond a one-pass remediation, so I recorded the pinned limitation everywhere it was mis-stated: plan Slice 4 step 6 + Slice 6, a new Consequence in ADR-022, a note in the README batch-mode section, and a rewritten test comment that no longer mis-cites step 6. 2. **Low (ADR-022 §7):** amended to state the registry stays at the launch root while per-run artifacts (and the friction aggregate, verified in `runPipeline.ts:489-490` via `repoRoot = runContext.repoRoot`) live inside the worktree, keeping the chicken-and-egg rationale. 3. **Low (mock silently swallows git calls):** `makeRepoGitRunner` now throws on unrecognized args, with an explicit no-op handler for `worktree add` (the one previously swallowed call, verified by tracing the real gate path: `rev-parse` ×2, `symbolic-ref`, `worktree add`). Gates re-run and green: `npm run build` ✓, `npm run lint` ✓, `npm test` ✓ (152 files, 2222/2222). Prettier: the test file and README are clean; the two doc files carry pre-existing warnings (verified present at the committed state via stash), and my edited regions add no new formatting issues. # Review Findings — 323 ## Summary All three findings fixed in a single pass. Medium: chose the "decide and record" branch of the suggested fix — batch/scan mode's in-place behavior is the real, working code (`runBatchMode`/`runScanMode` → `makeBatchGitSeam`, never through `resolveWorkDirForIssue`), so the limitation is now recorded instead of routing batch through the worktree (new code, follow-up): the plan's Slice 4 step 6 and Slice 6 "Batch and resume" are amended to match the as-built behavior, ADR-022 gains a "Batch and scan mode are out of scope" Consequence, the README batch-mode section notes that `worktree:` has no effect on batch runs, and the test comment at line 2688 now describes the pinned limitation instead of mis-citing step 6. Low (ADR-022 §7): amended — the worktree registry stays at the launch root, while per-run artifacts and the aggregated friction live inside the worktree (verified in code: `setupIssueRun(effectiveCwd)` → `createRunContext` and `aggregateDir = join(repoRoot, '.pi-loop', 'friction')` with `repoRoot = runContext.repoRoot`), keeping the registry chicken-and-egg rationale. Low (mock): `makeRepoGitRunner` now throws on unrecognized git args, with an explicit no-op handler for the previously swallowed `worktree add` call. Gates re-run: build ✓, lint ✓, tests ✓ (152 files, 2222/2222); prettier clean on the edited regions (two doc files carry pre-existing warnings unrelated to these edits). ## Critical (empty) ## High (empty) ## Medium - [x] `src/cli/services/runCli.test.ts:2688` — The new "batch mode ignores worktree config" test pins that batch/scan runs never apply the worktree section, and its comment cites plan Slice 4 step 6 as the authority ("Regression (step 6)") — but step 6 actually reads the opposite: "confirm the per-issue resolution path routes through the same function, so batch runs inherit the behavior without new code" (and Slice 6 repeats "Batch mode inherits the widened gate"). The code is consistent with the test (`runBatchMode` → `makeBatchGitSeam` at `src/cli/services/runCli.ts:497,698` indeed never routes through `resolveWorkDirForIssue`), but the plan's premise was factually wrong and the deviation is recorded nowhere: plan Slice 4 step 6, Slice 6, ADR-022, and the README all stay silent, so an operator who enables `worktree: {enabled: true, dir: ...}` gets per-issue isolation for a single ref yet batch runs silently mutate the launch checkout in place for every issue. Suggested fix: decide and record — either route batch per-issue resolution through the same worktree function (new code, likely a follow-up issue), or amend `docs/implementation-plan-configurable-worktree.md` (Slice 4 step 6 + Slice 6 "Batch and resume"), add a Consequence to ADR-022, and note in the README batch-mode section that worktree config does not apply to batch runs; in all cases rewrite the test comment (line 2688–2690) to describe the pinned limitation instead of citing step 6. ## Low - [x] `docs/adr/022-configurable-worktree-dir.md:143` — §7 states "Run artifacts, the worktree registry and the aggregated friction stay under **the repo pi-loop was launched from** (`originalCwd`) for both repo kinds", but the per-run run dir (and therefore `issue-context.json`, `run.log`, `error.json`, `*.diff`) is created inside the resolved worktree via `setupIssueRun(effectiveCwd)` → `createRunContext(...)`; the new tests now pin this for both repo kinds (`src/cli/services/runCli.test.ts:2531,2566`). Pre-existing for bare-repo runs (not introduced by this diff), but the ADR wording now contradicts pinned behavior, and only the registry actually lives at the launch root. Suggested fix: amend §7 to state that the worktree registry stays at the launch root while per-run artifacts live in the worktree (whose tracked `.gitignore` covers `.pi-loop/`), keeping the registry chicken-and-egg rationale. - [x] `src/cli/services/runCli.test.ts:2467` — `makeRepoGitRunner` returns `''` for any git args it doesn't explicitly handle, so a future git call added to the worktree-resolution path (e.g., in `ensureWorktree`/`resolveDefaultBranch`) would be silently swallowed instead of failing the tests. Suggested fix: throw (or record and assert on) unrecognized args in the mock so new git calls surface immediately. ## Gates | Gate | Command | Result | | --- | --- | --- | | Build | `npm run build` | pass | | Lint | `npm run lint` | pass | | Tests | `npm test` | pass — 2222/2222 (152 files), incl. 8 new worktree-gate tests | | Format | `npx prettier --check` on diff files | pass for both changed files (42 pre-existing warnings in unrelated files; parent commit's `runCli.ts` was dirtier than the diff's) |

david added 2 commits

2026-11-06 03:33:12 +00:00

323: Widen the early issue-fetch gate for worktree mode 0e3dea6c12