diff --git a/README.md b/README.md index a02b922373..7ebab03cbd 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ Gittensory CI and gittensory review score, gate, and comment on pull requests. T - **`Gittensory Orb Review Agent`** (`gate.*` / `settings.gateCheckMode` / `settings.reviewCheckMode`, off by default) — the authoritative GitHub Check Run carrying the gate's pass/fail verdict. This is the one worth making a required status check. - **`Gittensory Context`** (`settings.checkRunMode` / `settings.checkRunDetailLevel`, off by default) — a separate, purely advisory Check Run. At its default `checkRunDetailLevel: minimal` it publishes no findings at all; even at `standard`/`deep` it only re-renders content already shown elsewhere. Never make this one required. -- **Inline review comments** (`GITTENSORY_REVIEW_INLINE_COMMENTS` + `.gittensory.yml`'s `review.inline_comments`, off by both by default) — real, reply-able line-anchored PR review comment threads (CodeRabbit-style). This is the ONLY one of the three that posts an interactive per-line thread; the two check runs above never do. +- **Inline review comments** (`GITTENSORY_REVIEW_INLINE_COMMENTS` + `.gittensory.yml`'s `review.inline_comments`, off by both by default) — real, reply-able line-anchored PR review comment threads (CodeRabbit-style). This is the ONLY one of the three that posts an interactive per-line thread; the two check runs above never do. With `.gittensory.yml`'s `review.suggestions` also on, a precise line-anchored fix is additionally rendered as a one-click, committable GitHub suggested-change block. See [Tuning your reviews](https://gittensory.aethereal.dev/docs/tuning) for the full flag, setting, and `.gittensory.yml` reference. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 93be8b17bd..e92805e2d6 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -390,6 +390,7 @@ import { } from "../services/ai-review"; import { maybePostInlineComments, + shouldRenderSuggestions, shouldRequestInlineFindings, } from "../review/inline-comments"; import { evaluateClaCheck } from "../review/cla-check"; @@ -7637,6 +7638,7 @@ async function maybePublishPrPublicSurface( } | undefined; let inlineCommentsEnabledForReview = false; + let suggestionsEnabledForReview = false; let aiReviewExpected = false; let aiReviewWasReused = false; let gateFinalized = false; @@ -8236,6 +8238,7 @@ async function maybePublishPrPublicSurface( profile: reviewProfile, securityFocus: reviewSecurityFocus, inlineComments: reviewInlineComments, + suggestions: reviewSuggestions, pathInstructions: reviewPathInstructions, instructions: manifestReviewInstructions, tone: reviewTone, @@ -8248,6 +8251,10 @@ async function maybePublishPrPublicSurface( repoFullName, reviewInlineComments, ); + suggestionsEnabledForReview = shouldRenderSuggestions( + inlineCommentsEnabledForReview, + reviewSuggestions, + ); const reviewFilesForAi = await getReviewFiles(); const changedPaths = reviewFilesForAi.map((file) => file.path); // Per-repo review CONTEXT (#review-skills): fold the container-private review/AGENTS.md (or legacy @@ -9341,6 +9348,7 @@ async function maybePublishPrPublicSurface( getFiles: getReviewFiles, mode, inlineCommentsEnabled: inlineCommentsEnabledForReview, + suggestionsEnabled: suggestionsEnabledForReview, }); } if (decision.willLabel) { diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index 7ddd0d6a8b..48e8df4dd7 100644 --- a/src/review/inline-comments.ts +++ b/src/review/inline-comments.ts @@ -34,6 +34,17 @@ export function shouldRequestInlineFindings( return manifestToggle === true && isInlineCommentsEnabled(env) && isConvergenceRepoAllowed(env, repoFullName); } +/** PURE (#1956): should a `suggestion` be rendered as a GitHub-native ` ```suggestion ` block? This is an + * ADDITIONAL opt-in (`review.suggestions`) layered on top of inline comments being enabled at all — a + * suggestion has nothing to attach to without the inline comment it rides on, so it can never be true when + * `inlineCommentsEnabled` is false, regardless of the manifest toggle. */ +export function shouldRenderSuggestions( + inlineCommentsEnabled: boolean, + manifestToggle: boolean | undefined, +): boolean { + return inlineCommentsEnabled && manifestToggle === true; +} + /** A GitHub inline review comment anchored to a line on the RIGHT (added/context) side of the PR diff. */ export type ReviewInlineComment = { path: string; line: number; side: "RIGHT"; body: string }; @@ -66,17 +77,34 @@ export function rightSideLinesFromPatch(patch: string): Set { return lines; } -/** The inline comment body: a compact severity label + the finding. Public-safe by construction — the body was - * already run through the public-safe filter by composeInlineFindings before it reached here. */ -function formatInlineBody(finding: InlineFinding): string { +/** GitHub's suggested-change syntax requires the LITERAL ` ```suggestion ` fence; if the suggestion text itself + * contains a triple-backtick run, embedding it verbatim would prematurely close the fence and corrupt the + * comment (the rest of the finding body would spill out as raw, unintended markdown). Fail-safe (#1956): + * drop the suggestion block and keep the finding text rather than risk a malformed comment — mirrors the + * "a bad/blank suggestion is simply dropped while keeping the finding itself" discipline already applied when + * the suggestion is parsed (ai-review.ts's parseModelReview). */ +function safeSuggestionBlock(suggestion: string | undefined): string { + if (!suggestion || suggestion.includes("```")) return ""; + return `\n\n\`\`\`suggestion\n${suggestion}\n\`\`\``; +} + +/** The inline comment body: a compact severity label + the finding, plus a one-click GitHub suggested-change + * block when the finding carries a `suggestion` AND the caller has suggestions enabled (#1956). Public-safe by + * construction — both the body and the suggestion were already run through the public-safe filter by + * composeInlineFindings before they reached here. */ +function formatInlineBody(finding: InlineFinding, suggestionsEnabled: boolean): string { const label = finding.severity === "blocker" ? "Blocker" : "Nit"; - return `**${label}:** ${finding.body}`; + const suggestionBlock = suggestionsEnabled ? safeSuggestionBlock(finding.suggestion) : ""; + return `**${label}:** ${finding.body}${suggestionBlock}`; } /** PURE: turn the model's line-anchored findings into GitHub inline review comments, dropping any whose * (path, line) is not a commentable RIGHT-side line in that file's diff (so GitHub never 422s) and any file with - * no usable patch. Dedupes by path+line (first wins) and caps the total. Empty in / nothing anchorable ⇒ []. */ -export function selectInlineComments(findings: InlineFinding[], files: Pick[]): ReviewInlineComment[] { + * no usable patch. Dedupes by path+line (first wins) and caps the total. Empty in / nothing anchorable ⇒ []. + * `suggestionsEnabled` (#1956) gates whether a finding's `suggestion` is rendered as a committable GitHub + * suggested-change block — a suggestion is anchored to the SAME single line as its parent finding, so the + * existing line-validity check above already covers "drop it if the range can't be anchored". */ +export function selectInlineComments(findings: InlineFinding[], files: Pick[], suggestionsEnabled = false): ReviewInlineComment[] { const rightLinesByPath = new Map>(); for (const file of files) { const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; @@ -91,7 +119,7 @@ export function selectInlineComments(findings: InlineFinding[], files: Pick[]; mode: AgentActionMode; + suggestionsEnabled?: boolean | undefined; }, ): Promise<{ posted: number }> { - const comments = selectInlineComments(args.findings, args.files); + const comments = selectInlineComments(args.findings, args.files, args.suggestionsEnabled); if (comments.length === 0 || !args.commitId) return { posted: 0 }; try { await createPullRequestReviewComments(env, args.installationId, args.repoFullName, args.pullNumber, args.commitId, comments, args.mode); @@ -140,6 +169,7 @@ export async function maybePostInlineComments( getFiles: () => Promise[]>; mode: AgentActionMode; inlineCommentsEnabled: boolean; + suggestionsEnabled?: boolean | undefined; }, ): Promise { if (!args.inlineCommentsEnabled) return; @@ -153,5 +183,6 @@ export async function maybePostInlineComments( findings, files: await args.getFiles(), mode: args.mode, + suggestionsEnabled: args.suggestionsEnabled, }); } diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index a11ed958a7..f31d6303e1 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -316,6 +316,12 @@ export type FocusManifestReviewConfig = { * comments = byte-identical behavior. Operator-gated too (GITTENSORY_REVIEW_INLINE_COMMENTS + allowlist). * (#inline-comments) */ inlineComments: boolean | null; + /** `review.suggestions`: when true, an inline finding whose AI-provided fix is precise enough to anchor to a + * single line is ALSO rendered as a GitHub-native ` ```suggestion ` block a contributor can commit in one + * click. Only takes effect when inline comments are already on (a suggestion has nothing to attach to + * otherwise) — this is an ADDITIONAL opt-in on top of `review.inline_comments`, not a replacement gate. + * null/false (default, absent) = no suggestion blocks = byte-identical behavior. (#1956) */ + suggestions: boolean | null; /** `review.path_instructions`: per-path natural-language guidance handed to the AI reviewer when the PR's * changed files match the glob. Empty (default) ⇒ byte-identical reviewer prompt. (#review-path-instructions) */ pathInstructions: ReviewPathInstruction[]; @@ -568,7 +574,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -598,7 +604,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1528,7 +1534,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -1565,6 +1571,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const tone = parsePublicSafeText(r.tone, "review.tone", warnings); const securityFocus = normalizeOptionalBoolean(r.security_focus, "review.security_focus", warnings); const inlineComments = normalizeOptionalBoolean(r.inline_comments, "review.inline_comments", warnings); + const suggestions = normalizeOptionalBoolean(r.suggestions, "review.suggestions", warnings); const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); const instructions = parsePublicSafeText(r.instructions, "review.instructions", warnings); const excludePaths = parseReviewExcludePaths(r.exclude_paths, warnings); @@ -1581,6 +1588,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo tone !== null || securityFocus !== null || inlineComments !== null || + suggestions !== null || pathInstructions.length > 0 || instructions !== null || excludePaths.length > 0 || @@ -1601,6 +1609,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo tone, securityFocus, inlineComments, + suggestions, pathInstructions, instructions, excludePaths, @@ -1918,6 +1927,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.tone !== null) out.tone = review.tone; if (review.securityFocus !== null) out.security_focus = review.securityFocus; if (review.inlineComments !== null) out.inline_comments = review.inlineComments; + if (review.suggestions !== null) out.suggestions = review.suggestions; if (review.instructions !== null) out.instructions = review.instructions; if (review.pathInstructions.length > 0) out.path_instructions = review.pathInstructions.map((entry) => ({ path: entry.path, instructions: entry.instructions })); if (review.excludePaths.length > 0) out.exclude_paths = [...review.excludePaths]; @@ -2055,12 +2065,14 @@ export function composeManifestReviewInstructions(instructions: string | null, t * `review.exclude_paths` + `review.path_filters` + `review.ai_model`) from a possibly-null manifest (null = load * failure). A null manifest yields the byte-identical defaults. Centralized so the AI-review caller threads them * in one place with the null-manifest branch covered here (unit-tested) rather than inline in the processor. - * (#review-profile / #review-tone / #review-security-focus / #review-path-instructions / #review-exclude-paths / #2043 / #selfhost-ai-model-override) */ -export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[]; selfHostAiModel: SelfHostAiModelConfig } { + * (#review-profile / #review-tone / #review-security-focus / #review-path-instructions / #review-exclude-paths / #2043 / #selfhost-ai-model-override / #1956) */ +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; suggestions: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[]; selfHostAiModel: SelfHostAiModelConfig } { // inlineComments resolves to a strict boolean — true ONLY when the manifest explicitly set review.inline_comments: // true; null/false/absent ⇒ false. The caller ANDs this per-repo toggle with the operator flag + cutover allowlist. // securityFocus resolves the same way — true ONLY when the manifest explicitly set review.security_focus: true. - return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; + // suggestions resolves the same way (#1956) — the caller further ANDs it with the already-resolved + // inlineComments gate, since a suggestion has nothing to attach to without an inline comment. + return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, suggestions: manifest?.review.suggestions === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; } /** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 00348005f6..703a2e59d8 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -551,7 +551,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, @@ -2634,13 +2634,15 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { }); it("resolveReviewPromptOverrides: non-null manifest passes the config through; null manifest → defaults", () => { - const manifest = parseFocusManifest({ review: { profile: "chill", security_focus: true, inline_comments: true, path_instructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", exclude_paths: ["**/*.lock"], path_filters: ["src/**", "!src/generated/**"] } }); - expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); - // A null manifest (load failure) yields the byte-identical defaults; inline comments + security focus default OFF. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); + const manifest = parseFocusManifest({ review: { profile: "chill", security_focus: true, inline_comments: true, suggestions: true, path_instructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", exclude_paths: ["**/*.lock"], path_filters: ["src/**", "!src/generated/**"] } }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, suggestions: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); + // A null manifest (load failure) yields the byte-identical defaults; inline comments + suggestions + security focus default OFF. + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); // An explicit false / absent toggle both resolve to the strict-boolean false. expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { inline_comments: false } })).inlineComments).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).inlineComments).toBe(false); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { suggestions: false } })).suggestions).toBe(false); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).suggestions).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { security_focus: false } })).securityFocus).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).securityFocus).toBe(false); }); @@ -2661,6 +2663,23 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(bad.review.inlineComments).toBeNull(); expect(bad.warnings.some((w) => /review\.inline_comments.*must be a boolean/.test(w))).toBe(true); }); + + it("parses review.suggestions (default OFF), marks present, round-trips, and warns on a non-boolean (#1956)", () => { + expect(parseFocusManifest({ review: { suggestions: true } }).review.suggestions).toBe(true); + const on = parseFocusManifest({ review: { suggestions: true } }); + expect(on.review.present).toBe(true); // a suggestions-only manifest IS present + expect(parseFocusManifest({ review: reviewConfigToJson(on.review) }).review).toEqual(on.review); // survives round-trip + // Explicit false is retained (and marks present, since the maintainer set it). + const off = parseFocusManifest({ review: { suggestions: false } }); + expect(off.review.suggestions).toBe(false); + expect(off.review.present).toBe(true); + // Absent ⇒ null (the byte-identical default), config not present. + expect(parseFocusManifest({ review: {} }).review.suggestions).toBeNull(); + // A non-boolean is ignored with a warning. + const bad = parseFocusManifest({ review: { suggestions: "yes" } }); + expect(bad.review.suggestions).toBeNull(); + expect(bad.warnings.some((w) => /review\.suggestions.*must be a boolean/.test(w))).toBe(true); + }); }); describe("review.exclude_paths (#review-exclude-paths)", () => { diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index 45d3ef4e93..0cd115c533 100644 --- a/test/unit/inline-comments.test.ts +++ b/test/unit/inline-comments.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { generateKeyPairSync } from "node:crypto"; import type { InlineFinding } from "../../src/services/ai-review"; -import { isInlineCommentsEnabled, maybePostInlineComments, postInlineReviewComments, rightSideLinesFromPatch, selectInlineComments, shouldRequestInlineFindings } from "../../src/review/inline-comments"; +import { isInlineCommentsEnabled, maybePostInlineComments, postInlineReviewComments, rightSideLinesFromPatch, selectInlineComments, shouldRenderSuggestions, shouldRequestInlineFindings } from "../../src/review/inline-comments"; import { createTestEnv } from "../helpers/d1"; function envWithKey() { @@ -31,6 +31,16 @@ describe("shouldRequestInlineFindings (#inline-comments)", () => { }); }); +describe("shouldRenderSuggestions (#1956)", () => { + it("requires the manifest toggle AND inline comments already being enabled — a suggestion has nothing to attach to otherwise", () => { + expect(shouldRenderSuggestions(true, true)).toBe(true); + expect(shouldRenderSuggestions(true, false)).toBe(false); // manifest toggle off + expect(shouldRenderSuggestions(true, undefined)).toBe(false); // manifest toggle absent + expect(shouldRenderSuggestions(false, true)).toBe(false); // inline comments themselves are off + expect(shouldRenderSuggestions(false, false)).toBe(false); + }); +}); + describe("rightSideLinesFromPatch (#inline-comments)", () => { it("returns RIGHT-side line numbers for added + context lines, excluding deleted lines and the no-newline marker", () => { const patch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n+added3\n ctx4\n\\ No newline at end of file"; @@ -86,6 +96,38 @@ describe("selectInlineComments (#inline-comments)", () => { const many: InlineFinding[] = Array.from({ length: 12 }, (_, i) => ({ path: "src/big.ts", line: i + 1, severity: "nit", body: `b${i + 1}` })); expect(selectInlineComments(many, bigFiles)).toHaveLength(10); }); + + describe("suggestion blocks (#1956)", () => { + const withSuggestion: InlineFinding = { path: "src/a.ts", line: 2, severity: "nit", body: "Use const.", suggestion: "const x = 1;" }; + + it("defaults to OFF (backward compatible) — a suggestion is never rendered when the third argument is omitted", () => { + const out = selectInlineComments([withSuggestion], files); + expect(out).toEqual([{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit:** Use const." }]); + }); + + it("does not render a suggestion when explicitly disabled, even if the finding carries one", () => { + const out = selectInlineComments([withSuggestion], files, false); + expect(out[0]?.body).not.toContain("```suggestion"); + }); + + it("renders a GitHub-native suggested-change block when enabled and the finding carries a suggestion", () => { + const out = selectInlineComments([withSuggestion], files, true); + expect(out[0]?.body).toBe("**Nit:** Use const.\n\n```suggestion\nconst x = 1;\n```"); + }); + + it("renders no suggestion block (finding text only) when enabled but the finding has none", () => { + const noSuggestion: InlineFinding = { path: "src/a.ts", line: 2, severity: "nit", body: "Use const." }; + const out = selectInlineComments([noSuggestion], files, true); + expect(out[0]?.body).toBe("**Nit:** Use const."); + }); + + it("fails safe: drops a suggestion whose own text contains a triple-backtick run, to avoid corrupting the comment's markdown fence, but keeps the finding text", () => { + const breaksFence: InlineFinding = { path: "src/a.ts", line: 2, severity: "blocker", body: "Fix this.", suggestion: "```\nescape attempt\n```" }; + const out = selectInlineComments([breaksFence], files, true); + expect(out[0]?.body).toBe("**Blocker:** Fix this."); + expect(out[0]?.body).not.toContain("escape attempt"); + }); + }); }); describe("postInlineReviewComments (#inline-comments, fail-safe)", () => { @@ -183,4 +225,34 @@ describe("maybePostInlineComments (#inline-comments, review-path entry)", () => expect(getFiles).toHaveBeenCalledTimes(1); expect(calls[0]?.body).toMatchObject({ event: "COMMENT", comments: [{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit:** guard this" }] }); }); + + it("renders a suggested-change block end-to-end when suggestionsEnabled is threaded through (#1956)", async () => { + const getFiles = vi.fn(async () => files); + const withSuggestion: InlineFinding[] = [{ path: "src/a.ts", line: 2, severity: "nit", body: "guard this", suggestion: "if (x) guard();" }]; + const calls: Array<{ url: string; body: unknown }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + calls.push({ url, body: init?.body ? JSON.parse(String(init.body)) : null }); + if (url.endsWith("/pulls/3/reviews")) return Response.json({ id: 10 }); + return new Response("unexpected", { status: 500 }); + }); + await maybePostInlineComments(envWithKey(), { ...base, aiReview: { inlineFindings: withSuggestion }, getFiles, suggestionsEnabled: true }); + expect(calls[0]?.body).toMatchObject({ comments: [{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit:** guard this\n\n```suggestion\nif (x) guard();\n```" }] }); + }); + + it("omits the suggestion block end-to-end when suggestionsEnabled is not passed (default off, backward compatible)", async () => { + const getFiles = vi.fn(async () => files); + const withSuggestion: InlineFinding[] = [{ path: "src/a.ts", line: 2, severity: "nit", body: "guard this", suggestion: "if (x) guard();" }]; + const calls: Array<{ url: string; body: unknown }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + calls.push({ url, body: init?.body ? JSON.parse(String(init.body)) : null }); + if (url.endsWith("/pulls/3/reviews")) return Response.json({ id: 11 }); + return new Response("unexpected", { status: 500 }); + }); + await maybePostInlineComments(envWithKey(), { ...base, aiReview: { inlineFindings: withSuggestion }, getFiles }); + expect(calls[0]?.body).toMatchObject({ comments: [{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit:** guard this" }] }); + }); }); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index a272050a00..60085202fa 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null } }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null } }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead