issue-307: Surface the posted comment in the run summary and CLI outcome line #317

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

david commented

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

307: Surface the posted comment in the run summary and CLI outcome line

Closes #307 — 307.

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 12m 25s.

Review findings

The issue context confirms the picture: issue #307 depends on #306, but #306's merged PR already delivered both §8 surfaces at base. Let me verify the exact current text before editing, and check the base state of the test file section.Now applying the fixes. Medium (revert the ungrammatical article trim, keeping both surfaces consistent) and Low (strengthen the negative assertion so it guards both wordings):The diff is now behavior-neutral vs base (comments + a hardened test only), which is the honest state given #306 already delivered the §8 surfaces. Now re-run the gates. Let me check the scripts first:typecheck shows errors in test files I didn't touch. Let me check whether those are pre-existing at the base (the reviewer's gate was npm run build, which uses tsconfig.build.json):The same typecheck errors occur at the pre-existing HEAD, so they're not introduced by my change. Let me confirm they exist at base too, and that my edited files specifically are clean:99 typecheck errors at base — that script is red repo-wide independent of this branch (not a gate used by the review). Let me get back onto the feature branch:Back on the feature branch with edits intact. Now run the actual gates (build + lint), then unit tests:Build and lint pass. Now unit tests — first the touched file, then the full suite:All gates pass. Let me also run prettier check on the changed files as the reviewer did:All gates pass. My remediation is complete — here is the annotated findings document.

Review Findings — 307

Summary

Gates run after remediation: npm run build (tsc) PASS, npm run lint PASS, npm test PASS (148 files / 2128 tests), npx prettier --check on the changed files PASS, focused runCli.test.ts PASS (109 tests).

Fixed in this pass:

Medium — restored the grammatical ' — commented on the issue' suffix in runCli.ts and the matching expectation in runCli.test.ts, so the CLI outcome line no longer reads clipped and stays consistent with the run-summary surface ("Commented on the issue explaining why no code changes are needed"). The behavior of the branch vs base is now comment-only in runCli.ts.
Low — strengthened the negative assertion in runCli.test.ts to not.toContain('commented on'), so a regression appending either article wording (— commented on issue or — commented on the issue) in the no-marker path would fail the test.

Remaining:

High — left unresolved: it requires an issue-owner decision. I verified the finding against the repo state and issue context: base a971a96 (the #306 merge) already ships both §8 surfaces (buildNoCodeChangeSummary + the CLI suffix), so #307's deliverable was pre-empted by its dependency. This branch is now behavior-neutral vs base (explanatory comments in runBatch.ts/batchManifest.ts, an explanatory comment in runCli.ts, and the hardened negative assertion). Whether to close #307 as a no-op, revert the branch, or add a further surface is a product call outside the remediate pass, so I did not unilaterally wipe the branch or invent new behavior.

Critical

(no findings)

High

src/cli/services/runCli.ts:1281 — This diff does not implement issue #307's stated deliverable. The issue title ("Surface the posted comment in the run summary and CLI outcome line", ADR-021 §8 / implementation-plan Step 5) was already satisfied at the base commit: the run summary surfaces the post at src/orchestrator/services/runPipeline.ts:255-257 ("Commented on the issue explaining why no code changes are needed" / "…already posted previously (skipped duplicate)") and the CLI outcome line already appended a — commented on the issue suffix at base, both introduced and tested by #306 (merged in a971a96). This branch only trims "the" from the existing suffix, adds two explanatory comments (runBatch.ts, batchManifest.ts), and adds one negative assertion — no new surfacing behavior is delivered. Suggested fix: confirm with the issue owner whether #307 is already satisfied by #306's merged work; if so, drop/revert this cosmetic-only change and close #307 as a no-op rather than merging it as the issue's implementation. If a genuine gap remains, this diff should add the missing surface rather than reword an existing one.

Medium

src/cli/services/runCli.test.ts:1953 — The only code-behavior change in the diff removes the article from the suffix (' — commented on the issue' → ' — commented on issue'), making the outcome line read "…work: flipped a feature flag — commented on issue", which is clipped/ungrammatical and now inconsistent with the same feature's run-summary surface, which keeps the grammatical "Commented on the issue explaining why no code changes are needed" (runPipeline.ts:256). Both surfaces are printed adjacently in the same sink output for a real no-code-change run. Suggested fix: keep ' — commented on the issue' in runCli.ts:1281 and this test expectation (the plan's — commented on issue is illustrative wording, not a contract), or deliberately align both surfaces to the same phrasing.

Low

src/cli/services/runCli.test.ts:1928 — The new negative assertion not.toContain('— commented on issue') is under-specified: it only guards the exact new wording. A regression that appended the pre-existing article form '— commented on the issue' in the no-marker path would pass this test (the substring does not match because of the inserted "the "). Suggested fix: assert the full expected outcome line (e.g. toContain('Outcome: no-code-change … — work: flipped a feature flag') without any suffix) or use not.toContain('commented on') for a stronger guard.

