103: [Commit-model M4] Implement stage commit (D1 + D5) #117

david · 2026-08-01T05:40:05Z

david commented

2026-08-01 05:40:05 +00:00

103: [Commit-model M4] Implement stage commit (D1 + D5)

Implements 103.

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

Now I'll apply the three fixes. Let me start with all edits:

Now let me add a test for the improved commit error message:

Now let me run the gates:

Let me check the test file to verify what's there:

I see the issue — the test's runGit throws on ANY unhandled command, not just commit. Commands like diff, symbolic-ref, fetch, checkout all get hit before commit. Let me fix the test:

All gates pass. Let me verify the final state of changes:

All fixes applied and verified. Here's the annotated findings document:

# Review Findings — 103

## Summary

All three gates passed after remediation: **build** (tsc compile to dist/), **tests** (99 files, 1174 tests, all passed), and **lint** (eslint clean). Fixed all three findings: added JSDoc documenting the `stageAndCommit` failure mode (Medium 1), added a test with realistic diff content (Medium 2), and documented the `makeFakeGitWithChanges` status limitation (Low). Added a new test for actionable commit error messaging.

## Critical

_(none)_

## High

_(none)_

## Medium

- [x] `src/implement/services/runImplementStage.ts:99` — If `stageAndCommit()` succeeds on `git add` but fails on `git commit` (e.g., no git user configured), the working tree is left with staged-but-uncommitted changes and the run aborts. A subsequent resume would re-run Implement from scratch, potentially losing the agent's work. Suggested fix: document this failure mode in the stage JSDoc, or consider catching the commit error and surfacing a more actionable `GitError` message (e.g., "Configure `user.name`/`user.email` in git config"). **Fixed: Added both a JSDoc NOTE documenting the failure mode and a try/catch that wraps `GitError` with an actionable message mentioning `user.name`/`user.email`.**

- [x] `src/implement/services/runImplementStage.test.ts:214` — The test `'stages and commits with Pi-Loop-Stage: 1 trailer'` uses `makeFakeGitWithChanges('')` (empty diff string), meaning it verifies staging/committing happens even when the agent's diff is empty. The empty-diff hard failure (D5) is checked separately, but this test does not verify that a non-empty diff is also staged/committed. Suggested fix: add a test case using `makeFakeGitWithChanges('diff --git a/...')` to confirm the staging path exercises with realistic diff content. **Fixed: Added test `'stages and commits with realistic diff content'` using a multi-line `diff --git` string.**

## Low

- [x] `src/implement/services/runImplementStage.test.ts:33` — `makeFakeGitWithChanges` hardcodes the status output to `' M src/foo.ts\n'` (a modified tracked file). In real usage, the agent could produce untracked files (`??`) or staged changes (`M `). Suggested fix: document the limitation in the JSDoc or expand the fake to handle additional status patterns for broader coverage. **Fixed: Added NOTE to the JSDoc documenting the hardcoded status limitation and recommending a custom GitRunner for tests needing other patterns.**

## 103: [Commit-model M4] Implement stage commit (D1 + D5) Implements [103](https://git.excelera.net/david/pi-loop/issues/103). > 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 Now I'll apply the three fixes. Let me start with all edits: Now let me add a test for the improved commit error message: Now let me run the gates: Let me check the test file to verify what's there: I see the issue — the test's `runGit` throws on ANY unhandled command, not just `commit`. Commands like `diff`, `symbolic-ref`, `fetch`, `checkout` all get hit before `commit`. Let me fix the test: All gates pass. Let me verify the final state of changes: All fixes applied and verified. Here's the annotated findings document: ```markdown # Review Findings — 103 ## Summary All three gates passed after remediation: **build** (tsc compile to dist/), **tests** (99 files, 1174 tests, all passed), and **lint** (eslint clean). Fixed all three findings: added JSDoc documenting the `stageAndCommit` failure mode (Medium 1), added a test with realistic diff content (Medium 2), and documented the `makeFakeGitWithChanges` status limitation (Low). Added a new test for actionable commit error messaging. ## Critical _(none)_ ## High _(none)_ ## Medium - [x] `src/implement/services/runImplementStage.ts:99` — If `stageAndCommit()` succeeds on `git add` but fails on `git commit` (e.g., no git user configured), the working tree is left with staged-but-uncommitted changes and the run aborts. A subsequent resume would re-run Implement from scratch, potentially losing the agent's work. Suggested fix: document this failure mode in the stage JSDoc, or consider catching the commit error and surfacing a more actionable `GitError` message (e.g., "Configure `user.name`/`user.email` in git config"). **Fixed: Added both a JSDoc NOTE documenting the failure mode and a try/catch that wraps `GitError` with an actionable message mentioning `user.name`/`user.email`.** - [x] `src/implement/services/runImplementStage.test.ts:214` — The test `'stages and commits with Pi-Loop-Stage: 1 trailer'` uses `makeFakeGitWithChanges('')` (empty diff string), meaning it verifies staging/committing happens even when the agent's diff is empty. The empty-diff hard failure (D5) is checked separately, but this test does not verify that a non-empty diff is also staged/committed. Suggested fix: add a test case using `makeFakeGitWithChanges('diff --git a/...')` to confirm the staging path exercises with realistic diff content. **Fixed: Added test `'stages and commits with realistic diff content'` using a multi-line `diff --git` string.** ## Low - [x] `src/implement/services/runImplementStage.test.ts:33` — `makeFakeGitWithChanges` hardcodes the status output to `' M src/foo.ts\n'` (a modified tracked file). In real usage, the agent could produce untracked files (`??`) or staged changes (`M `). Suggested fix: document the limitation in the JSDoc or expand the fake to handle additional status patterns for broader coverage. **Fixed: Added NOTE to the JSDoc documenting the hardcoded status limitation and recommending a custom GitRunner for tests needing other patterns.** ```

david added 1 commit

2026-08-01 05:40:06 +00:00

103: [Commit-model M4] Implement stage commit (D1 + D5) c5976b3822

david merged commit aa7e82d4bb into main

2026-08-01 05:40:07 +00:00

david referenced this pull request

2026-08-01 05:40:08 +00:00

[Commit-model M4] Implement stage commit (D1 + D5) #103

david referenced this pull request from a commit

2026-08-01 05:40:08 +00:00

Merge pull request '103: [Commit-model M4] Implement stage commit (D1 + D5)' (#117) from feature/issue-103/commit-model-m4-implement-stage-commit-d1-d5 into main

Sign in to join this conversation.