issue-304: no-code-change-comment.json marker artifact module #314

david · 2026-09-05T19:10:19Z

david commented

2026-09-05 19:10:19 +00:00

304: no-code-change-comment.json marker artifact module

Closes #304 — 304.

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 8m 43s.

Review findings

Let me look at the DESIGN.md artifact inventory and check how sibling artifacts are documented.Now I have full context. The working tree is clean at the implement commit 83b6926. Let me apply the fix for finding 1 (validate postedAt against the shared ISO_8601 regex, matching the friction family), and add the missing tests.Let me verify the edits and run the targeted gates.Targeted tests and lint pass. Now the full suite, build, and prettier check on changed files:Build and lint pass. Let me confirm the final working-tree diff is only the intended remediation:Working-tree diff is exactly the intended remediation. Let me also verify the typecheck gate the reviewer flagged — my change must not add new errors beyond the pre-existing 99:All gates are green and no new typecheck errors. Let me quickly confirm the DOMAIN.md structure referenced by finding 2 before finalizing:All gates pass. Finding 1 is resolved; finding 2 remains intentionally unresolved (its suggested fix explicitly permits deferral, and the doc inventory row is scheduled for Step 9 of the same implementation plan, which updates DESIGN.md §3.4 and AGENTS.md together once the feature is fully wired — the inventory currently doesn't list the sibling no-code-change.json/mr-skipped.json artifacts either, so a partial row now would be inconsistent).

Review Findings — 304

Summary

Remediated finding 1: validateNoCodeChangeCommentArtifact now validates postedAt with the shared ISO_8601 regex (src/artifacts/helpers/iso8601.ts, matching the friction-family validators) instead of only rejecting empty strings, so whitespace-only and non-ISO timestamps are rejected symmetrically with issueKey; the error message is now postedAt must be an ISO-8601 string. Added two unit tests covering whitespace-only and non-ISO-8601 postedAt. Gates re-run after the fix: targeted suites ✅ (22 tests), full npm test ✅ (147 files / 2110 tests, +2 new), npm run lint ✅, npm run build ✅, Prettier check on changed files ✅, npm run typecheck still shows the same pre-existing 99 errors as the base commit with zero in the changed files. Finding 2 left unresolved: the DESIGN.md §3.4 / DOMAIN.md inventory update is scheduled for Step 9 of docs/implementation-plan-no-code-change-comment.md (which updates DESIGN.md and AGENTS.md together once the feature is fully wired); the finding's suggested fix explicitly permits deferral, and DESIGN.md/DOMAIN.md do not currently list sibling artifacts (no-code-change.json, mr-skipped.json), so adding only this row now would be inconsistent.

Critical

No critical findings.

High

No high findings.

Medium

No medium findings.

Low

src/artifacts/helpers/validateNoCodeChangeComment.ts:42 — postedAt is documented as an "ISO-8601 timestamp" (lines 10–11) but the validator only rejects empty strings; a whitespace-only postedAt: ' ' passes validation, and the check is asymmetric with issueKey (lines 35–37), which rejects whitespace via trim(). The codebase already has a shared loose ISO-8601 check (src/artifacts/helpers/iso8601.ts, ISO_8601) used by the friction validators. Suggested fix: validate postedAt with the shared ISO_8601 regex (matching the friction family), or — if the weak check is deliberate to mirror the older artifact validators (mrSkipped.checkedAt, errorReport.occurredAt) — add a short comment explaining the choice; either way add a unit test covering a malformed/whitespace-only postedAt.
src/artifacts/index.ts:75 — the new artifact schema is not yet recorded in DESIGN.md §3.3/§3.4 artifact inventory (nor in the DOMAIN.md artifact glossary), even though ADR-021's Consequences and implementation-plan Step 9 call for DESIGN.md §3.4 to gain no-code-change-comment.json ({ issueKey, postedAt }). Suggested fix: when the Step 9 doc-update work lands (or in a follow-up commit), add the inventory row and glossary entry for no-code-change-comment.json; if that update is intentionally deferred to a later issue, no code change is needed here.

## 304: no-code-change-comment.json marker artifact module Closes #304 — [304](https://git.excelera.net/david/pi-loop/issues/304). > 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 8m 43s. ## Review findings Let me look at the DESIGN.md artifact inventory and check how sibling artifacts are documented.Now I have full context. The working tree is clean at the implement commit `83b6926`. Let me apply the fix for finding 1 (validate `postedAt` against the shared `ISO_8601` regex, matching the friction family), and add the missing tests.Let me verify the edits and run the targeted gates.Targeted tests and lint pass. Now the full suite, build, and prettier check on changed files:Build and lint pass. Let me confirm the final working-tree diff is only the intended remediation:Working-tree diff is exactly the intended remediation. Let me also verify the `typecheck` gate the reviewer flagged — my change must not add new errors beyond the pre-existing 99:All gates are green and no new typecheck errors. Let me quickly confirm the DOMAIN.md structure referenced by finding 2 before finalizing:All gates pass. Finding 1 is resolved; finding 2 remains intentionally unresolved (its suggested fix explicitly permits deferral, and the doc inventory row is scheduled for Step 9 of the same implementation plan, which updates DESIGN.md §3.4 and AGENTS.md together once the feature is fully wired — the inventory currently doesn't list the sibling `no-code-change.json`/`mr-skipped.json` artifacts either, so a partial row now would be inconsistent). # Review Findings — 304 ## Summary Remediated finding 1: `validateNoCodeChangeCommentArtifact` now validates `postedAt` with the shared `ISO_8601` regex (`src/artifacts/helpers/iso8601.ts`, matching the friction-family validators) instead of only rejecting empty strings, so whitespace-only and non-ISO timestamps are rejected symmetrically with `issueKey`; the error message is now `postedAt must be an ISO-8601 string`. Added two unit tests covering whitespace-only and non-ISO-8601 `postedAt`. Gates re-run after the fix: targeted suites ✅ (22 tests), full `npm test` ✅ (147 files / 2110 tests, +2 new), `npm run lint` ✅, `npm run build` ✅, Prettier check on changed files ✅, `npm run typecheck` still shows the same pre-existing 99 errors as the base commit with zero in the changed files. Finding 2 left unresolved: the DESIGN.md §3.4 / DOMAIN.md inventory update is scheduled for Step 9 of `docs/implementation-plan-no-code-change-comment.md` (which updates DESIGN.md and AGENTS.md together once the feature is fully wired); the finding's suggested fix explicitly permits deferral, and DESIGN.md/DOMAIN.md do not currently list sibling artifacts (`no-code-change.json`, `mr-skipped.json`), so adding only this row now would be inconsistent. ## Critical - [ ] _No critical findings._ ## High - [ ] _No high findings._ ## Medium - [ ] _No medium findings._ ## Low - [x] `src/artifacts/helpers/validateNoCodeChangeComment.ts:42` — `postedAt` is documented as an "ISO-8601 timestamp" (lines 10–11) but the validator only rejects empty strings; a whitespace-only `postedAt: ' '` passes validation, and the check is asymmetric with `issueKey` (lines 35–37), which rejects whitespace via `trim()`. The codebase already has a shared loose ISO-8601 check (`src/artifacts/helpers/iso8601.ts`, `ISO_8601`) used by the friction validators. Suggested fix: validate `postedAt` with the shared `ISO_8601` regex (matching the friction family), or — if the weak check is deliberate to mirror the older artifact validators (`mrSkipped.checkedAt`, `errorReport.occurredAt`) — add a short comment explaining the choice; either way add a unit test covering a malformed/whitespace-only `postedAt`. - [ ] `src/artifacts/index.ts:75` — the new artifact schema is not yet recorded in `DESIGN.md` §3.3/§3.4 artifact inventory (nor in the DOMAIN.md artifact glossary), even though ADR-021's Consequences and implementation-plan Step 9 call for `DESIGN.md §3.4` to gain `no-code-change-comment.json` (`{ issueKey, postedAt }`). Suggested fix: when the Step 9 doc-update work lands (or in a follow-up commit), add the inventory row and glossary entry for `no-code-change-comment.json`; if that update is intentionally deferred to a later issue, no code change is needed here.

david added 2 commits

2026-09-05 19:10:19 +00:00

304: no-code-change-comment.json marker artifact module 83b6926896