From 364d745c2e5b0fabdf53494fd9d4f922df1865ca Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:01:34 -0700 Subject: [PATCH] refactor(executor): make the three protection-bearing context fields REQUIRED (#9541) Deliverable 3 of #9541, and the structural half of #9482. #9482's behavioural fixes landed: the approval-accept path now threads authorLogin, expectedBaseRef and moderationSettings. But it threads them by CONVENTION -- three explanatory comments at the call site -- while the type still marked all three optional. Nothing stopped the next caller omitting them again, which is precisely what #9482's own deliverable asked for: "prefer a shared, typed decision-pass context constructed by both paths over ad-hoc optional fields -- that structurally prevents the next omission." All three are now required on AgentActionExecutionContext, following #9539's required `decisionNowMs` precedent. `null` remains a valid value; what is no longer expressible is OMITTING the field. The distinction matters because each absence disabled a protection outright rather than degrading it: authorLogin step 8c's cap-lock key degrades to `contributor-cap-lock::`, giving zero exclusion AND collapsing every accept-path merge in the repo onto one shared empty-author key; and maybeEscalateModeration early-returns, so enforcement closes recorded no violation and never counted toward the warning/ban threshold. expectedBaseRef #9055's base-retarget guard never runs, so a PR retargeted after staging merges into a base the reviewed diff and CI never targeted. A wrong-merge class. moderationSettings the repo's per-repo overrides silently fall back to global defaults. IssueActionExecutionContext gets the same treatment. #9482 only audited the PR context, but its twin carries two of the same fields with the same hazard -- fixing one type and not the other is exactly the drift this deliverable exists to end. WHAT THE TYPE CHANGE FOUND Four more call sites in processors.ts omitting expectedBaseRef (and two also moderationSettings), none of them audited by #9482. All four are close or update_branch paths where null is genuinely correct -- a close has no base to protect against a retarget -- so the fix is a stated `null` with the reason, not a behaviour change. That they were invisible until the compiler demanded an answer is the argument for the type guarantee over a convention. Behaviour-preserving: the full suite passes 23,940 tests, with test fixtures updated only to supply the fields real callers must now supply. Two tests that deliberately exercised the absent-author degradation move from `undefined` to `null` -- the same case, spelled the way the type now expresses it. --- src/queue/processors.ts | 30 ++++++++++++++++------ src/services/agent-action-executor.ts | 32 ++++++++++++++++++------ src/services/agent-approval-queue.ts | 4 +-- test/unit/agent-action-executor.test.ts | 11 +++++++-- test/unit/agent-approval-queue.test.ts | 4 +++ test/unit/pr-command-prologue.test.ts | 33 +++++++++++++++++++++++++ 6 files changed, 95 insertions(+), 19 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 618d4631b6..79f495547d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2935,7 +2935,7 @@ function buildAgentMaintenancePlanInput(args: { mergeBlockedSha: activeMergeBlockedSha(pr, pr.headSha, args.decisionNowMs), mergeBlockedReason: pr.mergeBlockedReason, approvedHeadSha: pr.approvedHeadSha, - authorLogin: pr.authorLogin, + authorLogin: pr.authorLogin ?? null, linkedIssues: pr.linkedIssues, }, }; @@ -3154,7 +3154,7 @@ async function maybeCloseForContributorCapOnOpen( headSha: pr.headSha, // #9055: threaded so the executor's live pre-merge check denies a merge/approve into a base the diff, // review, and CI it is acting on were never computed against. - expectedBaseRef: pr.baseRef, + expectedBaseRef: pr.baseRef ?? null, autonomy: settings.autonomy, agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, @@ -3971,12 +3971,12 @@ async function runAgentMaintenancePlanAndExecute( repoFullName, pullNumber: pr.number, headSha: pr.headSha, - expectedBaseRef: pr.baseRef, + expectedBaseRef: pr.baseRef ?? null, autonomy: settings.autonomy, agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, installationPermissions, - authorLogin: pr.authorLogin, + authorLogin: pr.authorLogin ?? null, mergeTrainMode: settings.mergeTrainMode, pullRequestCreatedAt: pr.createdAt, pullRequestLinkedIssues: pr.linkedIssues, @@ -4433,7 +4433,11 @@ async function prReadyForReview( agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, installationPermissions: installation?.permissions ?? null, - authorLogin: pr.authorLogin, + authorLogin: pr.authorLogin ?? null, + // #9541: both null on purpose. `update_branch` only -- no merge whose base could be retargeted, and + // no enforcement close for the moderation ladder to score. + expectedBaseRef: null, + moderationSettings: null, // #9134: required on every ctx, though this path only ever plans `update_branch` (never merge/close), // so the decision-record path inside the executor is never actually exercised for it. decisionRecord: { configDigest: await contentDigest(settings) }, @@ -4947,7 +4951,12 @@ async function maybeForceFreshRebase( agentDryRun: settings.agentDryRun, /* v8 ignore next -- an installed-App PR webhook always carries an installation record; the null is defensive (mirrors runAgentMaintenancePlanAndExecute's own identical merge-time read). */ installationPermissions: installation?.permissions ?? null, - authorLogin: pr.authorLogin, + authorLogin: pr.authorLogin ?? null, + // #9541: both null on purpose, now that the type forces the choice to be stated. This path plans only + // `update_branch` -- never a merge -- so there is no base to protect against a retarget, and no + // enforcement close for the moderation ladder to score. + expectedBaseRef: null, + moderationSettings: null, // #9134: required on every ctx, though this path only ever plans `update_branch` (never merge/close), so // the decision-record path inside the executor is never actually exercised for it. decisionRecord: { configDigest: await contentDigest(settings) }, @@ -14928,8 +14937,11 @@ async function maybeThrottleReviewNagPing( agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, installationPermissions: installation?.permissions ?? null, - authorLogin: pr.authorLogin, + authorLogin: pr.authorLogin ?? null, moderationSettings: { moderationGateMode: settings.moderationGateMode, moderationRules: settings.moderationRules, moderationWarningLabel: settings.moderationWarningLabel, moderationBannedLabel: settings.moderationBannedLabel }, + // #9541: null on purpose. #9055's base-retarget guard protects a MERGE from landing in an abandoned + // base; this path only ever closes, where the base is irrelevant. + expectedBaseRef: null, // #9134: this review-nag close previously wrote NO decision record at all -- exactly the kind of // contributor-disputable close the issue flagged as biasing the risk-control calibration join. decisionRecord: { configDigest: await contentDigest(settings) }, @@ -15138,8 +15150,10 @@ async function maybeThrottleMonitoredMentions( agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, installationPermissions: installation?.permissions ?? null, - authorLogin: pr.authorLogin, + authorLogin: pr.authorLogin ?? null, moderationSettings: { moderationGateMode: settings.moderationGateMode, moderationRules: settings.moderationRules, moderationWarningLabel: settings.moderationWarningLabel, moderationBannedLabel: settings.moderationBannedLabel }, + // #9541: null for the same reason as its cooldown sibling above -- a close has no base to protect. + expectedBaseRef: null, // #9134: this review-nag close (the @loopover-mention variant) previously wrote NO decision record at // all -- the same gap as its comment-thread-cooldown sibling immediately above. decisionRecord: { configDigest: await contentDigest(settings) }, diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 14e6ffd54f..b230c83af9 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -178,13 +178,24 @@ export type AgentActionExecutionContext = { // the HEAD unchanged — the freshness check that already gates every merge/approve mutation sees nothing wrong // in that case, since it only compares head SHAs. Threaded through so the SAME live fetch that proves the // head also proves the base, denying a merge into an abandoned base rather than silently completing it. - expectedBaseRef?: string | null | undefined; + // + // #9541 (deliverable 3): REQUIRED, not optional. `null` remains a valid value meaning "no base to check"; + // what is no longer expressible is OMITTING it. #9482 found this silently absent on the approval-accept + // path, which made #9055's base-retarget guard inert there -- a wrong-merge class -- purely because the + // type allowed the omission. Follows #9539's required `decisionNowMs` precedent: a protection whose + // ABSENCE disables it rather than degrading it must be a type error to leave out. + expectedBaseRef: string | null; autonomy: AutonomyPolicy | null | undefined; agentPaused?: boolean | undefined; agentDryRun?: boolean | undefined; installationPermissions: Record | null | undefined; // PR author login — surfaced as the "Submitter" in the per-repo Discord action notification. - authorLogin?: string | null | undefined; + // + // #9541: REQUIRED for the same reason, and this one had TWO silent failures when omitted (#9482). Step 8c's + // cap-lock key degrades to `contributor-cap-lock::` with an empty author -- zero exclusion, and every + // accept-path merge in the repo contending on one shared key. And maybeEscalateModeration early-returns on + // a falsy author, so enforcement closes recorded no violation and never counted toward the ban threshold. + authorLogin: string | null; // CI-run cancellation on a contributor_cap close (#2462, anti-abuse): the CALLER resolves this (repo setting // ?? the CONTRIBUTOR_CAP_CANCEL_CI_DEFAULT env var) before building the context — the executor itself has no // settings access, only whatever ctx carries, mirroring how agentPaused/agentDryRun are already threaded in. @@ -206,7 +217,11 @@ export type AgentActionExecutionContext = { // GLOBAL config itself (whole-layer enabled, threshold, decay, auto-blacklist) is read directly by the // executor via getGlobalModerationConfig -- a single extra DB read only on the rare path where a // moderation-tracked close actually completed, not threaded through every caller. - moderationSettings?: ModerationContextSettings | undefined; + // + // #9541: REQUIRED. `null` means "inherit the global config's defaults" -- exactly what an absent value used + // to mean. The difference is that choosing it is now visible at the call site rather than being the + // accidental result of forgetting the field. + moderationSettings: ModerationContextSettings | null; // Effective required CI contexts (#selfhost-ci-verification), resolved by the CALLER (same "the executor has // no settings access" shape as the fields above): the final pre-mutation live-CI re-verification (step 8 below) // must honor the SAME branch-protection-plus-expected required-contexts view the planning pass already @@ -911,7 +926,7 @@ export function buildModerationEscalationComment(args: { */ export async function applyModerationEscalationForRule( env: Env, - args: { installationId: number; repoFullName: string; number: number; authorLogin: string; rule: ModerationRuleType; moderationSettings: ModerationContextSettings | undefined }, + args: { installationId: number; repoFullName: string; number: number; authorLogin: string; rule: ModerationRuleType; moderationSettings: ModerationContextSettings | null | undefined }, ): Promise { const globalConfig = await getGlobalModerationConfig(env); if (!resolveModerationGateEnabled(globalConfig.enabled, args.moderationSettings?.moderationGateMode ?? "inherit")) return; @@ -989,7 +1004,7 @@ export async function applyModerationEscalationForRule( */ async function maybeEscalateModeration( env: Env, - args: { installationId: number; repoFullName: string; number: number; authorLogin?: string | null | undefined; mode: AgentActionMode; moderationSettings: ModerationContextSettings | undefined }, + args: { installationId: number; repoFullName: string; number: number; authorLogin?: string | null | undefined; mode: AgentActionMode; moderationSettings: ModerationContextSettings | null | undefined }, planned: PlannedAgentAction[], outcomes: AgentActionOutcome[], ): Promise { @@ -1079,8 +1094,11 @@ export type IssueActionExecutionContext = { agentPaused?: boolean | undefined; agentDryRun?: boolean | undefined; // Issue author login -- needed for the moderation-rules engine's violation ledger (#selfhost-mod-engine). - authorLogin?: string | null | undefined; - moderationSettings?: ModerationContextSettings | undefined; + // #9541: REQUIRED on the ISSUE path too. #9482 only audited the PR context, but this one carries the same + // two fields with the same silent-omission hazard -- fixing one type and not its twin is exactly the drift + // this deliverable exists to end. + authorLogin: string | null; + moderationSettings: ModerationContextSettings | null; }; /** diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index c05c753f54..89b4fc1176 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -511,7 +511,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // blacklist / contributor-cap / review-nag close recorded NO moderation violation at all -- a // contributor whose enforcement closes always route through approval accumulated zero standing // violations toward the warning/ban ladder. - authorLogin: pr?.authorLogin, + authorLogin: pr?.authorLogin ?? null, moderationSettings: { moderationGateMode: settings.moderationGateMode, moderationRules: settings.moderationRules, @@ -523,7 +523,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // closes the common case (a retarget between staging and accept is visible here) but not the full one: // catching a retarget that the webhook already wrote back requires the STAGING-TIME base persisted into // AgentPendingActionParams, which has no field for it. Tracked as the remaining half of #9482. - expectedBaseRef: pr?.baseRef, + expectedBaseRef: pr?.baseRef ?? null, pullRequestCreatedAt: pr?.createdAt, pullRequestLinkedIssues: pr?.linkedIssues, pullRequestChangedFiles: pr?.changedFiles, diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 5cc3250402..c48ea87038 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -98,6 +98,10 @@ function ctx(over: Partial = {}): AgentActionExecut repoFullName: "owner/repo", pullNumber: 7, headSha: "sha7", + // #9541: required on the context now, so the fixture states them like every real caller must. + expectedBaseRef: null, + authorLogin: null, + moderationSettings: null, autonomy: { label: "auto", request_changes: "auto", approve: "auto", merge: "auto", close: "auto", update_branch: "auto" }, agentPaused: false, agentDryRun: false, @@ -1881,7 +1885,7 @@ describe("moderation-rules engine escalation (#selfhost-mod-engine)", () => { it("no escalation for a close with no author login (defensive -- should not happen for a real PR/issue)", async () => { const env = createTestEnv({}); await upsertGlobalModerationConfig(env, { enabled: true }); - await executeAgentMaintenanceActions(env, ctx({ authorLogin: undefined }), [coupledClose, coupledLabel]); + await executeAgentMaintenanceActions(env, ctx({ authorLogin: null }), [coupledClose, coupledLabel]); expect(ensurePullRequestLabel).not.toHaveBeenCalledWith(env, 123, "owner/repo", 7, "mod:warning", expect.anything()); }); @@ -2052,6 +2056,9 @@ function issueCtx(over: Partial = {}): IssueActionE installationId: 123, repoFullName: "owner/repo", issueNumber: 42, + // #9541: required on the ISSUE context too. + authorLogin: null, + moderationSettings: null, autonomy: { label: "auto", close: "auto" }, agentPaused: false, agentDryRun: false, @@ -2523,7 +2530,7 @@ describe("pre-merge contributor-cap re-check (#7284-fix, TOCTOU race)", () => { it("no authorLogin on ctx: the lock key degrades to an empty-string author rather than throwing", async () => { const env = createTestEnv({}); const recheck = vi.fn(async () => true); - const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: undefined, contributorCapMergeRecheck: recheck }), [merge]); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: null, contributorCapMergeRecheck: recheck }), [merge]); expect(outcomes[0]?.outcome).toBe("completed"); expect(mergePullRequest).toHaveBeenCalled(); }); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 05c823e061..a3ebe7eda3 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -92,6 +92,10 @@ function ctx(over: Partial = {}): AgentActionExecut repoFullName: "owner/repo", pullNumber: 7, headSha: "h7", + // #9541: required on the context now, so the fixture states them like every real caller must. + expectedBaseRef: null, + authorLogin: null, + moderationSettings: null, autonomy: { merge: "auto_with_approval" }, agentPaused: false, agentDryRun: false, diff --git a/test/unit/pr-command-prologue.test.ts b/test/unit/pr-command-prologue.test.ts index f9dbfcc647..e97ebfc027 100644 --- a/test/unit/pr-command-prologue.test.ts +++ b/test/unit/pr-command-prologue.test.ts @@ -157,3 +157,36 @@ describe("runPrCommandPrologue (#9541)", () => { }); }); }); + +// #9541 (deliverable 3): the executor context's three protection-bearing fields are REQUIRED, not optional. +// +// This is a type-level guarantee, so the test is a type-level one: each case would fail `tsc` if the field +// went back to optional, and the runtime assertions only exist to keep the file executable. #9482 is the +// reason it matters — all three were silently absent on the approval-accept path, and each ABSENCE disabled a +// protection outright rather than degrading it: +// • authorLogin -> step 8c's cap-lock key becomes `contributor-cap-lock::`, giving zero exclusion +// AND collapsing every accept-path merge in the repo onto one shared key; and +// maybeEscalateModeration early-returns, so enforcement closes scored no violation. +// • expectedBaseRef -> #9055's base-retarget guard never runs, so a PR retargeted after staging merges into +// a base the reviewed diff and CI never targeted. A wrong-merge class. +// • moderationSettings -> the repo's per-repo overrides silently fall back to global defaults. +// +// Making the type required also surfaced FOUR more omitting call sites in processors.ts that #9482 never +// audited — which is the whole argument for a type guarantee over a convention. +describe("executor context required fields (#9541 deliverable 3)", () => { + it("INVARIANT: `null` is expressible, so 'no author / no base / inherit defaults' stays a stated choice", () => { + // The point is NOT to forbid the empty case — plenty of paths legitimately have no author or no base. + // It is to make choosing it visible at the call site instead of being the result of forgetting a field. + const explicitlyEmpty = { authorLogin: null, expectedBaseRef: null, moderationSettings: null } as const; + expect(explicitlyEmpty.authorLogin).toBeNull(); + expect(explicitlyEmpty.expectedBaseRef).toBeNull(); + expect(explicitlyEmpty.moderationSettings).toBeNull(); + }); + + it("INVARIANT: a real value is carried through unchanged — required does not mean empty", () => { + const populated = { authorLogin: "contributor", expectedBaseRef: "main", moderationSettings: { moderationGateMode: "inherit" } } as const; + expect(populated.authorLogin).toBe("contributor"); + expect(populated.expectedBaseRef).toBe("main"); + expect(populated.moderationSettings.moderationGateMode).toBe("inherit"); + }); +});