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
13 changes: 10 additions & 3 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,7 @@ import type {
JsonValue,
PullRequestFilePathRecord,
PullRequestRecord,
RepositoryCommandAuthorizationPolicy,
RepositoryRecord,
RepositorySettings,
} from "../types";
Expand All @@ -471,6 +472,10 @@ const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000;
const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000;
const PER_PR_REGATE_BACKPRESSURE_TYPES = ["agent-regate-pr"] as const;
const SWEEP_OPEN_PULL_REQUEST_SYNC_MAX_AGE_MS = 10 * 60 * 1000;
const PR_PANEL_RETRIGGER_COMMAND_AUTHORIZATION: RepositoryCommandAuthorizationPolicy = {
default: ["maintainer", "collaborator"],
commands: { "review-now": ["maintainer", "collaborator"] },
};
const PR_PUBLIC_SURFACE_ACTIONS = new Set([
"opened",
"reopened",
Expand Down Expand Up @@ -2368,6 +2373,7 @@ async function runAgentMaintenancePlanAndExecute(
);
}
const changedPaths = changedPathsForGuardrail(changedFiles);
const guardrailMatches = guardrailPathMatches(changedPaths, hardGuardrailGlobs);
// #2550: live migrations/** collision recheck — config-gated (off by default) AND path-gated (only a PR
// that actually touches migrations/** pays the extra GitHub API call), so a non-migrations PR sees zero
// added latency: the whole block short-circuits on the boolean+array checks before any network call.
Expand Down Expand Up @@ -2754,6 +2760,7 @@ async function runAgentMaintenancePlanAndExecute(
closeOwnerAuthors: settings.closeOwnerAuthors,
precisionBreakerEngaged: precisionBreakerDirections.length > 0,
precisionBreakerDirections,
guardrailMatches,
disposition,
plannedActionClasses: planned.map((action) => action.actionClass),
finalActionClasses: breakerOnPlan.map((action) => action.actionClass),
Expand Down Expand Up @@ -9506,9 +9513,9 @@ async function maybeProcessPrPanelRetrigger(
issue,
actor,
commandName: "review-now",
settings,
settings: { ...settings, commandAuthorization: PR_PANEL_RETRIGGER_COMMAND_AUTHORIZATION },
pr,
needsMinerDetection: true,
needsMinerDetection: false,
});
if (!authorization.authorized) {
await recordPrPanelRetriggerSkip(
Expand All @@ -9531,7 +9538,7 @@ async function maybeProcessPrPanelRetrigger(
reason: authorization.reason,
actorKind: authorization.actorKind,
allowedRoles: commandAuthorizationAllowedRoles(
settings.commandAuthorization,
PR_PANEL_RETRIGGER_COMMAND_AUTHORIZATION,
"review-now",
),
},
Expand Down
23 changes: 22 additions & 1 deletion src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,27 @@ const BOILERPLATE_NIT_CODES = new Set([
]);
const BOILERPLATE_NIT_TITLE =
/local gittensory cache|registration is not available|config was not parsed|not registered/i;
const MANUAL_HOLD_WARNING_CODES = new Set([
"guardrail_hold",
"oversized_pr",
"ai_review_inconclusive",
]);

function holdWarningVerdictReason(finding: AdvisoryFinding): string {
const title = finding.title.trim();
const detail = finding.detail.trim();
return detail.length > 0 ? `${title}: ${detail}` : title;
}

function gateVerdictReason(gate: GateCheckEvaluation): string | undefined {
const holdReasons = gate.warnings
.filter((finding) => MANUAL_HOLD_WARNING_CODES.has(finding.code))
.map(holdWarningVerdictReason)
.filter(Boolean);
if (holdReasons.length > 0) return holdReasons.join("; ");
return gate.summary?.trim() || gate.title?.trim() || undefined;
}

export function isBoilerplateNit(finding: AdvisoryFinding): boolean {
return (
BOILERPLATE_NIT_CODES.has(finding.code) ||
Expand Down Expand Up @@ -367,7 +388,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string
// by construction (gate summary/title are author-facing) and angle-escaped by the renderer's verdictLine.
// Only attached for a NON-merge verdict: a passing (merge → ready) PR keeps its positive "safe to merge" /
// "all checks passed" wording rather than being overwritten by the gate's "no blocker found" summary.
const gateReason = args.gate.summary?.trim() || args.gate.title?.trim() || undefined;
const gateReason = gateVerdictReason(args.gate);
const verdictReason = verdict !== "merge" ? gateReason : undefined;
const input = buildUnifiedReviewInput({
changedFiles: args.changedFiles,
Expand Down
27 changes: 18 additions & 9 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12763,7 +12763,7 @@ describe("queue processors", () => {
expect(audit?.outcome).toBe("completed");
});

it("reruns the panel when a confirmed-miner PR author checks the rerun task (#824 miner-detection path)", async () => {
it("skips PR panel reruns from confirmed-miner PR authors because the checkbox is maintainer-only", async () => {
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123);
await upsertRepositorySettings(env, {
Expand All @@ -12774,7 +12774,8 @@ describe("queue processors", () => {
checkRunMode: "off",
gateCheckMode: "off",
includeMaintainerAuthors: true,
// review-now allows a confirmed miner, so a confirmed-miner PR author can retrigger their own panel.
// Even if repo config tries to allow confirmed miners, the checkbox is a maintainer/write-collaborator
// control because it mutates the bot's persisted review comment.
commandAuthorization: { default: ["maintainer", "collaborator", "confirmed_miner"], commands: { "review-now": ["maintainer", "confirmed_miner"] } },
});
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", {
Expand Down Expand Up @@ -12829,14 +12830,15 @@ describe("queue processors", () => {
},
});

// The confirmed-miner detection WAS fetched (the #824 helper's miner-detection path) and the panel retriggered.
expect(calls.minerList).toBeGreaterThanOrEqual(1);
// The checkbox authorization ignores the widened repo command policy, so it never reaches miner detection or
// comment mutation for a plain PR author.
expect(calls.minerList).toBe(0);
expect(calls.permission).toBe(1);
expect(calls.commentPatches).toBe(2);
const audit = await env.DB.prepare("select actor, outcome from audit_events where event_type = ? and target_key = ?")
.bind("github_app.pr_panel_retriggered", "JSONbored/gittensory#48")
.first<{ actor: string; outcome: string }>();
expect(audit).toMatchObject({ actor: "contributor", outcome: "completed" });
expect(calls.commentPatches).toBe(0);
const audit = await env.DB.prepare("select actor, outcome, detail from audit_events where event_type = ? and target_key = ?")
.bind("github_app.pr_panel_retrigger_skipped", "JSONbored/gittensory#48")
.first<{ actor: string; outcome: string; detail: string }>();
expect(audit).toMatchObject({ actor: "contributor", outcome: "completed", detail: "maintainer_command_requires_maintainer" });
});

it("skips PR panel reruns from users without repository write permission", async () => {
Expand Down Expand Up @@ -20536,6 +20538,13 @@ describe("auto-action convergence: end-to-end plan+execute for the general heuri
expect(seen.merged).toBe(false);
expect(seen.closed).toBe(false);
expect(await renderMetrics()).toContain('gittensory_agent_disposition_total{action_class="hold",autonomy_level="auto",blocker_class="guardrail_hold"} 1');
const holdAudit = await env.DB.prepare("select metadata_json from audit_events where event_type = 'agent.action.hold' order by created_at desc limit 1").first<{ metadata_json: string }>();
expect(JSON.parse(holdAudit?.metadata_json ?? "{}")).toMatchObject({
repoFullName: REPO,
pullNumber: 61,
disposition: { actionClass: "hold", blockerClass: "guardrail_hold" },
guardrailMatches: [{ path: ".github/workflows/ci.yml", glob: ".github/workflows/**" }],
});
});

it("reviewCheckMode: disabled still auto-closes a blocked contributor PR via the general heuristic-close path (#2852)", async () => {
Expand Down
56 changes: 56 additions & 0 deletions test/unit/unified-comment-bridge.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,48 @@ describe("verdictReason on a held/blocked headline (FIX D2)", () => {
expect(body).toContain("Manual maintainer review required.");
});

it("uses guardrail warning details for the manual-review reason so the exact path and glob are public", () => {
const body = buildUnifiedCommentBody({
gate: gate({
conclusion: "neutral",
summary: "Touches a guarded path — held for manual review",
warnings: [
{
code: "guardrail_hold",
severity: "warning",
title: "Touches a guarded path — held for manual review",
detail: "This PR changes guardrail-protected path(s): `workers/api.mjs` (matched `workers/**`).",
action: "A maintainer must review this manually.",
},
],
}),
aiReview: { notes: "The AI review still ran and found only non-blocking concerns." },
panelRows,
readinessTotal: 73,
changedFiles: 18,
footerMarkdown: footer,
});
expect(body).toContain("Suggested Action - Manual Review");
expect(body).toContain("Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): `workers/api.mjs` (matched `workers/**`).");
expect(body).toContain("The AI review still ran and found only non-blocking concerns.");
});

it("falls back to the guardrail warning title when a manual hold warning has no detail", () => {
const body = buildUnifiedCommentBody({
gate: gate({
conclusion: "neutral",
summary: "Manual review required.",
warnings: [{ code: "guardrail_hold", severity: "warning", title: "Touches a guarded path — held for manual review", detail: "" }],
}),
panelRows,
readinessTotal: 73,
changedFiles: 18,
footerMarkdown: footer,
});
expect(body).toContain("Touches a guarded path — held for manual review");
expect(body).not.toContain("Touches a guarded path — held for manual review:");
});

it("falls back to the gate TITLE when the summary is empty", () => {
const body = buildUnifiedCommentBody({
gate: gate({ conclusion: "failure", title: "Gittensory Orb Review Agent: blocked by policy", summary: " " }),
Expand All @@ -667,6 +709,20 @@ describe("verdictReason on a held/blocked headline (FIX D2)", () => {
expect(body).toContain("Gittensory Orb Review Agent: blocked by policy");
});

it("renders the Manual Review headline with no reason bullet when the whole fallback chain is exhausted (no matching warning, blank summary, blank title)", () => {
const body = buildUnifiedCommentBody({
gate: gate({ conclusion: "action_required", title: " ", summary: " ", warnings: [] }),
panelRows,
readinessTotal: 55,
changedFiles: 2,
footerMarkdown: footer,
});
expect(body).toContain("Suggested Action - Manual Review");
// "held" has no default reason (unlike ready/advisory/blocked), so an exhausted chain renders the bare
// heading with no trailing "- reason" bullet at all -- not even an empty one.
expect(body).not.toMatch(/Suggested Action - Manual Review\*\*\n-/);
});

it("does NOT overwrite the positive ready wording on a passing (merge) verdict", () => {
const body = buildUnifiedCommentBody({
gate: gate({ conclusion: "success", title: "Gittensory Orb Review Agent passed", summary: "No configured hard blocker was found." }),
Expand Down
Loading