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