From fb824f6ff9ae567f65a4fbacd15bb1aaec394d5a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 26 Jun 2026 06:14:54 -0700 Subject: [PATCH] feat(review): emit structured inline review findings + a review.inline_comments toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR A of a CodeRabbit-style quiet inline-comments feature (the inert data contract; the GitHub posting + wiring follow in PR B). The AI reviewer can now ALSO emit structured, line-anchored findings {path, line, severity, body}: ModelReview/result carry an inlineFindings array, parseModelReview parses it fail-safe (a malformed/absent field degrades to [], each item missing a usable path/line/body is dropped, never partial), and composeInlineFindings dedupes by path+line, drops public-unsafe bodies, and caps the total. The instruction is appended to the system prompt ONLY when the caller asks for it, so with the feature off the prompt is byte-identical and the model emits nothing. Adds a manifest-only .gittensory.yml review.inline_comments toggle (default OFF) wired the same way as review.profile — type, parse, present, serialize, resolve — with no DB column. Nothing consumes the findings yet, so behavior is unchanged until PR B. --- src/services/ai-review.ts | 79 ++++++++++++++++++++-- src/signals/focus-manifest.ts | 21 ++++-- test/unit/ai-review.test.ts | 103 ++++++++++++++++++++++++++--- test/unit/focus-manifest.test.ts | 30 +++++++-- test/unit/signals-coverage.test.ts | 2 +- 5 files changed, 208 insertions(+), 27 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 0b80e98fb9..52e46f6e5b 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -129,6 +129,13 @@ export type GittensoryAiReviewInput = { * instructions passed the manifest's public-safe filter at parse time). */ pathGuidance?: string | null | undefined; + /** + * `.gittensory.yml` `review.inline_comments` (#inline-comments) — when true (the caller has already ANDed the + * operator flag + cutover allowlist + the per-repo manifest toggle), the reviewer is asked to ALSO emit an + * `inlineFindings` array of line-anchored findings for quiet, non-blocking inline PR comments. Absent/false + * (the default) ⇒ no instruction is appended, so the prompt is byte-identical and the model emits none. + */ + inlineFindings?: boolean | undefined; }; /** A consensus critical defect, already public-safe, ready to become a gate blocker finding. */ @@ -138,7 +145,12 @@ export type GittensoryAiReviewResult = | { status: "disabled"; reason: string } | { status: "unavailable"; reason: string } | { status: "quota_exceeded"; estimatedNeurons: number; remainingBudget: number } - | { status: "ok"; advisoryNotes: string | null; consensusDefect: AiConsensusDefect | null; split: boolean; inconclusive: boolean; estimatedNeurons: number; reviewerCount: number }; + | { status: "ok"; advisoryNotes: string | null; consensusDefect: AiConsensusDefect | null; split: boolean; inconclusive: boolean; estimatedNeurons: number; reviewerCount: number; inlineFindings: InlineFinding[] }; + +/** A line-anchored review finding the model can emit for quiet inline PR comments (#inline-comments). `line` is + * the 1-based line number in the NEW (post-change) file; `severity` separates a must-fix from a nit. The body + * is made public-safe before it ever leaves the engine (see {@link composeInlineFindings}). */ +export type InlineFinding = { path: string; line: number; severity: "blocker" | "nit"; body: string }; export type ModelReview = { assessment: string; @@ -147,6 +159,9 @@ export type ModelReview = { blockers: string[]; nits: string[]; suggestions: string[]; + // Line-anchored findings for inline PR review comments (#inline-comments). ALWAYS present (parseModelReview + // sets []); populated only when the caller asked for them (input.inlineFindings) AND the model emitted any. + inlineFindings: InlineFinding[]; }; type AiGatewayOptions = { gateway?: { id: string } }; @@ -262,12 +277,31 @@ export function parseModelReview(text: string): ModelReview | null { const obj = JSON.parse(jsonText) as Record; const toList = (value: unknown): string[] => Array.isArray(value) ? value.filter((x): x is string => typeof x === "string").map((x) => x.trim()).filter(Boolean).slice(0, 6) : []; + // Fail-safe: a malformed/absent inlineFindings field degrades to []; each item missing a usable path / a + // positive line / a body is skipped, never partial. Severity defaults to "nit" unless it's exactly "blocker". + const toInlineFindings = (value: unknown): InlineFinding[] => + Array.isArray(value) + ? value + .flatMap((item): InlineFinding[] => { + if (!item || typeof item !== "object") return []; + const o = item as Record; + const path = typeof o.path === "string" ? o.path.trim() : ""; + // JSON numbers are always finite (NaN/Infinity can't appear), so a numeric `line` is real; trunc a + // float, and the `line > 0` guard below drops 0/negative anchors. + const line = typeof o.line === "number" ? Math.trunc(o.line) : 0; + const body = typeof o.body === "string" ? o.body.trim() : ""; + const severity: "blocker" | "nit" = o.severity === "blocker" ? "blocker" : "nit"; + return path && line > 0 && body ? [{ path, line, severity, body }] : []; + }) + .slice(0, 20) + : []; const assessment = typeof obj.assessment === "string" ? obj.assessment.trim() : ""; const blockers = toList(obj.blockers); const nits = toList(obj.nits); const suggestions = toList(obj.suggestions); + const inlineFindings = toInlineFindings(obj.inlineFindings); if (!assessment && blockers.length === 0 && nits.length === 0 && suggestions.length === 0) return null; - return { assessment, blockers, nits, suggestions }; + return { assessment, blockers, nits, suggestions, inlineFindings }; } catch { return null; } @@ -304,16 +338,23 @@ const REVIEW_PROFILE_SUFFIX: Record<"chill" | "assertive", string> = { "\n\nReview profile: ASSERTIVE. Beyond blocking defects, also surface minor improvements, style/consistency suggestions, and nitpicks — be thorough and exacting, clearly marking each non-blocking item as a nit.", }; +// `.gittensory.yml` review.inline_comments → an appended instruction to ALSO emit line-anchored findings for +// quiet inline PR comments (#inline-comments). Absent/off appends nothing (byte-identical). The model keeps the +// existing 4-field shape and simply ADDS an `inlineFindings` array. +const INLINE_FINDINGS_SUFFIX = + '\n\nINLINE FINDINGS: ALSO include an additional top-level field "inlineFindings" in the SAME JSON object — an array (possibly empty) of your most important findings, each anchored to a specific changed line, for inline PR comments. Each item: {"path": the changed file path EXACTLY as shown in the diff, "line": the 1-based line number in the NEW file (count forward from the "+" start in the nearest "@@ -old +new @@" hunk header) of an ADDED ("+") line you are commenting on, "severity": "blocker" or "nit", "body": the one-sentence finding}. Include ONLY findings you can place on a specific added line; OMIT any you cannot anchor precisely (a wrong line is worse than none). At most ~10 items.'; + /** The effective reviewer SYSTEM prompt. Appends the grounding-discipline suffix when the caller supplied one - * (flag GITTENSORY_REVIEW_GROUNDING on), then the `review.profile` tone suffix when set; both absent (default) - * → the base prompt, byte-identical to today. */ + * (flag GITTENSORY_REVIEW_GROUNDING on), the `review.profile` tone suffix when set, then the inline-findings + * instruction when the caller asked for them; all absent (default) → the base prompt, byte-identical to today. */ function buildSystemPrompt(input: GittensoryAiReviewInput): string { const groundingSuffix = input.grounding?.systemSuffix ?? ""; const profileSuffix = input.profile === "chill" || input.profile === "assertive" ? REVIEW_PROFILE_SUFFIX[input.profile] : ""; // `.gittensory.yml` review.path_instructions (#review-path-instructions): the caller pre-resolved the entries // matching this PR's files into a prompt section; empty ⇒ nothing appended (byte-identical). const pathSuffix = input.pathGuidance?.trim() ? input.pathGuidance : ""; - return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${profileSuffix}${pathSuffix}`; + const inlineSuffix = input.inlineFindings ? INLINE_FINDINGS_SUFFIX : ""; + return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${profileSuffix}${pathSuffix}${inlineSuffix}`; } /** One Workers-AI opinion with a per-slot reliable fallback and a 3× retry on the primary. */ @@ -448,6 +489,28 @@ export function composeAdvisoryNotes(reviews: ModelReview[]): string | null { return lines.join("\n").trim(); } +/** Hard cap on inline findings surfaced per review — a focused review leaves a handful of precise inline notes, + * not a wall of them (the prompt also asks the model to be selective). (#inline-comments) */ +const INLINE_FINDINGS_LIMIT = 10; + +/** Compose the public-safe, deduped, capped inline findings from one or two model reviews — the line-anchored + * counterpart of {@link composeAdvisoryNotes}. Dedupes by path+line (first wins), drops any body that fails the + * public-safe filter, and caps the total. Empty array when there is nothing safe to anchor. (#inline-comments) */ +export function composeInlineFindings(reviews: ModelReview[]): InlineFinding[] { + const seen = new Set(); + const out: InlineFinding[] = []; + for (const finding of reviews.flatMap((r) => r.inlineFindings)) { + if (out.length >= INLINE_FINDINGS_LIMIT) break; + const key = `${finding.path}:${finding.line}`; + if (seen.has(key)) continue; + const safeBody = toPublicSafe(finding.body); + if (!safeBody) continue; + seen.add(key); + out.push({ ...finding, body: safeBody }); + } + return out; +} + /** A CONSENSUS defect = BOTH reviews independently name at least one concrete blocker (the severity-disciplined * reviewbot model: a lone blocker in a dual review is a split, not a hard block). Requiring two independent * models to AGREE is itself the precision mechanism — the free Workers-AI models emit no calibrated confidence @@ -635,6 +698,9 @@ export async function runGittensoryAiReview(env: Env, input: GittensoryAiReviewI const reviewsForNotes = [advisoryReview, secondReview].filter((r): r is ModelReview => Boolean(r)); const advisoryNotes = reviewsForNotes.length > 0 ? composeAdvisoryNotes(reviewsForNotes) : null; + // Line-anchored inline findings (#inline-comments): empty unless the caller asked for them (the prompt suffix + // is conditional) AND the model emitted any. Inert until the posting path (PR B) consumes them. + const inlineFindings = composeInlineFindings(reviewsForNotes); await record(env, input, "ok", estimatedNeurons, consensusDefect ? "consensus defect" : aiReviewSplit ? "split" : inconclusive ? "inconclusive — held" : advisoryNotes ? "advisory notes" : "no usable output", { mode: input.mode, @@ -644,7 +710,7 @@ export async function runGittensoryAiReview(env: Env, input: GittensoryAiReviewI inconclusive, ...(byokFailure ? { byokFailure } : {}), }); - return { status: "ok", advisoryNotes, consensusDefect, split: aiReviewSplit, inconclusive, estimatedNeurons, reviewerCount: reviewsForNotes.length }; + return { status: "ok", advisoryNotes, consensusDefect, split: aiReviewSplit, inconclusive, estimatedNeurons, reviewerCount: reviewsForNotes.length, inlineFindings }; } async function record( @@ -672,6 +738,7 @@ export const __aiReviewInternals = { parseModelReview, coerceAiText, composeAdvisoryNotes, + composeInlineFindings, consensusDefectOf, combineReviews, synthesizeDefect, diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 31c010aa52..9e39a45af8 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -104,6 +104,11 @@ export type FocusManifestReviewConfig = { fields: Partial>; /** `review.profile`: chill / balanced / assertive. null (absent) = balanced = byte-identical reviewer prompt. */ profile: ReviewProfile | null; + /** `review.inline_comments`: when true, the AI reviewer ALSO leaves quiet, non-blocking inline PR comments on + * specific changed lines (in addition to the decision summary). null/false (default, absent) = no inline + * comments = byte-identical behavior. Operator-gated too (GITTENSORY_REVIEW_INLINE_COMMENTS + allowlist). + * (#inline-comments) */ + inlineComments: 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[]; @@ -232,7 +237,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }, warnings: [], }; @@ -245,7 +250,7 @@ export function isFocusManifestPublicSafe(text: string): boolean { } function emptyManifest(source: FocusManifestSource, warnings: string[] = []): FocusManifest { - return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] } }; + return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] } }; } function normalizeStringList(value: JsonValue | undefined, field: string, warnings: string[]): string[] { @@ -533,7 +538,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: {}, profile: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }; 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.`); @@ -554,6 +559,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const footerText = footerRecord ? parsePublicSafeText(footerRecord.text, "review.footer.text", warnings) : null; const note = parsePublicSafeText(r.note, "review.note", warnings); const profile = parseReviewProfile(r.profile, warnings); + const inlineComments = normalizeOptionalBoolean(r.inline_comments, "review.inline_comments", warnings); const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); const excludePaths = parseReviewExcludePaths(r.exclude_paths, warnings); const preMergeChecks = parseReviewPreMergeChecks(r.pre_merge_checks, warnings); @@ -562,6 +568,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo footerText !== null || note !== null || profile !== null || + inlineComments !== null || pathInstructions.length > 0 || excludePaths.length > 0 || preMergeChecks.length > 0 || @@ -570,6 +577,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo note, fields, profile, + inlineComments, pathInstructions, excludePaths, preMergeChecks, @@ -714,6 +722,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.footerText !== null) out.footer = { text: review.footerText }; if (review.note !== null) out.note = review.note; if (review.profile !== null) out.profile = review.profile; + if (review.inlineComments !== null) out.inline_comments = review.inlineComments; 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]; if (review.preMergeChecks.length > 0) { @@ -749,8 +758,10 @@ export function resolveReviewPathInstructions(pathInstructions: ReviewPathInstru * 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-path-instructions / #review-exclude-paths) */ -export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; pathInstructions: ReviewPathInstruction[]; excludePaths: string[] } { - return { profile: manifest?.review.profile ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], excludePaths: manifest?.review.excludePaths ?? [] }; +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; inlineComments: boolean; pathInstructions: ReviewPathInstruction[]; excludePaths: string[] } { + // 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. + return { profile: manifest?.review.profile ?? null, inlineComments: manifest?.review.inlineComments === true, pathInstructions: manifest?.review.pathInstructions ?? [], excludePaths: manifest?.review.excludePaths ?? [] }; } /** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 374876017f..8bf53a52b0 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -7,7 +7,11 @@ import { } from "../../src/services/ai-review"; import { createTestEnv } from "../helpers/d1"; -const { parseModelReview, coerceAiText, composeAdvisoryNotes, consensusDefectOf, combineReviews, toPublicSafe, runWorkersOpinion } = __aiReviewInternals; +const { parseModelReview, coerceAiText, composeAdvisoryNotes, composeInlineFindings, consensusDefectOf, combineReviews, toPublicSafe, runWorkersOpinion } = __aiReviewInternals; + +type InlineFinding = { path: string; line: number; severity: "blocker" | "nit"; body: string }; +type ModelReviewShape = { assessment: string; blockers: string[]; nits: string[]; suggestions: string[]; inlineFindings: InlineFinding[] }; +const reviewWithFindings = (inlineFindings: InlineFinding[]): ModelReviewShape => ({ assessment: "", blockers: [], nits: [], suggestions: [], inlineFindings }); function reviewJson(over: Partial<{ assessment: string; suggestions: string[]; nits: string[]; blockers: string[]; present: boolean; confidence: number; title: string; detail: string }> = {}): string { return JSON.stringify({ @@ -165,6 +169,20 @@ describe("review.profile shapes the reviewer system prompt (#review-profile)", ( expect(await runGuidance(undefined)).not.toContain("Path-specific review instructions"); expect(await runGuidance(" ")).not.toContain("Path-specific review instructions"); }); + + it("the inline-findings instruction is appended to the system prompt ONLY when requested (#inline-comments)", async () => { + const systemPromptOf = (run: ReturnType): string => ((run.mock.calls[0]?.[1] as { messages?: Array<{ content?: string }> })?.messages?.[0]?.content ?? ""); + const runInline = async (inlineFindings: boolean | undefined) => { + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ AI: { run } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000" }); + await runGittensoryAiReview(env, { ...baseInput, inlineFindings }); + return systemPromptOf(run); + }; + expect(await runInline(true)).toContain("INLINE FINDINGS"); + // Absent / false ⇒ byte-identical prompt (no inline instruction). + expect(await runInline(false)).not.toContain("INLINE FINDINGS"); + expect(await runInline(undefined)).not.toContain("INLINE FINDINGS"); + }); }); describe("runGittensoryAiReview block mode (consensus)", () => { @@ -406,7 +424,7 @@ describe("pure helpers", () => { }); describe("combineReviews (#dual-ai-combiner)", () => { - const r = (blockers: string[]) => ({ assessment: "", suggestions: [], nits: [], blockers }); + const r = (blockers: string[]) => ({ assessment: "", suggestions: [], nits: [], blockers, inlineFindings: [] }); const clean = r([]); const blocked = r(["Null deref in src/a.ts"]); @@ -446,7 +464,7 @@ describe("pure helpers", () => { }); it("consensusDefectOf requires a concrete blocker in BOTH reviews and drops unsafe titles", () => { - const r = (blockers: string[]) => ({ assessment: "", suggestions: [], nits: [], blockers }); + const r = (blockers: string[]) => ({ assessment: "", suggestions: [], nits: [], blockers, inlineFindings: [] }); expect(consensusDefectOf(r(["Null deref in src/a.ts"]), r(["Null deref in src/a.ts"]))).not.toBeNull(); expect(consensusDefectOf(r([]), r(["Null deref"]))).toBeNull(); // one has no blocker → split, not consensus expect(consensusDefectOf(r(["Null deref"]), r([]))).toBeNull(); @@ -454,13 +472,13 @@ describe("pure helpers", () => { }); it("consensusDefectOf falls back to b's blocker when a's is blank", () => { - const a = { assessment: "", suggestions: [], nits: [], blockers: [""] }; - const b = { assessment: "", suggestions: [], nits: [], blockers: ["Race condition in src/x.ts"] }; + const a = { assessment: "", suggestions: [], nits: [], blockers: [""], inlineFindings: [] }; + const b = { assessment: "", suggestions: [], nits: [], blockers: ["Race condition in src/x.ts"], inlineFindings: [] }; expect(consensusDefectOf(a, b)?.title).toBe("Race condition in src/x.ts"); }); it("consensusDefectOf uses the default title + detail when BOTH reviewers' blockers are blank", () => { - const blank = { assessment: "", suggestions: [], nits: [], blockers: [""] }; + const blank = { assessment: "", suggestions: [], nits: [], blockers: [""], inlineFindings: [] }; const out = consensusDefectOf(blank, { ...blank, blockers: [""] }); expect(out?.title).toContain("AI reviewers agree"); // both blockers[0] falsy → default title expect(out?.detail).toContain("independently flagged"); // joined detail empty → default detail @@ -484,11 +502,76 @@ describe("pure helpers", () => { }); it("composeAdvisoryNotes returns null when nothing is public-safe", () => { - expect(composeAdvisoryNotes([{ assessment: "reward payout farming", suggestions: ["payout"], nits: ["reward"], blockers: [] }])).toBeNull(); + expect(composeAdvisoryNotes([{ assessment: "reward payout farming", suggestions: ["payout"], nits: ["reward"], blockers: [], inlineFindings: [] }])).toBeNull(); + }); + + it("parseModelReview parses well-formed inline findings; severity defaults to nit unless exactly 'blocker' (#inline-comments)", () => { + const json = JSON.stringify({ + assessment: "ok", blockers: [], nits: [], suggestions: [], + inlineFindings: [ + { path: "src/a.ts", line: 12, severity: "blocker", body: "Null deref." }, + { path: "src/b.ts", line: 3, severity: "whatever", body: "Rename x." }, + ], + }); + expect(parseModelReview(json)?.inlineFindings).toEqual([ + { path: "src/a.ts", line: 12, severity: "blocker", body: "Null deref." }, + { path: "src/b.ts", line: 3, severity: "nit", body: "Rename x." }, + ]); + }); + + it("parseModelReview drops malformed inline findings (non-object / missing path|line|body / non-positive line), never partial", () => { + const json = JSON.stringify({ + assessment: "ok", blockers: [], nits: [], suggestions: [], + inlineFindings: [ + null, + "nope", + { line: 5, body: "no path" }, + { path: "src/a.ts", body: "no line" }, + { path: "src/c.ts", line: 7 }, + { path: "src/a.ts", line: 0, body: "zero line" }, + { path: "src/a.ts", line: 2.9, severity: "nit", body: "kept (truncated)" }, + ], + }); + expect(parseModelReview(json)?.inlineFindings).toEqual([{ path: "src/a.ts", line: 2, severity: "nit", body: "kept (truncated)" }]); + }); + + it("parseModelReview defaults inline findings to [] when absent or not an array", () => { + expect(parseModelReview(JSON.stringify({ assessment: "ok", blockers: [], nits: [], suggestions: [] }))?.inlineFindings).toEqual([]); + expect(parseModelReview(JSON.stringify({ assessment: "ok", blockers: [], nits: [], suggestions: [], inlineFindings: "nope" }))?.inlineFindings).toEqual([]); + }); + + it("composeInlineFindings dedupes by path+line (first wins) and drops public-unsafe bodies (#inline-comments)", () => { + const out = composeInlineFindings([ + reviewWithFindings([ + { path: "src/a.ts", line: 1, severity: "nit", body: "First." }, + { path: "src/a.ts", line: 1, severity: "blocker", body: "Duplicate line — dropped." }, + { path: "src/a.ts", line: 2, severity: "nit", body: "reward payout farming" }, + { path: "src/b.ts", line: 9, severity: "blocker", body: "Keep me." }, + ]), + ]); + expect(out).toEqual([ + { path: "src/a.ts", line: 1, severity: "nit", body: "First." }, + { path: "src/b.ts", line: 9, severity: "blocker", body: "Keep me." }, + ]); + }); + + it("composeInlineFindings caps the total at 10 across reviewers, and returns [] for no reviews", () => { + const many = Array.from({ length: 14 }, (_, i): InlineFinding => ({ path: `src/f${i}.ts`, line: i + 1, severity: "nit", body: `Body ${i}` })); + expect(composeInlineFindings([reviewWithFindings(many)])).toHaveLength(10); + expect(composeInlineFindings([])).toEqual([]); + }); + + it("runGittensoryAiReview emits composed inline findings only when the caller asks for them (#inline-comments)", async () => { + const json = JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [], inlineFindings: [{ path: "src/a.ts", line: 3, severity: "nit", body: "Guard the empty case." }] }); + const run = vi.fn(async () => ({ response: json })); + const env = createTestEnv({ AI: { run } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000" }); + const result = await runGittensoryAiReview(env, { ...baseInput, inlineFindings: true }); + expect(result.status).toBe("ok"); + if (result.status === "ok") expect(result.inlineFindings).toEqual([{ path: "src/a.ts", line: 3, severity: "nit", body: "Guard the empty case." }]); }); it("composeAdvisoryNotes renders only the sections that have public-safe content", () => { - const review = (over: Partial<{ assessment: string; suggestions: string[]; nits: string[]; blockers: string[] }>) => ({ assessment: over.assessment ?? "", suggestions: over.suggestions ?? [], nits: over.nits ?? [], blockers: over.blockers ?? [] }); + const review = (over: Partial<{ assessment: string; suggestions: string[]; nits: string[]; blockers: string[] }>) => ({ assessment: over.assessment ?? "", suggestions: over.suggestions ?? [], nits: over.nits ?? [], blockers: over.blockers ?? [], inlineFindings: [] }); const assessmentOnly = composeAdvisoryNotes([review({ assessment: "Looks good." })]); expect(assessmentOnly).toBe("Looks good."); const nitsOnly = composeAdvisoryNotes([review({ nits: ["Add a test."] })]); @@ -501,8 +584,8 @@ describe("pure helpers", () => { }); it("composeAdvisoryNotes merges + dedupes blockers/nits across two reviewers and renders both sections", () => { - const a = { assessment: "Solid change.", suggestions: ["Add a test."], nits: ["Rename x."], blockers: ["Null deref in src/a.ts."] }; - const b = { assessment: "Second look.", suggestions: ["Add a test."], nits: ["Rename x.", "Tighten the type."], blockers: ["Null deref in src/a.ts.", "Off-by-one in the loop bound."] }; + const a = { assessment: "Solid change.", suggestions: ["Add a test."], nits: ["Rename x."], blockers: ["Null deref in src/a.ts."], inlineFindings: [] }; + const b = { assessment: "Second look.", suggestions: ["Add a test."], nits: ["Rename x.", "Tighten the type."], blockers: ["Null deref in src/a.ts.", "Off-by-one in the loop bound."], inlineFindings: [] }; const out = composeAdvisoryNotes([a, b]) ?? ""; expect(out).toContain("Solid change."); // first reviewer's assessment wins expect(out).toContain("**Blockers**"); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 316e7682a5..0d992e1b8c 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -459,7 +459,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, firstTimeContributorGrace: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }, warnings: [], }); expect(policy.publicSafe.entryGuidance).toContain("Keep PRs focused."); @@ -1169,10 +1169,30 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { }); it("resolveReviewPromptOverrides: non-null manifest passes the config through; null manifest → defaults", () => { - const manifest = parseFocusManifest({ review: { profile: "chill", path_instructions: [{ path: "src/**", instructions: "be strict" }], exclude_paths: ["**/*.lock"] } }); - expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", pathInstructions: [{ path: "src/**", instructions: "be strict" }], excludePaths: ["**/*.lock"] }); - // A null manifest (load failure) yields the byte-identical defaults. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, pathInstructions: [], excludePaths: [] }); + const manifest = parseFocusManifest({ review: { profile: "chill", inline_comments: true, path_instructions: [{ path: "src/**", instructions: "be strict" }], exclude_paths: ["**/*.lock"] } }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", inlineComments: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], excludePaths: ["**/*.lock"] }); + // A null manifest (load failure) yields the byte-identical defaults; inline comments default OFF. + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, inlineComments: false, pathInstructions: [], excludePaths: [] }); + // 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); + }); + + it("parses review.inline_comments (default OFF), marks present, round-trips, and warns on a non-boolean (#inline-comments)", () => { + expect(parseFocusManifest({ review: { inline_comments: true } }).review.inlineComments).toBe(true); + const on = parseFocusManifest({ review: { inline_comments: true } }); + expect(on.review.present).toBe(true); // an inline-comments-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: { inline_comments: false } }); + expect(off.review.inlineComments).toBe(false); + expect(off.review.present).toBe(true); + // Absent ⇒ null (the byte-identical default), config not present. + expect(parseFocusManifest({ review: {} }).review.inlineComments).toBeNull(); + // A non-boolean is ignored with a warning. + const bad = parseFocusManifest({ review: { inline_comments: "yes" } }); + expect(bad.review.inlineComments).toBeNull(); + expect(bad.warnings.some((w) => /review\.inline_comments.*must be a boolean/.test(w))).toBe(true); }); }); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 4627f0c4fa..393a587e72 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -904,7 +904,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 }, profile: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, profile: null, inlineComments: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }, aiReview: { notes: "The change is focused.\n\n**Suggestions**\n- Add a test for the edge case." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead