issue-303: Deterministic no-code-change comment-body builder (buildNoCodeChangeCommentBody) #313

david · 2026-09-05T19:01:33Z

david commented

2026-09-05 19:01:33 +00:00

303: Deterministic no-code-change comment-body builder (buildNoCodeChangeCommentBody)

Closes #303 — 303.

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 9m 01s.

Review findings

Let me check the artifacts barrel exports and the surrounding conventions to understand what's exported and whether there is an established pattern for deriving the type list in tests.The grep pattern got mangled by the shell. Let me rerun with proper quoting and search the validate file plus tests for related conventions.The tool appears to pass the pattern through a shell. Let me use simpler patterns without |.Let me look at the existing validator test (same duplication pattern), the implementation plan, and package.json scripts.Let me check ADR-021 for the exact verbatim wording and look at codebase conventions for exporting valid-value arrays.Now let me check the git state and tsconfig to understand the working tree and typecheck settings before editing.The working tree is clean at the Stage-1 commit. Let me check prettier/eslint configs quickly, then apply the fixes.Configs are clear. Now I'll apply all fixes in a single pass:

Fix 1 — reword the docstring so the provider-agnostic claim scopes to the fixed frame:Fix 3a — export the canonical VALID_TYPES array and re-export it via the artifacts barrel:Adjacent consistency cleanup — the validator's own test duplicates the same list; drive it from the now-exported VALID_TYPES too (removing the unused NoCodeChangeType import):Let me verify the edits look right, then run the gates.Edits are in place. Now run the gates — lint first:Lint passes. Now build:Build passes. Now the affected test suites, then the full suite:Full suite green (145 files / 2088 tests). Now Prettier check on the changed files and a diagnostic typecheck to ensure no new errors touch the changed files:Prettier is clean. Let me verify none of the typecheck errors reference the changed files:All gates pass. Let me do a final review of the remediation diff:All fixes applied and gates verified. Final message below.

Review Findings — 303

Summary

