256: Wire MR-skip outcome into runPipeline #263

david · 2026-08-18T04:22:24Z

david commented

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

256: Wire MR-skip outcome into runPipeline

Implements 256.

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 28m 50s.

Review findings

Now let's add a test for the defensive throw branch (Low finding #2).Now I'll add a test for the defensive throw path (mrOutcome.skipped true but mr-skipped.json not written).Now let's check whether runPipeline actually throws synchronously (unhandled) or catches errors elsewhere - need to verify this test's expectation matches actual behavior.The whole body is wrapped in try/catch that converts thrown errors into ok: false results — it doesn't propagate. Let me fix the test.So the thrown error gets classified as agent-failure for stage mr and returned as ok: false. Let me fix the test accordingly.Now let's run tests, build, and lint.All 80 tests pass, including the new one. Now let's run full build, test, and lint.Build passes cleanly.All 141 files, 1970 tests pass.All gates pass: build, 1970 tests, and lint are clean. Both Low findings resolved.Both findings addressed. Here is the annotated findings document.

Review Findings — 256

Summary

Fixed both Low findings: replaced the readMrSkippedSignal(runDir) ?? (() => { throw ... })() IIFE with a plain if (artifact === undefined) { throw ... } guard for readability, matching the style used elsewhere in the function; and added a unit test (runPipeline.test.ts) that exercises the defensive throw path by simulating a runMr fake that returns skipped: true without writing mr-skipped.json, asserting the run surfaces an agent-failure for stage mr with the expected message. Build (npm run build), full test suite (npm test, 141 files / 1970 tests), and lint (npm run lint) all pass cleanly after the changes.

Critical

High

Medium

Low

src/orchestrator/services/runPipeline.ts:740-746 — The readMrSkippedSignal(runDir) ?? (() => { throw ... })() IIFE-in-?? pattern is a somewhat unusual/harder-to-scan way to assert non-null compared to a plain if (artifact === undefined) { throw ... } guard used elsewhere in the same function (e.g. the persistedMrResult path a few lines below just calls readMrResult directly and lets it throw naturally). Suggested fix: replace with a straightforward if (artifact === undefined) { throw new Error(...); } for readability, or add a one-line comment noting the branch is defensive/unreachable in practice (since runMrStage always writes the artifact before returning skipped: true).
src/orchestrator/services/runPipeline.ts:740-746 — This defensive "could not be read back" throw path has no unit test exercising it (understandably, since it requires simulating an inconsistent fake runMr that returns skipped: true without actually writing mr-skipped.json). Suggested fix: optionally add a small test asserting the thrown-error message/behavior for completeness, or note in a comment that it's intentionally untested defensive code.

## 256: Wire MR-skip outcome into runPipeline Implements [256](https://git.excelera.net/david/pi-loop/issues/256). > 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 28m 50s. ## Review findings Now let's add a test for the defensive throw branch (Low finding #2).Now I'll add a test for the defensive throw path (mrOutcome.skipped true but mr-skipped.json not written).Now let's check whether `runPipeline` actually throws synchronously (unhandled) or catches errors elsewhere - need to verify this test's expectation matches actual behavior.The whole body is wrapped in try/catch that converts thrown errors into `ok: false` results — it doesn't propagate. Let me fix the test.So the thrown error gets classified as `agent-failure` for stage `mr` and returned as `ok: false`. Let me fix the test accordingly.Now let's run tests, build, and lint.All 80 tests pass, including the new one. Now let's run full build, test, and lint.Build passes cleanly.All 141 files, 1970 tests pass.All gates pass: build, 1970 tests, and lint are clean. Both Low findings resolved.Both findings addressed. Here is the annotated findings document. # Review Findings — 256 ## Summary Fixed both Low findings: replaced the `readMrSkippedSignal(runDir) ?? (() => { throw ... })()` IIFE with a plain `if (artifact === undefined) { throw ... }` guard for readability, matching the style used elsewhere in the function; and added a unit test (`runPipeline.test.ts`) that exercises the defensive throw path by simulating a `runMr` fake that returns `skipped: true` without writing `mr-skipped.json`, asserting the run surfaces an `agent-failure` for stage `mr` with the expected message. Build (`npm run build`), full test suite (`npm test`, 141 files / 1970 tests), and lint (`npm run lint`) all pass cleanly after the changes. ## Critical ## High ## Medium ## Low - [x] src/orchestrator/services/runPipeline.ts:740-746 — The `readMrSkippedSignal(runDir) ?? (() => { throw ... })()` IIFE-in-`??` pattern is a somewhat unusual/harder-to-scan way to assert non-null compared to a plain `if (artifact === undefined) { throw ... }` guard used elsewhere in the same function (e.g. the `persistedMrResult` path a few lines below just calls `readMrResult` directly and lets it throw naturally). Suggested fix: replace with a straightforward `if (artifact === undefined) { throw new Error(...); }` for readability, or add a one-line comment noting the branch is defensive/unreachable in practice (since `runMrStage` always writes the artifact before returning `skipped: true`). - [x] src/orchestrator/services/runPipeline.ts:740-746 — This defensive "could not be read back" throw path has no unit test exercising it (understandably, since it requires simulating an inconsistent fake `runMr` that returns `skipped: true` without actually writing `mr-skipped.json`). Suggested fix: optionally add a small test asserting the thrown-error message/behavior for completeness, or note in a comment that it's intentionally untested defensive code.

david added 2 commits

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

256: Wire MR-skip outcome into runPipeline 07054be02d