-
Notifications
You must be signed in to change notification settings - Fork 460
Mitigate safe-outputs review failures when inline comment anchors are partially invalid #47356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -664,6 +664,32 @@ function createReviewBuffer() { | |
| // body-only review so that the overall review (and its footer body) is still submitted | ||
| // successfully. Matches both "Line could not be resolved" and "Path could not be resolved". | ||
| if ((errorMessage.includes("Line could not be resolved") || errorMessage.includes("Path could not be resolved")) && comments.length > 0) { | ||
| const unresolvableCommentIndices = extractUnresolvableCommentIndices(error, comments.length); | ||
| if (unresolvableCommentIndices.length > 0 && unresolvableCommentIndices.length < comments.length) { | ||
| const unresolvableCommentIndexSet = new Set(unresolvableCommentIndices); | ||
| const resolvableComments = comments.filter((_, index) => !unresolvableCommentIndexSet.has(index)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test only covers the "middle comment unresolvable" scenario. Missing edge cases: all comments unresolvable (should fall through to body-only), exactly one comment unresolvable at index 0, and the partial-retry itself failing (the inner 💡 Missing test cases
@copilot please address this. |
||
| const unresolvableComments = comments.filter((_, index) => unresolvableCommentIndexSet.has(index)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partial retry may silently drop the review body when 💡 Detailsbody: appendUnanchoredCommentsSection(
typeof requestParams.body === "string" ? requestParams.body : "",
unresolvableComments
),When Suggested fix: body: appendUnanchoredCommentsSection(requestParams.body ?? "", unresolvableComments),Or assert the invariant that |
||
| core.warning( | ||
| `PR review submission failed due to unresolvable comment line(s): ${errorMessage}. ` + | ||
| `Retrying with ${resolvableComments.length} resolvable inline comment(s); ` + | ||
| `${unresolvableComments.length} comment(s) will be appended to the review body.` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Detailsreturn buildReviewSuccessResult(review, event, resolvableComments.length, afterState);
Suggested fix: pass the total original comment count, or add a separate field (e.g. |
||
| ); | ||
| try { | ||
| const partialParams = { | ||
| ...requestParams, | ||
| comments: resolvableComments, | ||
| body: appendUnanchoredCommentsSection(typeof requestParams.body === "string" ? requestParams.body : "", unresolvableComments), | ||
| }; | ||
| const { data: review } = await createReviewWithRetry(partialParams); | ||
| await maybeSupersedeOlderReviews(review.id); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] 💡 Suggested fixReturn total comment count to preserve the existing contract: return buildReviewSuccessResult(review, event, resolvableComments.length + unresolvableComments.length, afterState);If @copilot please address this. |
||
| const afterState = await fetchAfterStateIfAvailable(); | ||
| core.info(`Created PR review #${review.id} (partial-anchor fallback): ${review.html_url}`); | ||
| return buildReviewSuccessResult(review, event, resolvableComments.length, afterState); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment_count undercounts preserved comments in partial-anchor fallback On the partial-anchor success path, Consider returning @copilot please address this. |
||
| } catch (partialRetryError) { | ||
| core.warning(`Failed to submit partially anchored PR review: ${getErrorMessage(partialRetryError)}. Falling back to body-only review.`); | ||
| } | ||
|
Comment on lines
+688
to
+690
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in commit |
||
| } | ||
|
|
||
| core.warning(`PR review submission failed due to unresolvable comment line(s): ${errorMessage}. Retrying as body-only review.`); | ||
| try { | ||
| const bodyOnlyParams = { ...requestParams }; | ||
|
|
@@ -825,6 +851,56 @@ function createPrReviewBufferRegistry() { | |
| } | ||
|
|
||
| module.exports = { createReviewBuffer, createPrReviewBufferRegistry }; | ||
|
|
||
| /** | ||
| * Parse API validation errors to identify specific inline comments that could not be resolved. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] @copilot please address this. |
||
| * Returns 0-based indexes corresponding to items in the buffered comments array. | ||
| * | ||
| * @param {unknown} error | ||
| * @param {number} totalComments | ||
| * @returns {number[]} | ||
| */ | ||
| function extractUnresolvableCommentIndices(error, totalComments) { | ||
| const indices = new Set(); | ||
| // prettier-ignore | ||
| const errorAsAny = /** @type {any} */ (error); | ||
| const candidateErrors = [errorAsAny, errorAsAny?.originalError, errorAsAny?.cause]; | ||
|
|
||
| for (const candidate of candidateErrors) { | ||
| if (!candidate || typeof candidate !== "object") { | ||
| continue; | ||
| } | ||
| // prettier-ignore | ||
| const candidateAsAny = /** @type {any} */ (candidate); | ||
| const apiErrors = candidateAsAny.response && candidateAsAny.response.data && Array.isArray(candidateAsAny.response.data.errors) ? candidateAsAny.response.data.errors : null; | ||
| if (!apiErrors) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const apiError of apiErrors) { | ||
| const field = typeof apiError?.field === "string" ? apiError.field : ""; | ||
| const message = typeof apiError?.message === "string" ? apiError.message : ""; | ||
| if (!message.includes("Line could not be resolved") && !message.includes("Path could not be resolved")) { | ||
| continue; | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] The field regex 💡 Suggested fixconst fieldMatch = field.match(/^comments\[(\d+)\]/);This anchors to the exact GitHub format and removes the ambiguous dot-notation branch. Add a test case with @copilot please address this. |
||
| const fieldMatch = field.match(/comments(?:\[|\.)(\d+)(?:\]|\.|$)/); | ||
| if (!fieldMatch) { | ||
| continue; | ||
| } | ||
|
|
||
| const parsedIndex = Number.parseInt(fieldMatch[1], 10); | ||
| if (!Number.isInteger(parsedIndex) || parsedIndex < 0 || parsedIndex >= totalComments) { | ||
| continue; | ||
| } | ||
|
|
||
| indices.add(parsedIndex); | ||
| } | ||
| } | ||
|
|
||
| return Array.from(indices).sort((a, b) => a - b); | ||
| } | ||
|
|
||
| /** | ||
| * Append a fallback section that preserves inline comment content when comments cannot be anchored. | ||
| * @param {string} reviewBody | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial retry may still fail silently if the GitHub API short-circuits its error list — the 422 response may not enumerate every unresolvable index, leaving undetected bad comments in
resolvableComments.💡 Details
The GitHub API often short-circuits validation after the first error in a batch. If there are two unresolvable comments but the 422 response only identifies index
1, then index3(also bad) remains inresolvableComments. The partial retry will fail again, the catch block fires, a warning is logged, and then the function falls through to the existing body-only fallback — which is correct but means the partial retry consumed an extra round-trip for nothing, and the user-visible warning is confusing.This is an inherent API limitation, but it's worth documenting with a comment so future maintainers don't assume the index list is exhaustive. It also means the test case (which mocks a single-index error) doesn't cover the more common real-world scenario.