refactor(executor): make the three protection-bearing context fields REQUIRED (#9541)

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) },

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<string, string> | 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:<repo>:` 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<void> {
   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<void> {
@@ -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;
 };
 
 /**

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,

test/unit/agent-action-executor.test.ts

@@ -98,6 +98,10 @@ function ctx(over: Partial<AgentActionExecutionContext> = {}): 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<IssueActionExecutionContext> = {}): 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();
   });

test/unit/agent-approval-queue.test.ts

@@ -92,6 +92,10 @@ function ctx(over: Partial<AgentActionExecutionContext> = {}): 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,

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:<repo>:`, 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");
+  });
+});