Wire MR-skip outcome into runPipeline #256

New issue

david · 2026-08-18T02:14:50Z

david commented

2026-08-18 02:14:50 +00:00

Summary

Make runPipeline (src/orchestrator/services/runPipeline.ts) branch on the new RunMrStageResult.skipped discriminant: skip write-back entirely and return a dedicated "nothing to ship" summary when the MR stage skipped, leaving the normal path unchanged otherwise.

Background

Depends on: Wire "nothing to ship" skip into runMrStage

runMrStage now returns a discriminated union ({ skipped: true, reason: 'no-changes', resultPath } or { skipped: false, resultPath, result, degraded }) instead of a single shape. runPipeline currently calls runMr and immediately assumes the old single shape — it destructures mrResult.result unconditionally, then always runs closeIssueAfterAutoMerge, computeWritebackOutcome, and runWritebackNonFatal.

This step teaches runPipeline to check mrResult.skipped and take a different path when it's true: no write-back (there's no MR URL to comment with — mirrors exactly how the existing noCodeChange upfront-skip path in this same file already skips write-back for the same reason), and a dedicated summary via a new buildMrSkippedSummary helper (modeled directly on the existing buildNoCodeChangeSummary function already in this file).

There is also a resume fallthrough path to handle: runPipeline has a shouldSkipStage('mr', resumeFromStage) branch (used when resuming a run that already completed its MR stage in a prior attempt) that currently reads mr-result.json via readMrResult(runDir). This must be updated to also recognize mr-skipped.json — read it first via readMrSkippedSignal, falling back to readMrResult — so a resumed-and-already-skipped run reports the dedicated summary correctly instead of throwing when mr-result.json doesn't exist on disk.

Implementation Details

src/orchestrator/helpers/buildFinalSummary.ts (or a new adjacent helper file, whichever matches where buildNoCodeChangeSummary-equivalent logic best fits — buildNoCodeChangeSummary itself currently lives directly in runPipeline.ts, so placing the new helper alongside it in the same file is also acceptable and keeps the change localized) — add:

function buildMrSkippedSummary(
  jiraKey: string,
  artifact: MrSkippedArtifact,
  frictionAggregate: FrictionAggregate | undefined,
): string {
  const lines: string[] = [];
  lines.push(`pi-loop completed for ${jiraKey}`);
  lines.push('No changes to ship — implement/remediate/docs produced no net diff');
  lines.push(formatFrictionSummaryLine(frictionAggregate));
  return lines.join('\n');
}

