issue-305: postNoCodeChangeComment — deterministic post-and-mark service #315

david · 2026-09-05T19:20:55Z

david commented

2026-09-05 19:20:55 +00:00

305: postNoCodeChangeComment — deterministic post-and-mark service

Closes #305 — 305.

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 10m 33s.

Review findings

I'll start by reading the relevant files to understand the current state.Now let me examine the artifacts module functions referenced by the service.Let me check the ADR-021 design doc, related artifacts tests, and how other writeback helpers handle similar situations.Let me look at how other writeback services use logInfo and check the barrel exports and related code for conventions.Let me check the artifacts barrel exports and the IssueCommentClient interface to plan the test imports.Now let me check phrasing conventions for log messages and skip behavior elsewhere in the codebase.Now I have full context. Let me verify the current file state matches what I read, then apply the fixes. First, the service file edits:Let me verify the service file changes:Service file updated. Now let me update the test file with the new coverage:Let me verify the test file is well-formed:Both files updated. Now let me run prettier on the changed files to normalize formatting, and confirm the diff is clean:Now let me run the focused tests for the changed service:All 9 tests pass (5 original + 4 new). Now let me run the project gates — lint and full test suite in parallel:Lint clean, full suite green (2119 tests, +4 new). Now the build/typecheck gate and prettier check on changed files:Let me confirm the git state — only intended files changed and no build artifacts leaked:Git state is clean — only the two intended files. Let me do a final review of the complete service diff:All remediation is complete and verified. Here is my final annotated findings report.

Review Findings — 305

Summary

Resolved both findings in a single pass on postNoCodeChangeComment (the ADR-021 §5/§6 deterministic post-and-mark service). For the Medium (at-most-once crash window): added two tests pinning the previously untested branches — (1) marker write fails after a successful post → service rejects and no marker exists (write-after-post duplicate window), (2) malformed marker present → ArtifactValidationError propagates and the client is never called — and qualified the JSDoc to scope the at-most-once invariant truthfully to the post-failure branch, documenting the residual duplicate window and malformed-marker failure mode. For the Low (observability): the marker-present skip path now emits a logInfo line (no-code-change comment already posted on <provider>:<key>; skipping), and two tests inject logInfo spies asserting the exact message on both the post path and the skip path. Gates re-run and passing: npm run lint ✅, npm run build ✅, npm test ✅ (148 files / 2119 tests — 4 new), npx prettier --check on the changed files ✅. Working tree contains only the two intended file changes.

Critical

High

Medium