All three Low findings were resolved in a single pass: the docstring now scopes the provider-agnostic claim to the fixed frame (noting reason/workPerformed are embedded verbatim and not sanitized), the token test was renamed to reflect what it actually guarantees and carries a scope comment, and the duplicated four-type enumeration was eliminated by exporting the canonical VALID_TYPES array through the artifacts barrel and iterating it in the test (with a cross-reference comment on the per-type sample list; the validator's own test now iterates VALID_TYPES too, closing the same silent-drift gap there). No findings remain unresolved.

Gates re-run after remediation: npm run lint ✅, npm run build ✅, npm test ✅ (145 files / 2088 tests), Prettier check on the five changed files ✅. Diagnostic npx tsc --noEmit remains red with 99 pre-existing errors — identical count to the pre-change baseline documented by the review, zero touching the changed files.

Critical

No findings.

High

No findings.

Medium

No findings.

Low

src/writeback/helpers/buildNoCodeChangeCommentBody.ts:12-13 — The docstring states the body "is provider-agnostic: it contains no MR/PR noun and no platform URL, so the same text works for Jira, GitHub, Forgejo, and GitLab." Because reason/workPerformed are embedded verbatim (per ADR-021 §3 and the byte-for-byte test at line 87), arbitrary agent-authored field text may legitimately contain "MR", "merge", "pull request", or a URL — the guarantee holds only for the fixed frame, not the full output. Suggested fix: reword to "the fixed frame is provider-agnostic (no MR/PR noun, no platform URL); reason/workPerformed are embedded verbatim per ADR-021 §3" so future readers do not infer sanitization, which would contradict the verbatim requirement.
src/writeback/helpers/buildNoCodeChangeCommentBody.test.ts:116 — The test name "contains no MR/PR noun and no URL for any artifact type" overstates an invariant the function cannot provide: it only asserts the fixed sample values (reason: 'Handled outside the repository', workPerformed: 'Done via external tooling') produce no tokens, not that any possible artifact does. Suggested fix: rename to e.g. "renders a fixed frame with no MR/PR noun or URL for any artifact type" and add a comment noting the assertion applies to the frame plus the fixed sample values (verbatim fields are outside this guarantee).
src/writeback/helpers/buildNoCodeChangeCommentBody.test.ts:117 (and the case list at lines 22-45) — The four-type enumeration ('database', 'config', 'docs', 'other') duplicates VALID_TYPES in src/artifacts/helpers/validateNoCodeChange.ts:4-8; if a fifth NoCodeChangeType is ever added, the "for any artifact type" coverage silently omits it (the loops stay compile-valid as a subset of the union). Suggested fix: derive the type list from the exported NoCodeChangeType union or add a cross-reference comment that the list must stay in sync with VALID_TYPES, so a schema extension updates the test surface.

## 303: Deterministic no-code-change comment-body builder (buildNoCodeChangeCommentBody) Closes #303 — [303](https://git.excelera.net/david/pi-loop/issues/303). > 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 9m 01s. ## Review findings Let me check the artifacts barrel exports and the surrounding conventions to understand what's exported and whether there is an established pattern for deriving the type list in tests.The grep pattern got mangled by the shell. Let me rerun with proper quoting and search the validate file plus tests for related conventions.The tool appears to pass the pattern through a shell. Let me use simpler patterns without `|`.Let me look at the existing validator test (same duplication pattern), the implementation plan, and package.json scripts.Let me check ADR-021 for the exact verbatim wording and look at codebase conventions for exporting valid-value arrays.Now let me check the git state and tsconfig to understand the working tree and typecheck settings before editing.The working tree is clean at the Stage-1 commit. Let me check prettier/eslint configs quickly, then apply the fixes.Configs are clear. Now I'll apply all fixes in a single pass: **Fix 1** — reword the docstring so the provider-agnostic claim scopes to the fixed frame:**Fix 3a** — export the canonical `VALID_TYPES` array and re-export it via the artifacts barrel:**Adjacent consistency cleanup** — the validator's own test duplicates the same list; drive it from the now-exported `VALID_TYPES` too (removing the unused `NoCodeChangeType` import):Let me verify the edits look right, then run the gates.Edits are in place. Now run the gates — lint first:Lint passes. Now build:Build passes. Now the affected test suites, then the full suite:Full suite green (145 files / 2088 tests). Now Prettier check on the changed files and a diagnostic typecheck to ensure no new errors touch the changed files:Prettier is clean. Let me verify none of the typecheck errors reference the changed files:All gates pass. Let me do a final review of the remediation diff:All fixes applied and gates verified. Final message below. # Review Findings — 303 ## Summary All three Low findings were resolved in a single pass: the docstring now scopes the provider-agnostic claim to the fixed frame (noting `reason`/`workPerformed` are embedded verbatim and not sanitized), the token test was renamed to reflect what it actually guarantees and carries a scope comment, and the duplicated four-type enumeration was eliminated by exporting the canonical `VALID_TYPES` array through the artifacts barrel and iterating it in the test (with a cross-reference comment on the per-type sample list; the validator's own test now iterates `VALID_TYPES` too, closing the same silent-drift gap there). No findings remain unresolved. Gates re-run after remediation: `npm run lint` ✅, `npm run build` ✅, `npm test` ✅ (145 files / 2088 tests), Prettier check on the five changed files ✅. Diagnostic `npx tsc --noEmit` remains red with 99 pre-existing errors — identical count to the pre-change baseline documented by the review, zero touching the changed files. ## Critical - No findings. ## High - No findings. ## Medium - No findings. ## Low - [x] `src/writeback/helpers/buildNoCodeChangeCommentBody.ts:12-13` — The docstring states the body "is provider-agnostic: it contains no MR/PR noun and no platform URL, so the same text works for Jira, GitHub, Forgejo, and GitLab." Because `reason`/`workPerformed` are embedded **verbatim** (per ADR-021 §3 and the byte-for-byte test at line 87), arbitrary agent-authored field text may legitimately contain "MR", "merge", "pull request", or a URL — the guarantee holds only for the fixed frame, not the full output. Suggested fix: reword to "the fixed frame is provider-agnostic (no MR/PR noun, no platform URL); `reason`/`workPerformed` are embedded verbatim per ADR-021 §3" so future readers do not infer sanitization, which would contradict the verbatim requirement. - [x] `src/writeback/helpers/buildNoCodeChangeCommentBody.test.ts:116` — The test name "contains no MR/PR noun and no URL for any artifact type" overstates an invariant the function cannot provide: it only asserts the fixed sample values (`reason: 'Handled outside the repository'`, `workPerformed: 'Done via external tooling'`) produce no tokens, not that any possible artifact does. Suggested fix: rename to e.g. "renders a fixed frame with no MR/PR noun or URL for any artifact type" and add a comment noting the assertion applies to the frame plus the fixed sample values (verbatim fields are outside this guarantee). - [x] `src/writeback/helpers/buildNoCodeChangeCommentBody.test.ts:117` (and the case list at lines 22-45) — The four-type enumeration (`'database', 'config', 'docs', 'other'`) duplicates `VALID_TYPES` in `src/artifacts/helpers/validateNoCodeChange.ts:4-8`; if a fifth `NoCodeChangeType` is ever added, the "for any artifact type" coverage silently omits it (the loops stay compile-valid as a subset of the union). Suggested fix: derive the type list from the exported `NoCodeChangeType` union or add a cross-reference comment that the list must stay in sync with `VALID_TYPES`, so a schema extension updates the test surface.

david added 2 commits

2026-09-05 19:01:33 +00:00

303: Deterministic no-code-change comment-body builder (buildNoCodeChangeCommentBody) cdb20d3646