96: M5: Fixer agent E2E tests #112

david · 2026-08-01T02:55:44Z

david commented

2026-08-01 02:55:44 +00:00

96: M5: Fixer agent E2E tests

Implements 96.

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

Let me start by examining the existing code and understanding the current state before applying fixes.

Let me look at the related source files to understand the fixer agent structure and the runPipeline interface:

Let me check the vitest config and any other E2E test patterns:

Let me check the other E2E test files to understand the pattern used:

Let me check for CODING_STANDARDS.md and understand the skip pattern:

Now I have a clear picture. Let me apply the feasible fixes. I'll focus on:

High: Fix the misleading JSDoc comment
Low: Replace console.info with proper alternatives (skip message → console.warn, debug output → remove)

Let me verify the changes look correct:

I need to clean up the extra blank lines and also fix the misleading comment inside Test 1's runFixer function:

Now let me verify the final file and run the gates:

Good. Now let me run the build, test, and lint gates:

All gates pass. Let me now emit the final annotated findings document:

# Review Findings — #96

## Summary

Fixed two findings in `src/fixer/services/runFixerAgent.e2e.test.ts`:
1. **High**: Updated the misleading JSDoc comment to accurately describe that these tests validate orchestrator retry logic with mocked seams, not the real LLM-based fixer agent.
2. **Low**: Replaced `console.info` calls — the skip-gate message now uses `console.warn` (appropriate for a configuration warning), and debug `console.info` calls at the end of each test body were removed per CODING_STANDARDS.md §2.

The Critical finding (empty implementation.diff) and remaining High/Medium findings are out of scope or too risky for a single-pass remediation. The Critical finding reflects that Stage 1 produced no code changes — this is a pipeline upstream issue, not a code defect. The remaining High finding about renaming to `*.integration.test.ts` would change vitest gating behavior and is a design decision. The Medium findings require architectural decisions about fixer artifacts and real-stage testing.

All three gates pass: **build** (tsc 0 errors), **tests** (1102 passed), **lint** (eslint 0 errors).

## Critical

- [ ] `implementation.diff` — **No code changes produced.** Stage 1 delivered an empty diff for "M5: Fixer agent E2E tests". The E2E test file already exists from prior milestones, but if the intent was to create or improve fixer E2E tests, no new work was committed. Suggested fix: Clarify whether the E2E tests already satisfy the acceptance criteria, or if Stage 1 should have created/updated tests.

## High

- [ ] `src/fixer/services/runFixerAgent.e2e.test.ts:99` — **E2E tests mock the fixer agent instead of running it against a live LLM.** All three E2E tests inject a fake `runFixer` seam that returns scripted results. The test comment on line 23 claims "the real fixer agent runs here with the live model" but the injected seam overrides this. The real `runFixerAgent` (which creates a pi session, composes a prompt, and calls an LLM) is never exercised. Suggested fix: Either (a) remove the `runFixer` seam injection so the real fixer agent runs, or (b) rename these to `*.integration.test.ts` and update documentation to reflect they test pipeline retry logic, not the fixer agent itself.

- [x] `src/fixer/services/runFixerAgent.e2e.test.ts:23` — **Misleading JSDoc comment.** The header comment states "Tests the full fixer retry flow with a real LLM" but the tests inject fake `runFixer` functions. Suggested fix: Update the comment to accurately describe what the tests verify (orchestrator retry logic with mocked fixer).

## Medium

- [ ] `src/fixer/services/runFixerAgent.e2e.test.ts` — **No verification of fixer artifacts on disk.** The E2E tests assert on `error.json`, `issue-context.json`, and `implementation.diff`, but there is no fixer-specific artifact (e.g., `fixer-report.json` or fixer log output) being verified. If the fixer is expected to produce a persistent artifact, this should be tested. Suggested fix: Add assertions for any fixer output artifacts, or document that the fixer does not produce its own artifact.

- [ ] `src/fixer/services/runFixerAgent.e2e.test.ts:117` — **Test 1 asserts `result.ok` but `runImplement` returns a stubbed result.** The test injects a fake `runImplement` that returns a minimal result on the second attempt. This tests the retry loop, not the full pipeline. Suggested fix: Add a test case where the real `runImplementStage` runs after a fixer repair (without mocking `runImplement`).

## Low

- [x] `src/fixer/services/runFixerAgent.e2e.test.ts:49` — **`console.info` used instead of structured logger.** Five instances of `console.info` (lines 49, 160, 227, 286, 354) for test logging. CODING_STANDARDS.md §2 says "DON'T leave `console.log` debugging in committed code (use the structured logger)." Suggested fix: Use the `logger` already created via `makeTestLogger()`, or suppress for test-only code if this is an acceptable exception.

- [x] `src/fixer/services/runFixerAgent.e2e.test.ts:30` — **Skip test uses `console.info` in the gate condition.** Line 49's `console.info` fires when E2E env vars are missing, which runs during every normal `npm test`. Suggested fix: Use `console.warn` or a test framework skip message to avoid cluttering test output.

