Skip to content

Commit 2d8caad

Browse files
Evajalehman
Eva
authored and
jalehman
committed
fix(plugin-host-hooks): batch review-comment fixes round 2
Addresses 12 Copilot review threads on PR #72287 in one consolidated commit. - host-hook-state.ts: short-circuit drainPluginNextTurnInjections before updateSessionStore when no injections are queued; add Array.isArray guards in both enqueue and drain so a malformed/hand-edited persisted value can't crash prompt-build for the whole session. - host-hooks.ts: drop the redundant local isPluginJsonValue import that was already covered by the re-export. - trusted-tool-policy.ts: block:false is no longer terminal — only block:true short-circuits the policy chain (matches the regular before_tool_call hook pipeline). Normalize block:true decisions to always carry a deterministic blockReason. - operator-scopes.ts + registry.ts: validate registerControlUiDescriptor's requiredScopes against the known OperatorScope set so untyped/JS plugins cannot project arbitrary strings to clients as if they were valid operator scopes. - registry.ts: scope tool-metadata uniqueness to (pluginId + toolName) instead of global toolName uniqueness, so different plugins owning different tools that happen to share a name can each register metadata. - tools-effective-inventory.ts + tools-catalog.ts: project tool metadata by `${pluginId} ${toolName}` keyed against the tool's owning pluginId, so plugin-X cannot spoof/override the displayName/description/risk/tags of plugin-Y's tool (or a core tool). - host-hook-runtime.ts: setPluginRunContext now requires explicit unset:true to delete (matches sessions.pluginPatch semantics); clearPluginRunContext normalizes the namespace through the same trim() used by set/get so callers passing whitespace don't leave orphan entries. - attempt.prompt-helpers.ts + run.ts: cache drained next-turn injections by hookCtx.runId so retries within the same run reuse the first-attempt drain (the destructive consume would otherwise return [] on retry). FIFO-bounded at 256 entries; cleared explicitly from the run-scoped finally in run.ts via forgetPromptBuildDrainCacheForRun. Test coverage: 3 new attempt.prompt-helpers.test.ts cases lock in the runId cache behavior. Existing 2,084 plugin + contracts-plugin tests still pass.
1 parent d7d9e47 commit 2d8caad

11 files changed

Lines changed: 250 additions & 18 deletions

File tree

