105: [Commit-model M6] Remediate stage commit (D1 + D5) #119

david · 2026-08-01T06:01:22Z

david commented

2026-08-01 06:01:22 +00:00

105: [Commit-model M6] Remediate stage commit (D1 + D5)

Implements 105.

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 reading the relevant files to understand the codebase before applying fixes.

Let me also check the captureDiff function to understand how baseRef is used:

Now I have a complete picture. Let me implement the fixes, working highest severity first.

Fix 1 (Critical): Persist `baseRef` as a JSON artifact

Now update the implement stage to persist baseRef:

Now update runPipeline.ts to read the persisted baseRef when implement is skipped:

Now update the call site:

Now let me check the pipeline tests for any references to defaultSkippedImplementResult:

I need to update the pipeline tests to write base-ref.json when testing resume scenarios that skip the implement stage:

Now let me add a test for the baseRef artifact persistence in the implement stage:

Fix 2 (High): Make `baseRef` required in `RunRemediateStageInput`

Now update the remediate tests to always pass baseRef:

I need to add baseRef to all test calls. Let me update the remediate tests:

Fix 3 (Medium): Fix fragile parameter typing in `commitRemediateChanges`

Now I need to update the ordering test to properly track all git operations:

Now let me run the build and tests to verify all fixes work:

One test failed. Let me fix it - the test that resumes from write-back also needs base-ref.json:

All tests pass. Now let me run lint:

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

# Review Findings — 105

## Summary

All four gates pass after remediation: **build ✅**, **tests ✅** (99 files, 1185 tests), **lint ✅**, **format ✅**. Fixed the critical resume-path gap by persisting `baseRef` as a JSON artifact, made `baseRef` required in the remediate input type, fixed fragile parameter typing, and extended test assertions to cover the full commit sequence.

## Critical

- [x] `src/orchestrator/services/runPipeline.ts:109` — `defaultSkippedImplementResult()` returns `baseRef: 'unknown'` when the implement stage is skipped during resume. When remediate then runs, it passes this to `captureDiff(runGit, repoRoot, 'unknown')`, which executes `git diff unknown -- .` and fails with a fatal git error. The `baseRef` captured by the implement stage is never persisted to disk as an artifact, so it is lost on process crash + resume from review or later. Suggested fix: persist `baseRef` as a JSON artifact (e.g., `base-ref.json`) in the implement stage and read it in `defaultSkippedImplementResult()` or `runRemediateStage` when `baseRef` is not provided. **FIXED: Created `src/artifacts/services/baseRefArtifact.ts` with `writeBaseRef`/`readBaseRef`. Implement stage now calls `writeBaseRef(runDir, baseRef)`. `defaultSkippedImplementResult(runDir)` reads the persisted artifact. Pipeline tests updated to write `base-ref.json` when simulating resume scenarios.**

## High

- [x] `src/remediate/services/runRemediateStage.ts:82` — In the short-circuit path (no unchecked findings), `captureDiff(runGit, repoRoot, baseRef)` is called with `baseRef` that may be `undefined` (it is optional in the input type). When `baseRef` is `undefined`, `captureDiff` falls back to `git diff -- .` which only captures staged changes, not the cumulative implement + remediate diff. This produces an incorrect `final.diff` when the implement stage already committed its changes. Suggested fix: either make `baseRef` required in `RunRemediateStageInput` or recapture it from the implement artifact when missing. **FIXED: Changed `baseRef?: string` to `baseRef: string` in `RunRemediateStageInput`. Updated all 17 remediate stage tests to always pass `baseRef`.**

## Medium

- [x] `src/remediate/services/runRemediateStage.test.ts:290` — The "captures the diff after the agent turn" test verifies the ordering `['stage', 'diff']` but does not assert that the commit step (`add`, `commit`) occurs after the diff. With the new commit behavior, the expected ordering should be `['stage', 'diff', 'status', 'add', 'commit']` (or at minimum verify commit follows diff). Suggested fix: extend the `order` tracking in the test's `runGit` handler to include `add` and `commit` operations and assert the full sequence. **FIXED: Updated the test's `runGit` handler to track all git operations via `order.push(args[0])`. Assertion now verifies `['stage', 'diff', 'status', 'add', 'commit']`.**

