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. + }); +});