issue-306: Post the comment at runPipeline's no-code-change short-circuit #316

david · 2026-09-05T19:44:57Z

david commented

2026-09-05 19:44:57 +00:00

306: Post the comment at runPipeline's no-code-change short-circuit

Closes #306 — 306.

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 23m 58s.

Review findings

Now let me look at the resume machinery to evaluate finding 2, and the CLI/summary surfaces for finding 3.Let me examine the test file structure and the existing no-code-change tests, then the artifacts/resume helper APIs.Let me check remaining surfaces that might assert the current text/behavior (e2e, commit-model, runBatch), and inspect lint config for ternary/style constraints.Baseline verified via review. Now let me apply the fixes, starting with runPipeline.ts (findings 1, 3, 4).Now update the type docstring in orchestrator.ts (finding 4's alternative branch), plus the summary docstring references:Now implement the resume mapping fix (finding 2 / Step 6) in determineResumeStage.ts:Now the CLI outcome line (finding 3) in runCli.ts:Now update the tests in runPipeline.test.ts. Let me view the exact blocks to edit:Now add Step 6 tests to determineResumeStage.test.ts. Let me inspect its helpers for the git-aware describe:Now add the CLI test (finding 3). Let me inspect the surrounding test helpers:Let me re-read the modified no-code branch to confirm coherence:Now run the targeted tests for the three touched test files:Targeted tests pass. Now run the full gates — lint, format check, full test suite, and build:Prettier hunk in runPipeline.ts is pre-existing on main (line 651 there), unrelated to this change. Now run the full gates:Typecheck shows errors; let me confirm none are new by comparing against the pristine origin/main baseline in a temporary worktree:Clean: error locations are identical to origin/main — no new typecheck errors. Let me clean up the temporary worktree and review the final diff:All gates green. Let me do a final review of the remediation diff:The diff is coherent. Let me sanity-check one existing resume no-code test that predates this feature, to ensure my skippedStages change didn't make its premise stale:All fixes are applied and gates pass. Final annotated findings:

Review Findings — 306

Summary

All four findings were resolved in a single pass:

Medium (stage attribution ordering): currentStage = 'write-back' is now set before makeCommentClient(...) so a client-construction failure (provider config/credential error) is attributed to the write-back stage, never implement. Added a unit test locking that attribution (attributes a comment-client construction failure to the write-back stage).
Medium (resume mapping): landed the ADR-021 §7 / implementation-plan Step 6 change in this branch — determineResumeStage.ts now has isImplementStageComplete (valid implementation.diff or valid no-code-change.json), used via isStageArtifactComplete in determineResumeStartStage, the allArtifactsValid pre-check, and the git-trailer artifact check. Real resume after a fatal no-code-change failure now returns review (implement skipped), so the implement LLM never re-runs and the resume-re-entry unit test's resumeFromStage: 'review' premise is real machinery output. Added three resume-machinery unit tests covering the valid-artifact, malformed-artifact, and git-aware paths.
Medium (silent post): buildNoCodeChangeSummary now renders a comment line when the post happened this run (and an "already posted" variant when the marker pre-existed on resume), and the CLI Outcome: no-code-change … line appends — commented on the issue when noCodeChangeComment is set. Added runCli unit coverage. Batch/manifest intentionally unchanged (ADR-021 §8 / plan Step 5).
Low (skippedStages): 'write-back' is only pushed when the post was actually skipped (marker pre-existed on resume); when the comment post runs, write-back ran and is not reported as skipped. Test expectations updated accordingly (posted-run lists now exclude write-back; marker-skip test asserts it is included), and the no-code-change variant's skippedStages docstring in orchestrator.ts now states the condition.

Gates: npm test passed (148 files / 2128 tests), npm run lint passed, npm run build passed; npm run typecheck still fails on the same error locations as origin/main (verified by diffing normalized tsc --noEmit logs against a throwaway worktree of origin/main — identical set, only the union-type message text now mentions the new noCodeChangeComment field). Prettier still flags the pre-existing docs-stage hunk in runPipeline.ts (confirmed present on origin/main, untouched). Gated PILOOP_E2E resume E2E (ADR-021 §9) not run — needs live creds. Remaining plan steps (AGENTS.md error-handling row, DESIGN.md artifact inventory — plan Step 9) are unchanged follow-up work.

Critical

(none)

High

(none)

Medium

src/orchestrator/services/runPipeline.ts:561 — currentStage = 'write-back' is assigned after makeCommentClient(...) runs, so if the comment client constructor throws (e.g. a provider config/credential error surfaced at construction — defaultMakeCommentClient calls requireJiraCreds/requireGitLabCreds which throw), the catch block attributes the failure to 'implement' (the current stage at that moment). That contradicts the branch's own comment and ADR-021 §5 ("Failures are FATAL … attributed to the write-back stage so error.json/console output name the real culprit rather than the implement stage") and would make resume target implement (re-running the implement LLM). Suggested fix: move currentStage = 'write-back'; above the makeCommentClient call (this is exactly what implementation-plan Step 4 §2 instructs).
src/orchestrator/services/runPipeline.test.ts:2567 — the resume-re-entry test simulates resumeFromStage: 'review', a state the real resume machinery cannot produce yet: determineResumeStage.ts:30 still maps implement to implementation.diff with no no-code-change.json special case (ADR-021 §7 / implementation-plan Step 6). A real fatal comment failure writes error.json (stage write-back) but the run has no Pi-Loop-Stage commit and no implementation.diff, so a subsequent resume falls back to artifact detection and returns resumeFromStage: 'implement' — re-running the implement LLM, exactly what §7 forbids ("The implement LLM never re-runs"), and widening the residual duplicate-post window if the crash lands after addComment but before the marker write. Suggested fix: land Step 6 (isImplementStageComplete treating a valid no-code-change.json as implement-complete evidence, used in determineResumeStartStage and the allArtifactsValid pre-check of determineResumeStageWithGit) in this change or sequence it immediately after, and update the resume unit/E2E assertions to go through the real determineResumeStage entry point rather than faking resumeFromStage: 'review'.

Medium (continued from above)

src/orchestrator/services/runPipeline.ts:578 — the newly introduced noCodeChangeComment result field has no downstream consumer: runCli.ts's Outcome: no-code-change … line and buildNoCodeChangeSummary (runPipeline.ts:236) are not updated to mention the comment, so operators and the run summary still see no trace that the issue was commented on (ADR-021 §8). This matches implementation-plan Step 5 being split out, so it may be intentional — but if Step 5 is not already tracked as a follow-up issue, it should be, because the branch now performs the post silently from the operator's point of view. Suggested fix: add the follow-up (or extend buildNoCodeChangeSummary/runCli outcome line in this change).

Low

src/orchestrator/services/runPipeline.ts:553 — the branch pushes 'write-back' into skippedStages and the new tests assert it, yet the branch now executes write-back work (the comment post is logged/attributed to the write-back stage at lines 566–575). The returned skippedStages: ['review','remediate','docs','mr','write-back'] tells consumers write-back was skipped when it ran. Suggested fix: drop 'write-back' from the push when the post actually executes (only keep it for the marker pre-existed / skipped-post case), and update the test expectation accordingly — or, if skippedStages is deliberately informational, tighten the type docstring at orchestrator.ts:225 so the claim is accurate.

## 306: Post the comment at runPipeline's no-code-change short-circuit Closes #306 — [306](https://git.excelera.net/david/pi-loop/issues/306). > 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 23m 58s. ## Review findings Now let me look at the resume machinery to evaluate finding 2, and the CLI/summary surfaces for finding 3.Let me examine the test file structure and the existing no-code-change tests, then the artifacts/resume helper APIs.Let me check remaining surfaces that might assert the current text/behavior (e2e, commit-model, runBatch), and inspect lint config for ternary/style constraints.Baseline verified via review. Now let me apply the fixes, starting with `runPipeline.ts` (findings 1, 3, 4).Now update the type docstring in `orchestrator.ts` (finding 4's alternative branch), plus the summary docstring references:Now implement the resume mapping fix (finding 2 / Step 6) in `determineResumeStage.ts`:Now the CLI outcome line (finding 3) in `runCli.ts`:Now update the tests in `runPipeline.test.ts`. Let me view the exact blocks to edit:Now add Step 6 tests to `determineResumeStage.test.ts`. Let me inspect its helpers for the git-aware describe:Now add the CLI test (finding 3). Let me inspect the surrounding test helpers:Let me re-read the modified no-code branch to confirm coherence:Now run the targeted tests for the three touched test files:Targeted tests pass. Now run the full gates — lint, format check, full test suite, and build:Prettier hunk in runPipeline.ts is pre-existing on main (line 651 there), unrelated to this change. Now run the full gates:Typecheck shows errors; let me confirm none are new by comparing against the pristine `origin/main` baseline in a temporary worktree:Clean: error locations are identical to `origin/main` — no new typecheck errors. Let me clean up the temporary worktree and review the final diff:All gates green. Let me do a final review of the remediation diff:The diff is coherent. Let me sanity-check one existing resume no-code test that predates this feature, to ensure my skippedStages change didn't make its premise stale:All fixes are applied and gates pass. Final annotated findings: # Review Findings — 306 ## Summary All four findings were resolved in a single pass: - **Medium (stage attribution ordering):** `currentStage = 'write-back'` is now set *before* `makeCommentClient(...)` so a client-construction failure (provider config/credential error) is attributed to the `write-back` stage, never `implement`. Added a unit test locking that attribution (`attributes a comment-client construction failure to the write-back stage`). - **Medium (resume mapping):** landed the ADR-021 §7 / implementation-plan Step 6 change in this branch — `determineResumeStage.ts` now has `isImplementStageComplete` (valid `implementation.diff` **or** valid `no-code-change.json`), used via `isStageArtifactComplete` in `determineResumeStartStage`, the `allArtifactsValid` pre-check, and the git-trailer artifact check. Real resume after a fatal no-code-change failure now returns `review` (implement skipped), so the implement LLM never re-runs and the resume-re-entry unit test's `resumeFromStage: 'review'` premise is real machinery output. Added three resume-machinery unit tests covering the valid-artifact, malformed-artifact, and git-aware paths. - **Medium (silent post):** `buildNoCodeChangeSummary` now renders a comment line when the post happened this run (and an "already posted" variant when the marker pre-existed on resume), and the CLI `Outcome: no-code-change …` line appends `— commented on the issue` when `noCodeChangeComment` is set. Added runCli unit coverage. Batch/manifest intentionally unchanged (ADR-021 §8 / plan Step 5). - **Low (skippedStages):** `'write-back'` is only pushed when the post was actually skipped (marker pre-existed on resume); when the comment post runs, write-back ran and is not reported as skipped. Test expectations updated accordingly (posted-run lists now exclude `write-back`; marker-skip test asserts it is included), and the no-code-change variant's `skippedStages` docstring in `orchestrator.ts` now states the condition. Gates: `npm test` **passed** (148 files / 2128 tests), `npm run lint` **passed**, `npm run build` **passed**; `npm run typecheck` still fails on the same error locations as `origin/main` (verified by diffing normalized `tsc --noEmit` logs against a throwaway worktree of `origin/main` — identical set, only the union-type message text now mentions the new `noCodeChangeComment` field). Prettier still flags the pre-existing docs-stage hunk in `runPipeline.ts` (confirmed present on `origin/main`, untouched). Gated `PILOOP_E2E` resume E2E (ADR-021 §9) not run — needs live creds. Remaining plan steps (AGENTS.md error-handling row, DESIGN.md artifact inventory — plan Step 9) are unchanged follow-up work. ## Critical - (none) ## High - (none) ## Medium - [x] src/orchestrator/services/runPipeline.ts:561 — `currentStage = 'write-back'` is assigned *after* `makeCommentClient(...)` runs, so if the comment client constructor throws (e.g. a provider config/credential error surfaced at construction — `defaultMakeCommentClient` calls `requireJiraCreds`/`requireGitLabCreds` which throw), the catch block attributes the failure to `'implement'` (the current stage at that moment). That contradicts the branch's own comment and ADR-021 §5 ("Failures are FATAL … attributed to the write-back stage so error.json/console output name the real culprit rather than the implement stage") and would make resume target `implement` (re-running the implement LLM). Suggested fix: move `currentStage = 'write-back';` above the `makeCommentClient` call (this is exactly what implementation-plan Step 4 §2 instructs). - [x] src/orchestrator/services/runPipeline.test.ts:2567 — the resume-re-entry test simulates `resumeFromStage: 'review'`, a state the real resume machinery cannot produce yet: `determineResumeStage.ts:30` still maps `implement` to `implementation.diff` with no `no-code-change.json` special case (ADR-021 §7 / implementation-plan Step 6). A real fatal comment failure writes `error.json` (stage `write-back`) but the run has no `Pi-Loop-Stage` commit and no `implementation.diff`, so a subsequent resume falls back to artifact detection and returns `resumeFromStage: 'implement'` — re-running the implement LLM, exactly what §7 forbids ("The implement LLM never re-runs"), and widening the residual duplicate-post window if the crash lands after `addComment` but before the marker write. Suggested fix: land Step 6 (`isImplementStageComplete` treating a valid `no-code-change.json` as implement-complete evidence, used in `determineResumeStartStage` and the `allArtifactsValid` pre-check of `determineResumeStageWithGit`) in this change or sequence it immediately after, and update the resume unit/E2E assertions to go through the real `determineResumeStage` entry point rather than faking `resumeFromStage: 'review'`. ## Medium (continued from above) - [x] src/orchestrator/services/runPipeline.ts:578 — the newly introduced `noCodeChangeComment` result field has no downstream consumer: `runCli.ts`'s `Outcome: no-code-change …` line and `buildNoCodeChangeSummary` (runPipeline.ts:236) are not updated to mention the comment, so operators and the run summary still see no trace that the issue was commented on (ADR-021 §8). This matches implementation-plan Step 5 being split out, so it may be intentional — but if Step 5 is not already tracked as a follow-up issue, it should be, because the branch now performs the post silently from the operator's point of view. Suggested fix: add the follow-up (or extend `buildNoCodeChangeSummary`/runCli outcome line in this change). ## Low - [x] src/orchestrator/services/runPipeline.ts:553 — the branch pushes `'write-back'` into `skippedStages` and the new tests assert it, yet the branch now *executes* write-back work (the comment post is logged/attributed to the `write-back` stage at lines 566–575). The returned `skippedStages: ['review','remediate','docs','mr','write-back']` tells consumers write-back was skipped when it ran. Suggested fix: drop `'write-back'` from the push when the post actually executes (only keep it for the marker pre-existed / skipped-post case), and update the test expectation accordingly — or, if `skippedStages` is deliberately informational, tighten the type docstring at `orchestrator.ts:225` so the claim is accurate.

david added 2 commits

2026-09-05 19:44:57 +00:00

306: Post the comment at runPipeline's no-code-change short-circuit 5f9160d405