## 307: Surface the posted comment in the run summary and CLI outcome line Closes #307 — [307](https://git.excelera.net/david/pi-loop/issues/307). > 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 12m 25s. ## Review findings The issue context confirms the picture: issue #307 depends on #306, but #306's merged PR already delivered both §8 surfaces at base. Let me verify the exact current text before editing, and check the base state of the test file section.Now applying the fixes. Medium (revert the ungrammatical article trim, keeping both surfaces consistent) and Low (strengthen the negative assertion so it guards both wordings):The diff is now behavior-neutral vs base (comments + a hardened test only), which is the honest state given #306 already delivered the §8 surfaces. Now re-run the gates. Let me check the scripts first:`typecheck` shows errors in test files I didn't touch. Let me check whether those are pre-existing at the base (the reviewer's gate was `npm run build`, which uses `tsconfig.build.json`):The same typecheck errors occur at the pre-existing HEAD, so they're not introduced by my change. Let me confirm they exist at base too, and that my edited files specifically are clean:99 typecheck errors at base — that script is red repo-wide independent of this branch (not a gate used by the review). Let me get back onto the feature branch:Back on the feature branch with edits intact. Now run the actual gates (build + lint), then unit tests:Build and lint pass. Now unit tests — first the touched file, then the full suite:All gates pass. Let me also run prettier check on the changed files as the reviewer did:All gates pass. My remediation is complete — here is the annotated findings document. # Review Findings — 307 ## Summary Gates run after remediation: `npm run build` (tsc) **PASS**, `npm run lint` **PASS**, `npm test` **PASS** (148 files / 2128 tests), `npx prettier --check` on the changed files **PASS**, focused `runCli.test.ts` **PASS** (109 tests). Fixed in this pass: - **Medium** — restored the grammatical `' — commented on the issue'` suffix in `runCli.ts` and the matching expectation in `runCli.test.ts`, so the CLI outcome line no longer reads clipped and stays consistent with the run-summary surface ("Commented on the issue explaining why no code changes are needed"). The behavior of the branch vs base is now comment-only in `runCli.ts`. - **Low** — strengthened the negative assertion in `runCli.test.ts` to `not.toContain('commented on')`, so a regression appending either article wording (`— commented on issue` or `— commented on the issue`) in the no-marker path would fail the test. Remaining: - **High** — left unresolved: it requires an issue-owner decision. I verified the finding against the repo state and issue context: base `a971a96` (the #306 merge) already ships both §8 surfaces (`buildNoCodeChangeSummary` + the CLI suffix), so #307's deliverable was pre-empted by its dependency. This branch is now behavior-neutral vs base (explanatory comments in `runBatch.ts`/`batchManifest.ts`, an explanatory comment in `runCli.ts`, and the hardened negative assertion). Whether to close #307 as a no-op, revert the branch, or add a further surface is a product call outside the remediate pass, so I did not unilaterally wipe the branch or invent new behavior. ## Critical - (no findings) ## High - [ ] src/cli/services/runCli.ts:1281 — This diff does not implement issue #307's stated deliverable. The issue title ("Surface the posted comment in the run summary and CLI outcome line", ADR-021 §8 / implementation-plan Step 5) was already satisfied at the base commit: the run summary surfaces the post at `src/orchestrator/services/runPipeline.ts:255-257` ("Commented on the issue explaining why no code changes are needed" / "…already posted previously (skipped duplicate)") and the CLI outcome line already appended a `— commented on the issue` suffix at base, both introduced and tested by #306 (merged in `a971a96`). This branch only trims "the" from the existing suffix, adds two explanatory comments (`runBatch.ts`, `batchManifest.ts`), and adds one negative assertion — no new surfacing behavior is delivered. Suggested fix: confirm with the issue owner whether #307 is already satisfied by #306's merged work; if so, drop/revert this cosmetic-only change and close #307 as a no-op rather than merging it as the issue's implementation. If a genuine gap remains, this diff should add the missing surface rather than reword an existing one. ## Medium - [x] src/cli/services/runCli.test.ts:1953 — The only code-behavior change in the diff removes the article from the suffix (`' — commented on the issue'` → `' — commented on issue'`), making the outcome line read "…work: flipped a feature flag — commented on issue", which is clipped/ungrammatical and now inconsistent with the same feature's run-summary surface, which keeps the grammatical "Commented on the issue explaining why no code changes are needed" (`runPipeline.ts:256`). Both surfaces are printed adjacently in the same sink output for a real no-code-change run. Suggested fix: keep `' — commented on the issue'` in `runCli.ts:1281` and this test expectation (the plan's `— commented on issue` is illustrative wording, not a contract), or deliberately align both surfaces to the same phrasing. ## Low - [x] src/cli/services/runCli.test.ts:1928 — The new negative assertion `not.toContain('— commented on issue')` is under-specified: it only guards the exact new wording. A regression that appended the pre-existing article form `'— commented on the issue'` in the no-marker path would pass this test (the substring does not match because of the inserted "the "). Suggested fix: assert the full expected outcome line (e.g. `toContain('Outcome: no-code-change … — work: flipped a feature flag')` without any suffix) or use `not.toContain('commented on')` for a stronger guard.

david added 2 commits

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

307: Surface the posted comment in the run summary and CLI outcome line f8e58816e4