Skip to content

Commit 69916ca

Browse files
authored
fix(agent): bound structured close reasons (#3213)
* fix(agent): bound structured close reasons * fix(agent): surface closeReasons truncation on the real executor audit path The executor's closeReasonsForAudit() pre-bounded the reason count before buildAgentActionAudit() ever saw it, so closeReasonsTruncated could never be set on a real close action -- only on a direct buildAgentActionAudit() call with an unbounded array. Count-bounding now happens exactly once, inside buildAgentActionAudit, so the persisted audit row's closeReasonCount/closeReasonsTruncated reflect the true original count regardless of caller. Also removes a resulting dead ?? fallback that TypeScript's own narrowing already made unreachable. * fix(agent): bound close-reason count before per-reason string truncation The prior fix restored truncation visibility but reintroduced the original cost problem: closeReasonsForAudit mapped every reason through boundAuditReason before any count cap ran, so an unbounded closeReasons array still cost O(N) string-length work on the hot executor path. Bound the count first (a cheap slice), map only the bounded subset, and carry the true original count to buildAgentActionAudit separately (as closeReasonCount) so the persisted audit row still correctly flags truncation.
1 parent d48c681 commit 69916ca

4 files changed

Lines changed: 67 additions & 8 deletions

File tree

src/services/agent-action-executor.ts

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels
2323
import { closeIssue, closePullRequest, createIssueComment, createPullRequestReview, dismissLatestBotApproval, mergePullRequest, updatePullRequestBranch } from "../github/pr-actions";
2424
import { fetchPullRequestFreshness, pullRequestFreshnessDetail } from "../github/pr-freshness";
2525
import { isActingAutonomyLevel, resolveAutonomy } from "../settings/autonomy";
26-
import { buildAgentActionAudit, formatAgentPermissionDenial, isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness, type AgentActionMode } from "../settings/agent-execution";
26+
import { boundStructuredCloseReasonsForPersistence, buildAgentActionAudit, formatAgentPermissionDenial, isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness, type AgentActionMode } from "../settings/agent-execution";
2727
import type { PlannedAgentAction } from "../settings/agent-actions";
2828
import type { AgentActionClass, AgentPendingActionParams, AutonomyLevel, AutonomyPolicy } from "../types";
2929
import { errorMessage } from "../utils/json";
@@ -50,10 +50,18 @@ function boundAuditReason(detail: string): string {
5050
return detail.length > AUDIT_REASON_MAX_LENGTH ? `${detail.slice(0, AUDIT_REASON_MAX_LENGTH)}…` : detail;
5151
}
5252

53-
function closeReasonsForAudit(action: PlannedAgentAction): string[] | undefined {
53+
function closeReasonsForAudit(action: PlannedAgentAction): { closeReasons: string[]; closeReasonCount: number } | undefined {
5454
if (action.actionClass !== "close") return undefined;
5555
const rawReasons = action.closeReasons?.length ? action.closeReasons : [action.reason];
56-
return rawReasons.map((reason) => boundAuditReason(reason));
56+
// Bound the COUNT first (a cheap slice) so the per-reason string truncation below only ever runs over the
57+
// persisted subset, never a potentially unbounded array -- the ORIGINAL count is carried separately as
58+
// closeReasonCount so buildAgentActionAudit can still flag truncation correctly even though closeReasons
59+
// itself is already bounded by the time it gets there (#3213 review: an unbounded .map(boundAuditReason)
60+
// here could exhaust Worker CPU/memory before any cap ran).
61+
return {
62+
closeReasons: boundStructuredCloseReasonsForPersistence(rawReasons).map((reason) => boundAuditReason(reason)),
63+
closeReasonCount: rawReasons.length,
64+
};
5765
}
5866

5967
// The PR-visible action classes that require an elevated GitHub App write permission. Most use
@@ -238,7 +246,7 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
238246
outcomes.push({ actionClass: action.actionClass, outcome, detail: boundedDetail });
239247
return recordAuditEvent(
240248
env,
241-
buildAgentActionAudit({ actionClass: action.actionClass, autonomyLevel, mode, outcome: auditOutcome, repoFullName: ctx.repoFullName, targetKey, actor: AGENT_ACTOR, reason: boundedDetail, closeReasons: closeReasonsForAudit(action) }),
249+
buildAgentActionAudit({ actionClass: action.actionClass, autonomyLevel, mode, outcome: auditOutcome, repoFullName: ctx.repoFullName, targetKey, actor: AGENT_ACTOR, reason: boundedDetail, ...closeReasonsForAudit(action) }),
242250
);
243251
};
244252

@@ -583,7 +591,7 @@ export async function executeIssueMaintenanceActions(env: Env, ctx: IssueActionE
583591
outcomes.push({ actionClass: action.actionClass, outcome, detail: boundedDetail });
584592
return recordAuditEvent(
585593
env,
586-
buildAgentActionAudit({ actionClass: action.actionClass, autonomyLevel, mode, outcome: auditOutcome, repoFullName: ctx.repoFullName, targetKey, actor: AGENT_ACTOR, reason: boundedDetail, closeReasons: closeReasonsForAudit(action) }),
594+
buildAgentActionAudit({ actionClass: action.actionClass, autonomyLevel, mode, outcome: auditOutcome, repoFullName: ctx.repoFullName, targetKey, actor: AGENT_ACTOR, reason: boundedDetail, ...closeReasonsForAudit(action) }),
587595
);
588596
};
589597

@@ -747,7 +755,7 @@ export function actionParams(action: PlannedAgentAction): AgentPendingActionPara
747755
...(action.reviewBody !== undefined ? { reviewBody: action.reviewBody } : {}),
748756
...(action.mergeMethod !== undefined ? { mergeMethod: action.mergeMethod } : {}),
749757
...(action.closeComment !== undefined ? { closeComment: action.closeComment } : {}),
750-
...(action.closeReasons !== undefined ? { closeReasons: action.closeReasons } : {}),
758+
...(action.closeReasons !== undefined ? { closeReasons: [...boundStructuredCloseReasonsForPersistence(action.closeReasons)] } : {}),
751759
...(action.expectedHeadSha !== undefined ? { expectedHeadSha: action.expectedHeadSha } : {}),
752760
...(action.dismissStaleApproval !== undefined ? { dismissStaleApproval: action.dismissStaleApproval } : {}),
753761
// Round-trip closeKind so a staged close's kind survives to accept-time — without it, the close-precision

src/settings/agent-execution.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,12 @@ import { isActingAutonomyLevel, resolveAutonomy } from "./autonomy";
1111
const PR_WRITE_ACTION_CLASSES: readonly AgentActionClass[] = ["review", "request_changes", "approve", "close", "update_branch"];
1212
const CONTENTS_WRITE_ACTION_CLASSES: readonly AgentActionClass[] = ["merge"];
1313

14+
export const STRUCTURED_CLOSE_REASONS_MAX_COUNT = 20;
15+
16+
export function boundStructuredCloseReasonsForPersistence<T>(closeReasons: readonly T[]): readonly T[] {
17+
return closeReasons.length > STRUCTURED_CLOSE_REASONS_MAX_COUNT ? closeReasons.slice(0, STRUCTURED_CLOSE_REASONS_MAX_COUNT) : closeReasons;
18+
}
19+
1420
export type AgentPermissionRequirement = { permission: string; requiredAccess: "write" };
1521

1622
// Whether the agent actually executes an action, only logs what it WOULD do, or is halted entirely (#776).
@@ -56,8 +62,14 @@ export function buildAgentActionAudit(input: {
5662
actor?: string | null | undefined;
5763
reason?: string | null | undefined;
5864
closeReasons?: readonly string[] | null | undefined;
65+
// The TRUE original count, when the caller has ALREADY bounded `closeReasons` itself for cost reasons
66+
// (closeReasonsForAudit bounds the count before per-reason string truncation to avoid unbounded work on the
67+
// hot path, #3213 review) -- falls back to closeReasons.length for a caller that passes the full array.
68+
closeReasonCount?: number | undefined;
5969
}): AuditEventRecord {
60-
const closeReasons = input.actionClass === "close" && input.closeReasons?.length ? [...input.closeReasons] : null;
70+
const closeReasonCount = input.actionClass === "close" ? (input.closeReasonCount ?? input.closeReasons?.length ?? 0) : 0;
71+
const closeReasons =
72+
input.actionClass === "close" && input.closeReasons?.length ? [...boundStructuredCloseReasonsForPersistence(input.closeReasons)] : null;
6173
return {
6274
eventType: `agent.action.${input.actionClass}`,
6375
actor: input.actor ?? null,
@@ -69,7 +81,7 @@ export function buildAgentActionAudit(input: {
6981
actionClass: input.actionClass,
7082
autonomyLevel: input.autonomyLevel,
7183
mode: input.mode,
72-
...(closeReasons ? { closeReasons, closeReasonCount: closeReasons.length } : {}),
84+
...(closeReasons ? { closeReasons, closeReasonCount, ...(closeReasonCount > closeReasons.length ? { closeReasonsTruncated: true } : {}) } : {}),
7385
},
7486
};
7587
}

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ import {
5959
type IssueActionExecutionContext,
6060
} from "../../src/services/agent-action-executor";
6161
import type { PlannedAgentAction } from "../../src/settings/agent-actions";
62+
import { STRUCTURED_CLOSE_REASONS_MAX_COUNT } from "../../src/settings/agent-execution";
6263
import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules";
6364
import { clearProcessLocalGlobalAgentFrozenCacheForTest, getGlobalContributorBlacklist, isGlobalAgentFrozen, setGlobalAgentFrozen, upsertGlobalModerationConfig, upsertPullRequestFromGitHub } from "../../src/db/repositories";
6465
import * as repositoriesModule from "../../src/db/repositories";
@@ -136,6 +137,13 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
136137
expect(replayed).toMatchObject({ actionClass: "close", requiresApproval: false, reason: "CI failed; blocker", closeComment: "closing", closeReasons: ["CI failed", "blocker"] });
137138
});
138139

