94: M3: runPipeline fixer integration #110

david · 2026-08-01T02:29:24Z

david commented

2026-08-01 02:29:24 +00:00

94: M3: runPipeline fixer integration

Implements 94.

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

Review findings

I'll start by examining the relevant files to understand the code structure before applying fixes.

Now I have all the context I need. Let me implement the fixes, starting with the highest severity.

Fix 1: Circular dependency (High)

Create src/shared/pipeline-types.ts with ErrorStage and update all imports.

Now update src/fixer/types/fixer.ts to import from shared (this breaks the cycle):

Now update src/orchestrator/types/orchestrator.ts to import from shared and re-export:

Fix 2: Error report `attempts` field inaccurate (Medium)

I need to export annotateAttempts from the retry module first:

Export annotateAttempts from the retry module:

Now update runPipeline.ts to import annotateAttempts and annotate the error:

Fix 3 & 4: Test fixes (Medium)

Let me read the test file to add the resume + fixer test and fix the as never assertions:

Fix the as never assertions by properly typing the mocks:

Now add the resume + fixer test. Let me find the end of the test file:

Now add a test for the attempts field accuracy:

Now let me run the build and tests to verify everything works:

I need to both import and re-export ErrorStage:

Build passed. Now run tests:

All tests passed. Now run lint:

Lint passed. Now run format to address the Low finding:

Format ran. Let me verify the changes are clean:

All gates pass. Let me verify the lint also passes after format:

All three gates pass (build ✓, lint ✓, tests ✓ with 1095 tests). Here is the annotated findings document:

# Review Findings — #94

## Summary

Fixed 5 of 6 findings. The circular dependency was resolved by extracting `ErrorStage` to a new shared module. The `attempts` field now correctly reflects total fixer retries. Added test coverage for resume + fixer interaction and properly typed mock functions. Ran `npm run format` for formatting. The `unreachable` dead code finding was left unresolved as the review itself noted "current code is fine, just a nit for future consistency."

All gates pass: **build** ✓, **lint** ✓, **tests** ✓ (1095 tests across 97 files, including 2 new tests).

## Critical

_(none)_

## High

- [x] `src/fixer/types/fixer.ts:7` — Circular module dependency: `fixer` imports `ErrorStage` from `orchestrator`, while `orchestrator/types/orchestrator.ts` imports `RunFixerAgentInput`/`RunFixerAgentResult` from `fixer`. This violates the vertical-slice module boundary principle (CODING_STANDARDS.md §4: "DON'T create cross-module circular dependencies"). Suggested fix: Extract `ErrorStage` to `src/shared/types/` (or `src/shared/orchestrator-types/`) so both modules import from a common shared barrel, breaking the cycle.

## Medium

- [x] `src/orchestrator/services/runPipeline.ts:253` — Error report `attempts` field is inaccurate after fixer retries. When the fixer retry loop exhausts all 3 attempts and re-throws, `classifyError` reports `attempts: 1` (the default from `getAttempts`) instead of the actual total (4 = 1 initial + 3 fixer retries). Suggested fix: Annotate the re-thrown error with the total attempt count using `annotateAttempts(error, attempt + 1)` before re-throwing on the last attempt (line 262).

- [x] `src/orchestrator/services/runPipeline.test.ts` — No test coverage for resume + fixer retry interaction. There is no test verifying what happens when resuming from a fixer-eligible stage (e.g., `resumeFromStage: 'implement'`) and that stage throws a GitError — does the fixer retry loop still activate? Suggested fix: Add a test case combining `resumeFromStage: 'implement'` with a GitError-throwing `runImplement` mock and asserting that `runFixer` is called.

- [x] `src/orchestrator/services/runPipeline.test.ts:966` — `runFixer as never` type assertion used to bypass type checking. The mock `vi.fn()` doesn't match the `(input: RunFixerAgentInput) => Promise<RunFixerAgentResult>` signature, requiring `as never`. While this pattern exists elsewhere in the file, it masks type mismatches. Suggested fix: Type the mock properly: `const runFixer = vi.fn(async (): Promise<RunFixerAgentResult> => ({ text: '', toolCalls: [], degraded: false }));`.

## Low

- [x] `src/fixer/index.ts:5` — Formatting-only changes mixed with functional changes. The diff consolidates multi-line export/import blocks into single lines (`export { FIXER_TOOLS, DEFAULT_FIXER_SKILL_BODY }` instead of the previous multi-line format). Suggested fix: Keep formatting changes in a separate commit or run `npm run format` before committing functional changes.

- [ ] `src/orchestrator/services/runPipeline.ts:280` — Dead code: `throw new Error('unreachable')`. While logically unreachable (the loop always returns or throws), TypeScript requires this for exhaustiveness. Suggested fix: Replace with `throw new Error('unreachable')` with a `// eslint-disable-next-line @typescript-eslint/unbound-method` comment, or use a custom `unreachable()` helper from `shared/` for consistency. _(Minor — current code is fine, just a nit for future consistency.)_