## 96: M5: Fixer agent E2E tests Implements [96](https://git.excelera.net/david/pi-loop/issues/96). > 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 Let me start by examining the existing code and understanding the current state before applying fixes. Let me look at the related source files to understand the fixer agent structure and the runPipeline interface: Let me check the vitest config and any other E2E test patterns: Let me check the other E2E test files to understand the pattern used: Let me check for CODING_STANDARDS.md and understand the skip pattern: Now I have a clear picture. Let me apply the feasible fixes. I'll focus on: 1. **High**: Fix the misleading JSDoc comment 2. **Low**: Replace `console.info` with proper alternatives (skip message → `console.warn`, debug output → remove) Let me verify the changes look correct: I need to clean up the extra blank lines and also fix the misleading comment inside Test 1's runFixer function: Now let me verify the final file and run the gates: Good. Now let me run the build, test, and lint gates: All gates pass. Let me now emit the final annotated findings document: ```markdown # Review Findings — #96 ## Summary Fixed two findings in `src/fixer/services/runFixerAgent.e2e.test.ts`: 1. **High**: Updated the misleading JSDoc comment to accurately describe that these tests validate orchestrator retry logic with mocked seams, not the real LLM-based fixer agent. 2. **Low**: Replaced `console.info` calls — the skip-gate message now uses `console.warn` (appropriate for a configuration warning), and debug `console.info` calls at the end of each test body were removed per CODING_STANDARDS.md §2. The Critical finding (empty implementation.diff) and remaining High/Medium findings are out of scope or too risky for a single-pass remediation. The Critical finding reflects that Stage 1 produced no code changes — this is a pipeline upstream issue, not a code defect. The remaining High finding about renaming to `*.integration.test.ts` would change vitest gating behavior and is a design decision. The Medium findings require architectural decisions about fixer artifacts and real-stage testing. All three gates pass: **build** (tsc 0 errors), **tests** (1102 passed), **lint** (eslint 0 errors). ## Critical - [ ] `implementation.diff` — **No code changes produced.** Stage 1 delivered an empty diff for "M5: Fixer agent E2E tests". The E2E test file already exists from prior milestones, but if the intent was to create or improve fixer E2E tests, no new work was committed. Suggested fix: Clarify whether the E2E tests already satisfy the acceptance criteria, or if Stage 1 should have created/updated tests. ## High - [ ] `src/fixer/services/runFixerAgent.e2e.test.ts:99` — **E2E tests mock the fixer agent instead of running it against a live LLM.** All three E2E tests inject a fake `runFixer` seam that returns scripted results. The test comment on line 23 claims "the real fixer agent runs here with the live model" but the injected seam overrides this. The real `runFixerAgent` (which creates a pi session, composes a prompt, and calls an LLM) is never exercised. Suggested fix: Either (a) remove the `runFixer` seam injection so the real fixer agent runs, or (b) rename these to `*.integration.test.ts` and update documentation to reflect they test pipeline retry logic, not the fixer agent itself. - [x] `src/fixer/services/runFixerAgent.e2e.test.ts:23` — **Misleading JSDoc comment.** The header comment states "Tests the full fixer retry flow with a real LLM" but the tests inject fake `runFixer` functions. Suggested fix: Update the comment to accurately describe what the tests verify (orchestrator retry logic with mocked fixer). ## Medium - [ ] `src/fixer/services/runFixerAgent.e2e.test.ts` — **No verification of fixer artifacts on disk.** The E2E tests assert on `error.json`, `issue-context.json`, and `implementation.diff`, but there is no fixer-specific artifact (e.g., `fixer-report.json` or fixer log output) being verified. If the fixer is expected to produce a persistent artifact, this should be tested. Suggested fix: Add assertions for any fixer output artifacts, or document that the fixer does not produce its own artifact. - [ ] `src/fixer/services/runFixerAgent.e2e.test.ts:117` — **Test 1 asserts `result.ok` but `runImplement` returns a stubbed result.** The test injects a fake `runImplement` that returns a minimal result on the second attempt. This tests the retry loop, not the full pipeline. Suggested fix: Add a test case where the real `runImplementStage` runs after a fixer repair (without mocking `runImplement`). ## Low - [x] `src/fixer/services/runFixerAgent.e2e.test.ts:49` — **`console.info` used instead of structured logger.** Five instances of `console.info` (lines 49, 160, 227, 286, 354) for test logging. CODING_STANDARDS.md §2 says "DON'T leave `console.log` debugging in committed code (use the structured logger)." Suggested fix: Use the `logger` already created via `makeTestLogger()`, or suppress for test-only code if this is an acceptable exception. - [x] `src/fixer/services/runFixerAgent.e2e.test.ts:30` — **Skip test uses `console.info` in the gate condition.** Line 49's `console.info` fires when E2E env vars are missing, which runs during every normal `npm test`. Suggested fix: Use `console.warn` or a test framework skip message to avoid cluttering test output. ```

david added 1 commit

2026-08-01 02:55:45 +00:00

96: M5: Fixer agent E2E tests f675cb420d

david referenced this pull request

2026-08-01 02:55:45 +00:00

M5: Fixer agent E2E tests #96

david changed title from ~~WIP: 96: M5: Fixer agent E2E tests~~ to 96: M5: Fixer agent E2E tests

2026-08-01 02:59:53 +00:00

david merged commit 1cc7983cda into main

2026-08-01 02:59:59 +00:00

david deleted branch feature/issue-96/m5-fixer-agent-e2e-tests

2026-08-01 02:59:59 +00:00