diff --git a/src/core/review/prompts/validator.ts b/src/core/review/prompts/validator.ts index 2d72935a..90bf6b3d 100644 --- a/src/core/review/prompts/validator.ts +++ b/src/core/review/prompts/validator.ts @@ -74,7 +74,11 @@ through the ${t.platformName} API, then updates the ${t.trackingCommentName ?? " * Anything you leave out of \`${validatedPath}\`, or mark as anything other than \`"approved"\`, will never reach the ${t.entityNoun}. That file is the whole contract. * An approved comment without a usable line anchor cannot be posted, so make sure - every approved comment keeps its \`path\` and \`line\`. + every approved comment keeps its \`path\`, \`side\`, and \`line\` (\`side: "RIGHT"\` + anchors to the new file line, \`side: "LEFT"\` anchors to the old file line). +* A line anchor that is not part of the diff (neither added, removed, nor context + inside a hunk) cannot be posted inline and degrades to a plain ${t.entityNoun} comment — + prefer re-anchoring such findings to the nearest related changed line. `; } diff --git a/src/core/review/tracking/types.ts b/src/core/review/tracking/types.ts index 0cead65d..d82af086 100644 --- a/src/core/review/tracking/types.ts +++ b/src/core/review/tracking/types.ts @@ -29,7 +29,9 @@ export type ReviewTrackingTelemetry = { export type ReviewPostOutcome = { /** Inline comments successfully posted. */ posted?: number | null; - /** Approved comments the API refused to anchor. */ + /** Approved comments posted as plain notes (line outside the diff). */ + fallbackPosted?: number | null; + /** Approved comments that failed to post (inline discussion + note fallback). */ failed?: number | null; /** Approved comments dropped before the API (malformed/no anchor). */ skipped?: number | null; diff --git a/src/create-prompt/terminology.ts b/src/create-prompt/terminology.ts index d3b2ef2f..60112472 100644 --- a/src/create-prompt/terminology.ts +++ b/src/create-prompt/terminology.ts @@ -18,7 +18,10 @@ export const GITHUB_TERMINOLOGY: ReviewTerminology = { repoExample: "owner/repo", pathFieldDescription: 'Relative file path (e.g., "src/index.ts")', lineFieldDescription: - "Target line number (single-line) or end line number (multi-line). Must be ≥ 0.", + "Target line number (single-line) or end line number (multi-line). Must be ≥ 0. " + + "Must be a line that appears in the PR diff; GitHub rejects inline comments on lines " + + "outside the diff, so a finding about untouched code should anchor to the nearest " + + "related changed line instead.", mutationToolForbiddance: "(inline comments, submit review, delete/minimize/reply/resolve, etc.)", submitReviewToolName: "github_pr___submit_review", diff --git a/src/entrypoints/gitlab-post-review.ts b/src/entrypoints/gitlab-post-review.ts index 9b9fe42f..94da0f02 100644 --- a/src/entrypoints/gitlab-post-review.ts +++ b/src/entrypoints/gitlab-post-review.ts @@ -20,7 +20,7 @@ import * as fs from "fs/promises"; import * as path from "path"; import { setupGitlabToken } from "../gitlab/token"; import { GitlabClient } from "../gitlab/api/client"; -import type { GitlabPosition } from "../gitlab/types"; +import type { GitlabMrDiff, GitlabPosition } from "../gitlab/types"; import { promptsDir, stateFilePath, @@ -41,6 +41,8 @@ export type ReviewComment = { export type PostResults = { posted: number; + /** Approved comments that could not anchor inline and went out as notes. */ + fallbackPosted: number; approved: number; rejected: number; failed: number; @@ -172,12 +174,84 @@ export function parseValidatedReview(raw: string): ParsedValidatedReview { }; } +// --- diff index ------------------------------------------------------------ + +/** + * Per-file map of which lines a positioned discussion can anchor to. + * + * GitLab's rules (Discussions API, "Create a new thread in the merge + * request diff"): an added line takes `new_line` only, a removed line takes + * `old_line` only, and an unchanged context line requires BOTH numbers. A + * line absent from every hunk cannot be anchored at all, no matter the + * payload. + */ +export type FileLineIndex = { + /** new-file line number -> "added", or the old line it pairs with. */ + newLines: Map; + /** old-file line number -> "removed", or the new line it pairs with. */ + oldLines: Map; +}; + +const HUNK_HEADER = /^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/; + +/** Indexes every hunk line, keyed by both `new_path` and `old_path`. */ +export function buildDiffIndex( + changes: GitlabMrDiff[], +): Map { + const files = new Map(); + + for (const change of changes) { + if (typeof change?.diff !== "string") continue; + const index: FileLineIndex = { newLines: new Map(), oldLines: new Map() }; + + const lines = change.diff.split("\n"); + if (lines[lines.length - 1] === "") lines.pop(); + + let oldN = 0; + let newN = 0; + let inHunk = false; + for (const raw of lines) { + const hunk = HUNK_HEADER.exec(raw); + if (hunk) { + oldN = Number(hunk[1]); + newN = Number(hunk[2]); + inHunk = true; + continue; + } + if (!inHunk || raw.startsWith("\\")) continue; + if (raw.startsWith("+")) { + index.newLines.set(newN, "added"); + newN += 1; + } else if (raw.startsWith("-")) { + index.oldLines.set(oldN, "removed"); + oldN += 1; + } else { + // Context line (" " prefix, or "" when trailing whitespace was + // stripped somewhere along the way). + index.newLines.set(newN, oldN); + index.oldLines.set(oldN, newN); + newN += 1; + oldN += 1; + } + } + + if (change.new_path) files.set(change.new_path, index); + if (change.old_path && change.old_path !== change.new_path) { + files.set(change.old_path, index); + } + } + + return files; +} + // --- posting --------------------------------------------------------------- type DiffRefs = { base_sha: string; head_sha: string; start_sha: string }; export type PostReviewResult = { posted: number; + /** Comments that went out as plain MR notes because they cannot anchor. */ + fallbackPosted: number; failures: Array<{ path: string; line: number | null; error: string }>; }; @@ -235,10 +309,52 @@ function buildPosition( return position; } +/** + * Reshapes `base` to the anchor form GitLab accepts for the line's role in + * the diff (added / removed / context), or names the reason no positioned + * discussion can ever work for this comment. + */ +function refineWithIndex( + comment: ReviewComment, + base: GitlabPosition, + file: FileLineIndex | undefined, +): { position: GitlabPosition } | { reason: string } { + const line = anchorLine(comment); + if (line === null) { + return { reason: "no usable line anchor" }; + } + + if (comment.side === "LEFT") { + const entry = file?.oldLines.get(line); + if (entry === undefined) { + return { reason: `old line ${line} is not part of the MR diff` }; + } + const position = { ...base, old_line: line }; + if (entry === "removed") { + delete position.new_line; + } else { + position.new_line = entry; + } + return { position }; + } + + const entry = file?.newLines.get(line); + if (entry === undefined) { + return { reason: `line ${line} is not part of the MR diff` }; + } + const position = { ...base, new_line: line }; + if (entry === "added") { + delete position.old_line; + } else { + position.old_line = entry; + } + return { position }; +} + /** Multi-line variant of {@link buildPosition}; null when there is no span. */ function buildMultiLinePosition( comment: ReviewComment, - diffRefs: DiffRefs, + base: GitlabPosition, ): GitlabPosition | null { const end = anchorLine(comment); const start = comment.startLine; @@ -247,7 +363,6 @@ function buildMultiLinePosition( } const left = comment.side === "LEFT"; - const base = buildPosition(comment, diffRefs); const pathForCode = left ? (base.old_path ?? base.new_path) : base.new_path; const code = (line: number) => lineCode(pathForCode, left ? line : null, left ? null : line); @@ -261,10 +376,25 @@ function buildMultiLinePosition( }; } +/** Body for the plain-note fallback when a comment cannot anchor inline. */ +export function fallbackNoteBody( + comment: ReviewComment, + line: number | null, +): string { + const filePath = + comment.side === "LEFT" ? (comment.old_path ?? comment.path) : comment.path; + const location = line !== null ? `${filePath}:${line}` : filePath; + return ( + `**\`${location}\`** (could not be posted inline, ` + + `so the finding is posted as a regular comment)\n\n${comment.body}` + ); +} + /** * Posts one discussion per comment; the summary goes into the tracking note - * rather than a second top-level note. A comment that cannot be posted - * lands in `failures` instead of throwing. + * rather than a second top-level note. A comment whose line cannot anchor + * inline (outside the diff, or refused by the API) falls back to a plain MR + * note; only a comment that fails both routes lands in `failures`. */ export async function postReview(options: { client: GitlabClient; @@ -273,7 +403,11 @@ export async function postReview(options: { comments: ReviewComment[]; }): Promise { const { client, projectId, mrIid, comments } = options; - const result: PostReviewResult = { posted: 0, failures: [] }; + const result: PostReviewResult = { + posted: 0, + fallbackPosted: 0, + failures: [], + }; if (comments.length === 0) { return result; @@ -287,18 +421,54 @@ export async function postReview(options: { ); } + // The changes feed the line index that decides which anchor shape GitLab + // will accept. Posting still works without it, one API refusal at a time. + let diffIndex: Map | null = null; + try { + const changes = await client.getMrChanges(projectId, mrIid); + diffIndex = Array.isArray(changes?.changes) + ? buildDiffIndex(changes.changes) + : null; + } catch (error) { + console.warn( + "Could not fetch MR changes; posting without a diff line index:", + error instanceof Error ? error.message : String(error), + ); + } + for (let i = 0; i < comments.length; i++) { const comment = comments[i]!; const line = anchorLine(comment); const where = `${comment.path}:${line ?? "?"}`; + const label = `[${i + 1}/${comments.length}]`; - // A multi-line anchor GitLab refuses still posts as a single line. - const attempts = [ - buildMultiLinePosition(comment, diffRefs), - buildPosition(comment, diffRefs), - ].filter((p): p is GitlabPosition => p !== null); - + const base = buildPosition(comment, diffRefs); + const attempts: GitlabPosition[] = []; let lastError = ""; + + if (diffIndex) { + const file = + diffIndex.get(comment.path) ?? + (comment.old_path !== null + ? diffIndex.get(comment.old_path) + : undefined); + const refined = refineWithIndex(comment, base, file); + if ("reason" in refined) { + // Unanchorable no matter the payload; go straight to the fallback. + lastError = refined.reason; + } else { + // A multi-line anchor GitLab refuses still posts as a single line. + const multi = buildMultiLinePosition(comment, refined.position); + if (multi) attempts.push(multi); + attempts.push(refined.position); + } + } else { + const multi = buildMultiLinePosition(comment, base); + if (multi) attempts.push(multi); + attempts.push(base); + } + + let posted = false; for (const position of attempts) { try { await client.createDiscussionOnDiff( @@ -307,21 +477,37 @@ export async function postReview(options: { comment.body, position, ); - lastError = ""; + posted = true; break; } catch (error) { lastError = error instanceof Error ? error.message : String(error); } } - if (lastError) { - result.failures.push({ path: comment.path, line, error: lastError }); - console.warn( - ` [${i + 1}/${comments.length}] failed ${where}: ${lastError}`, - ); - } else { + if (posted) { result.posted += 1; - console.log(` [${i + 1}/${comments.length}] posted ${where}`); + console.log(` ${label} posted ${where}`); + continue; + } + + try { + await client.createNote( + projectId, + mrIid, + fallbackNoteBody(comment, line), + ); + result.fallbackPosted += 1; + console.log( + ` ${label} posted ${where} as a regular note (${lastError})`, + ); + } catch (noteError) { + const noteMessage = + noteError instanceof Error ? noteError.message : String(noteError); + const error = lastError + ? `${lastError}; note fallback failed: ${noteMessage}` + : noteMessage; + result.failures.push({ path: comment.path, line, error }); + console.warn(` ${label} failed ${where}: ${error}`); } } @@ -411,6 +597,7 @@ async function run(): Promise { const results: PostResults = { posted: result.posted, + fallbackPosted: result.fallbackPosted, approved: parsed.approvedCount, rejected: parsed.rejectedCount, failed: result.failures.length, @@ -423,13 +610,19 @@ async function run(): Promise { await fs.mkdir(path.dirname(resultsPath), { recursive: true }); await fs.writeFile(resultsPath, JSON.stringify(results, null, 2)); console.log( - `Posted ${results.posted}/${parsed.approved.length} inline comments on MR !${mrIid} ` + + `Posted ${results.posted}/${parsed.approved.length} inline comments ` + + `(${results.fallbackPosted} as regular notes) on MR !${mrIid} ` + `(results: ${resultsPath}).`, ); - // Every anchor failing points at a systemic problem (stale diff refs, - // revoked token scope) rather than one bad line number, so surface it. - if (parsed.approved.length > 0 && results.posted === 0) { + // Unanchorable comments already fell back to plain notes, so nothing at + // all reaching the MR points at a systemic problem (revoked token scope, + // API unreachable) rather than bad line numbers. Surface it. + if ( + parsed.approved.length > 0 && + results.posted === 0 && + results.fallbackPosted === 0 + ) { throw new Error( `gitlab-post-review: all ${parsed.approved.length} approved comments failed to post. ` + `First error: ${results.failures[0]?.error ?? "unknown"}`, diff --git a/src/entrypoints/gitlab-update-comment-link.ts b/src/entrypoints/gitlab-update-comment-link.ts index f9d81958..f0f24129 100644 --- a/src/entrypoints/gitlab-update-comment-link.ts +++ b/src/entrypoints/gitlab-update-comment-link.ts @@ -35,6 +35,7 @@ async function readPostResults(): Promise { const results = JSON.parse(raw) as PostResults; return { posted: results.posted ?? null, + fallbackPosted: results.fallbackPosted ?? null, failed: results.failed ?? null, skipped: results.skipped ?? null, summaryBody: results.summaryBody ?? null, diff --git a/src/gitlab/api/client.ts b/src/gitlab/api/client.ts index 0747d4ee..eafa49c8 100644 --- a/src/gitlab/api/client.ts +++ b/src/gitlab/api/client.ts @@ -7,12 +7,26 @@ import type { GitlabPosition, } from "../types"; +/** + * The interesting part of a GitLab error is the response body (e.g. + * `{"message":"400 Bad request - Note {:line_code=>[...]}"}`), not the + * status text, so surface it in the error message where CI logs show it. + */ +function describeErrorBody(body: unknown): string { + if (body === null || body === undefined || body === "") { + return ""; + } + const text = typeof body === "string" ? body : JSON.stringify(body); + return text.length > 300 ? `${text.slice(0, 300)}...` : text; +} + export class GitlabApiError extends Error { status: number; body: unknown; constructor(status: number, message: string, body: unknown) { - super(`GitLab API ${status}: ${message}`); + const detail = describeErrorBody(body); + super(`GitLab API ${status}: ${message}${detail ? ` - ${detail}` : ""}`); this.name = "GitlabApiError"; this.status = status; this.body = body; diff --git a/src/gitlab/operations/tracking-note.ts b/src/gitlab/operations/tracking-note.ts index b106ccc5..efe7c230 100644 --- a/src/gitlab/operations/tracking-note.ts +++ b/src/gitlab/operations/tracking-note.ts @@ -76,8 +76,20 @@ export function buildTrackingNoteBody(options: TrackingNoteOptions): string { `${review.posted} inline ${review.posted === 1 ? "comment" : "comments"} posted`, ); } + if ( + typeof review.fallbackPosted === "number" && + review.fallbackPosted > 0 + ) { + counts.push( + `${review.fallbackPosted} posted as ${ + review.fallbackPosted === 1 ? "a regular note" : "regular notes" + } (line outside the diff)`, + ); + } if (typeof review.failed === "number" && review.failed > 0) { - counts.push(`${review.failed} could not be anchored to the diff`); + counts.push( + `${review.failed} could not be posted (inline + note fallback failed)`, + ); } if (typeof review.skipped === "number" && review.skipped > 0) { counts.push(`${review.skipped} skipped`); diff --git a/src/gitlab/prompts/terminology.ts b/src/gitlab/prompts/terminology.ts index 982ad25d..a4bfd61f 100644 --- a/src/gitlab/prompts/terminology.ts +++ b/src/gitlab/prompts/terminology.ts @@ -27,7 +27,10 @@ export const GITLAB_TERMINOLOGY: ReviewTerminology = { pathFieldDescription: 'Relative file path (use the new_path from the diff, e.g., "src/index.ts")', lineFieldDescription: - "Target line number in the new file (single-line) or end line number (multi-line). Must be ≥ 0.", + "Target line number in the new file (single-line) or end line number (multi-line). Must be ≥ 0. " + + "Must be a line that appears in the MR diff (an added or context line inside a hunk); " + + "GitLab cannot anchor inline comments to lines outside the diff, so a finding about " + + "untouched code should anchor to the nearest related changed line instead.", mutationToolForbiddance: "(no MR notes, discussions, description edits, approvals, or label changes — " + "whether through an MCP tool, the GitLab REST API, `glab`, or `curl`)", diff --git a/test/gitlab/api-client.test.ts b/test/gitlab/api-client.test.ts index 927084f0..e56d682d 100644 --- a/test/gitlab/api-client.test.ts +++ b/test/gitlab/api-client.test.ts @@ -104,6 +104,9 @@ describe("GitlabClient", () => { const e = err as GitlabApiError; expect(e.status).toBe(404); expect((e.body as { message: string }).message).toBe("404 Not Found"); + // The body detail is the actionable part (e.g. which position field + // GitLab refused), so it must survive into the message CI logs show. + expect(e.message).toContain('"message":"404 Not Found"'); } }); }); diff --git a/test/gitlab/post-review.test.ts b/test/gitlab/post-review.test.ts index 636280ac..b85b710c 100644 --- a/test/gitlab/post-review.test.ts +++ b/test/gitlab/post-review.test.ts @@ -4,6 +4,8 @@ import * as fs from "fs/promises"; import * as os from "os"; import * as path from "path"; import { + buildDiffIndex, + fallbackNoteBody, InvalidValidatedReviewError, parseValidatedReview, postReview, @@ -11,6 +13,7 @@ import { type ReviewComment, } from "../../src/entrypoints/gitlab-post-review"; import type { GitlabClient } from "../../src/gitlab/api/client"; +import type { GitlabMrDiff } from "../../src/gitlab/types"; const DIFF_REFS = { base_sha: "base-sha", @@ -18,6 +21,25 @@ const DIFF_REFS = { start_sha: "start-sha", }; +const mrDiff = (overrides: Partial): GitlabMrDiff => ({ + old_path: "src/x.ts", + new_path: "src/x.ts", + a_mode: "100644", + b_mode: "100644", + diff: "", + new_file: false, + renamed_file: false, + deleted_file: false, + ...overrides, +}); + +// @@ -8,5 +8,5 @@ over src/x.ts: +// new 8/9 context (old 8/9), new 10 added, new 11 context (old 10), +// old 11 removed, new 12 context (old 12). +const X_TS_DIFF = "@@ -8,5 +8,5 @@\n a\n b\n+c\n d\n-e\n f\n"; + +const SAMPLE_CHANGES: GitlabMrDiff[] = [mrDiff({ diff: X_TS_DIFF })]; + const comment = (overrides: Partial = {}): ReviewComment => ({ path: "src/x.ts", body: "[P1] Finding", @@ -29,25 +51,72 @@ const comment = (overrides: Partial = {}): ReviewComment => ({ ...overrides, }); +describe("buildDiffIndex", () => { + it("classifies added, removed, and context lines with pairing", () => { + const index = buildDiffIndex(SAMPLE_CHANGES).get("src/x.ts")!; + + expect(index.newLines.get(10)).toBe("added"); + expect(index.newLines.get(9)).toBe(9); + expect(index.newLines.get(11)).toBe(10); + expect(index.newLines.get(13)).toBeUndefined(); + expect(index.oldLines.get(11)).toBe("removed"); + expect(index.oldLines.get(12)).toBe(12); + }); + + it("registers a rename under both paths and skips meta lines", () => { + const index = buildDiffIndex([ + mrDiff({ + old_path: "old.ts", + new_path: "new.ts", + renamed_file: true, + diff: "@@ -1,2 +1,2 @@\n-a\n+b\n c\n\\ No newline at end of file\n", + }), + ]); + + expect(index.get("new.ts")).toBe(index.get("old.ts")!); + expect(index.get("new.ts")!.newLines.get(1)).toBe("added"); + expect(index.get("new.ts")!.newLines.get(2)).toBe(2); + // The "\ No newline" marker must not advance either counter. + expect(index.get("new.ts")!.newLines.get(3)).toBeUndefined(); + }); +}); + describe("postReview", () => { let client: GitlabClient; let discussions: ReturnType; + let notes: ReturnType; /** Discussion positions the client was asked to create, in order. */ const positions = () => (discussions.mock.calls as unknown[][]).map((c) => c[3] as any); + /** Bodies of the plain-note fallbacks, in order. */ + const noteBodies = () => + (notes.mock.calls as unknown[][]).map((c) => c[2] as string); + const post = (comments: ReviewComment[]) => postReview({ client, projectId: "4242", mrIid: 7, comments }); function setup( createDiscussion: (position: any) => unknown = () => ({ id: "disc" }), diffRefs: unknown = DIFF_REFS, + opts: { + changes?: GitlabMrDiff[]; + createNote?: () => unknown; + } = {}, ) { discussions = mock(async (...args: unknown[]) => createDiscussion(args[3])); + notes = mock(async () => (opts.createNote ?? (() => ({ id: 1 })))()); client = { getMr: mock(async () => ({ iid: 7, diff_refs: diffRefs })), + getMrChanges: mock(async () => { + // Without explicit changes the index is unavailable, exercising the + // legacy anchor-as-given path. + if (!opts.changes) throw new Error("changes unavailable"); + return { changes: opts.changes, diff_refs: DIFF_REFS }; + }), createDiscussionOnDiff: discussions, + createNote: notes, } as unknown as GitlabClient; } @@ -59,7 +128,7 @@ describe("postReview", () => { comment({ path: "new.ts", old_path: "old.ts" }), ]); - expect(result).toEqual({ posted: 2, failures: [] }); + expect(result).toEqual({ posted: 2, fallbackPosted: 0, failures: [] }); expect(positions()[0]).toEqual({ ...DIFF_REFS, position_type: "text", @@ -105,12 +174,12 @@ describe("postReview", () => { const result = await post([comment({ line: 20, startLine: 18 })]); - expect(result).toEqual({ posted: 1, failures: [] }); + expect(result).toEqual({ posted: 1, fallbackPosted: 0, failures: [] }); expect(positions()).toHaveLength(2); expect(positions()[1].line_range).toBeUndefined(); }); - it("records a comment GitLab refuses and keeps posting the rest", async () => { + it("posts a comment GitLab refuses as a plain note instead", async () => { setup((position) => { if (position.new_path === "gone.ts") throw new Error("400: not in diff"); return { id: "disc" }; @@ -121,18 +190,111 @@ describe("postReview", () => { comment(), ]); - expect(result.posted).toBe(1); + expect(result).toEqual({ posted: 1, fallbackPosted: 1, failures: [] }); + expect(noteBodies()).toHaveLength(1); + expect(noteBodies()[0]).toContain("gone.ts:3"); + expect(noteBodies()[0]).toContain("[P1] Finding"); + }); + + it("records a failure when the note fallback also fails", async () => { + setup( + () => { + throw new Error("400: not in diff"); + }, + DIFF_REFS, + { + createNote: () => { + throw new Error("401: token revoked"); + }, + }, + ); + + const result = await post([comment({ path: "gone.ts", line: 3 })]); + + expect(result.posted).toBe(0); + expect(result.fallbackPosted).toBe(0); expect(result.failures).toEqual([ - { path: "gone.ts", line: 3, error: "400: not in diff" }, + { + path: "gone.ts", + line: 3, + error: "400: not in diff; note fallback failed: 401: token revoked", + }, ]); }); + it("sends both line numbers for a context-line anchor", async () => { + setup(undefined, DIFF_REFS, { changes: SAMPLE_CHANGES }); + + // new 11 is an unchanged line pairing with old 10; GitLab requires both. + await post([comment({ line: 11 })]); + + expect(positions()[0]).toMatchObject({ new_line: 11, old_line: 10 }); + }); + + it("keeps an added-line anchor one-sided, dropping a stray old_line", async () => { + setup(undefined, DIFF_REFS, { changes: SAMPLE_CHANGES }); + + await post([comment({ line: 10, old_line: 3 })]); + + expect(positions()[0]).toMatchObject({ new_line: 10 }); + expect(positions()[0].old_line).toBeUndefined(); + }); + + it("anchors LEFT comments per the index (removed vs context)", async () => { + setup(undefined, DIFF_REFS, { changes: SAMPLE_CHANGES }); + + await post([ + comment({ side: "LEFT", line: null, old_line: 11 }), + comment({ side: "LEFT", line: null, old_line: 12 }), + ]); + + expect(positions()[0]).toMatchObject({ old_line: 11 }); + expect(positions()[0].new_line).toBeUndefined(); + expect(positions()[1]).toMatchObject({ old_line: 12, new_line: 12 }); + }); + + it("skips the API and posts a note for a line outside the diff", async () => { + setup(undefined, DIFF_REFS, { changes: SAMPLE_CHANGES }); + + const result = await post([comment({ line: 99 })]); + + expect(result).toEqual({ posted: 0, fallbackPosted: 1, failures: [] }); + // Unanchorable lines never reach the discussions endpoint. + expect(positions()).toHaveLength(0); + expect(noteBodies()[0]).toContain("src/x.ts:99"); + }); + it("fails when the MR carries no diff refs to anchor against", async () => { setup(undefined, null); await expect(post([comment()])).rejects.toThrow(/missing diff_refs/); }); }); +describe("fallbackNoteBody", () => { + const base: ReviewComment = { + path: "src/new.ts", + body: "finding", + line: 12, + startLine: null, + side: "RIGHT", + old_path: "src/old.ts", + old_line: null, + }; + + it("references the old path for LEFT-side comments", () => { + const body = fallbackNoteBody({ ...base, side: "LEFT" }, 12); + expect(body).toContain("src/old.ts:12"); + expect(body).not.toContain("src/new.ts"); + }); + + it("uses wording that also covers API refusals, not just out-of-diff lines", () => { + const body = fallbackNoteBody(base, 12); + expect(body).toContain("src/new.ts:12"); + expect(body).toContain("could not be posted inline"); + expect(body).not.toContain("outside the MR diff"); + }); +}); + describe("parseValidatedReview", () => { const validated = (results: unknown[], reviewSummary?: unknown) => JSON.stringify({ @@ -242,8 +404,15 @@ describe("gitlab-post-review entrypoint", () => { let calls: FetchCall[]; const originalFetch = globalThis.fetch; - /** Answers the MR lookup, and each discussion POST via `onDiscussion`. */ - function stubFetch(onDiscussion: (call: FetchCall) => Response) { + /** + * Answers the MR lookup and changes fetch; each discussion POST goes + * through `onDiscussion`, each plain-note POST through `onNote`. + */ + function stubFetch( + onDiscussion: (call: FetchCall) => Response, + onNote: (call: FetchCall) => Response = () => + new Response(JSON.stringify({ id: 1 })), + ) { globalThis.fetch = (async (input: any, init: any = {}) => { const call: FetchCall = { url: String(input), @@ -252,9 +421,12 @@ describe("gitlab-post-review entrypoint", () => { calls.push(call); const ok = (b: unknown, status = 200) => new Response(JSON.stringify(b), { status }); - return call.url.includes("/discussions") - ? onDiscussion(call) - : ok({ iid: 7, diff_refs: DIFF_REFS }); + if (call.url.includes("/discussions")) return onDiscussion(call); + if (call.url.endsWith("/changes")) { + return ok({ changes: SAMPLE_CHANGES, diff_refs: DIFF_REFS }); + } + if (call.url.endsWith("/notes")) return onNote(call); + return ok({ iid: 7, diff_refs: DIFF_REFS }); }) as typeof fetch; } @@ -327,41 +499,48 @@ describe("gitlab-post-review entrypoint", () => { await writeState(); await writeValidated( [ + // src/x.ts:10 is an added line in SAMPLE_CHANGES; gone.ts is not in + // the diff at all, so it must degrade to a plain note. approved(), approved({ path: "gone.ts", line: 99 }), { status: "rejected", comment: { path: "z.ts", body: "no", line: 1 } }, ], { body: "## Review summary" }, ); - stubFetch((call) => - call.body.position.new_path === "gone.ts" ? refused() : posted(), - ); + stubFetch(posted); await postReviewRun(); const discussions = calls.filter((c) => c.url.includes("/discussions")); + expect(discussions).toHaveLength(1); expect(discussions[0]!.url).toContain( "/projects/4242/merge_requests/7/discussions", ); expect(discussions[0]!.body.body).toBe("[P1] Finding"); - // The summary belongs to the tracking note, not a second MR note. - expect(calls.some((c) => c.url.endsWith("/notes"))).toBe(false); + // The only plain note is the out-of-diff fallback; the summary still + // belongs to the tracking note, never a second MR note. + const notes = calls.filter((c) => c.url.endsWith("/notes")); + expect(notes).toHaveLength(1); + expect(notes[0]!.body.body).toContain("gone.ts:99"); + expect(notes[0]!.body.body).not.toContain("## Review summary"); expect(await readPostResults()).toEqual({ posted: 1, + fallbackPosted: 1, approved: 2, rejected: 1, - failed: 1, + failed: 0, skipped: 0, summaryBody: "## Review summary", - failures: [{ path: "gone.ts", line: 99, error: expect.any(String) }], + failures: [], }); }); it("fails when every approved comment is rejected by the API", async () => { await writeState(); await writeValidated([approved()]); - stubFetch(refused); + // Both the positioned discussion and the note fallback are refused. + stubFetch(refused, refused); await expect(postReviewRun()).rejects.toThrow(/all 1 approved comments/); }); diff --git a/test/gitlab/prompts.test.ts b/test/gitlab/prompts.test.ts index 1f3eaa69..6603d695 100644 --- a/test/gitlab/prompts.test.ts +++ b/test/gitlab/prompts.test.ts @@ -108,6 +108,9 @@ describe("generateGitlabReviewValidatorPrompt", () => { expect(prompt).not.toContain("update_tracking_note"); // An approved comment with no anchor is silently unpostable. expect(prompt).toContain("without a usable line anchor"); + // Anchors outside the diff degrade to a plain note; the validator is + // told to prefer re-anchoring onto a changed line. + expect(prompt).toContain("not part of the diff"); }); it("enforces the approved/rejected ordering contract", () => { diff --git a/test/gitlab/tracking-note.test.ts b/test/gitlab/tracking-note.test.ts index 175c5280..4f572426 100644 --- a/test/gitlab/tracking-note.test.ts +++ b/test/gitlab/tracking-note.test.ts @@ -93,21 +93,27 @@ describe("buildTrackingNoteBody", () => { }); expect(body).toContain("Three issues worth fixing before merge."); expect(body).toContain("3 inline comments posted"); - expect(body).not.toContain("could not be anchored"); + expect(body).not.toContain("could not be posted"); expect(body).not.toContain("skipped"); }); - it("surfaces unanchored and skipped counts only when non-zero", () => { + it("surfaces fallback, unanchored, and skipped counts only when non-zero", () => { const body = buildTrackingNoteBody({ state: "success", - review: { posted: 1, failed: 2, skipped: 1 }, + review: { posted: 1, fallbackPosted: 1, failed: 2, skipped: 1 }, }); expect(body).toContain("1 inline comment posted"); - expect(body).toContain("2 could not be anchored to the diff"); + expect(body).toContain( + "1 posted as a regular note (line outside the diff)", + ); + expect(body).toContain( + "2 could not be posted (inline + note fallback failed)", + ); expect(body).toContain("1 skipped"); const empty = buildTrackingNoteBody({ state: "success", review: {} }); expect(empty).not.toContain("inline comment"); + expect(empty).not.toContain("regular note"); }); it("omits telemetry block entirely when telemetry is missing or empty", () => {