From ceef67efa5b7e125f17495b26912e884bb5af65a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 03:03:07 -0700 Subject: [PATCH] feat(gate): let maintainers be exempt from the missing-linked-issue penalty `gate.linkedIssue: block` exists to stop unlinked CONTRIBUTOR work. Applied to a maintainer's own PR it asks them to file an issue against their own repo before touching it, and holds the PR when they don't; the bot's own PRs (release-please, dependency bumps, generated-doc refreshes) hit it constantly for the same reason. On the production Orb `hold | missing_linked_issue` is the second-largest hold bucket -- 503 in a week. New `gate.linkedIssueMaintainerExempt`, config-as-code only (global or per-repo), following hardGuardrailGlobs: no DB column, so no dashboard toggle can silently disagree with the file, and no migration. It is ONE clamp -- block to advisory, maintainer-authored PRs only -- because the two halves are already separate concerns. The finding is PRODUCED on `requireLinkedIssue` (true for any mode but off) and only BLOCKS at `block`, so clamping to advisory keeps "No linked issue detected" visible in the review comment while removing exactly its power to fail the gate, hold, or close. Nothing is suppressed and nothing goes silent. Scoped to the MISSING case. Everything that inspects an issue that IS linked -- linkedIssueSatisfaction, linkedIssueHardRules, linkedIssueLabelPropagation -- keys on a cited issue and is untouched, so a maintainer who links one is scrutinised exactly like anyone else. "Maintainer" is the existing PROTECTED AUTHOR set (owner, per-repo admin, protected automation author) reused rather than redefined, so it cannot come to mean one thing here and another on the close path. Bots are included deliberately: they already have auto-close protection for the same reason and are a large share of the unlinked PRs in practice. Applied to `settings` before it reaches gateCheckPolicy rather than passed as an argument -- both things that must agree read that object, and the function already takes seven positional parameters. Wired at ALL FOUR call sites (webhook, sweep, two re-gate paths): wiring only the webhook path would mean a maintainer PR passing live and then being held when the sweep re-gates it. Closes #10158 --- .loopover.yml.example | 19 ++++ apps/loopover-ui/public/openapi.json | 4 + config/examples/loopover.full.yml | 19 ++++ .../loopover-engine/src/focus-manifest.ts | 20 ++++ ...us-manifest-linked-issue-exemption.test.ts | 50 ++++++++++ src/openapi/schemas.ts | 1 + src/queue/processors.ts | 54 ++++++++++- src/settings/linked-issue-exemption.ts | 83 ++++++++++++++++ src/signals/focus-manifest.ts | 2 + src/types.ts | 15 +++ test/unit/focus-manifest.test.ts | 5 +- test/unit/linked-issue-exemption.test.ts | 95 +++++++++++++++++++ 12 files changed, 361 insertions(+), 6 deletions(-) create mode 100644 packages/loopover-engine/test/focus-manifest-linked-issue-exemption.test.ts create mode 100644 src/settings/linked-issue-exemption.ts create mode 100644 test/unit/linked-issue-exemption.test.ts diff --git a/.loopover.yml.example b/.loopover.yml.example index 2b5a25c9b6..d89c38048a 100644 --- a/.loopover.yml.example +++ b/.loopover.yml.example @@ -161,6 +161,25 @@ gate: # that is a separate, deterministic rule, not this gate. linkedIssue: advisory + # Exempt MAINTAINER-authored PRs from the missing-linked-issue penalty above, without weakening it for + # contributors and without giving up any linked-issue analysis. Off (unset) by default. + # + # With `linkedIssue: block` a maintainer's own quick fix is failed/held for the same reason a drive-by + # contributor PR is, which is rarely what the block is for — it exists to stop unlinked contributor work, + # not to make the maintainer file an issue against themselves before touching their own repo. + # + # Set this true and, for MAINTAINER-authored PRs only (repo owner, per-repo admins, and your automation + # bots — the same protected-author set that is already exempt from auto-close), the gate is clamped from + # `block` to `advisory`: + # • the "No linked issue detected" finding is STILL raised and still shown in the review comment + # • it no longer fails the gate, holds the PR, or closes it + # • contributors are completely unaffected — `linkedIssue: block` keeps its full force for them + # + # It changes nothing about a PR that DOES link an issue. Every linked-issue analysis — satisfaction + # (`linkedIssueSatisfaction`), the hard rules (`settings.linkedIssueHardRules`), and label propagation — + # only runs when an issue is cited, so all of it applies to maintainers exactly as before. + linkedIssueMaintainerExempt: false + # Duplicate-PR gate — detects duplicate/superseding PRs. # off | advisory | block. Default: block. duplicates: block diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index dc8c345010..2bc98f29b9 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -10453,6 +10453,10 @@ "proceed", null ] + }, + "linkedIssueMaintainerExempt": { + "type": "boolean", + "nullable": true } }, "required": [ diff --git a/config/examples/loopover.full.yml b/config/examples/loopover.full.yml index f938254b53..e26d7e6536 100644 --- a/config/examples/loopover.full.yml +++ b/config/examples/loopover.full.yml @@ -175,6 +175,25 @@ gate: # that is a separate, deterministic rule, not this gate. linkedIssue: advisory + # Exempt MAINTAINER-authored PRs from the missing-linked-issue penalty above, without weakening it for + # contributors and without giving up any linked-issue analysis. Off (unset) by default. + # + # With `linkedIssue: block` a maintainer's own quick fix is failed/held for the same reason a drive-by + # contributor PR is, which is rarely what the block is for — it exists to stop unlinked contributor work, + # not to make the maintainer file an issue against themselves before touching their own repo. + # + # Set this true and, for MAINTAINER-authored PRs only (repo owner, per-repo admins, and your automation + # bots — the same protected-author set that is already exempt from auto-close), the gate is clamped from + # `block` to `advisory`: + # • the "No linked issue detected" finding is STILL raised and still shown in the review comment + # • it no longer fails the gate, holds the PR, or closes it + # • contributors are completely unaffected — `linkedIssue: block` keeps its full force for them + # + # It changes nothing about a PR that DOES link an issue. Every linked-issue analysis — satisfaction + # (`linkedIssueSatisfaction`), the hard rules (`settings.linkedIssueHardRules`), and label propagation — + # only runs when an issue is cited, so all of it applies to maintainers exactly as before. + linkedIssueMaintainerExempt: false + # Duplicate-PR gate — detects duplicate/superseding PRs. # off | advisory | block. Default: block. duplicates: block diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index 73f8a3ec2e..feb61b3bea 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -87,6 +87,22 @@ export type FocusManifestGateConfig = { checkMode: ReviewCheckMode | null; pack: GatePolicyPack | null; linkedIssue: GateRuleMode | null; + /** `gate.linkedIssueMaintainerExempt` (#10158): exempt MAINTAINER-authored PRs from the missing-linked-issue + * penalty, without weakening it for contributors and without giving up any linked-issue analysis. + * + * It clamps `linkedIssue` from `block` to `advisory` for those authors and nothing else. That one clamp is + * the whole feature, because the two halves are already separate concerns: the `missing_linked_issue` + * finding is PRODUCED on `requireLinkedIssue` (true whenever the mode is not `off`), and only BLOCKS when + * the resolved mode is `block`. Clamping to `advisory` therefore keeps the finding visible in the review + * comment while removing its power to fail the gate, hold the PR, or close it. + * + * Everything that inspects an issue that IS linked -- `linkedIssueSatisfaction`, `linkedIssueHardRules`, + * `linkedIssueLabelPropagation` -- is untouched by construction: each only fires when the PR cites at + * least one issue, which is precisely the case this knob says nothing about. + * + * null (unset) ⇒ byte-identical to before this existed. Deliberately scoped to the MISSING case: a + * maintainer who does link an issue gets the same scrutiny anyone else would. */ + linkedIssueMaintainerExempt: boolean | null; duplicates: GateRuleMode | null; /** `gate.readiness.mode`/`gate.readiness.minScore` -- this engine-layer pair folds into * `RepositorySettings.qualityGateMode`/`qualityGateMinScore` (src/signals/focus-manifest.ts), a third @@ -1364,6 +1380,7 @@ const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { pack: null, closeAuditHoldoutPct: null, linkedIssue: null, + linkedIssueMaintainerExempt: null, duplicates: null, readinessMode: null, readinessMinScore: null, @@ -1837,6 +1854,7 @@ const GATE_TOP_LEVEL_KEYS = new Set([ "checkMode", "pack", "linkedIssue", + "linkedIssueMaintainerExempt", "duplicates", "readiness", "aiReview", @@ -1916,6 +1934,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu checkMode: normalizeOptionalEnum(record.checkMode, "gate.checkMode", ["required", "visible", "disabled"] as const, warnings), pack: normalizeOptionalEnum(record.pack, "gate.pack", ["gittensor", "oss-anti-slop"] as const, warnings), linkedIssue: normalizeOptionalGateMode(record.linkedIssue, "gate.linkedIssue", warnings), + linkedIssueMaintainerExempt: normalizeOptionalBoolean(record.linkedIssueMaintainerExempt, "gate.linkedIssueMaintainerExempt", warnings), duplicates: normalizeOptionalGateMode(record.duplicates, "gate.duplicates", warnings), readinessMode: normalizeReadinessGateMode(readinessRecord?.mode, "gate.readiness.mode", warnings), readinessMinScore: normalizeOptionalScore(readinessRecord?.minScore, "gate.readiness.minScore", warnings), @@ -2064,6 +2083,7 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { if (gate.checkMode !== null) out.checkMode = gate.checkMode; if (gate.pack !== null) out.pack = gate.pack; if (gate.linkedIssue !== null) out.linkedIssue = gate.linkedIssue; + if (gate.linkedIssueMaintainerExempt !== null) out.linkedIssueMaintainerExempt = gate.linkedIssueMaintainerExempt; if (gate.duplicates !== null) out.duplicates = gate.duplicates; if (gate.readinessMode !== null || gate.readinessMinScore !== null) { const readiness: Record = {}; diff --git a/packages/loopover-engine/test/focus-manifest-linked-issue-exemption.test.ts b/packages/loopover-engine/test/focus-manifest-linked-issue-exemption.test.ts new file mode 100644 index 0000000000..adea686e61 --- /dev/null +++ b/packages/loopover-engine/test/focus-manifest-linked-issue-exemption.test.ts @@ -0,0 +1,50 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { gateConfigToJson, parseFocusManifest } from "../dist/focus-manifest.js"; + +// #10158 adds `gate.linkedIssueMaintainerExempt`. It lives in the ENGINE, which has its own c8 coverage run +// over dist/ with `--all`, so the root vitest suite's coverage of the same source does NOT satisfy the engine +// flag -- these lines need a test HERE or codecov/patch fails on lines that are in fact exercised. +// +// The clamp itself is root-side (src/settings/linked-issue-exemption.ts, tested there). What the engine owns +// is the three things that make the knob reachable at all: it parses, it survives a round-trip, and it is a +// RECOGNISED gate key. That last one is not a formality -- the key is validated against an allowlist +// (GATE_TOP_LEVEL_KEYS), and a knob added to the type and the parser but not the allowlist parses to its +// value and then warns "unknown key ...; ignoring it", i.e. is silently inert in every real config. + +test("parses gate.linkedIssueMaintainerExempt as a tri-state boolean", () => { + assert.equal(parseFocusManifest({ gate: { linkedIssueMaintainerExempt: true } }).gate.linkedIssueMaintainerExempt, true); + assert.equal(parseFocusManifest({ gate: { linkedIssueMaintainerExempt: false } }).gate.linkedIssueMaintainerExempt, false); +}); + +test("is null when absent, so an unset knob leaves the DB/global value alone", () => { + // null is "unset", distinct from false ("explicitly off"). resolveEffectiveSettings only overrides the + // effective setting when this is non-null, so conflating the two would make every repo that never mentions + // the key start overriding an inherited global with `false`. + assert.equal(parseFocusManifest({ gate: { linkedIssue: "block" } }).gate.linkedIssueMaintainerExempt, null); + assert.equal(parseFocusManifest({}).gate.linkedIssueMaintainerExempt, null); +}); + +test("REGRESSION: it is a RECOGNISED gate key — no 'unknown key' warning", () => { + // The failure this pins is silent: without the GATE_TOP_LEVEL_KEYS entry the field still parses, so every + // unit test on the parser passes, and the only symptom is a warning in the manifest guidance while the + // knob does nothing in production. + const warnings = parseFocusManifest({ gate: { linkedIssueMaintainerExempt: true } }).warnings; + assert.deepEqual(warnings.filter((w) => w.includes("linkedIssueMaintainerExempt")), []); +}); + +test("warns and yields null on a non-boolean, rather than coercing a truthy string", () => { + // A yml typo like `linkedIssueMaintainerExempt: yes-please` must not silently disable the linked-issue + // gate for maintainers. + const m = parseFocusManifest({ gate: { linkedIssueMaintainerExempt: "yes-please" } }); + assert.equal(m.gate.linkedIssueMaintainerExempt, null); + assert.ok(m.warnings.some((w) => w.includes("gate.linkedIssueMaintainerExempt"))); +}); + +test("round-trips through gateConfigToJson", () => { + const parsed = parseFocusManifest({ gate: { linkedIssue: "block", linkedIssueMaintainerExempt: true } }); + const reparsed = parseFocusManifest({ gate: gateConfigToJson(parsed.gate) }); + assert.equal(reparsed.gate.linkedIssueMaintainerExempt, true); + assert.equal(reparsed.gate.linkedIssue, "block"); +}); diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 2f2fce0307..093b1f46ca 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -729,6 +729,7 @@ export const RepositorySettingsSchema = z autoProjectMilestoneMatchBackend: z.enum(["github", "linear"]).optional(), gatePack: z.enum(["gittensor", "oss-anti-slop"]), linkedIssueGateMode: z.enum(["off", "advisory", "block"]), + linkedIssueMaintainerExempt: z.boolean().nullable().optional(), duplicatePrGateMode: z.enum(["off", "advisory", "block"]), qualityGateMode: z.enum(["off", "advisory", "block"]), qualityGateMinScore: z.number().nullable().optional(), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 84f057c089..353d7bd2e9 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -97,6 +97,7 @@ import { upsertRepositoryFromGitHub, getLatestAdvisoryForPullRequest, } from "../db/repositories"; +import { withLinkedIssueMaintainerExemption, type LinkedIssueExemptionAuthor } from "../settings/linked-issue-exemption"; import { renameRepositoryIdentity } from "../db/repo-identity-rename"; import { effectiveIssueCapForAccountAge, @@ -1649,7 +1650,17 @@ export async function sweepRepoRegate( }); const gate = evaluateGateCheck( advisory, - gateCheckPolicy(settings, null, undefined, pr.slopRisk ?? null, undefined, undefined, sweepCloseConfidenceOverride), + // #10158: the sweep must apply the SAME maintainer exemption the webhook path does, or the same PR + // gets one verdict live and a different one when the sweep re-gates it. + gateCheckPolicy( + withLinkedIssueMaintainerExemption(settings, await resolveLinkedIssueExemptionAuthor(env, sweepInstallationId, repoFullName, pr.authorLogin)), + null, + undefined, + pr.slopRisk ?? null, + undefined, + undefined, + sweepCloseConfidenceOverride, + ), ); verdicts[String(pr.number)] = gate.conclusion; if (gate.conclusion === "failure" || gate.conclusion === "action_required") @@ -5275,6 +5286,36 @@ async function maybeForceFreshRebase( // re-review a given PR at most once per this window. The re-review always re-fetches the LIVE CI, so the window // only bounds FREQUENCY, never correctness — a later out-of-window completion + the hourly sweep + the merge-time // re-check still catch the settled state. +/** + * The author facts the linked-issue maintainer exemption reads (#10158), resolved identically at every gate + * evaluation so the same PR cannot get different verdicts from the webhook path and the re-gate sweep. + * + * Exactly the PROTECTED AUTHOR set used elsewhere in this file (`protectedAuthor`) -- owner, per-repo admin, + * or a protected automation author -- rather than a second definition of "maintainer". `isPerTenantAdmin` + * short-circuits to an env-allowlist lookup unless per-repo admin mode is on, so this is cheap on the common + * path; it fails CLOSED (not a maintainer) on any error, which degrades to today's behaviour rather than + * silently widening the exemption. + */ +async function resolveLinkedIssueExemptionAuthor( + env: Env, + installationId: number | null, + repoFullName: string, + // Nullable because the callers' PR records differ on this: some carry a guaranteed login, others a + // possibly-absent one. Normalised HERE rather than coerced at four call sites, so an unknown author can + // only ever resolve to "not a maintainer" -- coercing `null` to `""` at a call site would work today and + // silently become an owner match the day a repo is owned by the empty string's uppercase twin. + authorLogin: string | null | undefined, +): Promise { + const login = (authorLogin ?? "").trim(); + if (login.length === 0) return { authorIsOwner: false, authorIsAdmin: false, authorIsAutomationBot: false }; + const repoOwner = repoOwnerLoginFromFullName(repoFullName); + return { + authorIsOwner: repoOwner.length > 0 && login.toLowerCase() === repoOwner.toLowerCase(), + authorIsAdmin: await isPerTenantAdmin(env, installationId, repoFullName, login).catch(() => false), + authorIsAutomationBot: isProtectedAutomationAuthor(login, env), + }; +} + const CI_COALESCE_WINDOW_SECONDS = 60; /** @@ -12214,7 +12255,12 @@ async function maybePublishPrPublicSurface( guardrailMatches: guardrailPathMatches(guardrailChangedPaths, hardGuardrailGlobs), }; const gatePolicy = gateCheckPolicy( - settings, + // #10158: the maintainer linked-issue exemption is applied to `settings` here rather than passed as an + // argument, because BOTH halves that must agree read this object -- `requireLinkedIssue` (does the + // finding get produced) and `linkedIssueGateMode` (does it block). Clamping once upstream makes them + // unable to disagree; gateCheckPolicy already takes seven positional arguments and does not need an + // eighth. Same call in the sweep and re-gate paths, so a PR cannot be judged differently by each. + withLinkedIssueMaintainerExemption(settings, await resolveLinkedIssueExemptionAuthor(env, installationId, repoFullName, pr.authorLogin)), readiness.total, confirmedContributor, slopRisk, @@ -13853,7 +13899,7 @@ async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload: if (!findingRef.ok) { await recordAuditEvent(env, { eventType: "github_app.finding_resolved_skipped", actor: req.actor, targetKey, outcome: "completed", detail: findingRef.reason, metadata: { deliveryId, repoFullName: req.repoFullName, reason: findingRef.reason } }); await recordGithubProductUsage(env, "finding_resolved_skipped", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "skipped", metadata: { reason: findingRef.reason } }); return true; } const { advisory } = await buildAuthorizedPrActionAdvisory(env, req.repoFullName, pr, settings); await appendPublishedAiReviewFindingsForResolve(env, req.repoFullName, pr, settings.aiReviewMode, advisory); - const gate = evaluateGateCheck(advisory, gateCheckPolicy(settings, null, undefined, pr.slopRisk ?? null, undefined, undefined, await resolveAutomaticCloseConfidence(env, req.repoFullName, await getAiReviewCloseConfidenceOverride(env, req.repoFullName)))); + const gate = evaluateGateCheck(advisory, gateCheckPolicy(withLinkedIssueMaintainerExemption(settings, await resolveLinkedIssueExemptionAuthor(env, req.installationId ?? null, req.repoFullName, pr.authorLogin)), null, undefined, pr.slopRisk ?? null, undefined, undefined, await resolveAutomaticCloseConfidence(env, req.repoFullName, await getAiReviewCloseConfidenceOverride(env, req.repoFullName)))); const selection = selectWarningsForResolve(gate.warnings, findingRef); if (selection.reason === "finding_not_found") { await recordAuditEvent(env, { eventType: "github_app.finding_resolved_skipped", actor: req.actor, targetKey, outcome: "completed", detail: selection.reason, metadata: { deliveryId, repoFullName: req.repoFullName, reason: selection.reason } }); await recordGithubProductUsage(env, "finding_resolved_skipped", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "skipped", metadata: { reason: selection.reason } }); return true; } const mode = resolveAgentActionMode({ globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), instanceMode: forcedSelfhostMode(env), agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun }); @@ -14100,7 +14146,7 @@ async function maybeProcessExplainCommand(env: Env, deliveryId: string, payload: } const { advisory } = await buildAuthorizedPrActionAdvisory(env, req.repoFullName, pr, settings); await appendPublishedAiReviewFindingsForResolve(env, req.repoFullName, pr, settings.aiReviewMode, advisory); - const gate = evaluateGateCheck(advisory, gateCheckPolicy(settings, null, undefined, pr.slopRisk ?? null, undefined, undefined, await resolveAutomaticCloseConfidence(env, req.repoFullName, await getAiReviewCloseConfidenceOverride(env, req.repoFullName)))); + const gate = evaluateGateCheck(advisory, gateCheckPolicy(withLinkedIssueMaintainerExemption(settings, await resolveLinkedIssueExemptionAuthor(env, req.installationId ?? null, req.repoFullName, pr.authorLogin)), null, undefined, pr.slopRisk ?? null, undefined, undefined, await resolveAutomaticCloseConfidence(env, req.repoFullName, await getAiReviewCloseConfidenceOverride(env, req.repoFullName)))); const selection = selectWarningsForResolve(gate.warnings, findingRef); if (selection.reason === "finding_not_found") { const notFound = sanitizePublicComment([AGENT_COMMAND_COMMENT_MARKER, "", "> [!NOTE]", `> **No review finding \`${findingRef.findingCode}\` on this PR**`, "> That id is not among this PR's current review findings — re-run `@loopover explain ` with an id from the review summary.", "", "---", loopoverFooter(env)].join("\n")); diff --git a/src/settings/linked-issue-exemption.ts b/src/settings/linked-issue-exemption.ts new file mode 100644 index 0000000000..ddd4dee70f --- /dev/null +++ b/src/settings/linked-issue-exemption.ts @@ -0,0 +1,83 @@ +// Maintainer exemption from the missing-linked-issue penalty (#10158). +// +// `gate.linkedIssue: block` exists to stop unlinked CONTRIBUTOR work. Applied to a maintainer's own PR it +// asks them to file an issue against their own repo before touching it, and then fails or holds the PR when +// they don't -- on JSONbored/loopover that was the second-largest hold bucket on the Orb. +// +// ── WHY ONE CLAMP IS THE WHOLE FEATURE ──────────────────────────────────────────────────────────────────── +// Producing the finding and blocking on it are already separate decisions: +// +// • `missing_linked_issue` is PRODUCED whenever `requireLinkedIssue` holds, which is true for any mode +// other than "off" (processors.ts: `settings.requireLinkedIssue || linkedIssueGateMode !== "off"`). +// • it BLOCKS only when resolveConfiguredGateMode (rules/advisory.ts) resolves "block". +// +// So clamping "block" -> "advisory" keeps the finding visible in the review comment and strips exactly its +// power to fail the gate, hold the PR, or close it. Nothing has to be suppressed, and nothing goes silent -- +// which is the point: the maintainer still sees "No linked issue detected", it just is not a verdict. +// +// ── WHAT IT DELIBERATELY DOES NOT TOUCH ─────────────────────────────────────────────────────────────────── +// Only the MISSING case. Every analysis of an issue that IS linked -- linkedIssueSatisfactionGateMode, the +// linkedIssueHardRules eligibility rules, linkedIssueLabelPropagation -- keys on a cited issue, so a +// maintainer who links one is scrutinised exactly like anyone else. That is why this is a clamp on one mode +// rather than a bypass flag threaded through the linked-issue paths: a bypass would have had to be excluded +// from each of them by hand, and the next such path would have been added without the exclusion. + +import type { GateRuleMode, RepositorySettings } from "../types"; + +/** The author-role facts this exemption reads: exactly the codebase's existing PROTECTED AUTHOR set + * (`authorIsAutomationBot || authorIsOwner || authorIsAdmin` -- processors.ts's `protectedAuthor`, mirrored + * by the planner's own close-eligibility check in agent-actions.ts). Reused rather than redefined, so + * "maintainer" cannot come to mean one thing here and another on the close path. + * + * Automation bots are included deliberately. They are already protected from auto-close for the same + * reason -- their PRs are the repo's own machinery, not drive-by contributions -- and in practice they open + * unlinked PRs routinely (release-please, dependency bumps, generated-doc refreshes), which is precisely the + * case this knob exists to stop treating as a violation. */ +export type LinkedIssueExemptionAuthor = { authorIsOwner: boolean; authorIsAdmin: boolean; authorIsAutomationBot: boolean }; + +/** True when this author is one the exemption can apply to. Module-local: the only consumer is the resolver + * below, and exporting a second entry point would invite a caller to test the role without applying the + * clamp -- two ways to ask the same question, which is how the modes and the finding drift apart. */ +function isExemptibleMaintainerAuthor(author: LinkedIssueExemptionAuthor): boolean { + return author.authorIsOwner || author.authorIsAdmin || author.authorIsAutomationBot; +} + +/** + * PURE. The linked-issue gate mode that actually applies to this PR. + * + * Returns `mode` unchanged in every case except the one the knob names: exemption enabled, author is a + * maintainer, and the configured mode is `block`. `advisory` and `off` are already non-blocking, so there is + * nothing to clamp and they pass through -- which matters because clamping them would silently *raise* + * `off` to `advisory` and start producing a finding a repo had switched off. + */ +export function effectiveLinkedIssueGateMode( + mode: GateRuleMode, + exemptMaintainers: boolean | null | undefined, + author: LinkedIssueExemptionAuthor, +): GateRuleMode { + if (exemptMaintainers !== true) return mode; + if (!isExemptibleMaintainerAuthor(author)) return mode; + return mode === "block" ? "advisory" : mode; +} + +/** + * The settings a gate evaluation should actually run under for THIS PR's author. + * + * Applied to the `settings` object before it reaches `gateCheckPolicy`, rather than by adding an author + * parameter to that function: it already takes seven positional arguments, and both things that need to agree + * -- `requireLinkedIssue` (which decides whether the finding is produced) and `linkedIssueGateMode` (which + * decides whether it blocks) -- read this same object. Clamping here means they cannot disagree, and every + * call site is corrected by construction instead of each one remembering to pass an extra argument. + * + * Returns the SAME object reference when nothing changes, so the overwhelmingly common path (no exemption + * configured, or a contributor's PR) allocates nothing and stays referentially identical for any caller that + * digests or compares it -- `configDigest` in decision-record.ts digests the resolved policy, so a gratuitous + * copy would be harmless but a gratuitous CHANGE would not. + */ +export function withLinkedIssueMaintainerExemption( + settings: RepositorySettings, + author: LinkedIssueExemptionAuthor, +): RepositorySettings { + const effective = effectiveLinkedIssueGateMode(settings.linkedIssueGateMode, settings.linkedIssueMaintainerExempt, author); + return effective === settings.linkedIssueGateMode ? settings : { ...settings, linkedIssueGateMode: effective }; +} diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index c11e436ec6..84bf6bdf32 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -495,6 +495,8 @@ function applyGateConfigOverrides(effective: RepositorySettings, gate: FocusMani else if (gate.enabled !== null) effective.reviewCheckMode = gate.enabled ? "required" : "disabled"; if (gate.pack !== null) effective.gatePack = gate.pack; if (gate.linkedIssue !== null) effective.linkedIssueGateMode = gate.linkedIssue; + // #10158: config-as-code only, so an unset key leaves whatever the caller spread in (undefined ⇒ off). + if (gate.linkedIssueMaintainerExempt !== null) effective.linkedIssueMaintainerExempt = gate.linkedIssueMaintainerExempt; if (gate.duplicates !== null) effective.duplicatePrGateMode = gate.duplicates; if (gate.readinessMode !== null) effective.qualityGateMode = gate.readinessMode; if (gate.readinessMinScore !== null) effective.qualityGateMinScore = gate.readinessMinScore; diff --git a/src/types.ts b/src/types.ts index 43a8892a79..99bb849ad4 100644 --- a/src/types.ts +++ b/src/types.ts @@ -972,6 +972,21 @@ export type RepositorySettings = { * status for scoring only). `oss-anti-slop` runs the deterministic rules against any author on any repo. */ gatePack: GatePolicyPack; linkedIssueGateMode: GateRuleMode; + /** #10158: exempt MAINTAINER-authored PRs from {@link linkedIssueGateMode}'s missing-linked-issue penalty, + * without weakening it for contributors and without giving up any linked-issue analysis. Config-as-code + * only (`.loopover.yml gate.linkedIssueMaintainerExempt`, global or per-repo), like {@link hardGuardrailGlobs} + * -- no DB column, so no dashboard toggle can silently disagree with the file. + * + * Resolved by {@link effectiveLinkedIssueGateMode} (src/settings/linked-issue-exemption.ts), which clamps + * `block` to `advisory` for those authors and does nothing else. The clamp IS the feature: the + * `missing_linked_issue` finding is produced on `requireLinkedIssue` (true for any mode but `off`) and only + * blocks at `block`, so `advisory` keeps the finding visible while stripping its power to fail the gate, + * hold, or close. + * + * Scoped to the MISSING case on purpose. A maintainer who DOES link an issue is analysed exactly like + * anyone else -- `linkedIssueSatisfactionGateMode`, `linkedIssueHardRules` and `linkedIssueLabelPropagation` + * all key on a cited issue and are untouched by this. Absent/false ⇒ byte-identical to before it existed. */ + linkedIssueMaintainerExempt?: boolean | null | undefined; duplicatePrGateMode: GateRuleMode; qualityGateMode: GateRuleMode; qualityGateMinScore?: number | null | undefined; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 4880c21c21..339f2bf14a 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -304,6 +304,7 @@ describe(".loopover.yml.example field-exhaustiveness (#1670)", () => { checkMode: "checkMode:", pack: "pack:", linkedIssue: "linkedIssue:", + linkedIssueMaintainerExempt: "linkedIssueMaintainerExempt:", duplicates: "duplicates:", readinessMode: "readiness:", readinessMinScore: "readiness:", @@ -1014,7 +1015,7 @@ describe("compileFocusManifestPolicy", () => { issueDiscoveryPolicy: "neutral", maintainerNotes: [], publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], - gate: { present: false, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, guardrailEscalationOnCleanReview: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, priorityEligibilityWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, + gate: { present: false, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: null, linkedIssueMaintainerExempt: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, guardrailEscalationOnCleanReview: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, priorityEligibilityWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, activeReviewReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { present: false, rag: null, reputation: null, safety: null, grounding: null, e2eTests: null, screenshots: null, improvementSignal: null, amsReputationBridge: null }, @@ -1208,7 +1209,7 @@ describe("parseFocusManifest gate config", () => { // the block→advisory deprecation-downgrade behavior itself is covered separately below. const m = parseFocusManifest({ gate: { linkedIssue: "block", duplicates: "advisory", readiness: { mode: "advisory", minScore: 70 } } }); expect(m.present).toBe(true); - expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, guardrailEscalationOnCleanReview: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, priorityEligibilityWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); + expect(m.gate).toEqual({ present: true, enabled: null, checkMode: null, pack: null, closeAuditHoldoutPct: null, linkedIssue: "block", linkedIssueMaintainerExempt: null, duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, sizeMaxFiles: null, sizeMaxLines: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewEffort: null, aiReviewSelfConsistencyRuns: null, guardrailEscalationProvider: null, guardrailEscalationModel: null, guardrailEscalationEffort: null, guardrailEscalationSelfConsistencyRuns: null, guardrailEscalationOnCleanReview: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewSalvageabilityMinScore: null, aiReviewLowConfidenceDisposition: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, linkedIssueSatisfaction: null, contentLaneDeliverable: null, backtestRegression: null, manifestPolicy: null, dryRun: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, priorityEligibilityWindowMinutes: null, staleBaseAheadByThreshold: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null, advisoryCheckRuns: null, ignoredCheckRuns: null, aiJudgmentBlockersMode: null, copycatMode: null, copycatMinScore: null }); }); it("parses gate.mergeReadiness, round-trips it, and warns on a bad value (#822)", () => { diff --git a/test/unit/linked-issue-exemption.test.ts b/test/unit/linked-issue-exemption.test.ts new file mode 100644 index 0000000000..863a7aa254 --- /dev/null +++ b/test/unit/linked-issue-exemption.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, it } from "vitest"; + +import { effectiveLinkedIssueGateMode, withLinkedIssueMaintainerExemption, type LinkedIssueExemptionAuthor } from "../../src/settings/linked-issue-exemption"; +import type { GateRuleMode, RepositorySettings } from "../../src/types"; + +// #10158: `gate.linkedIssue: block` exists to stop unlinked CONTRIBUTOR work. Applied to a maintainer's own +// PR it asks them to file an issue against their own repo first and then holds the PR when they don't -- +// `hold | missing_linked_issue` was the second-largest hold bucket on the production Orb. +// +// The exemption is ONE clamp: block -> advisory for maintainer-authored PRs. Everything below is about the +// ways a plausible implementation would over- or under-reach. + +const CONTRIBUTOR: LinkedIssueExemptionAuthor = { authorIsOwner: false, authorIsAdmin: false, authorIsAutomationBot: false }; +const OWNER: LinkedIssueExemptionAuthor = { authorIsOwner: true, authorIsAdmin: false, authorIsAutomationBot: false }; +const ADMIN: LinkedIssueExemptionAuthor = { authorIsOwner: false, authorIsAdmin: true, authorIsAutomationBot: false }; +const BOT: LinkedIssueExemptionAuthor = { authorIsOwner: false, authorIsAdmin: false, authorIsAutomationBot: true }; + +describe("effectiveLinkedIssueGateMode (#10158)", () => { + it("clamps block to advisory for a maintainer when the exemption is on", () => { + expect(effectiveLinkedIssueGateMode("block", true, OWNER)).toBe("advisory"); + }); + + it("covers every protected author: owner, per-repo admin, and automation bots", () => { + // Bots are included deliberately -- release-please, dependency bumps and generated-doc refreshes open + // unlinked PRs constantly, and they are already exempt from auto-close for the same reason. + for (const author of [OWNER, ADMIN, BOT]) { + expect(effectiveLinkedIssueGateMode("block", true, author)).toBe("advisory"); + } + }); + + it("INVARIANT: a CONTRIBUTOR is never exempted, which is the entire point of keeping the gate", () => { + expect(effectiveLinkedIssueGateMode("block", true, CONTRIBUTOR)).toBe("block"); + }); + + it("INVARIANT: does nothing at all when the knob is not explicitly true", () => { + // Absent/false/null must be byte-identical to before this existed. `!== true` rather than a falsy check + // so a stray string from a hand-edited yml cannot switch it on. + for (const off of [undefined, null, false] as const) { + expect(effectiveLinkedIssueGateMode("block", off, OWNER), String(off)).toBe("block"); + } + }); + + it("REGRESSION: never RAISES a mode — `off` stays off, it is not promoted to advisory", () => { + // The clamp is one-directional. Rewriting `off` to `advisory` would start producing a + // `missing_linked_issue` finding on a repo that had deliberately switched the gate off entirely, which is + // the opposite of an exemption. + expect(effectiveLinkedIssueGateMode("off", true, OWNER)).toBe("off"); + expect(effectiveLinkedIssueGateMode("advisory", true, OWNER)).toBe("advisory"); + }); + + it("is exhaustive over the mode vocabulary — only `block` is ever rewritten", () => { + for (const mode of ["off", "advisory", "block"] as const satisfies readonly GateRuleMode[]) { + const out = effectiveLinkedIssueGateMode(mode, true, OWNER); + expect(out, mode).toBe(mode === "block" ? "advisory" : mode); + } + }); +}); + +describe("withLinkedIssueMaintainerExemption", () => { + const base = { linkedIssueGateMode: "block", linkedIssueMaintainerExempt: true } as RepositorySettings; + + it("returns settings whose linkedIssueGateMode is the clamped mode", () => { + expect(withLinkedIssueMaintainerExemption(base, OWNER).linkedIssueGateMode).toBe("advisory"); + }); + + it("REGRESSION: returns the SAME object reference when nothing changes", () => { + // `configDigest` (decision-record.ts) digests the resolved policy. A gratuitous copy is harmless, but + // identity here documents that the overwhelmingly common path -- contributor PRs, and every repo that + // never sets the knob -- is untouched rather than merely equal. + expect(withLinkedIssueMaintainerExemption(base, CONTRIBUTOR)).toBe(base); + const off = { linkedIssueGateMode: "block" } as RepositorySettings; + expect(withLinkedIssueMaintainerExemption(off, OWNER)).toBe(off); + }); + + it("changes NOTHING else on the settings object", () => { + // The exemption is scoped to one mode. Anything that analyses an issue that IS linked -- + // linkedIssueSatisfactionGateMode, the hard rules, label propagation -- must survive untouched, since + // those are exactly the "keep all current analysis" half of the requirement. + const rich = { + linkedIssueGateMode: "block", + linkedIssueMaintainerExempt: true, + linkedIssueSatisfactionGateMode: "block", + duplicatePrGateMode: "block", + requireLinkedIssue: true, + } as unknown as RepositorySettings; + const out = withLinkedIssueMaintainerExemption(rich, OWNER); + expect(out.linkedIssueGateMode).toBe("advisory"); + expect(out.linkedIssueSatisfactionGateMode).toBe("block"); + expect(out.duplicatePrGateMode).toBe("block"); + expect(out.requireLinkedIssue).toBe(true); + // requireLinkedIssue staying true is load-bearing, not incidental: it is what keeps the + // `missing_linked_issue` FINDING being produced, so the maintainer still sees "No linked issue + // detected" -- only its power to block is removed. + }); +});