Skip to content

fix(code-scan): route file-only findings to general comments#9556

Open
serhiizghama wants to merge 2 commits into
promptfoo:mainfrom
serhiizghama:fix/file-only-findings-as-general-comments
Open

fix(code-scan): route file-only findings to general comments#9556
serhiizghama wants to merge 2 commits into
promptfoo:mainfrom
serhiizghama:fix/file-only-findings-as-general-comments

Conversation

@serhiizghama
Copy link
Copy Markdown

Problem

The fallback PR-posting path in the code-scan action drops comments when the scan server returns file-scoped findings with line: null.

prepareComments() in src/codeScan/util/github.ts classifies every comment that has a file field as a lineComment. When toReviewComment() maps such a comment for pulls.createReview(), comment.line becomes undefined. GitHub rejects inline review comments that have a path but no valid line number, so the entire fallback post silently fails.

Traced in issue #9422:

  • CommentSchema (and SARIF emitter) explicitly allow line: null for file-only findings
  • The prepareComments split was treating c.file && c.finding as sufficient for a line comment
  • The fix was already applied to the action layer in commit f6af1ec but the root prepareComments helper was not updated

Solution

Tighten the lineComments predicate to require a non-null line:

// before
const lineComments = sortedComments.filter((c) => c.file && c.finding);

// after
const lineComments = sortedComments.filter((c) => c.file && c.line != null && c.finding);

Widen generalComments to catch file-only findings:

// before
const generalComments = sortedComments.filter(
  (c) => !c.file && c.finding && c.severity !== CodeScanSeverity.NONE,
);

// after
const generalComments = sortedComments.filter(
  (c) => (!c.file || c.line == null) && c.finding && c.severity !== CodeScanSeverity.NONE,
);

File-only findings now go through issues.createComment() as general PR comments, which accepts any finding without a line location.

Testing

Added a test case to test/codeScans/util/github.test.ts that verifies a comment with file set and line: null ends up in generalComments, not lineComments.

All existing tests continue to pass (npx vitest run test/codeScans/util/github.test.ts).

When line is null, GitHub rejects inline review comments that have a path
but no line number. File-only findings must go to generalComments instead
of lineComments so the fallback posting path succeeds.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3b322a40e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const lineComments = sortedComments.filter((c) => c.file && c.line != null && c.finding);
const generalComments = sortedComments.filter(
(c) => !c.file && c.finding && c.severity !== CodeScanSeverity.NONE,
(c) => (!c.file || c.line == null) && c.finding && c.severity !== CodeScanSeverity.NONE,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include file paths for file-only general comments

When a finding has file set but line: null, this predicate now sends it through postGeneralComments(), whose body is built by buildCommentBody(comment) and never includes comment.file; inline comments get their location from GitHub, but these issue comments do not. For file-scoped findings whose finding text does not repeat the path (the schema carries the path separately, and SARIF/CLI output also render it separately), the PR comment is posted without the affected file, making the result hard to act on. Consider preserving the file path in the general-comment body for this newly routed case.

Useful? React with 👍 / 👎.

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