From 42b9f4d4ab4a00296e9f177a89f69bae6a6e8d92 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:33:31 -0700 Subject: [PATCH] refactor(review): extract the null-disables-label idiom into one helper (#4618) 9 of the 13 near-identical per-repo "close/hold with this label" settings (blacklistLabel, contributorCapLabel, reviewNagLabel, reviewEvasionLabel, manualReviewLabel, readyToMergeLabel, changesRequestedLabel, migrationCollisionLabel, pendingClosureLabel) share the exact same "explicit null disables the label, undefined uses the operator-facing default" resolution idiom -- copy-pasted independently 9 times across settings/agent-actions.ts and queue/processors.ts rather than routed through a shared function. Adds resolveNullableLabel(configured, fallback) and replaces every call site with it. Pure mechanical DRY refactor -- zero behavior change, confirmed by the full existing test suite (which already exercised the null/undefined/explicit-string cases for each field) passing unchanged. This addresses the duplication half of #4618 item 3. The other half -- collapsing the 13 fields' TYPE surface, DB columns, and yml keys into one labels map -- is deliberately deferred: the fields are not type-uniform (3 different nullability shapes), only 6 of 13 have DB columns with a default, 2 have nullable columns with no default, 5 are config-as-code-only with no DB column at all, and the 13 keys are live top-level `.gittensory.yml` keys today, so a real collapse needs a backward-compat parsing shim and a data migration across 8 differently- shaped columns -- correctly scoped as its own follow-up per the issue's own "expect multiple PRs" framing. --- src/queue/processors.ts | 7 ++++--- src/settings/agent-actions.ts | 24 ++++++++++++++++-------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8e25c88d93..e785d78f65 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -302,6 +302,7 @@ import { MAX_REVIEW_NAG_COOLDOWN_DAYS, isProtectedAutomationAuthor, planAgentMaintenanceActions, + resolveNullableLabel, type AgentActionPlanInput, type AgentDispositionLabelSettings, type PlannedAgentAction, @@ -14319,7 +14320,7 @@ async function closeReviewEvasionSelfCloseIfActive( () => undefined, ); } - const label = settings.reviewEvasionLabel === null ? null : (settings.reviewEvasionLabel ?? DEFAULT_REVIEW_EVASION_LABEL); + const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL); if (label !== null) { await ensurePullRequestLabel(env, installationId, repoFullName, pr.number, label, { createMissingLabel: true }).catch(() => undefined); } @@ -14468,7 +14469,7 @@ async function closeReviewEvasionDraftConversionIfActive( () => undefined, ); } - const label = settings.reviewEvasionLabel === null ? null : (settings.reviewEvasionLabel ?? DEFAULT_REVIEW_EVASION_LABEL); + const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL); if (label !== null) { await ensurePullRequestLabel(env, installationId, repoFullName, pr.number, label, { createMissingLabel: true }).catch(() => undefined); } @@ -14632,7 +14633,7 @@ async function closeRepeatedDraftCyclingIfDetected( () => undefined, ); } - const label = settings.reviewEvasionLabel === null ? null : (settings.reviewEvasionLabel ?? DEFAULT_REVIEW_EVASION_LABEL); + const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL); if (label !== null) { await ensurePullRequestLabel(env, installationId, repoFullName, pr.number, label, { createMissingLabel: true }).catch(() => undefined); } diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 240b463e03..0c88f53e7d 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -52,6 +52,14 @@ export const AGENT_LABEL_NEEDS_REVIEW = "manual-review"; // recurring failure mode separately from an ordinary guardrail hold. export const AGENT_LABEL_MIGRATION_COLLISION = "migration-collision"; +// #label-scoping (#4618): every per-repo configurable close/hold label shares this shape -- explicit `null` +// means "act without any label" (an operator opt-out), `undefined` (never configured) falls back to the +// operator-facing default below. Factored out of ~9 independently hand-copied ternaries of the exact same +// idiom across this file and queue/processors.ts. PURE. +export function resolveNullableLabel(configured: string | null | undefined, fallback: string): string | null { + return configured === null ? null : (configured ?? fallback); +} + // Maintainer-managed automation accounts whose PRs are never auto-closed. A recurring accumulator (e.g. // github-actions[bot] opening automation/readme-refresh) or a dependency PR must not be killed by a duplicate // or slop heuristic — the maintainer owns its lifecycle. (reviewbot wrongly auto-closed such an accumulator, @@ -439,11 +447,11 @@ type ResolvedAgentDispositionLabels = { function resolveAgentDispositionLabels(settings: AgentDispositionLabelSettings): ResolvedAgentDispositionLabels { return { - manualReview: settings.manualReviewLabel === null ? null : (settings.manualReviewLabel ?? AGENT_LABEL_NEEDS_REVIEW), - readyToMerge: settings.readyToMergeLabel === null ? null : (settings.readyToMergeLabel ?? AGENT_LABEL_READY), - changesRequested: settings.changesRequestedLabel === null ? null : (settings.changesRequestedLabel ?? AGENT_LABEL_CHANGES), - migrationCollision: settings.migrationCollisionLabel === null ? null : (settings.migrationCollisionLabel ?? AGENT_LABEL_MIGRATION_COLLISION), - pendingClosure: settings.pendingClosureLabel === null ? null : (settings.pendingClosureLabel ?? AGENT_LABEL_PENDING_CLOSURE), + manualReview: resolveNullableLabel(settings.manualReviewLabel, AGENT_LABEL_NEEDS_REVIEW), + readyToMerge: resolveNullableLabel(settings.readyToMergeLabel, AGENT_LABEL_READY), + changesRequested: resolveNullableLabel(settings.changesRequestedLabel, AGENT_LABEL_CHANGES), + migrationCollision: resolveNullableLabel(settings.migrationCollisionLabel, AGENT_LABEL_MIGRATION_COLLISION), + pendingClosure: resolveNullableLabel(settings.pendingClosureLabel, AGENT_LABEL_PENDING_CLOSURE), }; } @@ -630,7 +638,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // #label-scoping: this label is inseparable metadata on the close below, so it rides on `close` autonomy, // NOT the generic `label` class — a repo can enable close without also opting into the broad label dial. // Explicit `null` (vs. absent/undefined) means "close without any label." - const label = input.blacklistLabel === null ? null : (input.blacklistLabel ?? DEFAULT_BLACKLIST_LABEL); + const label = resolveNullableLabel(input.blacklistLabel, DEFAULT_BLACKLIST_LABEL); // Close is pushed BEFORE its coupled label (#label-close-split-brain) so the executor's outcome-correlation // guard always has the close's outcome already recorded by the time it evaluates the label. if (acting("close")) { @@ -658,7 +666,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne if (input.contributorCapMatch?.matched === true && capContributor) { const { authorLogin, openCount, cap, itemKind, scope } = input.contributorCapMatch; // #label-scoping: same close-autonomy-gated, null-clearable shape as the blacklist label above. - const label = input.contributorCapLabel === null ? null : (input.contributorCapLabel ?? DEFAULT_CONTRIBUTOR_CAP_LABEL); + const label = resolveNullableLabel(input.contributorCapLabel, DEFAULT_CONTRIBUTOR_CAP_LABEL); // Close is pushed BEFORE its coupled label (#label-close-split-brain) — see the closeKind doc comment above. if (acting("close")) { actions.push({ @@ -683,7 +691,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne if (input.reviewNagMatch?.matched === true && reviewNagContributor) { const { authorLogin, pingCount, maxPings } = input.reviewNagMatch; // #label-scoping: same close-autonomy-gated, null-clearable shape as the blacklist label above. - const label = input.reviewNagLabel === null ? null : (input.reviewNagLabel ?? DEFAULT_REVIEW_NAG_LABEL); + const label = resolveNullableLabel(input.reviewNagLabel, DEFAULT_REVIEW_NAG_LABEL); // Close is pushed BEFORE its coupled label (#label-close-split-brain) — see the closeKind doc comment above. if (acting("close")) { actions.push({