From bf36561864dcda7a8d18b783534a53d0c82add7b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 16:05:39 -0700 Subject: [PATCH 1/2] fix(review): surface guardrail hold details --- src/queue/processors.ts | 13 ++++++-- src/review/unified-comment-bridge.ts | 23 ++++++++++++- test/unit/queue.test.ts | 27 ++++++++++----- test/unit/unified-comment-bridge.test.ts | 42 ++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 13 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 0b5a333082..1b142370ee 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -459,6 +459,7 @@ import type { JsonValue, PullRequestFilePathRecord, PullRequestRecord, + RepositoryCommandAuthorizationPolicy, RepositoryRecord, RepositorySettings, } from "../types"; @@ -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", @@ -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. @@ -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), @@ -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( @@ -9531,7 +9538,7 @@ async function maybeProcessPrPanelRetrigger( reason: authorization.reason, actorKind: authorization.actorKind, allowedRoles: commandAuthorizationAllowedRoles( - settings.commandAuthorization, + PR_PANEL_RETRIGGER_COMMAND_AUTHORIZATION, "review-now", ), }, diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 80a3c59474..f18ab44fc5 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -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) || @@ -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, diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 2d7d9127f6..916c73bf16 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -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, { @@ -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", { @@ -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 () => { @@ -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 () => { diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index f35c31ad92..b720c80ea9 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -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: " " }), From 6bbde376d2b19188a26a9827cb2c97177da79bfe Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 21:24:09 -0700 Subject: [PATCH 2/2] test(review): cover the fully-exhausted gate-reason fallback chain Closes the codecov/patch gap on PR #3255: gateVerdictReason's final "|| undefined" arm (no matching manual-hold warning, blank gate summary, blank gate title) had no test, so the HELD verdict's no-reason-bullet rendering path was unexercised. --- test/unit/unified-comment-bridge.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index b720c80ea9..bd618829e8 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -709,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." }),