diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 1e3bed637e..834a476125 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -173,11 +173,12 @@ import type { CheckFailureDetail, MergeReadiness } from "../review/unified-comme import { buildIssueSlopAssessment, buildSlopAssessment, type SlopBand } from "../signals/slop"; import { runGittensoryAiSlopAdvisory } from "../services/ai-slop"; import { decidePublicSurface } from "../signals/settings-preview"; -import { buildFocusManifestGuidance, excludeReviewPaths, resolveReviewPathInstructions, resolveReviewPromptOverrides, type ReviewPathInstruction, type ReviewProfile } from "../signals/focus-manifest"; +import { buildFocusManifestGuidance, excludeReviewPaths, resolveReviewPathInstructions, resolveReviewPreMergeChecks, resolveReviewPromptOverrides, type ReviewPathInstruction, type ReviewProfile } from "../signals/focus-manifest"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import { resolveRepositorySettings } from "../settings/repository-settings"; import type { LocalBranchAnalysisInput } from "../signals/local-branch"; import { runGittensoryAiReview } from "../services/ai-review"; +import { evaluatePreMergeChecks } from "../review/pre-merge-checks"; import { secretLeakFinding } from "../review/safety"; import { aiCiRefutationActive, buildReviewGroundingText, checkSummaryText as checkFailureSummaryText, isGroundingEnabled } from "../review/grounding-wire"; import { buildReviewRagContext, isRagEnabled } from "../review/rag-wire"; @@ -2525,6 +2526,24 @@ async function maybePublishPrPublicSurface( }); } } + // Pre-merge checks (#review-pre-merge-checks, opt-in via .gittensory.yml review.pre_merge_checks). DETERMINISTIC + // content assertions (title/description must contain a phrase, a label must be present), optionally path-gated. + // Each FAILED check appends an advisory `pre_merge_check_failed` finding — or a blocking `pre_merge_check_required` + // one when the maintainer set enforce: true — BEFORE the gate evaluates. No AI judgment, so this can never cause + // an AI false-close. The manifest is cached (settings resolution loaded it), so this is a cheap hit; + // resolveReviewPreMergeChecks fail-safes to [] on a load error. Empty (default) ⇒ no finding (byte-identical). + const preMergeChecks = resolveReviewPreMergeChecks(await loadRepoFocusManifest(env, repoFullName).catch(() => null)); + if (preMergeChecks.length > 0) { + const checkFiles = await getReviewFiles(); // memoized — reuses the gate/slop diff when already resolved + advisory.findings.push( + ...evaluatePreMergeChecks(preMergeChecks, { + title: pr.title, + body: pr.body, + labels: pr.labels, + changedPaths: checkFiles.map((file) => file.path), + }), + ); + } // AI maintainer review (opt-in via aiReviewMode). Mutates `advisory` with a consensus defect (if any) // BEFORE the gate evaluates, and returns advisory notes for the panel. Inside the try so any AI diff --git a/src/review/pre-merge-checks.ts b/src/review/pre-merge-checks.ts new file mode 100644 index 0000000000..a0041eaaea --- /dev/null +++ b/src/review/pre-merge-checks.ts @@ -0,0 +1,45 @@ +import { matchesManifestPath, type PreMergeCheck } from "../signals/focus-manifest"; +import type { AdvisoryFinding } from "../types"; + +/** Finding code for a FAILED advisory (default) pre-merge check — surfaced but NEVER blocks. */ +export const PRE_MERGE_CHECK_ADVISORY_CODE = "pre_merge_check_failed"; +/** Finding code for a FAILED pre-merge check the maintainer marked `enforce: true` — a hard gate blocker + * (isConfiguredGateBlocker treats this code as blocking, like secret_leak). */ +export const PRE_MERGE_CHECK_BLOCKING_CODE = "pre_merge_check_required"; + +/** + * Evaluate the maintainer's `.gittensory.yml review.pre_merge_checks` against a PR — DETERMINISTICALLY, with no AI + * judgment. A check with `whenPaths` applies only when a changed path matches; it PASSES only when EVERY configured + * assertion holds (the title contains `titleContains`, the body contains `descriptionContains`, and the + * `requireLabel` label is present — all case-insensitive). Each FAILED check yields ONE finding: + * `pre_merge_check_required` (severity critical → the gate blocks under enforce) or `pre_merge_check_failed` + * (severity warning → advisory). Pure + side-effect-free; the caller pushes the findings into the advisory before + * the gate evaluates. Empty `checks` ⇒ no findings (byte-identical). + */ +export function evaluatePreMergeChecks( + checks: PreMergeCheck[], + ctx: { title?: string | null | undefined; body?: string | null | undefined; labels?: string[] | null | undefined; changedPaths: string[] }, +): AdvisoryFinding[] { + const title = (ctx.title ?? "").toLowerCase(); + const body = (ctx.body ?? "").toLowerCase(); + const labels = (ctx.labels ?? []).map((label) => label.toLowerCase()); + const findings: AdvisoryFinding[] = []; + for (const check of checks) { + // when_paths gate: a check with whenPaths applies ONLY to PRs that touch a matching path; an unmatched check + // is N/A (no finding). Empty whenPaths ⇒ the check always applies. + if (check.whenPaths.length > 0 && !ctx.changedPaths.some((path) => check.whenPaths.some((glob) => matchesManifestPath(path, glob)))) continue; + const unmet: string[] = []; + if (check.titleContains !== null && !title.includes(check.titleContains.toLowerCase())) unmet.push(`the title must contain "${check.titleContains}"`); + if (check.descriptionContains !== null && !body.includes(check.descriptionContains.toLowerCase())) unmet.push(`the description must contain "${check.descriptionContains}"`); + if (check.requireLabel !== null && !labels.includes(check.requireLabel.toLowerCase())) unmet.push(`the "${check.requireLabel}" label must be applied`); + if (unmet.length === 0) continue; // every configured assertion held → the check passed + findings.push({ + code: check.enforce ? PRE_MERGE_CHECK_BLOCKING_CODE : PRE_MERGE_CHECK_ADVISORY_CODE, + severity: check.enforce ? "critical" : "warning", + title: `Pre-merge check not satisfied: ${check.name}`, + detail: `This PR does not satisfy the maintainer pre-merge check "${check.name}": ${unmet.join("; ")}.`, + action: "Update the PR to satisfy the check, then re-run the gate.", + }); + } + return findings; +} diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 5524e1d235..a13bf99b5e 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -724,6 +724,12 @@ function isConfiguredGateBlocker(code: string, policy: GateCheckPolicy): boolean // (GITTENSORY_REVIEW_SAFETY); when the flag is off the finding never exists, so this branch is unreachable and the // gate verdict is byte-identical to today. if (code === "secret_leak") return true; + // A maintainer pre-merge check (#review-pre-merge-checks) marked `enforce: true` produces this DETERMINISTIC + // finding when it fails (a required title/description phrase or label is missing). It always blocks: the + // per-check `enforce` flag in `.gittensory.yml` IS the opt-in (mirroring secret_leak — the finding only exists + // when the maintainer configured an enforced check). The advisory variant (`pre_merge_check_failed`) is a plain + // warning and is never blocked here. No AI judgment is involved, so this can never cause an AI false-close. + if (code === "pre_merge_check_required") return true; // Focus-manifest policy (#555): the three enforceable manifest findings block ONLY when the maintainer // opts into manifestPolicy: block. Default off/advisory keeps them advisory-only. if (code === "manifest_blocked_path" || code === "manifest_linked_issue_required" || code === "manifest_missing_tests") { diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index b91e2540c0..6bf695540d 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -109,12 +109,31 @@ export type FocusManifestReviewConfig = { * reviewed (byte-identical). Gate/slop/secret-scan are UNAFFECTED — this only narrows the AI review. * (#review-exclude-paths) */ excludePaths: string[]; + /** `review.pre_merge_checks`: maintainer-declared DETERMINISTIC content assertions (title/description must + * contain a phrase, a label must be present), optionally gated to a path glob. Each FAILED check surfaces an + * advisory finding; a check with `enforce: true` becomes a hard gate blocker. Empty (default) ⇒ no finding + * (byte-identical). No AI judgment is involved. (#review-pre-merge-checks) */ + preMergeChecks: PreMergeCheck[]; }; /** One `review.path_instructions[]` entry: a manifest path glob + the public-safe instructions to apply when a * changed file matches it. */ export type ReviewPathInstruction = { path: string; instructions: string }; +/** One `review.pre_merge_checks[]` entry — a DETERMINISTIC pre-merge assertion. `whenPaths` (empty ⇒ always + * applies) gates the check to PRs that touch a matching path. The check PASSES only when EVERY configured + * assertion holds: the PR title contains `titleContains`, the body contains `descriptionContains`, and the + * `requireLabel` label is present (case-insensitive substring / label match). `enforce` ⇒ a failure is a hard + * gate blocker; default (false) ⇒ advisory only. All strings are public-safe-filtered at parse time. */ +export type PreMergeCheck = { + name: string; + whenPaths: string[]; + titleContains: string | null; + descriptionContains: string | null; + requireLabel: string | null; + enforce: boolean; +}; + // A hard cap so a hostile/huge manifest can't bloat the reviewer prompt (mirrors REVIEW_FIELD_KEYS discipline). const MAX_PATH_INSTRUCTIONS = 50; @@ -210,7 +229,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }, warnings: [], }; @@ -223,7 +242,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: [] } }; + return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] } }; } function normalizeStringList(value: JsonValue | undefined, field: string, warnings: string[]): string[] { @@ -501,7 +520,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: [] }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, profile: 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.`); @@ -524,35 +543,86 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const profile = parseReviewProfile(r.profile, warnings); const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); const excludePaths = parseReviewExcludePaths(r.exclude_paths, warnings); + const preMergeChecks = parseReviewPreMergeChecks(r.pre_merge_checks, warnings); return { - present: footerText !== null || note !== null || profile !== null || pathInstructions.length > 0 || excludePaths.length > 0 || Object.keys(fields).length > 0, + present: + footerText !== null || + note !== null || + profile !== null || + pathInstructions.length > 0 || + excludePaths.length > 0 || + preMergeChecks.length > 0 || + Object.keys(fields).length > 0, footerText, note, fields, profile, pathInstructions, excludePaths, + preMergeChecks, }; } -/** Parse `review.exclude_paths` — an array of manifest glob strings whose matching files are excluded from the AI - * review. Each must be a non-empty string; blanks/non-strings are dropped with a warning. Capped at - * MAX_PATH_INSTRUCTIONS so a hostile manifest can't bloat the matcher. (#review-exclude-paths) */ -function parseReviewExcludePaths(value: JsonValue | undefined, warnings: string[]): string[] { +/** Parse `review.pre_merge_checks` — an array of DETERMINISTIC pre-merge assertions. Each entry needs a non-empty + * public-safe `name` and at least ONE assertion (`title_contains` / `description_contains` / `require_label`, + * each public-safe); `when_paths` (optional) gates the check to PRs touching a matching glob; `enforce` (default + * false) makes a failure a hard blocker. Invalid entries are dropped with a warning; capped at + * MAX_PATH_INSTRUCTIONS so a hostile manifest can't bloat the gate. (#review-pre-merge-checks) */ +function parseReviewPreMergeChecks(value: JsonValue | undefined, warnings: string[]): PreMergeCheck[] { + if (value === undefined || value === null) return []; + if (!Array.isArray(value)) { + warnings.push(`Manifest "review.pre_merge_checks" must be a list of checks; ignoring it.`); + return []; + } + const out: PreMergeCheck[] = []; + for (const [index, entry] of value.entries()) { + if (out.length >= MAX_PATH_INSTRUCTIONS) { + warnings.push(`Manifest "review.pre_merge_checks" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`); + break; + } + if (entry === null || typeof entry !== "object" || Array.isArray(entry)) { + warnings.push(`Manifest "review.pre_merge_checks[${index}]" must be a mapping; ignoring it.`); + continue; + } + const e = entry as Record; + if (e.name === undefined || e.name === null) { + warnings.push(`Manifest "review.pre_merge_checks[${index}].name" is required; ignoring the entry.`); + continue; + } + const name = parsePublicSafeText(e.name, `review.pre_merge_checks[${index}].name`, warnings); + if (name === null) continue; // non-string / empty / not-public-safe → already warned + const titleContains = e.title_contains === undefined || e.title_contains === null ? null : parsePublicSafeText(e.title_contains, `review.pre_merge_checks[${index}].title_contains`, warnings); + const descriptionContains = e.description_contains === undefined || e.description_contains === null ? null : parsePublicSafeText(e.description_contains, `review.pre_merge_checks[${index}].description_contains`, warnings); + const requireLabel = e.require_label === undefined || e.require_label === null ? null : parsePublicSafeText(e.require_label, `review.pre_merge_checks[${index}].require_label`, warnings); + if (titleContains === null && descriptionContains === null && requireLabel === null) { + warnings.push(`Manifest "review.pre_merge_checks[${index}]" needs at least one of title_contains / description_contains / require_label; ignoring it.`); + continue; + } + const whenPaths = parseManifestGlobList(e.when_paths, `review.pre_merge_checks[${index}].when_paths`, warnings); + const enforce = normalizeOptionalBoolean(e.enforce, `review.pre_merge_checks[${index}].enforce`, warnings) === true; + out.push({ name, whenPaths, titleContains, descriptionContains, requireLabel, enforce }); + } + return out; +} + +/** Parse a manifest glob list (e.g. `review.exclude_paths`, a check's `when_paths`) — an array of non-empty + * string globs; blanks/non-strings are dropped with a warning. Capped at MAX_PATH_INSTRUCTIONS so a hostile + * manifest can't bloat the matcher. `fieldLabel` makes the warnings name the right field. */ +function parseManifestGlobList(value: JsonValue | undefined, fieldLabel: string, warnings: string[]): string[] { if (value === undefined || value === null) return []; if (!Array.isArray(value)) { - warnings.push(`Manifest "review.exclude_paths" must be a list of path globs; ignoring it.`); + warnings.push(`Manifest "${fieldLabel}" must be a list of path globs; ignoring it.`); return []; } const out: string[] = []; for (const [index, entry] of value.entries()) { if (out.length >= MAX_PATH_INSTRUCTIONS) { - warnings.push(`Manifest "review.exclude_paths" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`); + warnings.push(`Manifest "${fieldLabel}" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`); break; } const glob = typeof entry === "string" ? entry.trim() : ""; if (!glob) { - warnings.push(`Manifest "review.exclude_paths[${index}]" must be a non-empty string; ignoring it.`); + warnings.push(`Manifest "${fieldLabel}[${index}]" must be a non-empty string; ignoring it.`); continue; } out.push(glob); @@ -560,6 +630,11 @@ function parseReviewExcludePaths(value: JsonValue | undefined, warnings: string[ return out; } +/** Parse `review.exclude_paths` — globs whose matching files are excluded from the AI review. (#review-exclude-paths) */ +function parseReviewExcludePaths(value: JsonValue | undefined, warnings: string[]): string[] { + return parseManifestGlobList(value, "review.exclude_paths", warnings); +} + /** Parse `review.path_instructions` — an array of `{ path, instructions }` entries. Each must have a non-empty * string `path` (a manifest glob) and PUBLIC-SAFE string `instructions`; invalid/unsafe entries are dropped with * a warning. Capped at MAX_PATH_INSTRUCTIONS so a huge manifest can't bloat the reviewer prompt. */ @@ -620,6 +695,17 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.profile !== null) out.profile = review.profile; 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) { + out.pre_merge_checks = review.preMergeChecks.map((check) => { + const entry: Record = { name: check.name }; + if (check.whenPaths.length > 0) entry.when_paths = [...check.whenPaths]; + if (check.titleContains !== null) entry.title_contains = check.titleContains; + if (check.descriptionContains !== null) entry.description_contains = check.descriptionContains; + if (check.requireLabel !== null) entry.require_label = check.requireLabel; + if (check.enforce) entry.enforce = true; + return entry; + }); + } if (Object.keys(review.fields).length > 0) out.fields = { ...review.fields } as Record; return out; } @@ -646,6 +732,13 @@ export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { return { profile: manifest?.review.profile ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], excludePaths: manifest?.review.excludePaths ?? [] }; } +/** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized + * so the gate caller resolves them in one place with the null-manifest branch covered here (unit-tested) rather + * than inline in the processor. (#review-pre-merge-checks) */ +export function resolveReviewPreMergeChecks(manifest: FocusManifest | null): PreMergeCheck[] { + return manifest?.review.preMergeChecks ?? []; +} + /** Filter a PR's changed files down to the set the AI review should see — dropping any whose path matches a * `review.exclude_paths` glob (generated/vendored/lockfiles). Empty `excludePaths` ⇒ the same array (byte-identical * review). Pure; the gate/slop/secret-scan operate on the unfiltered files. (#review-exclude-paths) */ diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index f58682c442..c506c4c50f 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -11,6 +11,7 @@ import { resolveEffectiveSettings, excludeReviewPaths, resolveReviewPathInstructions, + resolveReviewPreMergeChecks, resolveReviewPromptOverrides, reviewConfigToJson, settingsOverrideToJson, @@ -431,7 +432,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: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [], preMergeChecks: [] }, warnings: [], }); expect(policy.publicSafe.entryGuidance).toContain("Keep PRs focused."); @@ -1161,3 +1162,60 @@ describe("review.exclude_paths (#review-exclude-paths)", () => { expect(excludeReviewPaths(files, [])).toBe(files); // empty → same reference (no-op) }); }); + +describe("review.pre_merge_checks (#review-pre-merge-checks)", () => { + it("parses checks (name + assertions + when_paths + enforce), marks present, and round-trips", () => { + const m = parseFocusManifest({ + review: { + pre_merge_checks: [ + { name: "Migration note", when_paths: ["migrations/**"], description_contains: "migration", enforce: true }, + { name: "Conventional title", title_contains: "(" }, + { name: "Breaking label", require_label: "breaking-change" }, + ], + }, + }); + expect(m.review.preMergeChecks).toEqual([ + { name: "Migration note", whenPaths: ["migrations/**"], titleContains: null, descriptionContains: "migration", requireLabel: null, enforce: true }, + { name: "Conventional title", whenPaths: [], titleContains: "(", descriptionContains: null, requireLabel: null, enforce: false }, + { name: "Breaking label", whenPaths: [], titleContains: null, descriptionContains: null, requireLabel: "breaking-change", enforce: false }, + ]); + expect(m.review.present).toBe(true); + expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.preMergeChecks).toEqual(m.review.preMergeChecks); + }); + + it("drops invalid entries with warnings: non-mapping, missing name, no assertion", () => { + const m = parseFocusManifest({ + review: { + pre_merge_checks: [ + "nope", // non-mapping + { title_contains: "x" }, // missing name + { name: "empty check" }, // no assertion + { name: 42, require_label: "x" }, // non-string (not public-safe) name → dropped at the name parse + { name: "ok", require_label: "ship" }, + ], + }, + }); + expect(m.review.preMergeChecks).toEqual([{ name: "ok", whenPaths: [], titleContains: null, descriptionContains: null, requireLabel: "ship", enforce: false }]); + expect(m.warnings.some((w) => /pre_merge_checks\[0\]/.test(w))).toBe(true); + expect(m.warnings.some((w) => /pre_merge_checks\[1\]\.name/.test(w))).toBe(true); + expect(m.warnings.some((w) => /pre_merge_checks\[2\].*at least one/.test(w))).toBe(true); + expect(m.warnings.some((w) => /pre_merge_checks\[3\]\.name/.test(w))).toBe(true); + }); + + it("ignores a non-array and caps the list; when_paths warnings name the right field", () => { + const bad = parseFocusManifest({ review: { pre_merge_checks: { name: "x" } } }); + expect(bad.review.preMergeChecks).toEqual([]); + expect(bad.warnings.some((w) => /pre_merge_checks.*must be a list/.test(w))).toBe(true); + const many = parseFocusManifest({ review: { pre_merge_checks: Array.from({ length: 60 }, (_, i) => ({ name: `c${i}`, require_label: "l" })) } }); + expect(many.review.preMergeChecks).toHaveLength(50); + expect(many.warnings.some((w) => /pre_merge_checks.*capped/.test(w))).toBe(true); + const badWhen = parseFocusManifest({ review: { pre_merge_checks: [{ name: "c", require_label: "l", when_paths: "src/**" }] } }); + expect(badWhen.warnings.some((w) => /pre_merge_checks\[0\]\.when_paths.*must be a list/.test(w))).toBe(true); + }); + + it("resolveReviewPreMergeChecks: non-null manifest passes checks through; null manifest → []", () => { + const manifest = parseFocusManifest({ review: { pre_merge_checks: [{ name: "c", require_label: "l" }] } }); + expect(resolveReviewPreMergeChecks(manifest)).toEqual(manifest.review.preMergeChecks); + expect(resolveReviewPreMergeChecks(null)).toEqual([]); + }); +}); diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index af8c4e5637..43cc01ca2f 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -126,6 +126,25 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => { expect(result.conclusion).toBe("failure"); expect(result.blockers.map((blocker) => blocker.code)).toContain("secret_leak"); }); + + it("an enforced pre-merge check (pre_merge_check_required) hard-blocks; the advisory variant never does (#review-pre-merge-checks)", () => { + const enforced: Advisory = { + ...missingIssueAdvisory(), + findings: [{ code: "pre_merge_check_required", title: "Pre-merge check not satisfied: Required label", severity: "critical", detail: "the 'approved' label must be applied", action: "apply it" }], + }; + const enforcedResult = evaluateGateCheck(enforced, gateCheckPolicy(settings(), null, true)); + expect(enforcedResult.conclusion).toBe("failure"); + expect(enforcedResult.blockers.map((blocker) => blocker.code)).toContain("pre_merge_check_required"); + + const advisoryOnly: Advisory = { + ...missingIssueAdvisory(), + findings: [{ code: "pre_merge_check_failed", title: "Pre-merge check not satisfied: Migration note", severity: "warning", detail: "the description must contain 'migration'", action: "add it" }], + }; + const advisoryResult = evaluateGateCheck(advisoryOnly, gateCheckPolicy(settings(), null, true)); + expect(advisoryResult.conclusion).toBe("success"); // advisory finding stays advisory — never blocks + expect(advisoryResult.blockers).toEqual([]); + expect(advisoryResult.warnings.map((warning) => warning.code)).toContain("pre_merge_check_failed"); + }); }); describe("policy pack (#692)", () => { diff --git a/test/unit/pre-merge-checks.test.ts b/test/unit/pre-merge-checks.test.ts new file mode 100644 index 0000000000..4b12aec419 --- /dev/null +++ b/test/unit/pre-merge-checks.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; + +import { evaluatePreMergeChecks, PRE_MERGE_CHECK_ADVISORY_CODE, PRE_MERGE_CHECK_BLOCKING_CODE } from "../../src/review/pre-merge-checks"; +import type { PreMergeCheck } from "../../src/signals/focus-manifest"; + +const check = (over: Partial = {}): PreMergeCheck => ({ + name: "Check", + whenPaths: [], + titleContains: null, + descriptionContains: null, + requireLabel: null, + enforce: false, + ...over, +}); + +describe("evaluatePreMergeChecks (#review-pre-merge-checks)", () => { + it("no findings when there are no checks (byte-identical)", () => { + expect(evaluatePreMergeChecks([], { title: "t", body: "b", labels: [], changedPaths: [] })).toEqual([]); + }); + + it("a satisfied check (all assertions hold, case-insensitive) yields no finding", () => { + const checks = [check({ name: "All", titleContains: "FEAT", descriptionContains: "Migration", requireLabel: "Ship" })]; + const out = evaluatePreMergeChecks(checks, { title: "feat: add", body: "includes a migration", labels: ["ship"], changedPaths: [] }); + expect(out).toEqual([]); + }); + + it("an advisory check failure → pre_merge_check_failed (warning); lists every unmet assertion", () => { + const checks = [check({ name: "Needs all", titleContains: "feat", descriptionContains: "why", requireLabel: "ready" })]; + const out = evaluatePreMergeChecks(checks, { title: "chore: x", body: "no rationale", labels: [], changedPaths: [] }); + expect(out).toHaveLength(1); + expect(out[0]?.code).toBe(PRE_MERGE_CHECK_ADVISORY_CODE); + expect(out[0]?.severity).toBe("warning"); + expect(out[0]?.detail).toContain('the title must contain "feat"'); + expect(out[0]?.detail).toContain('the description must contain "why"'); + expect(out[0]?.detail).toContain('the "ready" label must be applied'); + }); + + it("an enforced check failure → pre_merge_check_required (critical → the gate blocks)", () => { + const out = evaluatePreMergeChecks([check({ name: "Required", requireLabel: "approved", enforce: true })], { title: "t", body: "b", labels: ["other"], changedPaths: [] }); + expect(out).toHaveLength(1); + expect(out[0]?.code).toBe(PRE_MERGE_CHECK_BLOCKING_CODE); + expect(out[0]?.severity).toBe("critical"); + }); + + it("when_paths gates the check: skipped when no changed path matches, evaluated when one does", () => { + const checks = [check({ name: "Migrations documented", whenPaths: ["migrations/**"], descriptionContains: "migration", enforce: true })]; + // No matching path → N/A → no finding even though the description lacks the phrase. + expect(evaluatePreMergeChecks(checks, { title: "t", body: "no note", labels: [], changedPaths: ["src/a.ts"] })).toEqual([]); + // A matching path → evaluated → fails. + const out = evaluatePreMergeChecks(checks, { title: "t", body: "no note", labels: [], changedPaths: ["migrations/0099_x.sql"] }); + expect(out).toHaveLength(1); + expect(out[0]?.code).toBe(PRE_MERGE_CHECK_BLOCKING_CODE); + }); + + it("defaults null/absent title, body, and labels to empty (no crash; the assertion simply fails)", () => { + const out = evaluatePreMergeChecks([check({ name: "T", titleContains: "feat" })], { changedPaths: [] }); + expect(out).toHaveLength(1); + expect(out[0]?.detail).toContain('the title must contain "feat"'); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 222be6bc9e..8cc1db2ed4 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1368,6 +1368,85 @@ describe("queue processors", () => { expect(rcAudit).toBeFalsy(); }); + it("pre-merge checks (#review-pre-merge-checks): an enforced check that fails blocks the auto-merge", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload({ "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, { kind: "raw-github", url: "https://example.test" }, "2026-05-23T00:00:00.000Z"), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertInstallation(env, { + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "write", issues: "write" }, + events: ["pull_request"], + }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "enabled", + gateCheckMode: "enabled", + autonomy: { merge: "observe", request_changes: "observe" }, // evaluate + post the gate, take no merge/close action + agentDryRun: false, // so the gate check-run is actually POSTed (dry-run suppresses the write) and capturable + }); + await upsertOfficialMinerDetection(env, "contributor", { status: "confirmed", snapshot: queueMinerSnapshot("contributor") }, 60_000); + // The maintainer requires the "approved" label before merge — DETERMINISTIC, enforced. + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { review: { pre_merge_checks: [{ name: "Approved label required", require_label: "approved", enforce: true }] } }); + await upsertPullRequestFile(env, { repoFullName: "JSONbored/gittensory", pullNumber: 49, path: "src/feature.ts", status: "modified", additions: 5, deletions: 0, changes: 5, payload: {} }); + + let gateConclusion: string | undefined; + let gateText = ""; + const captureGate = (body: { name?: string; conclusion?: string; output?: { title?: string; summary?: string } }) => { + if ((body.name ?? "").includes("Gittensory Gate") && body.conclusion) { + gateConclusion = body.conclusion; + gateText = `${body.output?.title ?? ""} ${body.output?.summary ?? ""}`; + } + }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/commits/") && url.includes("/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/") && url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) { + if (init?.body) captureGate(JSON.parse(init.body.toString())); + return Response.json({ id: 901 }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "pre-merge-check-block", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 49, + title: "feat: add a feature", + state: "open", + user: { login: "contributor" }, + head: { sha: "gate124" }, + labels: [], // missing the required "approved" label → the enforced check FAILS + body: "Closes #1", + mergeable_state: "clean", + reviewDecision: "APPROVED", + }, + }, + }); + // The enforced pre-merge check failed → the gate check-run is a FAILURE that names the specific check. + expect(gateConclusion).toBe("failure"); + expect(gateText).toContain("Pre-merge check not satisfied: Approved label required"); + }); + it("REGRESSION (#audit-draft-maintenance): a clean DRAFT PR is never auto-merged/approved/closed (drafts are WIP)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertInstallation(env, { diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 322bb33608..11f8cfd0ed 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: [] }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, profile: 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