Skip to content

Fix PR comment matching for non-github-actions identities - #151

Open
svg153 wants to merge 1 commit into
Checkmarx:masterfrom
svg153:fix/pr-comment-detection-by-marker
Open

svg153 wants to merge 1 commit into
Checkmarx:masterfrom
svg153:fix/pr-comment-detection-by-marker

Conversation

@svg153

@svg153 svg153 commented Mar 19, 2026 •

Copy link
Copy Markdown

Summary

This PR updates the PR comment matching logic to identify KICS comments by their body marker (![kics-logo]() instead of matching by github-actions[bot] user.

Why

In GitHub Enterprise and custom setups, comments can be published by GitHub Apps, PAT users, or other bot identities. In those cases, matching by github-actions[bot] fails and KICS does not detect its previous comment correctly.

Behavior after this change

  • If an existing KICS comment is present (identified by the KICS marker), it is updated.
  • If no matching KICS comment exists, a new one is created.

Deletion case

If the previous KICS comment was deleted, there is nothing to update, so creating a new comment is the expected behavior. This keeps the action resilient and avoids depending on a specific author identity.

Change scope

  • src/commenter.js: remove author-login dependency and use optional chaining for body check.

Related: #53 and #126

@svg153
svg153 requested a review from a team as a code owner March 19, 2026 23:58
@svg153

svg153 commented Oct 8, 2026

Copy link
Copy Markdown
Author

Hi @cx-artur-ribeiro! Thanks again for taking care of #160/#161 via #162.

Could you or another maintainer take a look at this PR when you have a chance?

It addresses a separate GHES/custom-identity issue: when KICS posts its PR comment through an identity other than github-actions[bot], the existing author check prevents the action from finding and updating its previous comment, leading to duplicate comments. This patch matches the KICS body marker instead.

One reason this may have been "missed": #151 appears to be affected by a GitHub discovery/Search/API inconsistency.

  • The PR is open and accessible directly, and the REST PR listing includes it.
  • Searches by PR number, exact title, author (svg153) or head branch don't return it.
  • The REST PR listing filtered with head=svg153:fix/pr-comment-detection-by-marker returns an empty array, despite the direct PR resource reporting that head.
  • There's also a UI count discrepancy: the repository's top-level Pull requests tab shows 5, while the open-PR list view shows 4 and renders only four rows, with Fix PR comment matching for non-github-actions identities #151 missing.

I'm reporting the Search/API inconsistency to GitHub separately. I don't know the root cause, but it may explain why this PR has been difficult to discover.

Happy to rebase or refresh the branch against current master if that helps review.

Thanks!

@svg153

svg153 commented Oct 8, 2026

Copy link
Copy Markdown
Author

I'm reporting the Search/API inconsistency to GitHub separately. I don't know the root cause, but it may explain why this PR has been difficult to discover.

Quick update regarding the GitHub Search/API indexing issue mentioned above:

After posting my previous comment, PR #151 became visible again in the repository's pull request list.

The repository previously showed 5 open PRs in the navigation tab but only 4 in the actual list. It now correctly displays all 5, including this PR.

It looks like adding a comment may have triggered a reindex or metadata refresh, although the root cause remains unknown.

So the PR discovery issue appears to be resolved. But the "real issue", is still pending review ;).

Thanks!

@cx-artur-ribeiro

Copy link
Copy Markdown
Contributor

Hi @svg153 ,
Gracias for the contribution 🙏 !

I reviewed this and reproduced the scenario you describe.
The hardcoded github-actions[bot] author check means a comment posted by any other identity (a PAT user, a GitHub App, another bot, etc) is never matched, so each run creates a new comment.
Matching only on the ![kics-logo]( prefix fixes that and I confirmed it with a test using a non-bot author.

However, while testing this I found a second, independent cause of the same symptom.
The issues.listComments is called once without pagination, so it only returns the first 30 comments.
On a PR with more than 30 comments the existing KICS comment is missed even when the bot is the author and a new one is created. With only the author check removed that case still creates a duplicate.

I opened #163 with both changes:

  • Match the KICS comment by its body prefix only, without the author check.
  • List comments with octokit.paginate at 100 per page.

I also tested the case I was unsure about on a throwaway fork of the action.
A comment starting with ![kics-logo]( was posted by a regular user account, standing in for the PAT.
A workflow then ran the fixed postPRComment with the default GITHUB_TOKEN (issues: write and pull-requests: write).
It found the user's comment and updated it in place and there was still exactly one KICS comment afterwards.
I repeated it twice, resetting the comment in between, with the same result.

Two things I did not cover, so you know the limits:

  • Existing duplicate comments are not cleaned up, since find returns the oldest match.
  • I only tried a user-authored comment. I did not try a GitHub App installation token or GHES.
  • With read-only token permissions, the update would still be rejected, as it is today.

If you prefer, you can add the pagination change to your PR and I will close mine.
Otherwise I can merge mine and mention you and your PR in the description.

Thanks for exposing the problem and for the fix!

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.

2 participants