Skip to content

Commit b7fc58c

Browse files
author
Eva
committed
fix(plugin-hooks): harden review edge cases
1 parent b2cb897 commit b7fc58c

4 files changed

Lines changed: 109 additions & 9 deletions

File tree

‎src/plugins/hooks.before-agent-finalize.test.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,43 @@ describe("before_agent_finalize hook runner", () => {
6060
});
6161
});
6262

63+
it("skips empty retry instructions when merging revise decisions", async () => {
64+
const runner = createHookRunner(
65+
createMockPluginRegistry([
66+
{
67+
hookName: "before_agent_finalize",
68+
handler: vi.fn().mockResolvedValue({
69+
action: "revise",
70+
reason: "needs a retry but forgot the instruction",
71+
retry: { instruction: " ", idempotencyKey: "empty-retry" },
72+
}),
73+
},
74+
{
75+
hookName: "before_agent_finalize",
76+
handler: vi.fn().mockResolvedValue({
77+
action: "revise",
78+
reason: "rerun the focused tests",
79+
retry: {
80+
instruction: " rerun the focused tests ",
81+
idempotencyKey: "valid-retry",
82+
maxAttempts: 1,
83+
},
84+
}),
85+
},
86+
]),
87+
);
88+
89+
await expect(runner.runBeforeAgentFinalize(EVENT, TEST_PLUGIN_AGENT_CTX)).resolves.toEqual({
90+
action: "revise",
91+
reason: "needs a retry but forgot the instruction\n\nrerun the focused tests",
92+
retry: {
93+
instruction: "rerun the focused tests",
94+
idempotencyKey: "valid-retry",
95+
maxAttempts: 1,
96+
},
97+
});
98+
});
99+
63100
it("lets finalize override earlier revise decisions", async () => {
64101
const runner = createHookRunner(
65102
createMockPluginRegistry([

‎src/plugins/hooks.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -325,30 +325,44 @@ export function createHookRunner(
325325
acc: PluginHookBeforeAgentFinalizeResult | undefined,
326326
next: PluginHookBeforeAgentFinalizeResult,
327327
): PluginHookBeforeAgentFinalizeResult => {
328+
const normalizeRetry = (
329+
retry: PluginHookBeforeAgentFinalizeResult["retry"] | undefined,
330+
): PluginHookBeforeAgentFinalizeResult["retry"] | undefined => {
331+
const instruction = retry?.instruction.trim();
332+
if (!instruction) {
333+
return undefined;
334+
}
335+
return {
336+
...retry,
337+
instruction,
338+
};
339+
};
328340
if (acc?.action === "finalize") {
329341
return acc;
330342
}
331343
if (next.action === "finalize") {
332344
return { action: "finalize", reason: next.reason };
333345
}
334346
if (acc?.action === "revise" && next.action === "revise") {
347+
const retry = normalizeRetry(acc.retry) ?? normalizeRetry(next.retry);
335348
return {
336349
action: "revise",
337350
reason: concatOptionalTextSegments({
338351
left: acc.reason,
339352
right: next.reason,
340353
}),
341-
...((acc.retry ?? next.retry) ? { retry: acc.retry ?? next.retry } : {}),
354+
...(retry ? { retry } : {}),
342355
};
343356
}
344357
if (acc?.action === "revise") {
345358
return acc;
346359
}
347360
if (next.action === "revise") {
361+
const retry = normalizeRetry(next.retry);
348362
return {
349363
action: "revise",
350364
reason: next.reason,
351-
...(next.retry ? { retry: next.retry } : {}),
365+
...(retry ? { retry } : {}),
352366
};
353367
}
354368
return next.action === "continue" ? { action: "continue", reason: next.reason } : (acc ?? next);

‎src/plugins/loader.runtime-registry.test.ts‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,53 @@ describe("getCompatibleActivePluginRegistry", () => {
101101
).toBeUndefined();
102102
});
103103

104+
it("does not reuse a skip-runtime-deps registry for an explicit install request", () => {
105+
const registry = createEmptyPluginRegistry();
106+
const loadOptions = {
107+
config: {
108+
plugins: {
109+
allow: ["demo"],
110+
load: { paths: ["/tmp/demo.js"] },
111+
},
112+
},
113+
workspaceDir: "/tmp/workspace-a",
114+
};
115+
const { cacheKey: skipInstallCacheKey } = __testing.resolvePluginLoadCacheContext({
116+
...loadOptions,
117+
installBundledRuntimeDeps: false,
118+
});
119+
setActivePluginRegistry(registry, skipInstallCacheKey);
120+
121+
expect(
122+
__testing.getCompatibleActivePluginRegistry({
123+
...loadOptions,
124+
installBundledRuntimeDeps: true,
125+
}),
126+
).toBeUndefined();
127+
});
128+
129+
it("allows a skip-runtime-deps caller to reuse an active install-capable registry", () => {
130+
const registry = createEmptyPluginRegistry();
131+
const loadOptions = {
132+
config: {
133+
plugins: {
134+
allow: ["demo"],
135+
load: { paths: ["/tmp/demo.js"] },
136+
},
137+
},
138+
workspaceDir: "/tmp/workspace-a",
139+
};
140+
const { cacheKey: installCacheKey } = __testing.resolvePluginLoadCacheContext(loadOptions);
141+
setActivePluginRegistry(registry, installCacheKey);
142+
143+
expect(
144+
__testing.getCompatibleActivePluginRegistry({
145+
...loadOptions,
146+
installBundledRuntimeDeps: false,
147+
}),
148+
).toBe(registry);
149+
});
150+
104151
it("does not embed activation secrets in the loader cache key", () => {
105152
const { cacheKey } = __testing.resolvePluginLoadCacheContext({
106153
config: {

‎src/plugins/loader.ts‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -706,7 +706,7 @@ function hasExplicitCompatibilityInputs(options: PluginLoadOptions): boolean {
706706
options.forceSetupOnlyChannelPlugins === true ||
707707
options.requireSetupEntryForSetupOnlyChannelPlugins === true ||
708708
options.preferSetupRuntimeForChannelPlugins === true ||
709-
options.installBundledRuntimeDeps === false ||
709+
options.installBundledRuntimeDeps !== undefined ||
710710
options.loadModules === false
711711
);
712712
}
@@ -990,12 +990,14 @@ function resolvePluginLoadCompatibilityCacheKeys(
990990
exactCacheKey?: string,
991991
): Set<string> {
992992
const keys = new Set<string>([exactCacheKey ?? resolvePluginLoadCacheContext(options).cacheKey]);
993-
keys.add(
994-
resolvePluginLoadCacheContext({
995-
...options,
996-
installBundledRuntimeDeps: options.installBundledRuntimeDeps === false ? undefined : false,
997-
}).cacheKey,
998-
);
993+
if (options.installBundledRuntimeDeps === false) {
994+
keys.add(
995+
resolvePluginLoadCacheContext({
996+
...options,
997+
installBundledRuntimeDeps: undefined,
998+
}).cacheKey,
999+
);
1000+
}
9991001
return keys;
10001002
}
10011003

0 commit comments

Comments
 (0)