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();