Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down
45 changes: 45 additions & 0 deletions src/review/pre-merge-checks.ts
Original file line number Diff line number Diff line change
@@ -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;
}
6 changes: 6 additions & 0 deletions src/rules/advisory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down
115 changes: 104 additions & 11 deletions src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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: [],
};

Expand All @@ -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[] {
Expand Down Expand Up @@ -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.`);
Expand All @@ -524,42 +543,98 @@ 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<string, JsonValue>;
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);
}
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. */
Expand Down Expand Up @@ -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<string, JsonValue> = { 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<string, JsonValue>;
return out;
}
Expand All @@ -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) */
Expand Down
Loading
Loading