‎src/agents/pi-embedded-runner/run.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ import { log } from "./logger.js";
9090
import { resolveModelAsync } from "./model.js";
9191
import { createEmbeddedRunReplayState, observeReplayMetadata } from "./replay-state.js";
9292
import { handleAssistantFailover } from "./run/assistant-failover.js";
93+
import { forgetPromptBuildDrainCacheForRun } from "./run/attempt.prompt-helpers.js";
9394
import { createEmbeddedRunAuthController } from "./run/auth-controller.js";
9495
import { resolveAuthProfileFailureReason } from "./run/auth-profile-failure-policy.js";
9596
import { runEmbeddedAttemptWithBackend } from "./run/backend.js";
@@ -2432,6 +2433,7 @@ export async function runEmbeddedPiAgent(
24322433
};
24332434
}
24342435
} finally {
2436+
forgetPromptBuildDrainCacheForRun(params.runId);
24352437
await contextEngine.dispose?.();
24362438
stopRuntimeAuthRefreshTimer();
24372439
if (params.cleanupBundleMcpOnRunEnd === true) {

‎src/agents/pi-embedded-runner/run/attempt.prompt-helpers.test.ts‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,19 @@ const videoGenerationTaskStatusMocks = vi.hoisted(() => ({
88
buildActiveVideoGenerationTaskPromptContextForSession: vi.fn(),
99
}));
1010

11+
const hostHookStateMocks = vi.hoisted(() => ({
12+
drainPluginNextTurnInjectionContext: vi.fn(),
13+
}));
14+
1115
vi.mock("../../music-generation-task-status.js", () => musicGenerationTaskStatusMocks);
1216
vi.mock("../../video-generation-task-status.js", () => videoGenerationTaskStatusMocks);
17+
vi.mock("../../../plugins/host-hook-state.js", () => hostHookStateMocks);
1318

1419
import {
20+
forgetPromptBuildDrainCacheForRun,
1521
hasPromptSubmissionContent,
1622
resolveAttemptPrependSystemContext,
23+
resolvePromptBuildHookResult,
1724
} from "./attempt.prompt-helpers.js";
1825

1926
describe("resolveAttemptPrependSystemContext", () => {
@@ -104,3 +111,84 @@ describe("hasPromptSubmissionContent", () => {
104111
).toBe(true);
105112
});
106113
});
114+
115+
describe("resolvePromptBuildHookResult drain cache", () => {
116+
it("drains plugin next-turn injections at most once per runId across retry attempts", async () => {
117+
hostHookStateMocks.drainPluginNextTurnInjectionContext.mockReset();
118+
hostHookStateMocks.drainPluginNextTurnInjectionContext.mockResolvedValue({
119+
queuedInjections: [
120+
{
121+
id: "inj-1",
122+
pluginId: "demo",
123+
text: "first attempt context",
124+
placement: "prepend_context",
125+
createdAt: 1,
126+
},
127+
],
128+
prependContext: "first attempt context",
129+
});
130+
forgetPromptBuildDrainCacheForRun("run-cache-test");
131+
132+
const hookCtx = { runId: "run-cache-test", sessionKey: "agent:main:main" };
133+
134+
const first = await resolvePromptBuildHookResult({
135+
prompt: "hi",
136+
messages: [],
137+
hookCtx,
138+
});
139+
const second = await resolvePromptBuildHookResult({
140+
prompt: "hi",
141+
messages: [],
142+
hookCtx,
143+
});
144+
145+
expect(hostHookStateMocks.drainPluginNextTurnInjectionContext).toHaveBeenCalledTimes(1);
146+
expect(first.prependContext).toBe("first attempt context");
147+
expect(second.prependContext).toBe("first attempt context");
148+
149+
forgetPromptBuildDrainCacheForRun("run-cache-test");
150+
});
151+
152+
it("re-drains after the run-scoped cache is forgotten", async () => {
153+
hostHookStateMocks.drainPluginNextTurnInjectionContext.mockReset();
154+
hostHookStateMocks.drainPluginNextTurnInjectionContext.mockResolvedValueOnce({
155+
queuedInjections: [],
156+
prependContext: undefined,
157+
});
158+
hostHookStateMocks.drainPluginNextTurnInjectionContext.mockResolvedValueOnce({
159+
queuedInjections: [],
160+
prependContext: undefined,
161+
});
162+
163+
const hookCtx = { runId: "run-evict-test", sessionKey: "agent:main:main" };
164+
165+
await resolvePromptBuildHookResult({ prompt: "hi", messages: [], hookCtx });
166+
expect(hostHookStateMocks.drainPluginNextTurnInjectionContext).toHaveBeenCalledTimes(1);
167+
168+
forgetPromptBuildDrainCacheForRun("run-evict-test");
169+
170+
await resolvePromptBuildHookResult({ prompt: "hi", messages: [], hookCtx });
171+
expect(hostHookStateMocks.drainPluginNextTurnInjectionContext).toHaveBeenCalledTimes(2);
172+
});
173+
174+
it("drains every call when no runId is provided (no caching key)", async () => {
175+
hostHookStateMocks.drainPluginNextTurnInjectionContext.mockReset();
176+
hostHookStateMocks.drainPluginNextTurnInjectionContext.mockResolvedValue({
177+
queuedInjections: [],
178+
prependContext: undefined,
179+
});
180+
181+
await resolvePromptBuildHookResult({
182+
prompt: "hi",
183+
messages: [],
184+
hookCtx: { sessionKey: "agent:main:main" },
185+
});
186+
await resolvePromptBuildHookResult({
187+
prompt: "hi",
188+
messages: [],
189+
hookCtx: { sessionKey: "agent:main:main" },
190+
});
191+
192+
expect(hostHookStateMocks.drainPluginNextTurnInjectionContext).toHaveBeenCalledTimes(2);
193+
});
194+
});

‎src/agents/pi-embedded-runner/run/attempt.prompt-helpers.ts‎

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import type {
44
ContextEngineRuntimeContext,
55
} from "../../../context-engine/types.js";
66
import { drainPluginNextTurnInjectionContext } from "../../../plugins/host-hook-state.js";
7+
import { buildPluginAgentTurnPrepareContext } from "../../../plugins/host-hooks.js";
78
import type {
89
PluginAgentTurnPrepareResult,
910
PluginNextTurnInjectionRecord,
@@ -54,16 +55,59 @@ export type PromptBuildHookRunner = {
5455
) => Promise<PluginHookBeforeAgentStartResult | undefined>;
5556
};
5657

58+
// Cache drained next-turn injections by runId so retry attempts within the
59+
// same run reuse the first-attempt drain rather than calling drain again
60+
// (which destructively consumes from the session store and would return [] on
61+
// retry, dropping injection context). The cache is bounded to keep memory flat
62+
// across long-lived processes; entries are evicted FIFO once the cap is hit.
63+
const PROMPT_BUILD_DRAIN_CACHE_MAX = 256;
64+
const promptBuildDrainCache = new Map<string, PluginNextTurnInjectionRecord[]>();
65+
66+
function rememberDrainedInjections(
67+
runId: string,
68+
injections: PluginNextTurnInjectionRecord[],
69+
): void {
70+
if (promptBuildDrainCache.has(runId)) {
71+
promptBuildDrainCache.delete(runId);
72+
} else if (promptBuildDrainCache.size >= PROMPT_BUILD_DRAIN_CACHE_MAX) {
73+
const oldest = promptBuildDrainCache.keys().next().value;
74+
if (oldest !== undefined) {
75+
promptBuildDrainCache.delete(oldest);
76+
}
77+
}
78+
promptBuildDrainCache.set(runId, injections);
79+
}
80+
81+
/**
82+
* Releases the per-run drained-injection cache. Call when a run terminates so
83+
* the cap stays headroom for active runs.
84+
*/
85+
export function forgetPromptBuildDrainCacheForRun(runId: string | undefined): void {
86+
if (runId) {
87+
promptBuildDrainCache.delete(runId);
88+
}
89+
}
90+
5791
export async function resolvePromptBuildHookResult(params: {
5892
prompt: string;
5993
messages: unknown[];
6094
hookCtx: PluginHookAgentContext;
6195
hookRunner?: PromptBuildHookRunner | null;
6296
legacyBeforeAgentStartResult?: PluginHookBeforeAgentStartResult;
6397
}): Promise<PluginHookBeforePromptBuildResult> {
64-
const queuedContext = await drainPluginNextTurnInjectionContext({
65-
sessionKey: params.hookCtx.sessionKey,
66-
});
98+
const runId = params.hookCtx.runId;
99+
const cachedInjections = runId ? promptBuildDrainCache.get(runId) : undefined;
100+
const queuedContext = cachedInjections
101+
? {
102+
queuedInjections: cachedInjections,
103+
...buildPluginAgentTurnPrepareContext({ queuedInjections: cachedInjections }),
104+
}
105+
: await drainPluginNextTurnInjectionContext({
106+
sessionKey: params.hookCtx.sessionKey,
107+
});
108+
if (runId && !cachedInjections) {
109+
rememberDrainedInjections(runId, queuedContext.queuedInjections);
110+
}
67111
const turnPrepareResult =
68112
params.hookRunner?.runAgentTurnPrepare && params.hookRunner.hasHooks("agent_turn_prepare")
69113
? await params.hookRunner

‎src/agents/tools-effective-inventory.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,9 +242,13 @@ export function resolveEffectiveToolInventory(
242242
modelId: params.modelId,
243243
});
244244
const profile = effectivePolicy.providerProfile ?? effectivePolicy.profile ?? "full";
245+
// Key metadata by `${pluginId}${toolName}` so we only project metadata that
246+
// was registered BY the tool's owning plugin. Without this, plugin-X could
247+
// spoof/override the displayName/risk/tags of plugin-Y's (or a core) tool just
248+
// by registering metadata with the same toolName.
245249
const pluginToolMetadata = new Map(
246250
(getActivePluginRegistry()?.toolMetadata ?? []).map((entry) => [
247-
entry.metadata.toolName,
251+
`${entry.pluginId}
248252
entry.metadata,
249253
]),
250254
);
@@ -253,7 +257,9 @@ export function resolveEffectiveToolInventory(
253257
effectiveTools
254258
.map((tool) => {
255259
const source = resolveEffectiveToolSource(tool);
256-
const metadata = pluginToolMetadata.get(tool.name);
260+
const metadata = source.pluginId
261+
? pluginToolMetadata.get(`${source.pluginId}${tool.name}`)
262+
: undefined;
257263
return Object.assign(
258264
{
259265
id: tool.name,

‎src/gateway/operator-scopes.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,20 @@ export type OperatorScope =
1212
| typeof APPROVALS_SCOPE
1313
| typeof PAIRING_SCOPE
1414
| typeof TALK_SECRETS_SCOPE;
15+
16+
const KNOWN_OPERATOR_SCOPE_VALUES: readonly OperatorScope[] = [
17+
ADMIN_SCOPE,
18+
READ_SCOPE,
19+
WRITE_SCOPE,
20+
APPROVALS_SCOPE,
21+
PAIRING_SCOPE,
22+
TALK_SECRETS_SCOPE,
23+
];
24+
25+
export const KNOWN_OPERATOR_SCOPES: ReadonlySet<OperatorScope> = new Set(
26+
KNOWN_OPERATOR_SCOPE_VALUES,
27+
);
28+
29+
export function isOperatorScope(value: unknown): value is OperatorScope {
30+
return typeof value === "string" && KNOWN_OPERATOR_SCOPES.has(value as OperatorScope);
31+
}

‎src/gateway/server-methods/tools-catalog.ts‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,13 @@ function buildPluginGroups(params: {
9797
allowGatewaySubagentBinding: true,
9898
});
9999
const groups = new Map<string, ToolCatalogGroup>();
100+
// Key metadata by `${pluginId} ${toolName}` so we only project metadata that
101+
// was registered BY the tool's owning plugin. Without this scoping, plugin-X
102+
// could override the catalog label/description/risk/tags for another plugin's
103+
// tool by registering metadata with the same toolName.
100104
const pluginToolMetadata = new Map(
101105
(getActivePluginRegistry()?.toolMetadata ?? []).map((entry) => [
102-
entry.metadata.toolName,
106+
`${entry.pluginId} ${entry.metadata.toolName}`,
103107
entry.metadata,
104108
]),
105109
);
@@ -116,23 +120,26 @@ function buildPluginGroups(params: {
116120
pluginId,
117121
tools: [],
118122
} as ToolCatalogGroup);
123+
const ownedMetadata = meta?.pluginId
124+
? pluginToolMetadata.get(`${meta.pluginId} ${tool.name}`)
125+
: undefined;
119126
existing.tools.push({
120127
id: tool.name,
121128
label:
122-
normalizeOptionalString(pluginToolMetadata.get(tool.name)?.displayName) ??
129+
normalizeOptionalString(ownedMetadata?.displayName) ??
123130
normalizeOptionalString(tool.label) ??
124131
tool.name,
125132
description: summarizeToolDescriptionText({
126133
rawDescription:
127-
pluginToolMetadata.get(tool.name)?.description ??
134+
ownedMetadata?.description ??
128135
(typeof tool.description === "string" ? tool.description : undefined),
129136
displaySummary: tool.displaySummary,
130137
}),
131138
source: "plugin",
132139
pluginId,
133140
optional: meta?.optional,
134-
risk: pluginToolMetadata.get(tool.name)?.risk,
135-
tags: pluginToolMetadata.get(tool.name)?.tags,
141+
risk: ownedMetadata?.risk,
142+
tags: ownedMetadata?.tags,
136143
defaultProfiles: [],
137144
});
138145
groups.set(groupId, existing);

‎src/plugins/host-hook-runtime.ts‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,14 +76,20 @@ export function setPluginRunContext(params: {
7676
if (!runId || !namespace) {
7777
return false;
7878
}
79-
if (params.patch.unset || params.patch.value === undefined) {
79+
// Only an explicit `unset: true` deletes the run-context entry — silently
80+
// treating an accidentally-omitted `value` as a clear is surprising and
81+
// diverges from the stricter `sessions.pluginPatch` semantics.
82+
if (params.patch.unset === true) {
8083
clearPluginRunContext({
8184
pluginId: params.pluginId,
8285
runId,
8386
namespace,
8487
});
8588
return true;
8689
}
90+
if (params.patch.value === undefined) {
91+
return false;
92+
}
8793
if (!isPluginJsonValue(params.patch.value)) {
8894
return false;
8995
}
@@ -118,6 +124,19 @@ export function clearPluginRunContext(params: {
118124
runId?: string;
119125
namespace?: string;
120126
}): void {
127+
// Normalize namespace through the same trim() used by set/get so callers that
128+
// pass whitespace or differently-formatted strings hit the same Map keys and
129+
// don't leave orphan entries behind.
130+
const normalizedNamespace =
131+
params.namespace !== undefined ? normalizeNamespace(params.namespace) : undefined;
132+
// An empty-after-trim namespace is treated as "no namespace filter" rather
133+
// than as a literal-empty-string deletion: that matches the set/get rule that
134+
// empty namespaces are not addressable, and it avoids silently no-op-ing the
135+
// delete (which would otherwise look like a successful clear).
136+
const namespaceFilter =
137+
normalizedNamespace !== undefined && normalizedNamespace !== ""
138+
? normalizedNamespace
139+
: undefined;
121140
const state = getPluginHostRuntimeState();
122141
const runIds = params.runId ? [params.runId] : [...state.runContextByRunId.keys()];
123142
for (const runId of runIds) {
@@ -131,8 +150,8 @@ export function clearPluginRunContext(params: {
131150
if (!namespaces) {
132151
continue;
133152
}
134-
if (params.namespace) {
135-
namespaces.delete(params.namespace);
153+
if (namespaceFilter !== undefined) {
154+
namespaces.delete(namespaceFilter);
136155
} else {
137156
namespaces.clear();
138157
}

‎src/plugins/host-hook-state.ts‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,10 @@ export async function enqueuePluginNextTurnInjection(params: {
227227
return;
228228
}
229229
const injections = { ...entry.pluginNextTurnInjections };
230-
const existing = [...(injections[params.pluginId] ?? [])].filter(
230+
// Guard against malformed/hand-edited persisted state — a non-array value
231+
// here would crash the spread/filter and break the whole session's enqueue.
232+
const rawExisting = injections[params.pluginId];
233+
const existing = (Array.isArray(rawExisting) ? [...rawExisting] : []).filter(
231234
(candidate) => !isExpired(candidate, now),
232235
);
233236
const duplicate = record.idempotencyKey
@@ -259,6 +262,16 @@ export async function drainPluginNextTurnInjections(params: {
259262
if (!loaded.entry) {
260263
return [];
261264
}
265+
// Avoid the locked re-save in updateSessionStore when there is nothing queued.
266+
// Drain runs once per prompt build; the common case is no injections, so a
267+
// pre-flight read keeps prompt-build off the session-store write path.
268+
// (Concurrently-enqueued injections during this gap land on the next turn.)
269+
if (
270+
!loaded.entry.pluginNextTurnInjections ||
271+
Object.keys(loaded.entry.pluginNextTurnInjections).length === 0
272+
) {
273+
return [];
274+
}
262275
const now = params.now ?? Date.now();
263276
return await updateSessionStore(loaded.storePath, (store) => {
264277
const entry = store[loaded.storeKey];
@@ -275,6 +288,11 @@ export async function drainPluginNextTurnInjections(params: {
275288
if (!activePluginIds.has(pluginId)) {
276289
continue;
277290
}
291+
// Guard against malformed/hand-edited persisted state — a non-array value
292+
// here would crash .filter and break prompt-building for the session.
293+
if (!Array.isArray(entries)) {
294+
continue;
295+
}
278296
const liveEntries = entries.filter((candidate) => !isExpired(candidate, now));
279297
drained.push(...liveEntries);
280298
}

‎src/plugins/host-hooks.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import type {
66
PluginHookBeforeToolCallResult,
77
PluginHookToolContext,
88
} from "./hook-types.js";
9-
import { isPluginJsonValue } from "./host-hook-json.js";
109
import type { PluginJsonValue } from "./host-hook-json.js";
1110
import type {
1211
PluginAgentTurnPrepareResult,

0 commit comments

Comments
 (0)