src/orchestrator/services/runPipeline.ts — in the MR stage section (the currentStage = 'mr' block that currently does const mrResult = await runWithFixerRetry('mr', () => runMr(...)) then unconditionally proceeds to closeIssueAfterAutoMerge/computeWritebackOutcome/runWritebackNonFatal/buildFinalSummary):
- After runMr resolves, branch on mrResult.skipped.
- If true: push 'mr' and 'write-back' onto skippedStages; skip closeIssueAfterAutoMerge, computeWritebackOutcome, and runWritebackNonFatal entirely; recompute the friction aggregate the same way the normal path does (recomputeFrictionAggregate()); build the summary via buildMrSkippedSummary(context.key, artifact, frictionAggregate) (read the artifact via readMrSkippedSignal(runDir) since mrResult in the skip branch only carries resultPath, not the parsed artifact — or thread the artifact through from runMrStage if that's cleaner; either is fine as long as the summary gets a real MrSkippedArtifact); call emitPipelineFinish() as the normal path does; return the pipeline's success result shape with the mr-skipped variant (see the type change below).
- If false: existing behavior, completely unchanged — narrow to mrResult.result / mrResult.resultPath exactly as today's code already does (this is now just an explicit type-narrow rather than an implicit assumption).
- Also update the resume "MR was skipped during resume" fallthrough (the shouldSkipStage('mr', resumeFromStage) branch): change it to try readMrSkippedSignal(runDir) first; if that returns a value, build and return the mr-skipped summary/result the same way as the fresh-run skip path; otherwise fall back to the existing readMrResult(runDir)-based behavior.
src/orchestrator/types/orchestrator.ts — add a new success-variant to whatever RunPipelineResult-shaped return type this file/module defines (search for where the existing noCodeChange variant is defined, since this should be modeled the same way — e.g. a discriminated union member carrying mrSkipped: MrSkippedArtifact for downstream batch/manifest consumption, added in "Add mr-skipped outcome to batch mode" below).

Acceptance Criteria

When runMr returns { skipped: true, ... }, runPipeline never calls closeIssueAfterAutoMerge, computeWritebackOutcome, or runWritebackNonFatal.
skippedStages includes both 'mr' and 'write-back' on the skip path.
The returned summary matches buildMrSkippedSummary's output shape (three lines: completion line, "no changes to ship" line, friction line).
The pipeline result carries a distinguishable mr-skipped variant (e.g. an mrSkipped: MrSkippedArtifact field or discriminant) that a caller (batch mode) can detect.
The resume fallthrough path (shouldSkipStage('mr', ...) branch) checks mr-skipped.json first via readMrSkippedSignal, and only falls back to readMrResult if that returns undefined.
Existing "normal MR" pipeline tests are updated to assert against the narrowed skipped: false shape and continue to pass with no behavior change.
npm run build and npm run lint pass with no new errors/warnings.

Test Plan

Extend src/orchestrator/services/runPipeline.test.ts (or the equivalent existing suite):

Mocked runMr returning { skipped: true, reason: 'no-changes', resultPath } → verify write-back functions are never invoked, skippedStages contains 'mr' and 'write-back', the summary text matches buildMrSkippedSummary's shape, and the result carries the mr-skipped variant.
Resume path: a run dir where mr-skipped.json exists on disk and resumeFromStage causes the MR stage to be skipped-on-resume → verify the fallthrough reads mr-skipped.json (not mr-result.json) and produces the same dedicated summary as the fresh-run skip path.
Existing "normal MR" tests updated to assert against skipped: false and otherwise pass unchanged.

Run npm test -- runPipeline to confirm the updated/new suite passes, then npm test for the full suite to confirm no regressions.

## Summary Make `runPipeline` (`src/orchestrator/services/runPipeline.ts`) branch on the new `RunMrStageResult.skipped` discriminant: skip write-back entirely and return a dedicated "nothing to ship" summary when the MR stage skipped, leaving the normal path unchanged otherwise. ## Background **Depends on:** Wire "nothing to ship" skip into runMrStage `runMrStage` now returns a discriminated union (`{ skipped: true, reason: 'no-changes', resultPath }` or `{ skipped: false, resultPath, result, degraded }`) instead of a single shape. `runPipeline` currently calls `runMr` and immediately assumes the old single shape — it destructures `mrResult.result` unconditionally, then always runs `closeIssueAfterAutoMerge`, `computeWritebackOutcome`, and `runWritebackNonFatal`. This step teaches `runPipeline` to check `mrResult.skipped` and take a different path when it's `true`: no write-back (there's no MR URL to comment with — mirrors exactly how the existing `noCodeChange` upfront-skip path in this same file already skips write-back for the same reason), and a dedicated summary via a new `buildMrSkippedSummary` helper (modeled directly on the existing `buildNoCodeChangeSummary` function already in this file). There is also a **resume fallthrough path** to handle: `runPipeline` has a `shouldSkipStage('mr', resumeFromStage)` branch (used when resuming a run that already completed its MR stage in a prior attempt) that currently reads `mr-result.json` via `readMrResult(runDir)`. This must be updated to also recognize `mr-skipped.json` — read it first via `readMrSkippedSignal`, falling back to `readMrResult` — so a resumed-and-already-skipped run reports the dedicated summary correctly instead of throwing when `mr-result.json` doesn't exist on disk. ## Implementation Details - `src/orchestrator/helpers/buildFinalSummary.ts` (or a new adjacent helper file, whichever matches where `buildNoCodeChangeSummary`-equivalent logic best fits — `buildNoCodeChangeSummary` itself currently lives directly in `runPipeline.ts`, so placing the new helper alongside it in the same file is also acceptable and keeps the change localized) — add: ```typescript function buildMrSkippedSummary( jiraKey: string, artifact: MrSkippedArtifact, frictionAggregate: FrictionAggregate | undefined, ): string { const lines: string[] = []; lines.push(`pi-loop completed for ${jiraKey}`); lines.push('No changes to ship — implement/remediate/docs produced no net diff'); lines.push(formatFrictionSummaryLine(frictionAggregate)); return lines.join('\n'); } ``` - `src/orchestrator/services/runPipeline.ts` — in the MR stage section (the `currentStage = 'mr'` block that currently does `const mrResult = await runWithFixerRetry('mr', () => runMr(...))` then unconditionally proceeds to `closeIssueAfterAutoMerge`/`computeWritebackOutcome`/`runWritebackNonFatal`/`buildFinalSummary`): - After `runMr` resolves, branch on `mrResult.skipped`. - If `true`: push `'mr'` and `'write-back'` onto `skippedStages`; skip `closeIssueAfterAutoMerge`, `computeWritebackOutcome`, and `runWritebackNonFatal` entirely; recompute the friction aggregate the same way the normal path does (`recomputeFrictionAggregate()`); build the summary via `buildMrSkippedSummary(context.key, artifact, frictionAggregate)` (read the artifact via `readMrSkippedSignal(runDir)` since `mrResult` in the skip branch only carries `resultPath`, not the parsed artifact — or thread the artifact through from `runMrStage` if that's cleaner; either is fine as long as the summary gets a real `MrSkippedArtifact`); call `emitPipelineFinish()` as the normal path does; return the pipeline's success result shape with the mr-skipped variant (see the type change below). - If `false`: existing behavior, completely unchanged — narrow to `mrResult.result` / `mrResult.resultPath` exactly as today's code already does (this is now just an explicit type-narrow rather than an implicit assumption). - Also update the **resume "MR was skipped during resume" fallthrough** (the `shouldSkipStage('mr', resumeFromStage)` branch): change it to try `readMrSkippedSignal(runDir)` first; if that returns a value, build and return the mr-skipped summary/result the same way as the fresh-run skip path; otherwise fall back to the existing `readMrResult(runDir)`-based behavior. - `src/orchestrator/types/orchestrator.ts` — add a new success-variant to whatever `RunPipelineResult`-shaped return type this file/module defines (search for where the existing `noCodeChange` variant is defined, since this should be modeled the same way — e.g. a discriminated union member carrying `mrSkipped: MrSkippedArtifact` for downstream batch/manifest consumption, added in "Add mr-skipped outcome to batch mode" below). ## Acceptance Criteria - [ ] When `runMr` returns `{ skipped: true, ... }`, `runPipeline` never calls `closeIssueAfterAutoMerge`, `computeWritebackOutcome`, or `runWritebackNonFatal`. - [ ] `skippedStages` includes both `'mr'` and `'write-back'` on the skip path. - [ ] The returned summary matches `buildMrSkippedSummary`'s output shape (three lines: completion line, "no changes to ship" line, friction line). - [ ] The pipeline result carries a distinguishable mr-skipped variant (e.g. an `mrSkipped: MrSkippedArtifact` field or discriminant) that a caller (batch mode) can detect. - [ ] The resume fallthrough path (`shouldSkipStage('mr', ...)` branch) checks `mr-skipped.json` first via `readMrSkippedSignal`, and only falls back to `readMrResult` if that returns `undefined`. - [ ] Existing "normal MR" pipeline tests are updated to assert against the narrowed `skipped: false` shape and continue to pass with no behavior change. - [ ] `npm run build` and `npm run lint` pass with no new errors/warnings. ## Test Plan Extend `src/orchestrator/services/runPipeline.test.ts` (or the equivalent existing suite): - Mocked `runMr` returning `{ skipped: true, reason: 'no-changes', resultPath }` → verify write-back functions are never invoked, `skippedStages` contains `'mr'` and `'write-back'`, the summary text matches `buildMrSkippedSummary`'s shape, and the result carries the mr-skipped variant. - Resume path: a run dir where `mr-skipped.json` exists on disk and `resumeFromStage` causes the MR stage to be skipped-on-resume → verify the fallthrough reads `mr-skipped.json` (not `mr-result.json`) and produces the same dedicated summary as the fresh-run skip path. - Existing "normal MR" tests updated to assert against `skipped: false` and otherwise pass unchanged. Run `npm test -- runPipeline` to confirm the updated/new suite passes, then `npm test` for the full suite to confirm no regressions.

david added the

enhancement

label

2026-08-18 02:14:50 +00:00

david referenced this issue

2026-08-18 04:22:24 +00:00

256: Wire MR-skip outcome into runPipeline #263

david closed this issue

2026-08-18 04:22:27 +00:00

david commented

2026-08-18 04:22:27 +00:00

pi-loop opened and merged a pull request for this issue: #263