## 94: M3: runPipeline fixer integration Implements [94](https://git.excelera.net/david/pi-loop/issues/94). > This Merge Request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. ## Review findings I'll start by examining the relevant files to understand the code structure before applying fixes. Now I have all the context I need. Let me implement the fixes, starting with the highest severity. ## Fix 1: Circular dependency (High) Create `src/shared/pipeline-types.ts` with `ErrorStage` and update all imports. Now update `src/fixer/types/fixer.ts` to import from shared (this breaks the cycle): Now update `src/orchestrator/types/orchestrator.ts` to import from shared and re-export: ### Fix 2: Error report `attempts` field inaccurate (Medium) I need to export `annotateAttempts` from the retry module first: Export `annotateAttempts` from the retry module: Now update `runPipeline.ts` to import `annotateAttempts` and annotate the error: ### Fix 3 & 4: Test fixes (Medium) Let me read the test file to add the resume + fixer test and fix the `as never` assertions: Fix the `as never` assertions by properly typing the mocks: Now add the resume + fixer test. Let me find the end of the test file: Now add a test for the attempts field accuracy: Now let me run the build and tests to verify everything works: I need to both import and re-export `ErrorStage`: Build passed. Now run tests: All tests passed. Now run lint: Lint passed. Now run format to address the Low finding: Format ran. Let me verify the changes are clean: All gates pass. Let me verify the lint also passes after format: All three gates pass (build ✓, lint ✓, tests ✓ with 1095 tests). Here is the annotated findings document: ```markdown # Review Findings — #94 ## Summary Fixed 5 of 6 findings. The circular dependency was resolved by extracting `ErrorStage` to a new shared module. The `attempts` field now correctly reflects total fixer retries. Added test coverage for resume + fixer interaction and properly typed mock functions. Ran `npm run format` for formatting. The `unreachable` dead code finding was left unresolved as the review itself noted "current code is fine, just a nit for future consistency." All gates pass: **build** ✓, **lint** ✓, **tests** ✓ (1095 tests across 97 files, including 2 new tests). ## Critical _(none)_ ## High - [x] `src/fixer/types/fixer.ts:7` — Circular module dependency: `fixer` imports `ErrorStage` from `orchestrator`, while `orchestrator/types/orchestrator.ts` imports `RunFixerAgentInput`/`RunFixerAgentResult` from `fixer`. This violates the vertical-slice module boundary principle (CODING_STANDARDS.md §4: "DON'T create cross-module circular dependencies"). Suggested fix: Extract `ErrorStage` to `src/shared/types/` (or `src/shared/orchestrator-types/`) so both modules import from a common shared barrel, breaking the cycle. ## Medium - [x] `src/orchestrator/services/runPipeline.ts:253` — Error report `attempts` field is inaccurate after fixer retries. When the fixer retry loop exhausts all 3 attempts and re-throws, `classifyError` reports `attempts: 1` (the default from `getAttempts`) instead of the actual total (4 = 1 initial + 3 fixer retries). Suggested fix: Annotate the re-thrown error with the total attempt count using `annotateAttempts(error, attempt + 1)` before re-throwing on the last attempt (line 262). - [x] `src/orchestrator/services/runPipeline.test.ts` — No test coverage for resume + fixer retry interaction. There is no test verifying what happens when resuming from a fixer-eligible stage (e.g., `resumeFromStage: 'implement'`) and that stage throws a GitError — does the fixer retry loop still activate? Suggested fix: Add a test case combining `resumeFromStage: 'implement'` with a GitError-throwing `runImplement` mock and asserting that `runFixer` is called. - [x] `src/orchestrator/services/runPipeline.test.ts:966` — `runFixer as never` type assertion used to bypass type checking. The mock `vi.fn()` doesn't match the `(input: RunFixerAgentInput) => Promise<RunFixerAgentResult>` signature, requiring `as never`. While this pattern exists elsewhere in the file, it masks type mismatches. Suggested fix: Type the mock properly: `const runFixer = vi.fn(async (): Promise<RunFixerAgentResult> => ({ text: '', toolCalls: [], degraded: false }));`. ## Low - [x] `src/fixer/index.ts:5` — Formatting-only changes mixed with functional changes. The diff consolidates multi-line export/import blocks into single lines (`export { FIXER_TOOLS, DEFAULT_FIXER_SKILL_BODY }` instead of the previous multi-line format). Suggested fix: Keep formatting changes in a separate commit or run `npm run format` before committing functional changes. - [ ] `src/orchestrator/services/runPipeline.ts:280` — Dead code: `throw new Error('unreachable')`. While logically unreachable (the loop always returns or throws), TypeScript requires this for exhaustiveness. Suggested fix: Replace with `throw new Error('unreachable')` with a `// eslint-disable-next-line @typescript-eslint/unbound-method` comment, or use a custom `unreachable()` helper from `shared/` for consistency. _(Minor — current code is fine, just a nit for future consistency.)_ ```

david added 1 commit

2026-08-01 02:29:25 +00:00

94: M3: runPipeline fixer integration 25a391246c

david referenced this pull request

2026-08-01 02:29:25 +00:00

M3: runPipeline fixer integration #94

david changed title from ~~WIP: 94: M3: runPipeline fixer integration~~ to 94: M3: runPipeline fixer integration

2026-08-01 02:34:30 +00:00

david merged commit 2393b5f114 into main

2026-08-01 02:34:37 +00:00

david deleted branch feature/issue-94/m3-runpipeline-fixer-integration

2026-08-01 02:34:37 +00:00