src/writeback/services/postNoCodeChangeComment.ts:74-77 (with docstring claim at lines 57-59) — The at-most-once guarantee has an untested, undocumented crash window: the marker is written after client.addComment resolves, so if the marker write fails (fs error) or the process dies between the post and the write, the comment exists but no-code-change-comment.json does not — and on resume the service posts a duplicate (the docstring's "no marker = not posted" resume-invariant is only truthful for the client-failure branch). Relatedly, a partially written/corrupt marker makes readNoCodeChangeCommentSignal throw ArtifactValidationError on every resume attempt with no automated recovery. Neither branch (post succeeds → marker write throws; malformed marker present → error propagates) is covered by the new tests. Suggested fix: add two tests pinning these deterministic branches — (1) call with a non-writable/nonexistent runDir so writeNoCodeChangeCommentSignal throws after a successful addComment, asserting the service rejects and no marker exists; (2) pre-write a malformed marker file, asserting the error propagates — and either qualify the JSDoc to scope the invariant to the post-failure branch or make the marker write atomic (temp file + rename) and document the residual duplicate window.

Low

src/writeback/services/postNoCodeChangeComment.ts:71-72 — logInfo is an injectable seam (part of PostNoCodeChangeCommentInput) but is never asserted in the tests and the marker-present skip path emits no log line at all; for an unattended pipeline whose run.log is the operator's only trace, a resumed run that silently skips a post is invisible at the service level. Suggested fix: exercise logInfo in a test (inject a spy and assert the message on the post path) and/or emit a logInfo line such as no-code-change comment already posted (marker present); skipping on the skip path so future wiring produces an observable trace.

## 305: postNoCodeChangeComment — deterministic post-and-mark service Closes #305 — [305](https://git.excelera.net/david/pi-loop/issues/305). > 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 10m 33s. ## Review findings I'll start by reading the relevant files to understand the current state.Now let me examine the artifacts module functions referenced by the service.Let me check the ADR-021 design doc, related artifacts tests, and how other writeback helpers handle similar situations.Let me look at how other writeback services use `logInfo` and check the barrel exports and related code for conventions.Let me check the artifacts barrel exports and the IssueCommentClient interface to plan the test imports.Now let me check phrasing conventions for log messages and skip behavior elsewhere in the codebase.Now I have full context. Let me verify the current file state matches what I read, then apply the fixes. First, the service file edits:Let me verify the service file changes:Service file updated. Now let me update the test file with the new coverage:Let me verify the test file is well-formed:Both files updated. Now let me run prettier on the changed files to normalize formatting, and confirm the diff is clean:Now let me run the focused tests for the changed service:All 9 tests pass (5 original + 4 new). Now let me run the project gates — lint and full test suite in parallel:Lint clean, full suite green (2119 tests, +4 new). Now the build/typecheck gate and prettier check on changed files:Let me confirm the git state — only intended files changed and no build artifacts leaked:Git state is clean — only the two intended files. Let me do a final review of the complete service diff:All remediation is complete and verified. Here is my final annotated findings report. # Review Findings — 305 ## Summary Resolved both findings in a single pass on `postNoCodeChangeComment` (the ADR-021 §5/§6 deterministic post-and-mark service). For the **Medium** (at-most-once crash window): added two tests pinning the previously untested branches — (1) marker write fails after a successful post → service rejects and no marker exists (write-after-post duplicate window), (2) malformed marker present → `ArtifactValidationError` propagates and the client is never called — and qualified the JSDoc to scope the at-most-once invariant truthfully to the post-failure branch, documenting the residual duplicate window and malformed-marker failure mode. For the **Low** (observability): the marker-present skip path now emits a `logInfo` line (`no-code-change comment already posted on <provider>:<key>; skipping`), and two tests inject `logInfo` spies asserting the exact message on both the post path and the skip path. Gates re-run and passing: `npm run lint` ✅, `npm run build` ✅, `npm test` ✅ (148 files / 2119 tests — 4 new), `npx prettier --check` on the changed files ✅. Working tree contains only the two intended file changes. ## Critical ## High ## Medium - [x] `src/writeback/services/postNoCodeChangeComment.ts:74-77` (with docstring claim at lines 57-59) — The at-most-once guarantee has an untested, undocumented crash window: the marker is written **after** `client.addComment` resolves, so if the marker write fails (fs error) or the process dies between the post and the write, the comment exists but `no-code-change-comment.json` does not — and on resume the service posts a duplicate (the docstring's "no marker = not posted" resume-invariant is only truthful for the client-failure branch). Relatedly, a partially written/corrupt marker makes `readNoCodeChangeCommentSignal` throw `ArtifactValidationError` on every resume attempt with no automated recovery. Neither branch (post succeeds → marker write throws; malformed marker present → error propagates) is covered by the new tests. Suggested fix: add two tests pinning these deterministic branches — (1) call with a non-writable/nonexistent `runDir` so `writeNoCodeChangeCommentSignal` throws after a successful `addComment`, asserting the service rejects and no marker exists; (2) pre-write a malformed marker file, asserting the error propagates — and either qualify the JSDoc to scope the invariant to the post-failure branch or make the marker write atomic (temp file + rename) and document the residual duplicate window. ## Low - [x] `src/writeback/services/postNoCodeChangeComment.ts:71-72` — `logInfo` is an injectable seam (part of `PostNoCodeChangeCommentInput`) but is never asserted in the tests and the marker-present skip path emits no log line at all; for an unattended pipeline whose run.log is the operator's only trace, a resumed run that silently skips a post is invisible at the service level. Suggested fix: exercise `logInfo` in a test (inject a spy and assert the message on the post path) and/or emit a `logInfo` line such as `no-code-change comment already posted (marker present); skipping` on the skip path so future wiring produces an observable trace.

david added 2 commits

2026-09-05 19:20:56 +00:00

305: postNoCodeChangeComment — deterministic post-and-mark service 3cca206b61