Skip to content

Commit f160cbb

Browse files
authored
fix(review): hold the gate when an enforced pre-merge check's files can't be resolved (#1371)
review.pre_merge_checks built changedPaths from getReviewFiles(), which returns [] when the PR's files are unresolved (review fired before detail-sync AND the inline fetch failed). evaluatePreMergeChecks then treated every whenPaths-gated check as N/A (`[].some()` is false) and SKIPPED it — including enforce:true checks — so a guarded PR with green CI auto-MERGED, silently bypassing the maintainer's hard requirement. A PR always touches >=1 file, so an empty resolved set means UNRESOLVED. Thread filesResolved into evaluatePreMergeChecks: when false, an enforced whenPaths check emits a new `pre_merge_check_unresolved` finding that isEvaluationBlocker treats as a NEUTRAL gate (HELD, re-evaluates on the next sync) — never silently skipping the requirement (auto-merge bypass) and never hard-closing the contributor on a transient resolution miss. Advisory checks are dropped (no noise); path-less checks are unaffected.
1 parent 5d12b53 commit f160cbb

5 files changed

Lines changed: 59 additions & 5 deletions

File tree

src/queue/processors.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2560,12 +2560,16 @@ async function maybePublishPrPublicSurface(
25602560
const preMergeChecks = resolveReviewPreMergeChecks(await loadRepoFocusManifest(env, repoFullName).catch(() => null));
25612561
if (preMergeChecks.length > 0) {
25622562
const checkFiles = await getReviewFiles(); // memoized — reuses the gate/slop diff when already resolved
2563+
// An empty resolved file set means the changed paths could not be resolved (a PR always touches >=1 file),
2564+
// so a path-gated check cannot be evaluated — pass filesResolved=false so an ENFORCED whenPaths check HOLDS
2565+
// the gate (re-evaluates later) instead of silently skipping a hard requirement into an auto-merge (#review-audit).
25632566
advisory.findings.push(
25642567
...evaluatePreMergeChecks(preMergeChecks, {
25652568
title: pr.title,
25662569
body: pr.body,
25672570
labels: pr.labels,
25682571
changedPaths: checkFiles.map((file) => file.path),
2572+
filesResolved: checkFiles.length > 0,
25692573
}),
25702574
);
25712575
}

src/review/pre-merge-checks.ts

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ export const PRE_MERGE_CHECK_ADVISORY_CODE = "pre_merge_check_failed";
66
/** Finding code for a FAILED pre-merge check the maintainer marked `enforce: true` — a hard gate blocker
77
* (isConfiguredGateBlocker treats this code as blocking, like secret_leak). */
88
export const PRE_MERGE_CHECK_BLOCKING_CODE = "pre_merge_check_required";
9+
/** Finding code emitted when an ENFORCED `whenPaths`-gated check cannot be evaluated because the PR's changed-file
10+
* set could not be resolved. isEvaluationBlocker (advisory.ts) treats this as a NEUTRAL gate (HELD, re-evaluates
11+
* automatically) — never silently skipping a hard requirement (auto-merge bypass) and never hard-closing the
12+
* contributor on a transient resolution miss. (#review-audit) */
13+
export const PRE_MERGE_CHECK_UNRESOLVED_CODE = "pre_merge_check_unresolved";
914

1015
/**
1116
* Evaluate the maintainer's `.gittensory.yml review.pre_merge_checks` against a PR — DETERMINISTICALLY, with no AI
@@ -18,16 +23,33 @@ export const PRE_MERGE_CHECK_BLOCKING_CODE = "pre_merge_check_required";
1823
*/
1924
export function evaluatePreMergeChecks(
2025
checks: PreMergeCheck[],
21-
ctx: { title?: string | null | undefined; body?: string | null | undefined; labels?: string[] | null | undefined; changedPaths: string[] },
26+
ctx: { title?: string | null | undefined; body?: string | null | undefined; labels?: string[] | null | undefined; changedPaths: string[]; filesResolved?: boolean | undefined },
2227
): AdvisoryFinding[] {
2328
const title = (ctx.title ?? "").toLowerCase();
2429
const body = (ctx.body ?? "").toLowerCase();
2530
const labels = (ctx.labels ?? []).map((label) => label.toLowerCase());
31+
const filesResolved = ctx.filesResolved ?? true; // absent ⇒ caller asserts a trustworthy changedPaths set
2632
const findings: AdvisoryFinding[] = [];
2733
for (const check of checks) {
2834
// when_paths gate: a check with whenPaths applies ONLY to PRs that touch a matching path; an unmatched check
29-
// is N/A (no finding). Empty whenPaths ⇒ the check always applies.
30-
if (check.whenPaths.length > 0 && !ctx.changedPaths.some((path) => check.whenPaths.some((glob) => matchesManifestPath(path, glob)))) continue;
35+
// is N/A (no finding). Empty whenPaths ⇒ the check always applies (title/description/label only).
36+
if (check.whenPaths.length > 0) {
37+
if (!filesResolved) {
38+
// The changed-file set could not be resolved, so we cannot evaluate this path gate. HOLD the gate for an
39+
// ENFORCED check (re-evaluates when files resolve) instead of silently skipping a hard requirement (which
40+
// would let a guarded PR auto-merge). An advisory check is just dropped (no noise on a transient miss).
41+
if (check.enforce)
42+
findings.push({
43+
code: PRE_MERGE_CHECK_UNRESOLVED_CODE,
44+
severity: "warning",
45+
title: `Pre-merge check held — changed files not resolved: ${check.name}`,
46+
detail: `Gittensory could not resolve this PR's changed files to evaluate the path-gated check "${check.name}"; the gate is held and re-evaluates automatically.`,
47+
action: "No action needed — the gate re-evaluates once the PR's files are available.",
48+
});
49+
continue;
50+
}
51+
if (!ctx.changedPaths.some((path) => check.whenPaths.some((glob) => matchesManifestPath(path, glob)))) continue;
52+
}
3153
const unmet: string[] = [];
3254
if (check.titleContains !== null && !title.includes(check.titleContains.toLowerCase())) unmet.push(`the title must contain "${check.titleContains}"`);
3355
if (check.descriptionContains !== null && !body.includes(check.descriptionContains.toLowerCase())) unmet.push(`the description must contain "${check.descriptionContains}"`);

src/rules/advisory.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -705,7 +705,10 @@ function conclusionForSeverity(severity: AdvisorySeverity, findings: AdvisoryFin
705705
}
706706

707707
function isEvaluationBlocker(code: string): boolean {
708-
return code === "repo_not_registered" || code === "repo_not_seen" || code === "pr_not_cached";
708+
// pre_merge_check_unresolved: an enforced path-gated pre-merge check whose changed-file set could not be
709+
// resolved — gittensory cannot evaluate it yet, so the gate is NEUTRAL (held) and re-evaluates on the next
710+
// sync, rather than auto-merging past the unverified requirement or hard-closing on a transient miss. (#review-audit)
711+
return code === "repo_not_registered" || code === "repo_not_seen" || code === "pr_not_cached" || code === "pre_merge_check_unresolved";
709712
}
710713

711714
function isConfiguredGateBlocker(code: string, policy: GateCheckPolicy): boolean {

test/unit/gate-check-policy.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,16 @@ describe("AI fail-closed hold (#ai-fail-closed)", () => {
145145
expect(advisoryResult.blockers).toEqual([]);
146146
expect(advisoryResult.warnings.map((warning) => warning.code)).toContain("pre_merge_check_failed");
147147
});
148+
149+
it("an unresolved-files enforced pre-merge check HOLDS the gate (neutral), never close or pass (#review-audit)", () => {
150+
const held: Advisory = {
151+
...missingIssueAdvisory(),
152+
findings: [{ code: "pre_merge_check_unresolved", title: "Pre-merge check held — changed files not resolved: Migrations documented", severity: "warning", detail: "could not resolve files", action: "re-evaluates automatically" }],
153+
};
154+
const result = evaluateGateCheck(held, gateCheckPolicy(settings(), null, true));
155+
expect(result.conclusion).toBe("neutral"); // held: not a pass (would auto-merge past the unverified check) nor a failure (would auto-close on a transient miss)
156+
expect(result.blockers).toEqual([]);
157+
});
148158
});
149159

150160
describe("policy pack (#692)", () => {

test/unit/pre-merge-checks.test.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { describe, expect, it } from "vitest";
22

3-
import { evaluatePreMergeChecks, PRE_MERGE_CHECK_ADVISORY_CODE, PRE_MERGE_CHECK_BLOCKING_CODE } from "../../src/review/pre-merge-checks";
3+
import { evaluatePreMergeChecks, PRE_MERGE_CHECK_ADVISORY_CODE, PRE_MERGE_CHECK_BLOCKING_CODE, PRE_MERGE_CHECK_UNRESOLVED_CODE } from "../../src/review/pre-merge-checks";
44
import type { PreMergeCheck } from "../../src/signals/focus-manifest";
55

66
const check = (over: Partial<PreMergeCheck> = {}): PreMergeCheck => ({
@@ -52,6 +52,21 @@ describe("evaluatePreMergeChecks (#review-pre-merge-checks)", () => {
5252
expect(out[0]?.code).toBe(PRE_MERGE_CHECK_BLOCKING_CODE);
5353
});
5454

55+
it("filesResolved=false HOLDS an enforced whenPaths check (unresolved code) but skips an advisory one and still evaluates non-path checks (#review-audit)", () => {
56+
const enforced = check({ name: "Migrations documented", whenPaths: ["migrations/**"], descriptionContains: "migration", enforce: true });
57+
const advisory = check({ name: "advisory path check", whenPaths: ["migrations/**"], descriptionContains: "migration", enforce: false });
58+
const noPath = check({ name: "JIRA in title", titleContains: "JIRA-", enforce: true }); // no whenPaths → unaffected
59+
// Files could not be resolved (changedPaths empty + filesResolved false): the enforced path check HOLDS, the
60+
// advisory path check is dropped, and the path-less check still evaluates normally.
61+
const out = evaluatePreMergeChecks([enforced, advisory, noPath], { title: "no ref", body: "", labels: [], changedPaths: [], filesResolved: false });
62+
expect(out).toHaveLength(2);
63+
expect(out.find((f) => f.title.includes("Migrations documented"))?.code).toBe(PRE_MERGE_CHECK_UNRESOLVED_CODE);
64+
expect(out.find((f) => f.title.includes("JIRA in title"))?.code).toBe(PRE_MERGE_CHECK_BLOCKING_CODE);
65+
expect(out.some((f) => f.title.includes("advisory path check"))).toBe(false);
66+
// With files resolved (default), the same enforced check is N/A when no path matches — no hold.
67+
expect(evaluatePreMergeChecks([enforced], { title: "t", body: "", labels: [], changedPaths: ["src/a.ts"] })).toEqual([]);
68+
});
69+
5570
it("defaults null/absent title, body, and labels to empty (no crash; the assertion simply fails)", () => {
5671
const out = evaluatePreMergeChecks([check({ name: "T", titleContains: "feat" })], { changedPaths: [] });
5772
expect(out).toHaveLength(1);

0 commit comments

Comments
 (0)