diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 88497ddc07..593330300a 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -401,6 +401,13 @@ review: # configured level are suppressed from inline comments — never from gate blockers. Default: null (show all). # min_finding_severity: major + # Display-only caps on how many blockers/nits render in the unified comment. Each field is a non-negative integer; + # unset within the object ⇒ no cap for that list. Whole object absent ⇒ legacy 12-item cap (byte-identical). + # Never affects gate decisions. Default: absent. + # max_findings: + # blockers: 5 + # nits: 10 + # Inline-comment layer toggles (#1956 / #1958). Bool | null. Default: null/false — byte-identical. # Requires operator flag GITTENSORY_REVIEW_INLINE_COMMENTS + cutover allowlist + review.inline_comments: true. # inline_comments: false diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index 78ef7c062b..d570b32a9b 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -414,6 +414,13 @@ review: # configured level are suppressed from inline comments — never from gate blockers. Default: null (show all). # min_finding_severity: major + # Display-only caps on how many blockers/nits render in the unified comment. Each field is a non-negative integer; + # unset within the object ⇒ no cap for that list. Whole object absent ⇒ legacy 12-item cap (byte-identical). + # Never affects gate decisions. Default: absent. + # max_findings: + # blockers: 5 + # nits: 10 + # Inline-comment layer toggles (#1956 / #1958). Bool | null. Default: null/false — byte-identical. # Requires operator flag GITTENSORY_REVIEW_INLINE_COMMENTS + cutover allowlist + review.inline_comments: true. # inline_comments: false diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 51e7386caf..3aceb30a98 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -374,6 +374,8 @@ import { type ReviewPathInstruction, type ReviewProfile, type ReviewFindingSeverity, + type MaxFindingsConfig, + maxFindingsPresent, type SelfHostAiModelConfig, type VisualConfig, } from "../signals/focus-manifest"; @@ -7754,6 +7756,7 @@ async function maybePublishPrPublicSurface( let effortScoreEnabledForReview = false; let findingCategoriesEnabledForReview = false; let minFindingSeverityForReview: ReviewFindingSeverity | null = null; + let maxFindingsForReview: MaxFindingsConfig | undefined; let aiReviewExpected = false; let aiReviewWasReused = false; let gateFinalized = false; @@ -8258,6 +8261,9 @@ async function maybePublishPrPublicSurface( changedFilesSummaryEnabledForReview = deterministicReviewOverrides.changedFilesSummary; effortScoreEnabledForReview = deterministicReviewOverrides.effortScore; minFindingSeverityForReview = deterministicReviewOverrides.minFindingSeverity; + if (maxFindingsPresent(deterministicReviewOverrides.maxFindings)) { + maxFindingsForReview = deterministicReviewOverrides.maxFindings; + } const aiReviewWillRun = !authorBlacklisted && !isFrozenForManualReview && @@ -9496,6 +9502,7 @@ async function maybePublishPrPublicSurface( ...(findingCategoriesEnabledForReview && aiReview?.inlineFindings?.length ? { findingCategories: aiReview.inlineFindings } : {}), + ...(maxFindingsForReview !== undefined ? { maxFindings: maxFindingsForReview } : {}), }); } else { deterministicBody = buildPublicPrIntelligenceComment(commentArgs); diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 5b7faf1f48..64d349b1b6 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -325,6 +325,8 @@ export type UnifiedCommentBridgeArgs = { * `classifyFindingCategory` — never omitted from the count. Default OFF (the processor passes this only when * the manifest opts in — see `resolveReviewPromptOverrides`'s `findingCategories`). (#1958) */ findingCategories?: FindingCategoryInput[] | undefined; + /** Display-only caps on rendered blockers/nits (`review.max_findings` port). Omitted ⇒ legacy 12-item cap. (#2049) */ + maxFindings?: { blockers: number | null; nits: number | null } | undefined; /** The disposition holds this PR for owner review because its diff touches a hard-guardrail path — so an * otherwise-ready comment renders "held for review" instead of "safe to merge". (#guarded-hold-comment) */ heldForReview?: boolean | undefined; @@ -608,6 +610,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string ...(args.heldForReview ? { heldForReview: true } : {}), ...(args.neverClosed ? { neverClosed: true } : {}), ...(args.preflightHeld ? { preflightHeld: true } : {}), + ...(args.maxFindings !== undefined ? { maxFindings: args.maxFindings } : {}), }); // Prepend the marker verbatim (matching the legacy body, which leads with the marker then a blank line) diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index de3632463c..fa480991f5 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -220,6 +220,8 @@ export interface UnifiedCommentContext { preflightHeld?: boolean; /** Public freshness marker for the posted/updated review comment. Rendered as UTC when provided. */ reviewedAt?: string | number | Date | undefined; + /** Display-only caps on rendered blockers/nits (`review.max_findings`). Omitted ⇒ legacy 12-item cap per list. (#2049) */ + maxFindings?: { blockers: number | null; nits: number | null } | undefined; } const STATUS_META: Record = { @@ -361,8 +363,36 @@ function verdictLine(status: UnifiedCommentStatus, input: UnifiedReviewInput, ct } } -/** Dedupe + cap a list of lines (case-insensitive), so blockers/nits never balloon the comment. */ -function dedupeLines(items: string[], cap = 12): string[] { +/** Legacy unified-comment display cap when `review.max_findings` is absent (byte-identical). */ +export const LEGACY_FINDINGS_DISPLAY_CAP = 12; + +/** Truncate a deduped findings list for display. `cap === null` ⇒ show all lines. */ +export function truncateDisplayedFindingLines( + lines: readonly string[], + cap: number | null, +): { visible: string[]; omitted: number } { + if (cap === null || lines.length <= cap) { + return { visible: [...lines], omitted: 0 }; + } + return { visible: lines.slice(0, cap), omitted: lines.length - cap }; +} + +function resolveFindingsDisplayCap( + maxFindings: UnifiedCommentContext["maxFindings"], + kind: "blockers" | "nits", +): number | null { + if (!maxFindings) return LEGACY_FINDINGS_DISPLAY_CAP; + return maxFindings[kind]; +} + +/** Escape angle brackets in caller-provided public text so raw HTML, HTML comments, + * or stray closing tags cannot change the GitHub comment structure. */ +function escapePublicHtmlAngles(text: string): string { + return text.replace(/[<>]/g, (char) => (char === "<" ? "<" : ">")); +} + +/** Dedupe a list of lines (case-insensitive) so blockers/nits never repeat in the comment. */ +function dedupeLines(items: string[], cap?: number): string[] { const seen = new Set(); const out: string[] = []; for (const raw of items) { @@ -372,17 +402,11 @@ function dedupeLines(items: string[], cap = 12): string[] { if (seen.has(key)) continue; seen.add(key); out.push(line); - if (out.length >= cap) break; + if (cap !== undefined && out.length >= cap) break; } return out; } -/** Escape angle brackets in caller-provided public text so raw HTML, HTML comments, - * or stray closing tags cannot change the GitHub comment structure. */ -function escapePublicHtmlAngles(text: string): string { - return text.replace(/[<>]/g, (char) => (char === "<" ? "<" : ">")); -} - function bullets(items: string[]): string { return dedupeLines(items) .map((i) => `- ${escapePublicHtmlAngles(i)}`) @@ -504,13 +528,29 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi if (input.summary.trim()) blocks.push(`**Review summary**\n${escapePublicHtmlAngles(input.summary.trim())}`); - const nits = dedupeLines(input.nits ?? []); - if (nits.length) blocks.push(details("Nits", taskList(nits), `${nits.length} non-blocking`)); + const nitsDeduped = dedupeLines(input.nits ?? []); + if (nitsDeduped.length) { + const nitsTrunc = truncateDisplayedFindingLines(nitsDeduped, resolveFindingsDisplayCap(ctx.maxFindings, "nits")); + const nitsBody = + taskList(nitsTrunc.visible) + (nitsTrunc.omitted > 0 ? `\n\n_+${nitsTrunc.omitted} more nit(s) not shown._` : ""); + const nitsSub = + nitsTrunc.omitted > 0 + ? `${nitsTrunc.visible.length} non-blocking (+${nitsTrunc.omitted} more)` + : `${nitsDeduped.length} non-blocking`; + blocks.push(details("Nits", nitsBody, nitsSub)); + } - const blockers = dedupeLines(input.blockers ?? []); - if (blockers.length) { + const blockersDeduped = dedupeLines(input.blockers ?? []); + if (blockersDeduped.length) { + const blockersTrunc = truncateDisplayedFindingLines( + blockersDeduped, + resolveFindingsDisplayCap(ctx.maxFindings, "blockers"), + ); const heading = status === "blocked" ? "Why this is blocked" : "Concerns raised — review before merging"; - blocks.push(`**${heading}**\n${bullets(blockers)}`); + const blockersBody = + bullets(blockersTrunc.visible) + + (blockersTrunc.omitted > 0 ? `\n\n_+${blockersTrunc.omitted} more blocker(s) not shown._` : ""); + blocks.push(`**${heading}**\n${blockersBody}`); } // Failing CI checks — list WHICH checks failed and WHY (codecov %/test/lint reason) under the "CI failing" diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index e96b52a971..a4b29d73a5 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -353,6 +353,10 @@ export type FocusManifestReviewConfig = { * configured level are suppressed from inline comments — never from gate blockers. null (default, absent) ⇒ every * finding shown = byte-identical behavior. (#2048) */ minFindingSeverity: ReviewFindingSeverity | null; + /** `review.max_findings`: display-only caps on how many blockers/nits render in the unified comment. Each field is + * a non-negative integer; null/absent within the object ⇒ no cap for that list. The whole object absent ⇒ the + * legacy 12-item display cap (byte-identical). Never affects gate decisions. (#2049) */ + maxFindings: MaxFindingsConfig; /** `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[]; @@ -439,6 +443,18 @@ export type AutoReviewConfig = { autoPauseAfterReviewedCommits: number | null; }; +/** `review.max_findings` display caps — null per field means no cap for that list when the object is present. */ +export type MaxFindingsConfig = { + blockers: number | null; + nits: number | null; +}; + +export const EMPTY_MAX_FINDINGS_CONFIG: MaxFindingsConfig = { blockers: null, nits: null }; + +export function maxFindingsPresent(config: MaxFindingsConfig): boolean { + return config.blockers !== null || config.nits !== null; +} + export const EMPTY_AUTO_REVIEW_CONFIG: AutoReviewConfig = { skipDrafts: null, ignoreAuthors: [], @@ -684,7 +700,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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: 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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, 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 }, @@ -714,7 +730,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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: 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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, 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 }, @@ -835,6 +851,20 @@ function normalizeAutoReviewSizeCap(value: JsonValue | undefined, field: string, return value; } +/** Parse `review.max_findings` display caps. Absent ⇒ empty (legacy renderer cap applies). (#2049) */ +function normalizeMaxFindingsConfig(value: JsonValue | undefined, warnings: string[]): MaxFindingsConfig { + if (value === undefined || value === null) return { ...EMPTY_MAX_FINDINGS_CONFIG }; + if (typeof value !== "object" || Array.isArray(value)) { + warnings.push(`Manifest field "review.max_findings" must be an object; ignoring it.`); + return { ...EMPTY_MAX_FINDINGS_CONFIG }; + } + const record = value as Record; + return { + blockers: normalizeOptionalNonNegativeInt(record.blockers, "review.max_findings.blockers", warnings), + nits: normalizeOptionalNonNegativeInt(record.nits, "review.max_findings.nits", warnings), + }; +} + /** Normalize an optional confidence threshold in [0,1] (#7) — a fractional value (NOT a 0-100 score), so it is * clamped into range WITHOUT rounding. Absent/null ⇒ null (the resolver leaves the gate's 0.93 default in place); * a non-finite/non-number value is ignored with a warning. */ @@ -1665,7 +1695,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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: 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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, 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.`); @@ -1712,6 +1742,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo REVIEW_FINDING_SEVERITY_LADDER, warnings, ); + const maxFindings = normalizeMaxFindingsConfig(r.max_findings, warnings); const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); const instructions = parsePublicSafeText(r.instructions, "review.instructions", warnings); const excludePaths = parseReviewExcludePaths(r.exclude_paths, warnings); @@ -1735,6 +1766,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo effortScore !== null || findingCategories !== null || minFindingSeverity !== null || + maxFindingsPresent(maxFindings) || pathInstructions.length > 0 || instructions !== null || excludePaths.length > 0 || @@ -1764,6 +1796,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo effortScore, findingCategories, minFindingSeverity, + maxFindings, pathInstructions, instructions, excludePaths, @@ -2208,6 +2241,12 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.effortScore !== null) out.effort_score = review.effortScore; if (review.findingCategories !== null) out.finding_categories = review.findingCategories; if (review.minFindingSeverity !== null) out.min_finding_severity = review.minFindingSeverity; + if (maxFindingsPresent(review.maxFindings)) { + const maxFindings: Record = {}; + if (review.maxFindings.blockers !== null) maxFindings.blockers = review.maxFindings.blockers; + if (review.maxFindings.nits !== null) maxFindings.nits = review.maxFindings.nits; + out.max_findings = maxFindings; + } 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]; @@ -2423,7 +2462,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; findingCategories: boolean; minFindingSeverity: ReviewFindingSeverity | 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; findingCategories: boolean; minFindingSeverity: ReviewFindingSeverity | null; maxFindings: MaxFindingsConfig; 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. @@ -2435,7 +2474,7 @@ export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { // (never touches the AI prompt) and only needs the unified-comment convergence feature to be on. // findingCategories resolves the same way (#1958) — like suggestions, the caller further ANDs it with the // already-resolved inlineComments gate, since a category has nothing to categorize without an inline finding. - 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, findingCategories: manifest?.review.findingCategories === true, minFindingSeverity: manifest?.review.minFindingSeverity ?? 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, findingCategories: manifest?.review.findingCategories === true, minFindingSeverity: manifest?.review.minFindingSeverity ?? null, maxFindings: manifest?.review.maxFindings ?? { ...EMPTY_MAX_FINDINGS_CONFIG }, 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/config-templates.test.ts b/test/unit/config-templates.test.ts index 2ce7c24539..91c404abaa 100644 --- a/test/unit/config-templates.test.ts +++ b/test/unit/config-templates.test.ts @@ -60,7 +60,7 @@ describe("config/examples review templates (#1682)", () => { it("documents shipped unified-comment display toggles in gittensory.full.yml (#2069)", () => { const full = readConfigExample("gittensory.full.yml"); - for (const field of ["changed_files_summary", "effort_score", "min_finding_severity"]) { + for (const field of ["changed_files_summary", "effort_score", "min_finding_severity", "max_findings"]) { expect(full, `missing review field ${field}`).toMatch(new RegExp(`# ${field}:`)); } expect(full).not.toMatch(/Planned display toggle/); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 40178044e1..e9d9b95b86 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -27,6 +27,7 @@ import { resolveReviewPromptOverrides, composeManifestReviewInstructions, EMPTY_AUTO_REVIEW_CONFIG, + EMPTY_MAX_FINDINGS_CONFIG, EMPTY_SELF_HOST_AI_MODEL_CONFIG, EMPTY_VISUAL_CONFIG, resolveReviewSelfHostAiModel, @@ -356,6 +357,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { effortScore: "effort_score:", findingCategories: "finding_categories:", minFindingSeverity: "min_finding_severity:", + maxFindings: "max_findings:", pathInstructions: "path_instructions:", instructions: "instructions:", excludePaths: "exclude_paths:", @@ -766,7 +768,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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: 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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, 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 }, @@ -2871,9 +2873,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, finding_categories: 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, changedFilesSummary: true, effortScore: true, findingCategories: true, minFindingSeverity: null, 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, findingCategories: true, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, 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 + finding categories + security focus default OFF. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, changedFilesSummary: false, effortScore: false, findingCategories: false, minFindingSeverity: 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, findingCategories: false, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, 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); @@ -2986,6 +2988,28 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(resolveReviewPromptOverrides(major).minFindingSeverity).toBe("major"); expect(resolveReviewPromptOverrides(parseFocusManifest({})).minFindingSeverity).toBeNull(); }); + + it("parses review.max_findings, round-trips, and warns on invalid values (#2049)", () => { + const capped = parseFocusManifest({ review: { max_findings: { blockers: 3, nits: 5 } } }); + expect(capped.review.maxFindings).toEqual({ blockers: 3, nits: 5 }); + expect(capped.review.present).toBe(true); + expect(parseFocusManifest({ review: reviewConfigToJson(capped.review) }).review.maxFindings).toEqual({ + blockers: 3, + nits: 5, + }); + expect(parseFocusManifest({ review: {} }).review.maxFindings).toEqual({ ...EMPTY_MAX_FINDINGS_CONFIG }); + const partial = parseFocusManifest({ review: { max_findings: { nits: 2 } } }); + expect(partial.review.maxFindings).toEqual({ blockers: null, nits: 2 }); + const badShape = parseFocusManifest({ review: { max_findings: "five" } }); + expect(badShape.review.maxFindings).toEqual({ ...EMPTY_MAX_FINDINGS_CONFIG }); + expect(badShape.warnings.some((w) => /review\.max_findings.*must be an object/.test(w))).toBe(true); + const badValue = parseFocusManifest({ review: { max_findings: { blockers: -1, nits: 1.5 } } }); + expect(badValue.review.maxFindings).toEqual({ blockers: null, nits: null }); + expect(badValue.warnings.some((w) => /review\.max_findings\.blockers/.test(w))).toBe(true); + expect(badValue.warnings.some((w) => /review\.max_findings\.nits/.test(w))).toBe(true); + expect(resolveReviewPromptOverrides(capped).maxFindings).toEqual({ blockers: 3, nits: 5 }); + expect(resolveReviewPromptOverrides(parseFocusManifest({})).maxFindings).toEqual({ ...EMPTY_MAX_FINDINGS_CONFIG }); + }); }); describe("review.exclude_paths (#review-exclude-paths)", () => { diff --git a/test/unit/max-findings-display.test.ts b/test/unit/max-findings-display.test.ts new file mode 100644 index 0000000000..c187c79ac2 --- /dev/null +++ b/test/unit/max-findings-display.test.ts @@ -0,0 +1,143 @@ +import { describe, expect, it } from "vitest"; +import { + deriveUnifiedStatus, + LEGACY_FINDINGS_DISPLAY_CAP, + renderUnifiedReviewComment, + truncateDisplayedFindingLines, + type UnifiedReviewInput, +} from "../../src/review/unified-comment"; +import { buildUnifiedCommentBody } from "../../src/review/unified-comment-bridge"; +import type { GateCheckEvaluation } from "../../src/rules/advisory"; + +const base: UnifiedReviewInput = { + changedFiles: 2, + reviewerCount: 1, + recommendations: ["merge"], + summary: "Looks good.", +}; + +function gate(over: Partial = {}): GateCheckEvaluation { + return { + enabled: true, + conclusion: "success", + title: "Gate passed", + summary: "No blocker.", + blockers: [], + warnings: [], + ...over, + }; +} + +describe("truncateDisplayedFindingLines", () => { + const lines = ["a", "b", "c", "d", "e"]; + + it("returns all lines when cap is null", () => { + expect(truncateDisplayedFindingLines(lines, null)).toEqual({ visible: lines, omitted: 0 }); + }); + + it("returns all lines when under the cap", () => { + expect(truncateDisplayedFindingLines(lines, 10)).toEqual({ visible: lines, omitted: 0 }); + }); + + it("truncates at the cap and reports omitted count", () => { + expect(truncateDisplayedFindingLines(lines, 3)).toEqual({ visible: ["a", "b", "c"], omitted: 2 }); + }); + + it("handles a zero cap with a footer-eligible omission count", () => { + expect(truncateDisplayedFindingLines(lines, 0)).toEqual({ visible: [], omitted: 5 }); + }); +}); + +describe("renderUnifiedReviewComment max_findings display caps (#2049)", () => { + it("keeps the legacy 12-nit cap when maxFindings is omitted (byte-identical)", () => { + const md = renderUnifiedReviewComment( + { ...base, nits: Array.from({ length: 13 }, (_, i) => `Distinct nit ${i + 1}`) }, + {}, + ); + expect(md).toContain("Distinct nit 12"); + expect(md).not.toContain("Distinct nit 13"); + expect(md).not.toContain("more nit(s) not shown"); + expect(LEGACY_FINDINGS_DISPLAY_CAP).toBe(12); + }); + + it("truncates nits with a +N more footer when maxFindings.nits is set", () => { + const md = renderUnifiedReviewComment( + { ...base, nits: ["nit one", "nit two", "nit three", "nit four"] }, + { maxFindings: { blockers: null, nits: 2 } }, + ); + expect(md).toContain("nit one"); + expect(md).toContain("nit two"); + expect(md).not.toContain("nit three"); + expect(md).toContain("_+2 more nit(s) not shown._"); + expect(md).toContain("2 non-blocking (+2 more)"); + }); + + it("truncates blockers with a +N more footer when maxFindings.blockers is set", () => { + const md = renderUnifiedReviewComment( + { ...base, decision: "close", blockers: ["blocker A", "blocker B", "blocker C"] }, + { maxFindings: { blockers: 1, nits: null } }, + ); + expect(md).toContain("blocker A"); + expect(md).not.toContain("blocker B"); + expect(md).toContain("_+2 more blocker(s) not shown._"); + }); + + it("shows all blockers when maxFindings.blockers is null inside a configured object", () => { + const blockers = Array.from({ length: 15 }, (_, i) => `Blocker ${i + 1}`); + const md = renderUnifiedReviewComment( + { ...base, decision: "close", blockers }, + { maxFindings: { blockers: null, nits: 3 } }, + ); + expect(md).toContain("Blocker 15"); + expect(md).not.toContain("more blocker(s) not shown"); + }); + + it("does not change gate-derived status when display lists are truncated", () => { + const input: UnifiedReviewInput = { + ...base, + decision: "merge", + readiness: { ciState: "passed" }, + blockers: Array.from({ length: 20 }, (_, i) => `Hidden blocker ${i + 1}`), + }; + const withoutCap = deriveUnifiedStatus(input, {}); + const withCap = deriveUnifiedStatus(input, { maxFindings: { blockers: 1, nits: 1 } }); + expect(withoutCap).toBe("ready"); + expect(withCap).toBe("ready"); + }); +}); + +describe("buildUnifiedCommentBody max_findings wiring (#2049)", () => { + const footer = "footer"; + + it("forwards maxFindings into the renderer when provided", () => { + const notes = `Several nits.\n\n**Nits (4)**\n- nit 1\n- nit 2\n- nit 3\n- nit 4`; + const body = buildUnifiedCommentBody({ + gate: gate(), + aiReview: { notes }, + panelRows: [], + readinessTotal: 80, + changedFiles: 1, + footerMarkdown: footer, + maxFindings: { blockers: null, nits: 2 }, + }); + expect(body).toContain("nit 1"); + expect(body).toContain("nit 2"); + expect(body).not.toContain("nit 3"); + expect(body).toContain("_+2 more nit(s) not shown._"); + }); + + it("omits maxFindings forwarding when the arg is absent (legacy cap)", () => { + const nits = Array.from({ length: 13 }, (_, i) => `- legacy nit ${i + 1}`).join("\n"); + const notes = `Nits.\n\n**Nits (13)**\n${nits}`; + const body = buildUnifiedCommentBody({ + gate: gate(), + aiReview: { notes }, + panelRows: [], + readinessTotal: 80, + changedFiles: 1, + footerMarkdown: footer, + }); + expect(body).toContain("legacy nit 12"); + expect(body).not.toContain("legacy nit 13"); + }); +}); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index ef1877d937..4203d5d187 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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: 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, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: 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