140+
it("bounds structured closeReasons in approval-queue params", () => {
141+
const closeReasons = Array.from({ length: STRUCTURED_CLOSE_REASONS_MAX_COUNT + 1 }, (_, index) => `blocker ${index}`);
142+
const persisted = actionParams({ actionClass: "close", requiresApproval: true, reason: closeReasons.join("; "), closeComment: "closing", closeReasons });
143+
144+
expect(persisted.closeReasons).toEqual(closeReasons.slice(0, STRUCTURED_CLOSE_REASONS_MAX_COUNT));
145+
});
146+
139147
it("LIVE: executes each action class via its GitHub primitive and audits completed", async () => {
140148
const env = createTestEnv({});
141149
const outcomes = await executeAgentMaintenanceActions(env, ctx(), [label, requestChanges, approve, merge, close, updateBranch]);
@@ -223,6 +231,20 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
223231
expect(metadata.closeReasons[1].length).toBeLessThan(longReason.length);
224232
});
225233

234+
it("bounds structured closeReasons count before storing audit metadata, and flags the real close-action audit row as truncated", async () => {
235+
const env = createTestEnv({});
236+
const closeReasons = Array.from({ length: STRUCTURED_CLOSE_REASONS_MAX_COUNT + 1 }, (_, index) => `blocker ${index}`);
237+
const outcomes = await executeAgentMaintenanceActions(env, ctx(), [{ actionClass: "close", requiresApproval: false, reason: closeReasons.join("; "), closeComment: "closing", closeReasons }]);
238+
expect(outcomes[0]?.outcome).toBe("completed");
239+
240+
const audit = await auditFor(env, "close");
241+
const metadata = JSON.parse(audit?.metadata_json ?? "{}");
242+
expect(metadata.closeReasons).toEqual(closeReasons.slice(0, STRUCTURED_CLOSE_REASONS_MAX_COUNT));
243+
// The REAL count (before bounding), so an over-limit close is distinguishable from an exactly-at-limit one.
244+
expect(metadata.closeReasonCount).toBe(STRUCTURED_CLOSE_REASONS_MAX_COUNT + 1);
245+
expect(metadata.closeReasonsTruncated).toBe(true);
246+
});
247+
226248
it("#label-scoping: a label action's autonomyClass (not the literal actionClass) governs the durable re-check", async () => {
227249
const env = createTestEnv({});
228250
// autonomy.label is OFF; autonomy.close is ON — a label authorized via autonomyClass: "close" must still

test/unit/agent-execution.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
agentRequiresContentsWrite,
55
agentRequiresPrWrite,
66
buildAgentActionAudit,
7+
STRUCTURED_CLOSE_REASONS_MAX_COUNT,
78
formatAgentPermissionDenial,
89
isGlobalAgentPause,
910
requiredAgentActionPermissions,
@@ -119,6 +120,22 @@ describe("buildAgentActionAudit", () => {
119120
closeReasons: [],
120121
});
121122
expect(emptyCloseAudit.metadata).not.toHaveProperty("closeReasons");
123+
124+
const manyCloseReasons = Array.from({ length: STRUCTURED_CLOSE_REASONS_MAX_COUNT + 1 }, (_, index) => `blocker ${index}`);
125+
const truncatedCloseAudit = buildAgentActionAudit({
126+
actionClass: "close",
127+
autonomyLevel: "auto",
128+
mode: "live",
129+
outcome: "completed",
130+
repoFullName: "owner/repo",
131+
reason: "many blockers",
132+
closeReasons: manyCloseReasons,
133+
});
134+
expect(truncatedCloseAudit.metadata).toMatchObject({
135+
closeReasons: manyCloseReasons.slice(0, STRUCTURED_CLOSE_REASONS_MAX_COUNT),
136+
closeReasonCount: manyCloseReasons.length,
137+
closeReasonsTruncated: true,
138+
});
122139
});
123140
});
124141

0 commit comments

Comments
 (0)