Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions src/services/agent-action-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,8 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
}
// 9) live — perform the real mutation, recording success or the error.
try {
await performAction(env, ctx, action);
await audit("completed", action.reason);
const detailOverride = await performAction(env, ctx, action);
await audit("completed", detailOverride ?? action.reason);
// CI-run cancellation on a contributor_cap close (#2462, anti-abuse): stop burning CI minutes on a PR
// that was just closed for exceeding the contributor cap. Best-effort, AFTER the close already
// succeeded -- cancelInFlightWorkflowRunsForHeadSha never throws, so a missing actions:write grant (or
Expand Down Expand Up @@ -665,7 +665,12 @@ async function handleMergeFailure(env: Env, ctx: AgentActionExecutionContext, er
}).catch(() => undefined);
}

async function performAction(env: Env, ctx: AgentActionExecutionContext, action: PlannedAgentAction): Promise<void> {
/** Performs the action's real GitHub mutation. Returns an optional audit-detail override — used only by the
* "assign" case (below) to distinguish a real assignee from the by:<login> fallback, since GitHub silently
* drops an ineligible assignee rather than erroring, so the caller's generic `audit("completed", action.reason)`
* would otherwise look identical for both outcomes. Every other case implicitly returns undefined, keeping the
* caller's original `action.reason` detail. */
async function performAction(env: Env, ctx: AgentActionExecutionContext, action: PlannedAgentAction): Promise<string | undefined> {
switch (action.actionClass) {
case "label":
// Flag-then-close double-check: a `label` action may ADD (default) or REMOVE its label, and may carry an
Expand Down Expand Up @@ -729,7 +734,7 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action:
}
case "assign": {
const login = action.assignee ?? "";
if (!login) return;
if (!login) return undefined;
const result = await ensurePullRequestAssignee(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, login);
if (!result.applied) {
// GitHub silently drops an assignee lacking push/triage access to the repo -- the common case for an
Expand All @@ -739,8 +744,11 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action:
// at 50, so a longer prefix can push a valid max-length login past the limit and fail this fallback for
// exactly the contributors it exists to cover.
await ensurePullRequestLabel(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, `by:${login}`, { createMissingLabel: true });
// Audit-visibility gap fix: without this override, "completed" always carries the planner's generic
// "auto-assign PR opener" reason, so audit_events can't distinguish a real assignee from this fallback.
return `assignee refused by GitHub — fell back to a by:${login} label`;
}
return;
return undefined;
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions test/unit/agent-action-executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,10 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
expect(ensurePullRequestAssignee).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "alice");
expect(ensurePullRequestLabel).not.toHaveBeenCalled();
expect(outcomes[0]?.outcome).toBe("completed");
// REGRESSION (#audit-assign-fallback-visibility): a real, sticking assignee keeps the planner's generic
// reason as the audit detail -- only the fallback path below overrides it.
const audit = await env.DB.prepare("select detail from audit_events where event_type = 'agent.action.assign' order by created_at desc limit 1").first<{ detail: string }>();
expect(audit?.detail).toBe("auto-assign PR opener");
});

it("LIVE assign (#3182): falls back to a per-login label when GitHub silently drops an ineligible assignee", async () => {
Expand All @@ -536,6 +540,11 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
const outcomes = await executeAgentMaintenanceActions(env, ctx({ autonomy: { assign: "auto" } }), [assign]);
expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "by:external-contributor", { createMissingLabel: true });
expect(outcomes[0]?.outcome).toBe("completed");
// REGRESSION (#audit-assign-fallback-visibility): the audit detail must distinguish this fallback from a
// real applied assignee -- previously both cases recorded the identical generic planner reason, so
// audit_events had no way to tell a silently-refused assignee from a successful one.
const audit = await env.DB.prepare("select detail from audit_events where event_type = 'agent.action.assign' order by created_at desc limit 1").first<{ detail: string }>();
expect(audit?.detail).toBe("assignee refused by GitHub — fell back to a by:external-contributor label");
});

it("assign with no login is a no-op (defensive — the planner always sets it, but the executor must not call GitHub with an empty login)", async () => {
Expand Down
Loading