Mitigate safe-outputs review failures when inline comment anchors are partially invalid#47356
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47356 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Improves safe-output review recovery by preserving valid inline comments when only some anchors fail.
Changes:
- Extracts invalid comment indexes from GitHub validation errors.
- Retries reviews with valid inline comments and moves invalid ones into the body.
- Adds coverage for successful partial fallback.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/pr_review_buffer.cjs |
Implements partial-anchor fallback logic. |
actions/setup/js/pr_review_buffer.test.cjs |
Tests successful mixed inline/body retry. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| } catch (partialRetryError) { | ||
| core.warning(`Failed to submit partially anchored PR review: ${getErrorMessage(partialRetryError)}. Falling back to body-only review.`); | ||
| } |
There was a problem hiding this comment.
Added in commit 3b8cdb2. The new test ("should fall back to body-only with all comments when partial-anchor retry also fails") uses three mock calls: the first fails with an indexed-error response, the second (partial retry) also rejects, and the third (body-only) succeeds. It then asserts that mockGithub.rest.pulls.createReview was called exactly three times, the third call has no comments property, and the body contains all three original comments in the fallback section.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Partial-anchor fallback for inline comment resolution
The approach is well-reasoned and the test coverage is solid. One non-blocking issue found.
What works well
extractUnresolvableCommentIndicescorrectly walkserror,error.originalError, anderror.causefor robustness.- The regex
/comments(?:\[|\.)(\d+)(?:\]|\.|$)/handles both bracket and dot notation for field names. - Bounds-checking the parsed index against
totalCommentsprevents out-of-range access. - The fallback chain (partial → body-only) correctly preserves existing behavior when partial retry also fails.
- Test directly validates the retry call arguments and body content.
One issue (see inline comment): comment_count in the success result only counts resolvable inline comments, not the unresolvable ones preserved in the body — see inline comment on line 687.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 21.2 AIC · ⌖ 4.59 AIC · ⊞ 5K
| await maybeSupersedeOlderReviews(review.id); | ||
| const afterState = await fetchAfterStateIfAvailable(); | ||
| core.info(`Created PR review #${review.id} (partial-anchor fallback): ${review.html_url}`); | ||
| return buildReviewSuccessResult(review, event, resolvableComments.length, afterState); |
There was a problem hiding this comment.
comment_count undercounts preserved comments in partial-anchor fallback
On the partial-anchor success path, buildReviewSuccessResult is called with resolvableComments.length as the comment count. But unresolvable comments are not lost — they are preserved in the review body. Callers relying on comment_count will see a lower number than actual total feedback delivered, skewing any downstream reporting/metrics.
Consider returning comments.length (the full original count) or adding separate anchored_comment_count / unanchored_comment_count fields to the result.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a correctness issue and coverage gaps.
📋 Key Themes & Highlights
Key Themes
comment_countunder-reports (line 684): the success result counts only resolvable inline comments, silently dropping unresolvable ones from the metric.- Regex edge case (line 886):
/comments(?:\[|\.)(\d+)(?:\]|\.|$)/fails to match the bare"comments[1]"field format that GitHub actually emits in 422 responses — the trailing group requires a character after]. - Missing test coverage (line 670): the inner
catchpath (partial retry also fails → fall through to body-only) and the "all comments unresolvable" branch have no tests. - Function placement (line 856):
extractUnresolvableCommentIndicesis declared aftermodule.exports, which works due to hoisting but is fragile.
Positive Highlights
- ✅ Clean two-phase fallback strategy (partial-anchor → body-only) — well-structured and preserves the existing safety net.
- ✅ Error path is well-logged with actionable
core.warningmessages. - ✅ New test covers the happy-path partial-anchor scenario clearly with Arrange/Act/Assert structure.
- ✅
candidateErrorschain (error,originalError,cause) is a solid defensive pattern for Octokit error shapes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 36.8 AIC · ⌖ 4.96 AIC · ⊞ 6.7K
Comment /matt to run again
| body: appendUnanchoredCommentsSection(typeof requestParams.body === "string" ? requestParams.body : "", unresolvableComments), | ||
| }; | ||
| const { data: review } = await createReviewWithRetry(partialParams); | ||
| await maybeSupersedeOlderReviews(review.id); |
There was a problem hiding this comment.
[/diagnosing-bugs] comment_count in the success result reflects only resolvable inline comments, not the unresolvable ones moved to the review body — callers relying on this count may see a misleadingly low number.
💡 Suggested fix
Return total comment count to preserve the existing contract:
return buildReviewSuccessResult(review, event, resolvableComments.length + unresolvableComments.length, afterState);If buildReviewSuccessResult distinguishes inline vs body counts, thread both through explicitly.
@copilot please address this.
| if (!message.includes("Line could not be resolved") && !message.includes("Path could not be resolved")) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] The field regex /comments(?:\[|\.)(\d+)(?:\]|\.|$)/ accepts dot-notation (comments.1.line) which GitHub's API does not emit — but more importantly it won't match comments[1] (no trailing char) when the field is exactly "comments[1]". The trailing group (?:\]|\.|$) requires a char after ], so a bare "comments[1]" (common in GitHub 422 payloads) silently drops the index.
💡 Suggested fix
const fieldMatch = field.match(/^comments\[(\d+)\]/);This anchors to the exact GitHub format and removes the ambiguous dot-notation branch.
Add a test case with field: "comments[1]" (no trailing content) to cover the regression.
@copilot please address this.
| 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)); |
There was a problem hiding this comment.
[/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 catch path).
💡 Missing test cases
- All comments unresolvable —
unresolvableCommentIndices.length === comments.lengthshould skip partial retry and fall through to body-only. - Index 0 unresolvable — verifies off-by-one safety in the filter.
- Partial retry also throws — the inner
catchpath logs a warning and falls through to body-only; this path has no test coverage.
@copilot please address this.
| module.exports = { createReviewBuffer, createPrReviewBufferRegistry }; | ||
|
|
||
| /** | ||
| * Parse API validation errors to identify specific inline comments that could not be resolved. |
There was a problem hiding this comment.
[/diagnosing-bugs] extractUnresolvableCommentIndices is defined after module.exports — it is hoisted as a function declaration so it works today, but this placement is non-idiomatic and risks a future refactor breaking hoisting (e.g. if the function is converted to a const arrow). Move it above module.exports alongside the other helpers.
@copilot please address this.
There was a problem hiding this comment.
Review: Partial inline-anchor fallback
Non-blocking observations — the fallback chain is sound but has a few rough edges worth addressing before this pattern is extended further.
📋 Findings (3 medium)
-
Line 671 — review body may be silently dropped: the
typeof body === 'string'guard replacesundefined/nullwith"", losing the built review body. Use?? ""instead, or assert the invariant. -
Line 675 —
comment_countunderstated:resolvableComments.lengthexcludes demoted body-fallback comments. Callers expecting total count will see a misleading value. -
Line 667 — API may not enumerate all unresolvable indices: GitHub 422 responses can short-circuit after the first error. When they do, the partial retry will include additional bad comments, fail again, and fall through to body-only — which is safe but wastes a round-trip and emits a confusing warning. Worth a comment so future maintainers don't assume the index list is complete.
The implementation is otherwise correct: the Set-based filter, regex parsing, fallback chain ordering, and test coverage for the happy path all look good.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 40 AIC · ⌖ 4.88 AIC · ⊞ 5.7K
Comment /review to run again
| if (unresolvableCommentIndices.length > 0 && unresolvableCommentIndices.length < comments.length) { | ||
| const unresolvableCommentIndexSet = new Set(unresolvableCommentIndices); | ||
| const resolvableComments = comments.filter((_, index) => !unresolvableCommentIndexSet.has(index)); | ||
| const unresolvableComments = comments.filter((_, index) => unresolvableCommentIndexSet.has(index)); |
There was a problem hiding this comment.
Partial retry may silently drop the review body when requestParams.body is not a string — the empty-string fallback discards the entire review body.
💡 Details
body: appendUnanchoredCommentsSection(
typeof requestParams.body === "string" ? requestParams.body : "",
unresolvableComments
),When requestParams.body is undefined or null, this passes "" to appendUnanchoredCommentsSection, silently discarding the AI-generated review summary. The existing body-only fallback at line 686 avoids this problem by spreading requestParams directly.
Suggested fix:
body: appendUnanchoredCommentsSection(requestParams.body ?? "", unresolvableComments),Or assert the invariant that body is always a string by this point.
| 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.` |
There was a problem hiding this comment.
comment_count in the success result is understated — only resolvableComments.length is reported, not the total number of comments processed.
💡 Details
return buildReviewSuccessResult(review, event, resolvableComments.length, afterState);unresolvableComments were still submitted as part of the review body, so they are not lost — but any caller that consumes result.comment_count to validate, log, or display how many comments were reviewed will see a count that excludes the demoted comments. This can silently mislead downstream logic (e.g. "expected N comments, got M") or surface confusing metrics.
Suggested fix: pass the total original comment count, or add a separate field (e.g. anchored_comment_count / body_fallback_comment_count) so callers can distinguish the two.
| // 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); |
There was a problem hiding this comment.
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 index 3 (also bad) remains in resolvableComments. 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.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
Excellent work on this PR! 👏 You have tackled a tricky edge case in PR review submission where some inline comments fail to anchor while others succeed. What stands out:
This is exactly the kind of resilient error handling that makes for a great user experience — reviewers wontlosevaluablefeedbackwhenGitHubs validation is finicky about line numbers or paths. The PR is ready for review! ✅
|
|
🎉 This pull request is included in a new release. Release: |
Safe-outputs review submission can fail entirely when a batched
pulls.createReviewincludes at least one unresolvable inline anchor (422: "Line could not be resolved"/"Path could not be resolved"). This change preserves valid inline feedback by degrading only the invalid subset instead of collapsing the whole review to body-only.Failure mode addressed
submitReviewpreviously retried as fully body-only whenever any inline anchor was unresolvable.Fallback strategy upgrade
comments[1].line).Implementation scope
actions/setup/js/pr_review_buffer.cjssubmitReviewto prefer partial anchoring.Coverage
actions/setup/js/pr_review_buffer.test.cjs