Skip to content

fix: use comment.body for noop detection and set per_page: 100#833

Merged
kitsuyui merged 2 commits into
mainfrom
fix/audit-implicit-behavior-get-managed-comment-body-field-001
May 23, 2026
Merged

fix: use comment.body for noop detection and set per_page: 100#833
kitsuyui merged 2 commits into
mainfrom
fix/audit-implicit-behavior-get-managed-comment-body-field-001

Conversation

@kitsuyui
Copy link
Copy Markdown
Owner

Summary

Two related issues in getManagedComment (src/github.ts):

  1. Wrong field for noop detectiongetManagedComment stored
    comment.body_text in the returned Comment.body, but
    createLuckyCommentAction compares pastComment.body against the
    full Markdown string (which includes the <!-- happy-commit -->
    HTML comment marker). body_text is the plain-text rendering with
    HTML comments stripped, so this comparison never matched and the
    noop branch was unreachable. Fix: use comment.body instead.

  2. Missing per_page on listCommentsoctokit.issues.listComments
    was called without per_page, defaulting to 30 items. On PRs with
    more than 30 comments the managed comment could be missed, causing a
    duplicate to be created. Fix: add per_page: 100.

Changes

  • src/github.ts: comment.body_text || ''comment.body || ''
  • src/github.ts: add per_page: 100 to listComments
  • src/github.spec.ts: update listComments expectation to include per_page: 100

Verification

  • All 40 tests pass (bun run test)
  • Lint clean (bun run lint)
  • Build succeeds (bun run build)
  • madge: No circular dependencies found

Trade-offs

per_page: 100 covers most real-world PRs. Full pagination (octokit.paginate)
would handle the extreme edge case of 100+ comments but adds complexity; it can
be addressed separately if needed.

comment.body_text strips HTML comments, so the COMMENT_MARKER
(<!-- happy-commit -->) was never present in it. This caused
createLuckyCommentAction to always take the update path instead
of noop when the comment body was unchanged.

Also add per_page: 100 to listComments so the managed comment
is not silently missed on PRs with more than 30 comments.
@kitsuyui kitsuyui force-pushed the fix/audit-implicit-behavior-get-managed-comment-body-field-001 branch from 8366a81 to 5dd7344 Compare May 23, 2026 16:42
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 23, 2026

🎉 Happy commit!

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 23, 2026

gh-counter

PR gate

Removed Added +/-
TODO/FIXME 0 0 0

Repo dashboard

main (8b69839) #833 (4854cc1) +/-
TODO/FIXME 0 0 0

Reported by gh-counter

@github-actions
Copy link
Copy Markdown

Code Metrics Report

main (8b69839) #833 (4854cc1) +/-
Coverage 99.1% 99.1% 0.0%
Code to Test Ratio 1:0.5 1:0.5 -0.1
Test Execution Time 2s 1s -1s
Details
  |                     | main (8b69839) | #833 (4854cc1) | +/-  |
  |---------------------|----------------|----------------|------|
  | Coverage            |          99.1% |          99.1% | 0.0% |
  |   Files             |              5 |              5 |    0 |
  |   Lines             |            115 |            115 |    0 |
  |   Covered           |            114 |            114 |    0 |
- | Code to Test Ratio  |          1:0.5 |          1:0.5 | -0.1 |
  |   Code              |           1509 |           1511 |   +2 |
+ |   Test              |            833 |            834 |   +1 |
+ | Test Execution Time |             2s |             1s |  -1s |

Code coverage of files in pull request scope (100.0% → 100.0%)

Files Coverage +/- Status
src/github.ts 100.0% 0.0% modified

Reported by octocov

@github-actions
Copy link
Copy Markdown

gh-build-size

Target main #833 +/-
Total action artifacts 23,296,622 B 23,296,665 B +43 B
Runtime bundle 1,153,256 B 1,153,269 B +13 B
Source maps 17,127,708 B 17,127,738 B +30 B

Reported by gh-build-size

@kitsuyui kitsuyui merged commit 7382600 into main May 23, 2026
5 checks passed
@kitsuyui kitsuyui deleted the fix/audit-implicit-behavior-get-managed-comment-body-field-001 branch May 23, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant