228: Friction: wire remediate stage — parse friction first, strip-before-findings, artifact write #243

david · 2026-08-17T02:24:17Z

david commented

2026-08-17 02:24:17 +00:00

228: Friction: wire remediate stage — parse friction first, strip-before-findings, artifact write

Implements 228.

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 17m 33s.

Review findings

Now let me look at the logging implementation and the review stage for context:I have a full picture now. Let me implement the fixes. Starting with the shared persistFriction helper (Medium finding #1):Now export it from the friction barrel:Now update runImplementStage.ts to use the shared helper:Now update runRemediateStage.ts to use the shared helper and drop the local duplicate:Now update runPipeline.ts for the High and Low findings:Now let me add a unit test for the shared persistFriction helper:Now add the pipeline wiring and friction-strip regression tests to runPipeline.test.ts:Now let me run the build to check for type errors:Build passes. Now lint:Lint passes. Now run the targeted test files:All targeted tests pass. Now run the full test suite:All 1899 tests pass. Re-run lint and build to confirm everything is still green after the test edits:All gates green. Let me verify the complete diff of my remediation changes:Let me check whether any other tests assert the raw annotatedFindings flows to the MR stage:No conflicting assertions. Let me make one small doc clarification on the remediate result type so the raw-vs-MR-facing distinction is explicit:Final gate run:All gates green: build (tsc) PASS, lint (eslint) PASS, test (vitest) PASS — 1899 tests / 136 files.

Remediation summary

I resolved all four findings in a single pass:

High — friction leaking into the MR description: runPipeline now hands stripFrictionSection(remediateResult.annotatedFindings) to runMr, so the ## Friction section (and its diagnostics like "the build failed because the lockfile was stale") never reaches the Draft MR body. Added a regression test in runPipeline.test.ts that builds the description exactly as the MR stage does and asserts no ## Friction section and no diagnostic text.
Medium — duplicated persistFriction: extracted the shared stamp-and-write helper to src/friction/services/persistFriction.ts, exported it via the friction barrel, and switched both runImplementStage and runRemediateStage to call it (the remediate stage's local copy was deleted). Added focused unit tests (persistFriction.test.ts: provenance stamping, empty artifact, unknown model, wellFormed passthrough, non-fatal write failure).
Medium — missing pipeline wiring assertion: added a runPipeline test asserting runRemediate receives runId and a stage-scoped logger (a warning written through it carries the [HH:mm remediate] prefix), plus the friction-strip regression above.
Low — skipped-stage defaults report empty friction: defaultSkippedImplementResult / defaultSkippedRemediateResult now read friction.json from runDir via a readSkippedFriction helper with an empty fallback on read/validation error; the stale NOTE comments were removed.

No findings remain unresolved.

# Review Findings — 228

## Summary

The change wires ADR-016 friction into the remediate stage: the `## Friction` section is parsed first and stripped before findings parsing/validation, `friction.json` + `FRICTION.md` are persisted with provenance stamps, and the wrapper/skill contract is updated. All gates ran and passed: build (tsc) PASS, lint (eslint) PASS, test (vitest) PASS (1899 tests / 136 files). All four findings were resolved: the MR-facing annotated findings are now friction-stripped in `runPipeline` (High), the `persistFriction` stamp-and-write logic is extracted into a shared `src/friction/services/persistFriction.ts` used by both implement and remediate (Medium), dedicated `runPipeline` wiring tests assert `runId`/stage-scoped logger and the friction-free MR handoff (Medium), and the skipped-stage resume defaults now read `friction.json` from disk with an empty fallback (Low).

## Critical

## High

- [x] src/orchestrator/services/runPipeline.ts:607 — The remediate agent's raw final message (which now ends with the `## Friction` section) is forwarded as `annotatedFindings` to the MR stage, and `src/mr/helpers/buildMrDescription.ts:72` embeds it verbatim in the Draft MR body under "## Review findings". This contradicts ADR-016 §7 ("no MR-description section") and IMPLEMENTATION_PLAN.md Post-M9 Validation ("friction entries never appear in ... the MR"), so diagnostic content (e.g. "the build failed because the lockfile was stale", "guessed the wrong command") now appears in the human-facing MR. Suggested fix: pass the stripped findings text to the MR description — e.g. return `stripFrictionSection(annotatedFindings)` from `runRemediateStage` (src/remediate/services/runRemediateStage.ts:200) as the MR-facing field, or call `stripFrictionSection(remediateResult.annotatedFindings)` in `runPipeline` before handing it to `runMr` — and add a regression test in `buildMrDescription.test.ts`/`runPipeline.test.ts` asserting the description contains no `## Friction` section.

## Medium

- [x] src/remediate/services/runRemediateStage.ts:217 — The new local `persistFriction` helper (stamp `stage`/`runId`/`model ?? 'unknown'`/`recordedAt`, `writeFriction` + `writeFrictionMd`, catch → `logWarn`) duplicates the inline block in `src/implement/services/runImplementStage.ts:118-160` almost line-for-line. Per AGENTS.md ("Keep new deterministic logic in small, individually testable modules behind clear seams"), this shared deterministic logic should live in one place. Suggested fix: extract a shared `persistFrictionArtifact({ runDir, runId, stage, model, entries, wellFormed, logWarn })` helper into `src/friction/` (e.g. `src/friction/services/persistFriction.ts`, exported via the friction barrel) and call it from both `runImplementStage` and `runRemediateStage`.

- [x] src/orchestrator/services/runPipeline.test.ts:174 — The pipeline wiring added in `runPipeline.ts:509-510` (`runId: runContext.runId`, `logger: logger.forStage('remediate')` passed into `runRemediate`) has no dedicated assertion: existing `runRemediate` fakes were only updated to satisfy the new required `friction` field, and no test verifies the stage receives the run id/logger (nor that the MR description excludes the friction section — see the High finding). Suggested fix: add a `runPipeline` test that asserts `runRemediate` is called with `runId` and a stage-scoped logger, and that the `annotatedFindings` handed to `runMr` is stripped of the friction section.

## Low

- [x] src/orchestrator/services/runPipeline.ts:163 — `defaultSkippedRemediateResult` (and `defaultSkippedImplementResult` at line 134) return `friction: { entries: [], wellFormed: true }` without reading the on-disk `friction.json` from the earlier attempt, so a resume that skips a stage would report empty friction even when the original attempt recorded entries. The code NOTE acknowledges this as a latent divergence, and nothing consumes the field today, but the ADR-016 §6–7 aggregate/run-summary consumer must read `friction.json` from disk in these defaults rather than trust the in-memory result. Suggested fix: when wiring the aggregate consumer, have the skipped-stage defaults read `friction.json` from `runDir` (with an empty fallback on read/validation error).

## 228: Friction: wire remediate stage — parse friction first, strip-before-findings, artifact write Implements [228](https://git.excelera.net/david/pi-loop/issues/228). > 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 17m 33s. ## Review findings Now let me look at the logging implementation and the review stage for context:I have a full picture now. Let me implement the fixes. Starting with the shared `persistFriction` helper (Medium finding #1):Now export it from the friction barrel:Now update `runImplementStage.ts` to use the shared helper:Now update `runRemediateStage.ts` to use the shared helper and drop the local duplicate:Now update `runPipeline.ts` for the High and Low findings:Now let me add a unit test for the shared `persistFriction` helper:Now add the pipeline wiring and friction-strip regression tests to `runPipeline.test.ts`:Now let me run the build to check for type errors:Build passes. Now lint:Lint passes. Now run the targeted test files:All targeted tests pass. Now run the full test suite:All 1899 tests pass. Re-run lint and build to confirm everything is still green after the test edits:All gates green. Let me verify the complete diff of my remediation changes:Let me check whether any other tests assert the raw annotatedFindings flows to the MR stage:No conflicting assertions. Let me make one small doc clarification on the remediate result type so the raw-vs-MR-facing distinction is explicit:Final gate run:All gates green: **build (tsc) PASS, lint (eslint) PASS, test (vitest) PASS — 1899 tests / 136 files**. ## Remediation summary I resolved all four findings in a single pass: - **High — friction leaking into the MR description:** `runPipeline` now hands `stripFrictionSection(remediateResult.annotatedFindings)` to `runMr`, so the `## Friction` section (and its diagnostics like "the build failed because the lockfile was stale") never reaches the Draft MR body. Added a regression test in `runPipeline.test.ts` that builds the description exactly as the MR stage does and asserts no `## Friction` section and no diagnostic text. - **Medium — duplicated `persistFriction`:** extracted the shared stamp-and-write helper to `src/friction/services/persistFriction.ts`, exported it via the friction barrel, and switched both `runImplementStage` and `runRemediateStage` to call it (the remediate stage's local copy was deleted). Added focused unit tests (`persistFriction.test.ts`: provenance stamping, empty artifact, `unknown` model, `wellFormed` passthrough, non-fatal write failure). - **Medium — missing pipeline wiring assertion:** added a `runPipeline` test asserting `runRemediate` receives `runId` and a stage-scoped logger (a warning written through it carries the `[HH:mm remediate]` prefix), plus the friction-strip regression above. - **Low — skipped-stage defaults report empty friction:** `defaultSkippedImplementResult` / `defaultSkippedRemediateResult` now read `friction.json` from `runDir` via a `readSkippedFriction` helper with an empty fallback on read/validation error; the stale NOTE comments were removed. No findings remain unresolved. ```markdown # Review Findings — 228 ## Summary The change wires ADR-016 friction into the remediate stage: the `## Friction` section is parsed first and stripped before findings parsing/validation, `friction.json` + `FRICTION.md` are persisted with provenance stamps, and the wrapper/skill contract is updated. All gates ran and passed: build (tsc) PASS, lint (eslint) PASS, test (vitest) PASS (1899 tests / 136 files). All four findings were resolved: the MR-facing annotated findings are now friction-stripped in `runPipeline` (High), the `persistFriction` stamp-and-write logic is extracted into a shared `src/friction/services/persistFriction.ts` used by both implement and remediate (Medium), dedicated `runPipeline` wiring tests assert `runId`/stage-scoped logger and the friction-free MR handoff (Medium), and the skipped-stage resume defaults now read `friction.json` from disk with an empty fallback (Low). ## Critical ## High - [x] src/orchestrator/services/runPipeline.ts:607 — The remediate agent's raw final message (which now ends with the `## Friction` section) is forwarded as `annotatedFindings` to the MR stage, and `src/mr/helpers/buildMrDescription.ts:72` embeds it verbatim in the Draft MR body under "## Review findings". This contradicts ADR-016 §7 ("no MR-description section") and IMPLEMENTATION_PLAN.md Post-M9 Validation ("friction entries never appear in ... the MR"), so diagnostic content (e.g. "the build failed because the lockfile was stale", "guessed the wrong command") now appears in the human-facing MR. Suggested fix: pass the stripped findings text to the MR description — e.g. return `stripFrictionSection(annotatedFindings)` from `runRemediateStage` (src/remediate/services/runRemediateStage.ts:200) as the MR-facing field, or call `stripFrictionSection(remediateResult.annotatedFindings)` in `runPipeline` before handing it to `runMr` — and add a regression test in `buildMrDescription.test.ts`/`runPipeline.test.ts` asserting the description contains no `## Friction` section. ## Medium - [x] src/remediate/services/runRemediateStage.ts:217 — The new local `persistFriction` helper (stamp `stage`/`runId`/`model ?? 'unknown'`/`recordedAt`, `writeFriction` + `writeFrictionMd`, catch → `logWarn`) duplicates the inline block in `src/implement/services/runImplementStage.ts:118-160` almost line-for-line. Per AGENTS.md ("Keep new deterministic logic in small, individually testable modules behind clear seams"), this shared deterministic logic should live in one place. Suggested fix: extract a shared `persistFrictionArtifact({ runDir, runId, stage, model, entries, wellFormed, logWarn })` helper into `src/friction/` (e.g. `src/friction/services/persistFriction.ts`, exported via the friction barrel) and call it from both `runImplementStage` and `runRemediateStage`. - [x] src/orchestrator/services/runPipeline.test.ts:174 — The pipeline wiring added in `runPipeline.ts:509-510` (`runId: runContext.runId`, `logger: logger.forStage('remediate')` passed into `runRemediate`) has no dedicated assertion: existing `runRemediate` fakes were only updated to satisfy the new required `friction` field, and no test verifies the stage receives the run id/logger (nor that the MR description excludes the friction section — see the High finding). Suggested fix: add a `runPipeline` test that asserts `runRemediate` is called with `runId` and a stage-scoped logger, and that the `annotatedFindings` handed to `runMr` is stripped of the friction section. ## Low - [x] src/orchestrator/services/runPipeline.ts:163 — `defaultSkippedRemediateResult` (and `defaultSkippedImplementResult` at line 134) return `friction: { entries: [], wellFormed: true }` without reading the on-disk `friction.json` from the earlier attempt, so a resume that skips a stage would report empty friction even when the original attempt recorded entries. The code NOTE acknowledges this as a latent divergence, and nothing consumes the field today, but the ADR-016 §6–7 aggregate/run-summary consumer must read `friction.json` from disk in these defaults rather than trust the in-memory result. Suggested fix: when wiring the aggregate consumer, have the skipped-stage defaults read `friction.json` from `runDir` (with an empty fallback on read/validation error). ```

david added 2 commits

2026-08-17 02:24:18 +00:00

228: Friction: wire remediate stage — parse friction first, strip-before-findings, artifact write c727a31fa1