feat: fail the release-notes check when a PR body has multiple Notes: lines

[claude]

Requested by David Sanders · Slack thread

Before: A PR body with several one-line Notes: entries passed the release-notes check, but Clerk only ever read the first one. The remaining lines were silently dropped from the frozen release-notes comment and from the bodies trop generates for backports, so notes went missing without anyone being told.

After: When a PR body contains more than one Notes: line, Clerk posts a red release-notes commit status ("Multiple Notes: lines; use one Notes: with a bulleted list") and stops there, so the author fixes the body before merge. The supported multi-line form, a bare Notes: followed by * item bullets, is unaffected and still counts as a single note.

This makes the release-notes check fail on repeated Notes: lines instead of silently keeping only the first.

How: Adds countNotesInPRBody() to src/note-utils.ts, which counts lines that start a Notes: block in either supported form (/^Notes:(?: |\r?\n)/gim, case-insensitive like the existing matchers). submitFeedbackForPR in src/index.ts checks it before the existing found/missing handling and posts the failure status when the count is greater than 1; the dependabot and build: branches are untouched. Tests cover null/no-notes, the single-line form, the bulleted form, repeated lines (including \r\n and lowercase notes:), and the failure status end-to-end via the probot/nock harness. The README now says that multiple Notes: lines fail the check and points at the bulleted form.

🤖 Generated with Claude Code

https://claude.ai/code/session_01P2w3uMqUQWk6Ke7HwyqUGY

---

Generated by Claude Code

[claude]

Beyond the inline findings, I also checked whether running the multiple-Notes: check ahead of the dependabot/build: bypass branches (src/index.ts) could misfire on auto-generated PR bodies — those bodies don't naturally contain repeated Notes: lines, so the check ordering isn't currently exploitable there.

Extended reasoning...

Three confirmed findings are already queued as inline comments covering real correctness gaps: the updatePRBodyForNoNotes append branch can produce a body with two Notes: lines for malformed existing notes, countNotesInPRBody's regex over-counts bare Notes:\n headings that aren't followed by bullets (inconsistent with findNoteInPRBody), and the new multi-Notes guard in submitFeedbackForPR runs unconditionally, so it can suppress the merge-time release-notes comment entirely for a merged PR with a benign-looking body pattern. I independently read src/index.ts and src/note-utils.ts and confirmed the code matches these descriptions, including the exact regex mismatch between countNotesInPRBody (/^Notes:(?: |\r?\n)/gim) and findNoteInPRBody's stricter bullet-list matcher.

I additionally checked the dependabot/build:-prefix bypass ordering (the check happens before those branches) to see if it introduces a distinct false-positive path for auto-generated PRs; dependabot and build: PR bodies are template-generated and don't contain user-authored Notes: text, so this ordering isn't currently a practical exploit path, unlike the three confirmed issues. Given the confirmed findings touch the core correctness of the new feature and one directly affects the merge-time comment flow, a human should review before merging.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🔴 src/note-utils.ts — updatePRBodyForNoNotes's append branch only checks for the oneline Notes: text form before appending Notes: none; a build:/dependabot PR body that already has a bare/malformed Notes: line (no space+content, no bullets) still takes this branch, producing a body with two Notes: lines. GitHub fires pull_request.edited for the bot's own pulls.update() call, re-running submitFeedbackForPR, and the new countNotesInPRBody now sees 2 and posts the failure status on a PR the bot itself just tried to auto-clear. Fix: before appending, check whether the body already contains any Notes:-starting line (not just the oneline-with-content form) and replace/strip it instead of blindly appending a second one.

  Extended reasoning...

  A build:-prefixed PR body contains Notes: followed by a blank line and free (non-bulleted) text, e.g. ...\n\nNotes:\n\nsome text\n. findNoteInPRBody's oneline regex needs Notes: with a space+content and its multiline regex needs * bullets right after Notes:\n, so neither matches and it returns null, while countNotesInPRBody(original) = 1 (matches Notes:\n via the \r?\n alt). Since releaseNotes is falsy and the PR title starts with build:, index.ts calls updatePRBodyForNoNotes(body); its if test (note-utils.ts:11) requires the oneline-with-content form and fails to match anywhere in the body, so the else branch (line 25) runs and appends \n\n---\n\nNotes: none, yielding a body with two Notes:-starting lines. This body is persisted via pulls.update, GitHub emits pull_request.edited, and probotRunner re-invokes submitFeedbackForPR for the still-open PR. countNotesInPRBody(newBody) now returns 2, so the new guard (src/index.ts:22-33) posts a failure commit status ('Multiple Notes: lines...') even though before this diff the same double-Notes:…

  Verification: normal. Root cause is a regex-scope mismatch introduced by this change. countNotesInPRBody (src/note-utils.ts:38, /^Notes:(?: |\r?\n)/gim) counts a bare Notes:\n line via its \r?\n alternative, but that same bare line is NOT recognized by findNoteInPRBody (src/note-utils.ts:44-45 require Notes: +inline content or * bullets) nor by updatePRBodyForNoNotes's test (src/note-utils.ts:11…

[claude]

Addressed by 8bb8863: a bare Notes: line without bullets no longer counts, so the body updatePRBodyForNoNotes produces (Notes: + Notes: none) counts as 1 and no longer trips the guard; updatePRBodyForNoNotes itself is unchanged as pre-existing behavior.

---

Generated by Claude Code

[claude]

Commit 8bb8863 correctly addresses both issues I raised in my earlier review (the bare-Notes:-without-bullets false positive, and running the guard at merge time). While re-checking, I found one more narrow edge case worth a human look before merge: countNotesInPRBody's multiline branch anchors on ^ (which, under the m flag, also matches the very start of the string), whereas findNoteInPRBody's multiline matcher requires an actual preceding \r?\n before Notes:. So a body beginning with Notes: immediately followed by bullets (nothing before it) plus one genuine Notes: ... line elsewhere is counted as 2 notes and fails the check, even though findNoteInPRBody only ever recognizes the second one as a real note — this is already implicit in the test suite's own assertion for 'Notes:\n* One.\n\nNotes: Two.\n' expecting a count of 2.

Extended reasoning...

I confirmed both of my previously-flagged issues are fixed in commit 8bb8863: submitFeedbackForPR now guards the multiple-Notes check with !shouldComment (skipping it at merge time), and countNotesInPRBody's multiline alternative now requires (?:\*.+...)+ bullets after a bare Notes:, matching findNoteInPRBody's recognition of that form. dsanders11 has also approved, and no other reviewer objections are outstanding.

However, tracing the regexes character-by-character (sandbox tooling here doesn't permit running node/python, so I did this by hand against the exact test-suite strings), I found that countNotesInPRBody's multiline branch uses ^ under the gim flags, which matches both the start of a line after \n and the absolute start of the string. findNoteInPRBody's multiline matcher (src/note-utils.ts:47), by contrast, requires a literal preceding \r?\n before Notes: — it has no ^-style alternative for start-of-string. This means a bare Notes:+bullets block positioned as the very first characters of the PR body is counted by countNotesInPRBody but never recognized by findNoteInPRBody. When such a body also contains one genuine Notes: text line elsewhere, countNotesInPRBody returns 2 (confirmed by the test at test/note.test.ts asserting countNotesInPRBody('Notes:\n* One.\n\nNotes: Two.\n') equals 2), triggering the new failure status, while findNoteInPRBody would only ever have extracted "Two." as the single real note. This is a residual asymmetry between the two functions' recognition logic that the doc comment above countNotesInPRBody claims not to have.

This is narrow (requires the body to start with zero characters before Notes:) but real, and it directly works against the PR's own goal of only failing bodies with genuinely ambiguous/duplicate notes. Given this is now the third round of edge-case correctness issues found in this exact function, and the trigger condition is plausible for minimal/backport-style PR bodies, I think it merits a human look rather than outright approval, even though my two previous concerns are resolved and no other reviewer has objected.

[claude]

Fixed in 1c9c54b: findNoteInPRBody's bulleted matcher now accepts a Notes: block at the start of the body, same as its one-line form, so both functions recognize the same notes.

---

Generated by Claude Code

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the new inline finding, I also checked whether the dependabot/build: Notes: none auto-insertion could combine with a later human-added Notes: line to trip the new multiple-Notes failure — that ordering ambiguity already existed before this change (whichever Notes: block happened to come first was silently picked), so this change surfacing it as a failure instead is not a regression.

Extended reasoning...

This run's own two previously-raised findings (bare Notes: without bullets being double-counted, and the multi-Notes guard incorrectly firing on the merge-time shouldComment=true path) have both been fixed in the current code: countNotesInPRBody's regex now requires bullet lines to follow a bare Notes:, mirroring findNoteInPRBody, and the guard in submitFeedbackForPR is now conditioned on !shouldComment. A new confirmed finding was surfaced this run (the unfilled GitHub PR-template Notes: <!-- ... --> placeholder being counted as a second real note, since countNotesInPRBody doesn't strip HTML comments the way findNoteInPRBody does before deciding a match is empty) — that is being posted as an inline comment, so I'm not restating it here. I additionally examined the dependabot/build-prefix interaction candidate that the earlier investigation flagged and ruled out: since the pre-existing findNoteInPRBody already resolved ambiguity between multiple Notes: blocks by picking whichever appeared first in body order (a pre-existing latent issue, not introduced by this diff), the new check's tendency to instead fail loudly in that same ambiguous scenario is arguably an improvement rather than a regression, so it doesn't warrant a separate finding.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.