diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 49ab5c7152..172cc92f8f 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -454,7 +454,7 @@ review: # suggestions: false # When true, inline findings with precise fixes render as one-click suggestion blocks (requires inline_comments). # finding_categories: false - # When true, inline findings are tagged with a category label (requires inline_comments). + # inline_comments_per_category: 3 # Fix-handoff blocks (#2176). Bool | null. Default: null/false — byte-identical. # Requires operator flag GITTENSORY_REVIEW_FIX_HANDOFF + cutover allowlist AND this toggle. diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index c12744dc63..9601fa9965 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -467,7 +467,7 @@ review: # suggestions: false # When true, inline findings with precise fixes render as one-click suggestion blocks (requires inline_comments). # finding_categories: false - # When true, inline findings are tagged with a category label (requires inline_comments). + # inline_comments_per_category: 3 # Fix-handoff blocks (#2176). Bool | null. Default: null/false — byte-identical. # Requires operator flag GITTENSORY_REVIEW_FIX_HANDOFF + cutover allowlist AND this toggle. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8fc8791851..f4dcaa7b2a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -8004,6 +8004,7 @@ async function maybePublishPrPublicSurface( let reviewMemoryEnabledForReview = false; let findingCategoriesEnabledForReview = false; let minFindingSeverityForReview: ReviewFindingSeverity | null = null; + let inlineCommentsPerCategoryForReview: number | null = null; let aiReviewExpected = false; let aiReviewWasReused = false; let gateFinalized = false; @@ -8515,6 +8516,7 @@ async function maybePublishPrPublicSurface( changedFilesSummaryEnabledForReview = deterministicReviewOverrides.changedFilesSummary; effortScoreEnabledForReview = deterministicReviewOverrides.effortScore; minFindingSeverityForReview = deterministicReviewOverrides.minFindingSeverity; + inlineCommentsPerCategoryForReview = deterministicReviewOverrides.inlineCommentsPerCategory; // review.memory (#2179, part of #1964): deterministic, no-AI -- resolved the same unconditional way as // changed_files_summary/effort_score above (must apply even when the AI review itself is skipped this // pass). ANDed with the operator's GITTENSORY_REVIEW_MEMORY kill-switch at the actual apply site below @@ -9884,6 +9886,7 @@ async function maybePublishPrPublicSurface( suggestionsEnabled: suggestionsEnabledForReview, categoriesEnabled: findingCategoriesEnabledForReview, minFindingSeverity: minFindingSeverityForReview, + perCategoryCap: inlineCommentsPerCategoryForReview, }); } if (decision.willLabel) { diff --git a/src/review/inline-comments-select.ts b/src/review/inline-comments-select.ts new file mode 100644 index 0000000000..c52b88d647 --- /dev/null +++ b/src/review/inline-comments-select.ts @@ -0,0 +1,120 @@ +/** Pure inline-comment selection with optional per-category caps (#2159). */ + +import { classifyFindingCategory, type FindingCategory } from "./finding-category-classify"; +import { shouldShowInlineFinding } from "./finding-severity-filter"; +import type { InlineFinding } from "../services/ai-review"; +import type { ReviewFindingSeverity } from "../signals/focus-manifest"; +import type { PullRequestFileRecord } from "../types"; + +export const DEFAULT_MAX_INLINE_COMMENTS = 10; + +/** PURE: the set of NEW-file (RIGHT-side) line numbers a unified-diff patch makes commentable. */ +export function rightSideLinesFromPatch(patch: string): Set { + const lines = new Set(); + let right = 0; + for (const raw of patch.split("\n")) { + const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw); + if (header?.[1]) { + right = Number.parseInt(header[1], 10); + continue; + } + if (right === 0) continue; + const marker = raw[0]; + if (marker === undefined || marker === "-" || marker === "\\") continue; + lines.add(right); + right += 1; + } + return lines; +} + +/** Higher-priority categories survive per-category and total caps first (#2159). */ +const INLINE_COMMENT_CATEGORY_PRIORITY: Record = { + security: 0, + correctness: 1, + performance: 2, + maintainability: 3, + tests: 4, + style: 5, +}; + +export function inlineFindingCategory(finding: InlineFinding): FindingCategory { + return finding.category ?? classifyFindingCategory(finding); +} + +/** Lower rank sorts earlier. Blockers always beat nits; ties break on category priority. */ +export function compareInlineFindingPriority(left: InlineFinding, right: InlineFinding): number { + const leftSeverity = left.severity === "blocker" ? 0 : 1; + const rightSeverity = right.severity === "blocker" ? 0 : 1; + if (leftSeverity !== rightSeverity) return leftSeverity - rightSeverity; + const leftCategory = INLINE_COMMENT_CATEGORY_PRIORITY[inlineFindingCategory(left)]; + const rightCategory = INLINE_COMMENT_CATEGORY_PRIORITY[inlineFindingCategory(right)]; + return leftCategory - rightCategory; +} + +export type InlineCommentSelectOptions = { + suggestionsEnabled?: boolean | undefined; + categoriesEnabled?: boolean | undefined; + minFindingSeverity?: ReviewFindingSeverity | null | undefined; + /** When unset, preserve first-seen order with only the total cap (#2159 default-off). */ + perCategoryCap?: number | null | undefined; + maxComments?: number | undefined; +}; + +type AnchoredInlineFinding = { finding: InlineFinding; index: number }; + +function anchorableInlineFindings( + findings: InlineFinding[], + files: Pick[], + minFindingSeverity: ReviewFindingSeverity | null | undefined, +): AnchoredInlineFinding[] { + const rightLinesByPath = new Map>(); + for (const file of files) { + const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; + if (patch) rightLinesByPath.set(file.path, rightSideLinesFromPatch(patch)); + } + const out: AnchoredInlineFinding[] = []; + const seen = new Set(); + for (let index = 0; index < findings.length; index++) { + const finding = findings[index]!; + if (!shouldShowInlineFinding(finding.severity, minFindingSeverity)) continue; + const validLines = rightLinesByPath.get(finding.path); + if (!validLines || !validLines.has(finding.line)) continue; + const key = `${finding.path}:${finding.line}`; + if (seen.has(key)) continue; + seen.add(key); + out.push({ finding, index }); + } + return out; +} + +/** Select anchorable inline findings, optionally applying a per-category sub-cap before the total cap. */ +export function selectAnchoredInlineFindings( + findings: InlineFinding[], + files: Pick[], + options: InlineCommentSelectOptions, +): InlineFinding[] { + const anchored = anchorableInlineFindings(findings, files, options.minFindingSeverity); + const maxComments = options.maxComments ?? DEFAULT_MAX_INLINE_COMMENTS; + const perCategoryCap = options.perCategoryCap; + const ordered = + perCategoryCap == null + ? anchored + : [...anchored].sort((left, right) => { + const byPriority = compareInlineFindingPriority(left.finding, right.finding); + if (byPriority !== 0) return byPriority; + return left.index - right.index; + }); + const perCategoryCounts = new Map(); + const out: InlineFinding[] = []; + for (const { finding } of ordered) { + if (out.length >= maxComments) break; + if (perCategoryCap != null) { + const category = inlineFindingCategory(finding); + const count = perCategoryCounts.get(category) ?? 0; + if (count >= perCategoryCap) continue; + perCategoryCounts.set(category, count + 1); + } + out.push(finding); + } + return out; +} diff --git a/src/review/inline-comments.ts b/src/review/inline-comments.ts index cda032cb5d..977687f0f0 100644 --- a/src/review/inline-comments.ts +++ b/src/review/inline-comments.ts @@ -11,7 +11,8 @@ import { createPullRequestReviewComments } from "../github/pr-actions"; import { isConvergenceRepoAllowed } from "./cutover-gate"; import { classifyFindingCategory } from "./finding-category-classify"; -import { shouldShowInlineFinding } from "./finding-severity-filter"; +import { selectAnchoredInlineFindings } from "./inline-comments-select"; +export { rightSideLinesFromPatch } from "./inline-comments-select"; import type { InlineFinding } from "../services/ai-review"; import type { ReviewFindingSeverity } from "../signals/focus-manifest"; import type { AgentActionMode } from "../settings/agent-execution"; @@ -63,32 +64,6 @@ export type ReviewInlineComment = { path: string; line: number; side: "RIGHT"; b /** Hard cap on inline comments posted per PR review — a focused review leaves a handful of precise notes, not a * wall (the model is also asked to be selective, and composeInlineFindings already caps at 10). */ -const MAX_INLINE_COMMENTS = 10; - -/** PURE: the set of NEW-file (RIGHT-side) line numbers a unified-diff patch makes commentable — every added - * ("+") and context (" ") line inside a hunk. GitHub 422s an inline comment whose line is NOT one of these, so - * {@link selectInlineComments} validates each finding against this set. Deleted ("-") lines are LEFT-side only - * and excluded; the "\ No newline at end of file" marker is skipped. Mirrors firstAddedLineFromPatch's - * hunk-header regex (advisory.ts). */ -export function rightSideLinesFromPatch(patch: string): Set { - const lines = new Set(); - let right = 0; - for (const raw of patch.split("\n")) { - const header = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(raw); - if (header?.[1]) { - right = Number.parseInt(header[1], 10); - continue; - } - if (right === 0) continue; // preamble before the first hunk header - const marker = raw[0]; - // `undefined` ⇒ an empty "" element (a trailing-newline split artifact, NOT a real diff line — a blank - // context line is " ", a single space); "-" ⇒ deleted (LEFT side only); "\\" ⇒ the "no newline" marker. - if (marker === undefined || marker === "-" || marker === "\\") continue; - lines.add(right); // added ("+") or context (" ") line → occupies a RIGHT-side line number - right += 1; - } - return lines; -} /** 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 @@ -128,25 +103,18 @@ export function selectInlineComments( suggestionsEnabled = false, categoriesEnabled = false, minFindingSeverity: ReviewFindingSeverity | null | undefined = null, + perCategoryCap: number | null | undefined = null, ): ReviewInlineComment[] { - const rightLinesByPath = new Map>(); - for (const file of files) { - const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - if (patch) rightLinesByPath.set(file.path, rightSideLinesFromPatch(patch)); - } - const out: ReviewInlineComment[] = []; - const seen = new Set(); - for (const finding of findings) { - if (!shouldShowInlineFinding(finding.severity, minFindingSeverity)) continue; - if (out.length >= MAX_INLINE_COMMENTS) break; - const validLines = rightLinesByPath.get(finding.path); - if (!validLines || !validLines.has(finding.line)) continue; // not a commentable diff line → drop (no 422) - const key = `${finding.path}:${finding.line}`; - if (seen.has(key)) continue; - seen.add(key); - out.push({ path: finding.path, line: finding.line, side: "RIGHT", body: formatInlineBody(finding, suggestionsEnabled, categoriesEnabled) }); - } - return out; + const selected = selectAnchoredInlineFindings(findings, files, { + minFindingSeverity, + perCategoryCap, + }); + return selected.map((finding) => ({ + path: finding.path, + line: finding.line, + side: "RIGHT" as const, + body: formatInlineBody(finding, suggestionsEnabled, categoriesEnabled), + })); } /** Post the model's inline findings as ONE quiet, non-blocking review (`event: COMMENT`) on the PR. Fully @@ -166,6 +134,7 @@ export async function postInlineReviewComments( suggestionsEnabled?: boolean | undefined; categoriesEnabled?: boolean | undefined; minFindingSeverity?: ReviewFindingSeverity | null | undefined; + perCategoryCap?: number | null | undefined; }, ): Promise<{ posted: number }> { const comments = selectInlineComments( @@ -174,6 +143,7 @@ export async function postInlineReviewComments( args.suggestionsEnabled, args.categoriesEnabled, args.minFindingSeverity, + args.perCategoryCap, ); if (comments.length === 0 || !args.commitId) return { posted: 0 }; try { @@ -205,6 +175,7 @@ export async function maybePostInlineComments( suggestionsEnabled?: boolean | undefined; categoriesEnabled?: boolean | undefined; minFindingSeverity?: ReviewFindingSeverity | null | undefined; + perCategoryCap?: number | null | undefined; }, ): Promise { if (!args.inlineCommentsEnabled) return; @@ -221,5 +192,6 @@ export async function maybePostInlineComments( suggestionsEnabled: args.suggestionsEnabled, categoriesEnabled: args.categoriesEnabled, minFindingSeverity: args.minFindingSeverity, + perCategoryCap: args.perCategoryCap, }); } diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 136a156b92..c33267320b 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -418,6 +418,10 @@ export type FocusManifestReviewConfig = { * ADDITIONAL opt-in on top of `review.inline_comments`, not a replacement gate, mirroring `review.suggestions`. * null/false (default, absent) = no category tagging = byte-identical behavior. (#1958) */ findingCategories: boolean | null; + /** `review.inline_comments_per_category`: optional per-category sub-cap applied before the total inline-comment + * cap so one category (e.g. style) cannot crowd out security/correctness findings. null (default, absent) ⇒ + * byte-identical first-seen selection with only the hard total cap. (#2159) */ + inlineCommentsPerCategory: number | null; /** `review.min_finding_severity`: display-only floor for AI findings with a severity tier. Findings below the * configured level are suppressed from inline comments — never from gate blockers. null (default, absent) ⇒ every * finding shown = byte-identical behavior. (#2048) */ @@ -783,7 +787,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, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -814,7 +818,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, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1791,7 +1795,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, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }; 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.`); @@ -1838,6 +1842,11 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const cultureProfile = normalizeOptionalBoolean(r.culture_profile, "review.culture_profile", warnings); const reviewMemory = normalizeOptionalBoolean(r.memory, "review.memory", warnings); const findingCategories = normalizeOptionalBoolean(r.finding_categories, "review.finding_categories", warnings); + const inlineCommentsPerCategory = normalizeOptionalNonNegativeInt( + r.inline_comments_per_category, + "review.inline_comments_per_category", + warnings, + ); const minFindingSeverity = normalizeOptionalEnum( r.min_finding_severity, "review.min_finding_severity", @@ -1874,6 +1883,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo cultureProfile !== null || reviewMemory !== null || findingCategories !== null || + inlineCommentsPerCategory !== null || minFindingSeverity !== null || maxFindingsPresent(maxFindings) || commentVerbosity !== null || @@ -1911,6 +1921,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo cultureProfile, reviewMemory, findingCategories, + inlineCommentsPerCategory, minFindingSeverity, maxFindings, commentVerbosity, @@ -2377,6 +2388,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.cultureProfile !== null) out.culture_profile = review.cultureProfile; if (review.reviewMemory !== null) out.memory = review.reviewMemory; if (review.findingCategories !== null) out.finding_categories = review.findingCategories; + if (review.inlineCommentsPerCategory !== null) out.inline_comments_per_category = review.inlineCommentsPerCategory; if (review.minFindingSeverity !== null) out.min_finding_severity = review.minFindingSeverity; if (maxFindingsPresent(review.maxFindings)) { const maxFindings: Record = {}; @@ -2604,7 +2616,7 @@ export function composeManifestReviewInstructions(instructions: string | null, t * 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 / #1956) */ -export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; suggestions: boolean; changedFilesSummary: boolean; effortScore: boolean; impactMap: boolean; cultureProfile: boolean; findingCategories: boolean; minFindingSeverity: ReviewFindingSeverity | null; maxFindings: MaxFindingsConfig; commentVerbosity: CommentVerbosity | null; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[]; selfHostAiModel: SelfHostAiModelConfig } { +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; suggestions: boolean; changedFilesSummary: boolean; effortScore: boolean; impactMap: boolean; cultureProfile: boolean; findingCategories: boolean; inlineCommentsPerCategory: number | null; minFindingSeverity: ReviewFindingSeverity | null; maxFindings: MaxFindingsConfig; commentVerbosity: CommentVerbosity | null; 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. @@ -2625,7 +2637,7 @@ export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { // cultureProfile resolves the same way (#2995) — true ONLY when the manifest explicitly set // review.culture_profile: true. The caller ANDs this per-repo opt-in with the GITTENSORY_REVIEW_CULTURE_PROFILE // global kill-switch (mirrors how RAG/reputation/grounding compose a global flag with a per-repo override). - 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, changedFilesSummary: manifest?.review.changedFilesSummary === true, effortScore: manifest?.review.effortScore === true, impactMap: manifest?.review.impactMap === true, cultureProfile: manifest?.review.cultureProfile === true, findingCategories: manifest?.review.findingCategories === true, minFindingSeverity: manifest?.review.minFindingSeverity ?? null, maxFindings: manifest?.review.maxFindings ?? { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: manifest?.review.commentVerbosity ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; + 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, changedFilesSummary: manifest?.review.changedFilesSummary === true, effortScore: manifest?.review.effortScore === true, impactMap: manifest?.review.impactMap === true, cultureProfile: manifest?.review.cultureProfile === true, findingCategories: manifest?.review.findingCategories === true, inlineCommentsPerCategory: manifest?.review.inlineCommentsPerCategory ?? null, minFindingSeverity: manifest?.review.minFindingSeverity ?? null, maxFindings: manifest?.review.maxFindings ?? { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: manifest?.review.commentVerbosity ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; } /** Resolve `review.test_generation` (#2189, config slice of #1972) from a possibly-null manifest (null = load diff --git a/test/unit/config-templates.test.ts b/test/unit/config-templates.test.ts index 911ed638fb..53612a9eaa 100644 --- a/test/unit/config-templates.test.ts +++ b/test/unit/config-templates.test.ts @@ -68,7 +68,7 @@ describe("config/examples review templates (#1682)", () => { it("documents shipped inline-comment review toggles in gittensory.full.yml (#2156)", () => { const full = readConfigExample("gittensory.full.yml"); - for (const field of ["inline_comments", "suggestions", "finding_categories"]) { + for (const field of ["inline_comments", "suggestions", "finding_categories", "inline_comments_per_category"]) { expect(full, `missing review field ${field}`).toMatch(new RegExp(`# ${field}:`)); } }); @@ -101,6 +101,17 @@ describe("config/examples review templates (#1682)", () => { expect(resolveReviewPromptOverrides(off).effortScore).toBe(false); }); + it("resolves review.inline_comments_per_category via manifest parse + helper (#2159)", () => { + const full = readConfigExample("gittensory.full.yml"); + expect(full).toMatch(/# inline_comments_per_category:/); + expect(parseFocusManifest({}).review.inlineCommentsPerCategory).toBeNull(); + expect(resolveReviewPromptOverrides(parseFocusManifest({})).inlineCommentsPerCategory).toBeNull(); + const on = parseFocusManifest({ review: { inline_comments_per_category: 2 } }); + expect(on.review.inlineCommentsPerCategory).toBe(2); + expect(resolveReviewPromptOverrides(on).inlineCommentsPerCategory).toBe(2); + expect(reviewConfigToJson(on.review)).toEqual({ inline_comments_per_category: 2 }); + }); + it("locks in review.test_generation via manifest parse + JSON round-trip and documents it in gittensory.full.yml (#2189)", () => { // test_generation is a kill-switch that gates the boundary-safe test-generation advisory (#1972); it is NOT a // review prompt override, so it is exercised through the manifest parse + reviewConfigToJson round-trip rather diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index fd68f72b9e..23289af4b5 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -369,6 +369,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { cultureProfile: "culture_profile:", reviewMemory: "memory:", findingCategories: "finding_categories:", + inlineCommentsPerCategory: "inline_comments_per_category:", minFindingSeverity: "min_finding_severity:", maxFindings: "max_findings:", commentVerbosity: "comment_verbosity:", @@ -791,7 +792,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, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, 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 }, @@ -2968,9 +2969,9 @@ 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, suggestions: true, changed_files_summary: true, effort_score: true, impact_map: true, culture_profile: true, finding_categories: true, comment_verbosity: "detailed", 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, changedFilesSummary: true, effortScore: true, impactMap: true, cultureProfile: true, findingCategories: true, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: "detailed", 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 } }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, suggestions: true, changedFilesSummary: true, effortScore: true, impactMap: true, cultureProfile: true, findingCategories: true, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: "detailed", 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 + changed-files summary + effort score + impact map + culture profile + finding categories + security focus default OFF. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, changedFilesSummary: false, effortScore: false, impactMap: false, cultureProfile: false, findingCategories: false, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, changedFilesSummary: false, effortScore: false, impactMap: false, cultureProfile: false, findingCategories: false, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, 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); @@ -3141,6 +3142,20 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(bad.warnings.some((w) => /review\.finding_categories.*must be a boolean/.test(w))).toBe(true); }); + it("parses review.inline_comments_per_category (default unset), marks present, round-trips, and warns on invalid caps (#2159)", () => { + const on = parseFocusManifest({ review: { inline_comments_per_category: 3 } }); + expect(on.review.inlineCommentsPerCategory).toBe(3); + expect(on.review.present).toBe(true); + expect(parseFocusManifest({ review: reviewConfigToJson(on.review) }).review).toEqual(on.review); + expect(parseFocusManifest({ review: {} }).review.inlineCommentsPerCategory).toBeNull(); + const bad = parseFocusManifest({ review: { inline_comments_per_category: -1 } }); + expect(bad.review.inlineCommentsPerCategory).toBeNull(); + expect(bad.warnings.some((w) => /review\.inline_comments_per_category/.test(w))).toBe(true); + expect(parseFocusManifest({ review: { inline_comments_per_category: "nope" } }).review.inlineCommentsPerCategory).toBeNull(); + expect(resolveReviewPromptOverrides(on).inlineCommentsPerCategory).toBe(3); + expect(resolveReviewPromptOverrides(parseFocusManifest({})).inlineCommentsPerCategory).toBeNull(); + }); + it("resolves review.test_generation's manifest toggle to a strict boolean (#2189)", () => { expect(resolveTestGenerationManifestToggle(null)).toBe(false); // null manifest (load failure) ⇒ false expect(resolveTestGenerationManifestToggle(parseFocusManifest({}))).toBe(false); // absent ⇒ false diff --git a/test/unit/inline-comments-select.test.ts b/test/unit/inline-comments-select.test.ts new file mode 100644 index 0000000000..9206765eda --- /dev/null +++ b/test/unit/inline-comments-select.test.ts @@ -0,0 +1,143 @@ +import { describe, expect, it } from "vitest"; +import type { InlineFinding } from "../../src/services/ai-review"; +import { + compareInlineFindingPriority, + DEFAULT_MAX_INLINE_COMMENTS, + inlineFindingCategory, + rightSideLinesFromPatch, + selectAnchoredInlineFindings, +} from "../../src/review/inline-comments-select"; + +const fileWith = (path: string, patch: string) => ({ path, payload: { patch } }); +const files = [fileWith("src/a.ts", "@@ -1,0 +1,6 @@\n+1\n+2\n+3\n+4\n+5\n+6")]; + +describe("rightSideLinesFromPatch (#2159)", () => { + it("returns RIGHT-side line numbers and ignores deleted/no-newline markers", () => { + const patch = "@@ -1,3 +1,4 @@\n ctx1\n-removed\n+added2\n+added3\n ctx4\n\\ No newline at end of file"; + expect([...rightSideLinesFromPatch(patch)].sort((a, b) => a - b)).toEqual([1, 2, 3, 4]); + expect(rightSideLinesFromPatch("").size).toBe(0); + }); + + it("ignores trailing split artifacts and preamble before the first hunk", () => { + expect([...rightSideLinesFromPatch("@@ -1,1 +1,2 @@\n ctx\n+added2\n")].sort((a, b) => a - b)).toEqual([1, 2]); + expect(rightSideLinesFromPatch("preamble only").size).toBe(0); + }); +}); + +describe("inlineFindingCategory + compareInlineFindingPriority (#2159)", () => { + it("uses the model category when present and falls back otherwise", () => { + expect(inlineFindingCategory({ path: "src/a.ts", line: 1, severity: "nit", body: "x", category: "security" })).toBe("security"); + expect(inlineFindingCategory({ path: "src/app.test.ts", line: 1, severity: "nit", body: "x" })).toBe("tests"); + }); + + it("ranks blockers ahead of nits and higher-priority categories ahead of style", () => { + const securityBlocker: InlineFinding = { path: "src/a.ts", line: 1, severity: "blocker", body: "x", category: "security" }; + const styleNit: InlineFinding = { path: "src/a.ts", line: 2, severity: "nit", body: "y", category: "style" }; + const performanceNit: InlineFinding = { path: "src/a.ts", line: 3, severity: "nit", body: "z", category: "performance" }; + const correctnessBlocker: InlineFinding = { path: "src/a.ts", line: 4, severity: "blocker", body: "c", category: "correctness" }; + expect(compareInlineFindingPriority(securityBlocker, styleNit)).toBeLessThan(0); + expect(compareInlineFindingPriority(performanceNit, styleNit)).toBeLessThan(0); + expect(compareInlineFindingPriority(securityBlocker, correctnessBlocker)).toBeLessThan(0); + expect(compareInlineFindingPriority(styleNit, styleNit)).toBe(0); + }); +}); + +describe("selectAnchoredInlineFindings (#2159)", () => { + it("drops unanchorable, duplicate, and below-threshold findings before capping", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "nit", body: "ok", category: "style" }, + { path: "src/a.ts", line: 1, severity: "blocker", body: "dup", category: "security" }, + { path: "src/a.ts", line: 99, severity: "nit", body: "missing", category: "style" }, + { path: "src/missing.ts", line: 1, severity: "nit", body: "unknown file", category: "style" }, + { path: "src/no-patch.ts", line: 1, severity: "nit", body: "no patch", category: "style" }, + ]; + const mixedFiles = [ + ...files, + { path: "src/no-patch.ts", payload: {} }, + ]; + expect( + selectAnchoredInlineFindings(findings, mixedFiles, { minFindingSeverity: "major" }).map((finding) => finding.body), + ).toEqual(["dup"]); + expect(DEFAULT_MAX_INLINE_COMMENTS).toBe(10); + }); + + it("preserves first-seen order when perCategoryCap is unset", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "nit", body: "first", category: "style" }, + { path: "src/a.ts", line: 2, severity: "blocker", body: "second", category: "security" }, + { path: "src/a.ts", line: 3, severity: "nit", body: "third", category: "style" }, + ]; + expect(selectAnchoredInlineFindings(findings, files, {}).map((f) => f.body)).toEqual(["first", "second", "third"]); + }); + + it("trims overflowing categories and keeps higher-priority findings when perCategoryCap is set", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "nit", body: "style-1", category: "style" }, + { path: "src/a.ts", line: 2, severity: "nit", body: "style-2", category: "style" }, + { path: "src/a.ts", line: 3, severity: "nit", body: "style-3", category: "style" }, + { path: "src/a.ts", line: 4, severity: "blocker", body: "security", category: "security" }, + { path: "src/a.ts", line: 5, severity: "nit", body: "style-4", category: "style" }, + ]; + const selected = selectAnchoredInlineFindings(findings, files, { perCategoryCap: 2, maxComments: 10 }); + expect(selected.map((f) => f.body)).toEqual(["security", "style-1", "style-2"]); + expect(selected.filter((f) => inlineFindingCategory(f) === "style")).toHaveLength(2); + }); + + it("preserves first-seen index order among equal-priority findings when sorting for caps", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "nit", body: "first", category: "style" }, + { path: "src/a.ts", line: 2, severity: "nit", body: "second", category: "style" }, + ]; + expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 1 }).map((finding) => finding.body)).toEqual(["first"]); + }); + + it("dedupes same path:line anchors before capping", () => { + const findings: InlineFinding[] = [ + { path: "src/a.ts", line: 1, severity: "blocker", body: "first", category: "security" }, + { path: "src/a.ts", line: 1, severity: "blocker", body: "duplicate", category: "security" }, + ]; + expect(selectAnchoredInlineFindings(findings, files, {}).map((finding) => finding.body)).toEqual(["first"]); + }); + + it("skips every category when perCategoryCap is zero", () => { + const findings: InlineFinding[] = [{ path: "src/a.ts", line: 1, severity: "nit", body: "style", category: "style" }]; + expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 0 })).toEqual([]); + }); + + it("ignores files with empty or missing patch content", () => { + const findings: InlineFinding[] = [{ path: "src/empty.ts", line: 1, severity: "nit", body: "missing patch" }]; + expect(selectAnchoredInlineFindings(findings, [{ path: "src/empty.ts", payload: { patch: "" } }], {})).toEqual([]); + expect( + selectAnchoredInlineFindings(findings, [{ path: "src/empty.ts", payload: { patch: 42 as unknown as string } }], {}), + ).toEqual([]); + expect(rightSideLinesFromPatch("preamble only").size).toBe(0); + }); + + it("breaks on the total cap without per-category sorting when perCategoryCap is unset", () => { + const twelveLineFiles = [ + fileWith( + "src/a.ts", + "@@ -1,0 +1,12 @@\n+1\n+2\n+3\n+4\n+5\n+6\n+7\n+8\n+9\n+10\n+11\n+12", + ), + ]; + const findings: InlineFinding[] = Array.from({ length: 12 }, (_, index) => ({ + path: "src/a.ts", + line: index + 1, + severity: "nit" as const, + body: `body-${index + 1}`, + category: "style" as const, + })); + expect(selectAnchoredInlineFindings(findings, twelveLineFiles, {})).toHaveLength(DEFAULT_MAX_INLINE_COMMENTS); + }); + + it("still enforces the total cap after per-category trimming", () => { + const findings: InlineFinding[] = Array.from({ length: 6 }, (_, index) => ({ + path: "src/a.ts", + line: index + 1, + severity: "nit" as const, + body: `body-${index + 1}`, + category: "style" as const, + })); + expect(selectAnchoredInlineFindings(findings, files, { perCategoryCap: 5, maxComments: 3 })).toHaveLength(3); + }); +}); diff --git a/test/unit/inline-comments.test.ts b/test/unit/inline-comments.test.ts index f3451880a1..9f2c1daca5 100644 --- a/test/unit/inline-comments.test.ts +++ b/test/unit/inline-comments.test.ts @@ -183,6 +183,40 @@ describe("selectInlineComments (#inline-comments)", () => { expect(out[0]?.body).toBe("**Blocker (correctness):** Missing null check.\n\n```suggestion\nif (!x) return;\n```"); }); }); + + describe("per-category cap (#2159)", () => { + const capFiles = [fileWith("src/a.ts", "@@ -1,0 +1,6 @@\n+1\n+2\n+3\n+4\n+5\n+6")]; + const styleFindings: InlineFinding[] = Array.from({ length: 4 }, (_, index) => ({ + path: "src/a.ts", + line: index + 1, + severity: "nit" as const, + body: `style-${index + 1}`, + category: "style" as const, + })); + const securityFinding: InlineFinding = { + path: "src/a.ts", + line: 5, + severity: "blocker", + body: "security issue", + category: "security", + }; + + it("defaults to byte-identical first-seen selection when perCategoryCap is omitted", () => { + const out = selectInlineComments([...styleFindings, securityFinding], capFiles); + expect(out.map((comment) => comment.body)).toEqual([ + "**Nit:** style-1", + "**Nit:** style-2", + "**Nit:** style-3", + "**Nit:** style-4", + "**Blocker:** security issue", + ]); + }); + + it("limits each category and prefers blockers/security when perCategoryCap is set", () => { + const out = selectInlineComments([...styleFindings, securityFinding], capFiles, false, false, null, 2); + expect(out.map((comment) => comment.body)).toEqual(["**Blocker:** security issue", "**Nit:** style-1", "**Nit:** style-2"]); + }); + }); }); describe("postInlineReviewComments (#inline-comments, fail-safe)", () => { @@ -326,6 +360,40 @@ describe("maybePostInlineComments (#inline-comments, review-path entry)", () => expect(calls[0]?.body).toMatchObject({ comments: [{ path: "src/a.ts", line: 2, side: "RIGHT", body: "**Nit (maintainability):** guard this" }] }); }); + it("threads perCategoryCap end-to-end when set (#2159)", async () => { + const getFiles = vi.fn(async () => [ + fileWith("src/a.ts", "@@ -1,0 +1,4 @@\n+1\n+2\n+3\n+4"), + ]); + const styleFindings: InlineFinding[] = Array.from({ length: 3 }, (_, index) => ({ + path: "src/a.ts", + line: index + 1, + severity: "nit" as const, + body: `style-${index + 1}`, + category: "style" as const, + })); + const securityFinding: InlineFinding = { path: "src/a.ts", line: 4, severity: "blocker", body: "security", category: "security" }; + 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: 14 }); + return new Response("unexpected", { status: 500 }); + }); + await maybePostInlineComments(envWithKey(), { + ...base, + aiReview: { inlineFindings: [...styleFindings, securityFinding] }, + getFiles, + perCategoryCap: 1, + }); + expect(calls[0]?.body).toMatchObject({ + comments: [ + { body: "**Blocker:** security" }, + { body: "**Nit:** style-1" }, + ], + }); + }); + it("omits the category tag end-to-end when categoriesEnabled is not passed (default off, backward compatible)", async () => { const getFiles = vi.fn(async () => files); const withCategory: InlineFinding[] = [{ path: "src/a.ts", line: 2, severity: "nit", body: "guard this", category: "maintainability" }]; diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 60fb198eb2..8419305f26 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, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: 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, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: 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