- [ ] `src/remediate/services/runRemediateStage.test.ts:344` — The new commit tests use `makeFakeGitWithChanges` which returns empty strings for all commands except `status` and `diff`. The `add` and `commit` commands return `''` which is technically valid (git returns empty stdout on success), but this means the fake silently swallows any git errors that might occur during add/commit. Suggested fix: add a dedicated test helper that can be configured to fail on specific commands, to verify error propagation for both `add` and `commit` failure paths. **LEFT UNRESOLVED: The existing "surfaces actionable error when commit fails" test already verifies error propagation for commit failures. The `add` failure path is less likely to fail independently (it's a local filesystem operation), and adding a fully configurable fake git helper is out of scope for this single-pass remediation.**

- [x] `src/remediate/services/runRemediateStage.ts:144` — The `commitRemediateChanges` function uses `Parameters<typeof hasStagedOrUncommittedChanges>[N]` for parameter types, which creates a fragile coupling to the signature of that function. If `hasStagedOrUncommittedChanges` gains or reorders parameters, this silently breaks. Suggested fix: use explicit types (`runGit: GitRunner, cwd: string`) for clarity and resilience. **FIXED: Replaced `Parameters<typeof hasStagedOrUncommittedChanges>[0]` with `GitRunner` and `[1]` with `string`.**

## Low

- [ ] `src/remediate/services/runRemediateStage.test.ts:7` — The import `import { GitError, type GitRunner } from '../../shared/git/index.js';` mixes a value import (`GitError`) with a type-only import (`type GitRunner`). Per CODING_STANDARDS.md §2 (grouped imports), consider separating these into two lines for clarity: `import { GitError } from ...` and `import type { GitRunner } from ...`. **LEFT UNRESOLVED: The mixed import style `import { value, type Type } from '...'` is valid TypeScript and follows the "grouped imports" convention. The CODING_STANDARDS.md does not mandate separating value and type imports. This is a minor style preference, not a violation.**

## 105: [Commit-model M6] Remediate stage commit (D1 + D5) Implements [105](https://git.excelera.net/david/pi-loop/issues/105). > 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 reading the relevant files to understand the codebase before applying fixes. Let me also check the captureDiff function to understand how baseRef is used: Now I have a complete picture. Let me implement the fixes, working highest severity first. ## Fix 1 (Critical): Persist `baseRef` as a JSON artifact Now update the implement stage to persist `baseRef`: Now update `runPipeline.ts` to read the persisted `baseRef` when implement is skipped: Now update the call site: Now let me check the pipeline tests for any references to `defaultSkippedImplementResult`: I need to update the pipeline tests to write `base-ref.json` when testing resume scenarios that skip the implement stage: Now let me add a test for the baseRef artifact persistence in the implement stage: ## Fix 2 (High): Make `baseRef` required in `RunRemediateStageInput` Now update the remediate tests to always pass `baseRef`: I need to add `baseRef` to all test calls. Let me update the remediate tests: ## Fix 3 (Medium): Fix fragile parameter typing in `commitRemediateChanges` Now I need to update the ordering test to properly track all git operations: Now let me run the build and tests to verify all fixes work: One test failed. Let me fix it - the test that resumes from write-back also needs `base-ref.json`: All tests pass. Now let me run lint: All gates pass. Let me now emit the final annotated findings document: ```markdown # Review Findings — 105 ## Summary All four gates pass after remediation: **build ✅**, **tests ✅** (99 files, 1185 tests), **lint ✅**, **format ✅**. Fixed the critical resume-path gap by persisting `baseRef` as a JSON artifact, made `baseRef` required in the remediate input type, fixed fragile parameter typing, and extended test assertions to cover the full commit sequence. ## Critical - [x] `src/orchestrator/services/runPipeline.ts:109` — `defaultSkippedImplementResult()` returns `baseRef: 'unknown'` when the implement stage is skipped during resume. When remediate then runs, it passes this to `captureDiff(runGit, repoRoot, 'unknown')`, which executes `git diff unknown -- .` and fails with a fatal git error. The `baseRef` captured by the implement stage is never persisted to disk as an artifact, so it is lost on process crash + resume from review or later. Suggested fix: persist `baseRef` as a JSON artifact (e.g., `base-ref.json`) in the implement stage and read it in `defaultSkippedImplementResult()` or `runRemediateStage` when `baseRef` is not provided. **FIXED: Created `src/artifacts/services/baseRefArtifact.ts` with `writeBaseRef`/`readBaseRef`. Implement stage now calls `writeBaseRef(runDir, baseRef)`. `defaultSkippedImplementResult(runDir)` reads the persisted artifact. Pipeline tests updated to write `base-ref.json` when simulating resume scenarios.** ## High - [x] `src/remediate/services/runRemediateStage.ts:82` — In the short-circuit path (no unchecked findings), `captureDiff(runGit, repoRoot, baseRef)` is called with `baseRef` that may be `undefined` (it is optional in the input type). When `baseRef` is `undefined`, `captureDiff` falls back to `git diff -- .` which only captures staged changes, not the cumulative implement + remediate diff. This produces an incorrect `final.diff` when the implement stage already committed its changes. Suggested fix: either make `baseRef` required in `RunRemediateStageInput` or recapture it from the implement artifact when missing. **FIXED: Changed `baseRef?: string` to `baseRef: string` in `RunRemediateStageInput`. Updated all 17 remediate stage tests to always pass `baseRef`.** ## Medium - [x] `src/remediate/services/runRemediateStage.test.ts:290` — The "captures the diff after the agent turn" test verifies the ordering `['stage', 'diff']` but does not assert that the commit step (`add`, `commit`) occurs after the diff. With the new commit behavior, the expected ordering should be `['stage', 'diff', 'status', 'add', 'commit']` (or at minimum verify commit follows diff). Suggested fix: extend the `order` tracking in the test's `runGit` handler to include `add` and `commit` operations and assert the full sequence. **FIXED: Updated the test's `runGit` handler to track all git operations via `order.push(args[0])`. Assertion now verifies `['stage', 'diff', 'status', 'add', 'commit']`.** - [ ] `src/remediate/services/runRemediateStage.test.ts:344` — The new commit tests use `makeFakeGitWithChanges` which returns empty strings for all commands except `status` and `diff`. The `add` and `commit` commands return `''` which is technically valid (git returns empty stdout on success), but this means the fake silently swallows any git errors that might occur during add/commit. Suggested fix: add a dedicated test helper that can be configured to fail on specific commands, to verify error propagation for both `add` and `commit` failure paths. **LEFT UNRESOLVED: The existing "surfaces actionable error when commit fails" test already verifies error propagation for commit failures. The `add` failure path is less likely to fail independently (it's a local filesystem operation), and adding a fully configurable fake git helper is out of scope for this single-pass remediation.** - [x] `src/remediate/services/runRemediateStage.ts:144` — The `commitRemediateChanges` function uses `Parameters<typeof hasStagedOrUncommittedChanges>[N]` for parameter types, which creates a fragile coupling to the signature of that function. If `hasStagedOrUncommittedChanges` gains or reorders parameters, this silently breaks. Suggested fix: use explicit types (`runGit: GitRunner, cwd: string`) for clarity and resilience. **FIXED: Replaced `Parameters<typeof hasStagedOrUncommittedChanges>[0]` with `GitRunner` and `[1]` with `string`.** ## Low - [ ] `src/remediate/services/runRemediateStage.test.ts:7` — The import `import { GitError, type GitRunner } from '../../shared/git/index.js';` mixes a value import (`GitError`) with a type-only import (`type GitRunner`). Per CODING_STANDARDS.md §2 (grouped imports), consider separating these into two lines for clarity: `import { GitError } from ...` and `import type { GitRunner } from ...`. **LEFT UNRESOLVED: The mixed import style `import { value, type Type } from '...'` is valid TypeScript and follows the "grouped imports" convention. The CODING_STANDARDS.md does not mandate separating value and type imports. This is a minor style preference, not a violation.** ```

david added 1 commit

2026-08-01 06:01:23 +00:00

105: [Commit-model M6] Remediate stage commit (D1 + D5) 8a5d3013e9

david merged commit 3abadc2260 into main

2026-08-01 06:01:24 +00:00

david referenced this pull request from a commit

2026-08-01 06:01:24 +00:00

Merge pull request '105: [Commit-model M6] Remediate stage commit (D1 + D5)' (#119) from feature/issue-105/commit-model-m6-remediate-stage-commit-d1-d5 into main

david referenced this pull request

2026-08-01 06:01:24 +00:00

[Commit-model M6] Remediate stage commit (D1 + D5) #105

Sign in to join this conversation.