From 0e43a0a8b8b540758d8b963e9c5bd1eab04593d3 Mon Sep 17 00:00:00 2001 From: Vineeth N K Date: Sun, 14 Jun 2026 21:52:31 +0530 Subject: [PATCH] fix(ai-review): don't fail the whole review when inline lines can't anchor GitHub rejects an entire pulls.createReview call with 422 "Line could not be resolved" when any comment targets a line outside the PR diff, common for unsupported languages where the semantic diff falls back to whitespace filtering and line numbers no longer map to diff positions. ClearPR let that 422 fail the review and post a "Review failed" comment, losing all findings (surfaced by ClearPR reviewing its own markdown docs PR). Now postInlineComments returns whether the comments anchored. On a 422 it degrades instead of throwing, and the summary lists the findings as text (severity, path:line, body) so nothing is lost. Other errors still propagate. Adds statusCode to GitHubApiError for precise detection and a summary-builder test for the fallback path. --- src/github/domain/errors/github.errors.ts | 2 + .../build-review-summary.use-case.spec.ts | 35 +++++++++++++ .../build-review-summary.use-case.ts | 50 +++++++++++++----- .../use-cases/orchestrate-review.use-case.ts | 6 +-- src/review/domain/ports/review-poster.port.ts | 5 +- .../adapters/github-review-poster.adapter.ts | 52 +++++++++++++------ test/pipeline.e2e-spec.ts | 3 +- 7 files changed, 119 insertions(+), 34 deletions(-) diff --git a/src/github/domain/errors/github.errors.ts b/src/github/domain/errors/github.errors.ts index 3f99b70..edc18b0 100644 --- a/src/github/domain/errors/github.errors.ts +++ b/src/github/domain/errors/github.errors.ts @@ -3,9 +3,11 @@ import { DomainError } from '../../../shared/domain/errors/domain-error.base.js' export class GitHubApiError extends DomainError { readonly code = 'GITHUB_API_ERROR'; readonly isTransient: boolean; + readonly statusCode: number; constructor(message: string, statusCode: number) { super(message); + this.statusCode = statusCode; this.isTransient = statusCode >= 500; } } diff --git a/src/review/application/use-cases/build-review-summary.use-case.spec.ts b/src/review/application/use-cases/build-review-summary.use-case.spec.ts index 78e4ff3..5398f72 100644 --- a/src/review/application/use-cases/build-review-summary.use-case.spec.ts +++ b/src/review/application/use-cases/build-review-summary.use-case.spec.ts @@ -45,6 +45,41 @@ describe('BuildReviewSummaryUseCase', () => { expect(result).not.toContain('No issues found.'); }); + it('lists finding details in the summary when inline comments could not be anchored', () => { + const parsed: ParsedReview = { + summary: 'mixed', + comments: [ + { + path: 'README.md', + line: 12, + side: 'RIGHT', + severity: Severity.WARNING, + body: 'unclear wording', + }, + { path: 'docs/faq.md', line: 3, side: 'RIGHT', severity: Severity.INFO, body: 'typo' }, + ], + }; + + const anchored = useCase.execute({ diff: baseDiff, parsed, hasGuidelines: false }); + const unanchored = useCase.execute({ + diff: baseDiff, + parsed, + hasGuidelines: false, + inlineAnchored: false, + }); + + // Anchored: counts only, no per-finding detail + expect(anchored).not.toContain('unclear wording'); + expect(anchored).not.toContain('could not be anchored'); + + // Unanchored: findings rendered as text so they are not lost + expect(unanchored).toContain('could not be anchored'); + expect(unanchored).toContain('`README.md:12`'); + expect(unanchored).toContain('unclear wording'); + expect(unanchored).toContain('`docs/faq.md:3`'); + expect(unanchored).toContain('typo'); + }); + it('appends guidelines footer when hasGuidelines is true', () => { const parsed: ParsedReview = { summary: '', diff --git a/src/review/application/use-cases/build-review-summary.use-case.ts b/src/review/application/use-cases/build-review-summary.use-case.ts index 73951df..dfb9123 100644 --- a/src/review/application/use-cases/build-review-summary.use-case.ts +++ b/src/review/application/use-cases/build-review-summary.use-case.ts @@ -1,20 +1,22 @@ import { Injectable } from '@nestjs/common'; import { Severity } from '../../domain/value-objects/severity.vo.js'; -import type { ParsedReview } from '../../application/types/review-result.types.js'; +import type { + ParsedReview, + ParsedReviewComment, +} from '../../application/types/review-result.types.js'; interface BuildReviewSummaryInput { diff: { totalRawLines: number; totalSemanticLines: number; noiseReductionPct: number }; parsed: ParsedReview; hasGuidelines: boolean; + // False when inline comments could not be anchored to the diff; the findings + // are then listed in the summary so they are not lost. + inlineAnchored?: boolean; } @Injectable() export class BuildReviewSummaryUseCase { - execute({ diff, parsed, hasGuidelines }: BuildReviewSummaryInput): string { - const criticals = parsed.comments.filter((c) => c.severity === Severity.CRITICAL).length; - const warnings = parsed.comments.filter((c) => c.severity === Severity.WARNING).length; - const infos = parsed.comments.filter((c) => c.severity === Severity.INFO).length; - + execute({ diff, parsed, hasGuidelines, inlineAnchored = true }: BuildReviewSummaryInput): string { const lines = [ '## ClearPR Review', '', @@ -23,14 +25,9 @@ export class BuildReviewSummaryUseCase { ]; if (parsed.comments.length > 0) { - lines.push('### Findings'); - if (criticals > 0) lines.push(`- ${criticals} critical`); - if (warnings > 0) lines.push(`- ${warnings} warning${warnings > 1 ? 's' : ''}`); - if (infos > 0) lines.push(`- ${infos} info`); - lines.push(''); + lines.push(...this.renderFindings(parsed.comments, inlineAnchored)); } else { - lines.push('No issues found.'); - lines.push(''); + lines.push('No issues found.', ''); } if (hasGuidelines) { @@ -39,4 +36,31 @@ export class BuildReviewSummaryUseCase { return lines.join('\n'); } + + private renderFindings(comments: ParsedReviewComment[], inlineAnchored: boolean): string[] { + const criticals = comments.filter((c) => c.severity === Severity.CRITICAL).length; + const warnings = comments.filter((c) => c.severity === Severity.WARNING).length; + const infos = comments.filter((c) => c.severity === Severity.INFO).length; + + const lines = ['### Findings']; + if (criticals > 0) lines.push(`- ${criticals} critical`); + if (warnings > 0) lines.push(`- ${warnings} warning${warnings > 1 ? 's' : ''}`); + if (infos > 0) lines.push(`- ${infos} info`); + lines.push(''); + + if (!inlineAnchored) { + lines.push( + '> Inline comments could not be anchored to the diff (e.g. an unsupported language), so the findings are listed here:', + '', + ); + for (const comment of comments) { + lines.push( + `- **[${comment.severity}]** \`${comment.path}:${comment.line}\` ${comment.body}`, + ); + } + lines.push(''); + } + + return lines; + } } diff --git a/src/review/application/use-cases/orchestrate-review.use-case.ts b/src/review/application/use-cases/orchestrate-review.use-case.ts index 1d1ba4d..eab6dd9 100644 --- a/src/review/application/use-cases/orchestrate-review.use-case.ts +++ b/src/review/application/use-cases/orchestrate-review.use-case.ts @@ -163,15 +163,15 @@ export class OrchestrateReviewUseCase { review.comments = comments; // Post to GitHub - if (comments.length > 0) { - await this.reviewPoster.postInlineComments(context, comments); - } + const inlineAnchored = + comments.length > 0 ? await this.reviewPoster.postInlineComments(context, comments) : true; // Build and post summary const summary = this.buildReviewSummary.execute({ diff: diffResult, parsed, hasGuidelines: guidelines !== null, + inlineAnchored, }); await this.publishSummary(context, review, summary); diff --git a/src/review/domain/ports/review-poster.port.ts b/src/review/domain/ports/review-poster.port.ts index 32f86c3..9fcc433 100644 --- a/src/review/domain/ports/review-poster.port.ts +++ b/src/review/domain/ports/review-poster.port.ts @@ -2,7 +2,10 @@ import type { ReviewContext } from '../types/review-context.types.js'; import { type ReviewComment } from '../entities/review-comment.entity.js'; export abstract class ReviewPosterPort { - abstract postInlineComments(context: ReviewContext, comments: ReviewComment[]): Promise; + // Returns true if the inline comments were anchored to the diff, false if + // GitHub could not resolve the lines (so findings should be surfaced in the + // summary instead). Throws for any other failure. + abstract postInlineComments(context: ReviewContext, comments: ReviewComment[]): Promise; abstract postSummary(context: ReviewContext, summary: string): Promise; abstract updateSummary(context: ReviewContext, commentId: number, summary: string): Promise; abstract postProgressPlaceholder(context: ReviewContext): Promise; diff --git a/src/review/infrastructure/adapters/github-review-poster.adapter.ts b/src/review/infrastructure/adapters/github-review-poster.adapter.ts index 63467c2..2891235 100644 --- a/src/review/infrastructure/adapters/github-review-poster.adapter.ts +++ b/src/review/infrastructure/adapters/github-review-poster.adapter.ts @@ -1,31 +1,51 @@ -import { Injectable } from '@nestjs/common'; +import { Injectable, Logger } from '@nestjs/common'; import { ReviewPosterPort } from '../../domain/ports/review-poster.port.js'; import { type ReviewComment } from '../../domain/entities/review-comment.entity.js'; import { type ReviewContext } from '../../domain/types/review-context.types.js'; import { GitHubClientService } from '../../../github/infrastructure/adapters/github-client.service.js'; +import { GitHubApiError } from '../../../github/domain/errors/github.errors.js'; @Injectable() export class GitHubReviewPosterAdapter extends ReviewPosterPort { + private readonly logger = new Logger(GitHubReviewPosterAdapter.name); + constructor(private readonly githubClient: GitHubClientService) { super(); } - async postInlineComments(context: ReviewContext, comments: ReviewComment[]): Promise { - if (comments.length === 0) return; + async postInlineComments(context: ReviewContext, comments: ReviewComment[]): Promise { + if (comments.length === 0) return true; - await this.githubClient.createPullRequestReview( - parseInt(context.installationId, 10), - context.owner, - context.repo, - context.prNumber, - '', - comments.map((c) => ({ - path: c.filePath, - line: c.line, - body: `**[${c.severity}]** ${c.body}`, - side: c.side, - })), - ); + try { + await this.githubClient.createPullRequestReview( + parseInt(context.installationId, 10), + context.owner, + context.repo, + context.prNumber, + '', + comments.map((c) => ({ + path: c.filePath, + line: c.line, + body: `**[${c.severity}]** ${c.body}`, + side: c.side, + })), + ); + return true; + } catch (error) { + // GitHub rejects the whole review (422) when a comment line is not part + // of the PR diff - common for unsupported languages where the semantic + // diff falls back to whitespace filtering and line numbers do not map. + // Degrade instead of failing the review: the caller lists the findings + // in the summary. + if (error instanceof GitHubApiError && error.statusCode === 422) { + this.logger.warn( + { prNumber: context.prNumber, reason: error.message }, + 'Inline comments could not be anchored to the diff; falling back to summary findings', + ); + return false; + } + throw error; + } } async postSummary(context: ReviewContext, summary: string): Promise { diff --git a/test/pipeline.e2e-spec.ts b/test/pipeline.e2e-spec.ts index c72278e..49d83f1 100644 --- a/test/pipeline.e2e-spec.ts +++ b/test/pipeline.e2e-spec.ts @@ -287,9 +287,10 @@ class FakeLlmProvider extends LlmProviderPort { class FakeReviewPoster extends ReviewPosterPort { private nextCommentId = 1000; - async postInlineComments(_context: ReviewContext, comments: ReviewComment[]): Promise { + async postInlineComments(_context: ReviewContext, comments: ReviewComment[]): Promise { await Promise.resolve(); capture.inlineComments.push(comments); + return true; } async postSummary(_context: ReviewContext, summary: string): Promise { await Promise.resolve();