Wire "nothing to ship" skip into runMrStage #255

New issue

david · 2026-08-18T02:13:49Z

david commented

2026-08-18 02:13:49 +00:00

Summary

Make runMrStage (src/mr/services/runMrStage.ts) detect "nothing to ship" and short-circuit into a skipped result — writing mr-skipped.json and never committing, pushing, or creating an MR/PR — instead of always proceeding to MR creation.

Background

Depends on: Add git-native "nothing to ship" helper
Depends on: Add mr-skipped.json artifact type + validator

runMrStage currently always runs its git orchestration (ensureBranchCheckedOut → ensureCommitted → ensurePushed) and then creates an MR/PR via REST, even when the combined implement + remediate + docs changes produced no net diff. ensureCommitted already has a defensive empty-check that skips just the git commit call, but the branch is still pushed and an MR is still opened.

This step generalizes that check into a full stage-level skip, using the hasNothingToShip helper (from the git-helper step) at the right point in the git orchestration — before any push or MR/PR creation happens, so no empty/pointless branch is ever pushed to origin and no empty MR is ever opened.

Per the design record for this feature (see this repo's docs/adr/017-mr-skip-when-no-changes.md and docs/implementation-plan-mr-skip.md for the full rationale): the local branch is not deleted on skip — it stays checked out locally (harmless, since nothing was pushed), which keeps resume simple (a later resume attempt finds the branch already there rather than needing to recreate it).

Implementation Details

src/mr/types/mr.ts — change RunMrStageResult from its current single-shape interface to a discriminated union:
```
export type RunMrStageResult =
  | { skipped: false; resultPath: string; result: MrResult; degraded: boolean }
  | { skipped: true; reason: 'no-changes'; resultPath: string };
```
This is a breaking change to the exported type — every caller/test that currently destructures { resultPath, result, degraded } directly off RunMrStageResult needs updating to narrow on skipped first (see the follow-up "Wire MR-skip outcome into runPipeline" step for the runPipeline caller-side change; this step should also update any existing tests in runMrStage.test.ts that construct/consume the old shape).
src/mr/services/runMrStage.ts:
- In resolveMrResult's git-orchestration call site (currently await runGitCommands({...}) followed unconditionally by createMr), after ensureBranchCheckedOut runs (the branch must exist locally to check hasCommitsOnBranch against a real branch name) and before ensureCommitted/ensurePushed/MR creation, call the hasNothingToShip helper with the resolved sourceBranch and targetBranch.
- If hasNothingToShip returns true:
  - Do not call ensureCommitted, ensurePushed, or createMr/auto-merge at all.
  - Build an MrSkippedArtifact ({ reason: 'no-changes', jiraKey: context.key, checkedAt: now() }) and write it via writeMrSkippedSignal(runDir, artifact).
  - Return { skipped: true, reason: 'no-changes', resultPath } from runMrStage (where resultPath is what writeMrSkippedSignal returned).
- If hasNothingToShip returns false: proceed with the existing flow completely unchanged (ensureCommitted → ensurePushed → createMr → optional auto-merge → writeMrResult), but wrap the final return in { skipped: false, resultPath, result, degraded: false } instead of today's bare { resultPath, result, degraded: false }.
- You'll likely need to restructure runMrStage's top-level function body and/or resolveMrResult's signature slightly to thread the skip check in between ensureBranchCheckedOut and the rest of runGitCommands — consider splitting runGitCommands so the branch-checkout step is separated from commit/push, or inlining the check directly. Keep the change as small and readable as possible; do not restructure unrelated parts of the file.
- Do not add any git branch -D or branch-deletion call on the skip path.

Acceptance Criteria

When the working tree/branch has nothing to ship (per hasNothingToShip), runMrStage never calls commit, push, or the MR-client's createMr.
On skip, runMrStage returns { skipped: true, reason: 'no-changes', resultPath } and mr-skipped.json is written at resultPath with the expected shape (reason, jiraKey, checkedAt).
On skip, the local feature branch is left checked out — no branch-deletion command is issued.
When there IS something to ship, runMrStage behaves exactly as before (commit/push/create MR unchanged) and returns { skipped: false, resultPath, result, degraded: false }.
A case where hasCommitsOnBranch is true (prior stage commits exist from stages 1/3/4) but the working tree is clean does not trigger a skip — there IS something to ship (it's already committed), so the normal push/MR-creation path runs.
All existing runMrStage tests are updated to the new discriminated-union shape and pass; new skip-path tests pass.
npm run build and npm run lint pass with no new errors/warnings.

Test Plan

Extend src/mr/services/runMrStage.test.ts (or wherever the existing suite lives):

Mocked git reporting "nothing to ship" (no commits ahead of target, clean working tree after staging) → assert commit, push, and the MR-client's createMr are never invoked; assert the returned result is { skipped: true, reason: 'no-changes', resultPath }; assert mr-skipped.json was written with the expected fields.
Existing "normal MR creation" test cases updated to assert skipped: false on the result and that result/resultPath/degraded are present and correct (no behavior change otherwise — same commit/push/create-MR calls as before).
A regression case: hasCommitsOnBranch returns true, working tree is clean → confirm the skip does not trigger, and the branch proceeds to push + MR creation as normal.

Run npm test -- runMrStage to confirm the updated/new suite passes, then npm test for the full suite (this is a breaking type change to RunMrStageResult, so expect and fix compile errors in any other file that destructures the old shape).

## Summary Make `runMrStage` (`src/mr/services/runMrStage.ts`) detect "nothing to ship" and short-circuit into a skipped result — writing `mr-skipped.json` and never committing, pushing, or creating an MR/PR — instead of always proceeding to MR creation. ## Background **Depends on:** Add git-native "nothing to ship" helper **Depends on:** Add mr-skipped.json artifact type + validator `runMrStage` currently always runs its git orchestration (`ensureBranchCheckedOut` → `ensureCommitted` → `ensurePushed`) and then creates an MR/PR via REST, even when the combined implement + remediate + docs changes produced no net diff. `ensureCommitted` already has a *defensive* empty-check that skips just the `git commit` call, but the branch is still pushed and an MR is still opened. This step generalizes that check into a full stage-level skip, using the `hasNothingToShip` helper (from the git-helper step) at the right point in the git orchestration — before any push or MR/PR creation happens, so no empty/pointless branch is ever pushed to origin and no empty MR is ever opened. Per the design record for this feature (see this repo's `docs/adr/017-mr-skip-when-no-changes.md` and `docs/implementation-plan-mr-skip.md` for the full rationale): the local branch is **not** deleted on skip — it stays checked out locally (harmless, since nothing was pushed), which keeps resume simple (a later resume attempt finds the branch already there rather than needing to recreate it). ## Implementation Details - `src/mr/types/mr.ts` — change `RunMrStageResult` from its current single-shape interface to a discriminated union: ```typescript export type RunMrStageResult = | { skipped: false; resultPath: string; result: MrResult; degraded: boolean } | { skipped: true; reason: 'no-changes'; resultPath: string }; ``` This is a breaking change to the exported type — every caller/test that currently destructures `{ resultPath, result, degraded }` directly off `RunMrStageResult` needs updating to narrow on `skipped` first (see the follow-up "Wire MR-skip outcome into runPipeline" step for the `runPipeline` caller-side change; this step should also update any existing tests in `runMrStage.test.ts` that construct/consume the old shape). - `src/mr/services/runMrStage.ts`: - In `resolveMrResult`'s git-orchestration call site (currently `await runGitCommands({...})` followed unconditionally by `createMr`), after `ensureBranchCheckedOut` runs (the branch must exist locally to check `hasCommitsOnBranch` against a real branch name) and before `ensureCommitted`/`ensurePushed`/MR creation, call the `hasNothingToShip` helper with the resolved `sourceBranch` and `targetBranch`. - If `hasNothingToShip` returns `true`: - Do not call `ensureCommitted`, `ensurePushed`, or `createMr`/auto-merge at all. - Build an `MrSkippedArtifact` (`{ reason: 'no-changes', jiraKey: context.key, checkedAt: now() }`) and write it via `writeMrSkippedSignal(runDir, artifact)`. - Return `{ skipped: true, reason: 'no-changes', resultPath }` from `runMrStage` (where `resultPath` is what `writeMrSkippedSignal` returned). - If `hasNothingToShip` returns `false`: proceed with the existing flow completely unchanged (`ensureCommitted` → `ensurePushed` → `createMr` → optional auto-merge → `writeMrResult`), but wrap the final return in `{ skipped: false, resultPath, result, degraded: false }` instead of today's bare `{ resultPath, result, degraded: false }`. - You'll likely need to restructure `runMrStage`'s top-level function body and/or `resolveMrResult`'s signature slightly to thread the skip check in between `ensureBranchCheckedOut` and the rest of `runGitCommands` — consider splitting `runGitCommands` so the branch-checkout step is separated from commit/push, or inlining the check directly. Keep the change as small and readable as possible; do not restructure unrelated parts of the file. - Do **not** add any `git branch -D` or branch-deletion call on the skip path. ## Acceptance Criteria - [ ] When the working tree/branch has nothing to ship (per `hasNothingToShip`), `runMrStage` never calls `commit`, `push`, or the MR-client's `createMr`. - [ ] On skip, `runMrStage` returns `{ skipped: true, reason: 'no-changes', resultPath }` and `mr-skipped.json` is written at `resultPath` with the expected shape (`reason`, `jiraKey`, `checkedAt`). - [ ] On skip, the local feature branch is left checked out — no branch-deletion command is issued. - [ ] When there IS something to ship, `runMrStage` behaves exactly as before (commit/push/create MR unchanged) and returns `{ skipped: false, resultPath, result, degraded: false }`. - [ ] A case where `hasCommitsOnBranch` is `true` (prior stage commits exist from stages 1/3/4) but the working tree is clean does **not** trigger a skip — there IS something to ship (it's already committed), so the normal push/MR-creation path runs. - [ ] All existing `runMrStage` tests are updated to the new discriminated-union shape and pass; new skip-path tests pass. - [ ] `npm run build` and `npm run lint` pass with no new errors/warnings. ## Test Plan Extend `src/mr/services/runMrStage.test.ts` (or wherever the existing suite lives): - Mocked git reporting "nothing to ship" (no commits ahead of target, clean working tree after staging) → assert `commit`, `push`, and the MR-client's `createMr` are never invoked; assert the returned result is `{ skipped: true, reason: 'no-changes', resultPath }`; assert `mr-skipped.json` was written with the expected fields. - Existing "normal MR creation" test cases updated to assert `skipped: false` on the result and that `result`/`resultPath`/`degraded` are present and correct (no behavior change otherwise — same commit/push/create-MR calls as before). - A regression case: `hasCommitsOnBranch` returns `true`, working tree is clean → confirm the skip does **not** trigger, and the branch proceeds to push + MR creation as normal. Run `npm test -- runMrStage` to confirm the updated/new suite passes, then `npm test` for the full suite (this is a breaking type change to `RunMrStageResult`, so expect and fix compile errors in any other file that destructures the old shape).

david added the

enhancement

label

2026-08-18 02:13:49 +00:00

david referenced this issue

2026-08-18 03:53:27 +00:00

255: Wire "nothing to ship" skip into runMrStage #262

david closed this issue

2026-08-18 03:53:30 +00:00

david commented

2026-08-18 03:53:30 +00:00

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