Skip to content

Commit 5513d73

Browse files
committed
fix(permissions): require explicit denied ask binding
1 parent 750ffaf commit 5513d73

4 files changed

Lines changed: 159 additions & 33 deletions

File tree

‎src/permission/worker-grant.test.ts‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -286,19 +286,32 @@ describe("WorkerGrantStore lifecycle", () => {
286286
expect(store.invalidateSession("worker-1", "again")).toBe(0);
287287
});
288288

289-
test("attachToAsk stamps the questionId and keeps pending", () => {
289+
test("attachToAsk binds only an explicit requestId and keeps it pending", () => {
290290
const store = new WorkerGrantStore();
291291
const envelope = store.register(createDeniedCallEnvelope(descriptor()));
292292
// Reads pin the clock: fixture now predates wall time, so an unpinned
293293
// read would sweep the envelope as expired.
294-
expect(store.attachToAsk("worker-1", "ask-7", undefined, 1_000_001)).toBe(
295-
envelope,
296-
);
297-
expect(envelope.questionId).toBe("ask-7");
294+
expect(
295+
store.attachToAsk("worker-1", "ask-generic", undefined, 1_000_001),
296+
).toBeUndefined();
297+
expect(envelope.questionId).toBeUndefined();
298298
expect(envelope.status).toBe("pending");
299299
expect(store.peek(envelope.requestId)).toBe(envelope);
300300
expect(
301-
store.attachToAsk("worker-9", "ask-8", undefined, 1_000_001),
301+
store.attachToAsk("worker-1", "ask-7", envelope.requestId, 1_000_001),
302+
).toBe(envelope);
303+
expect(envelope.questionId).toBe("ask-7");
304+
expect(
305+
store.attachToAsk(
306+
"worker-1",
307+
"ask-rebound",
308+
envelope.requestId,
309+
1_000_001,
310+
),
311+
).toBeUndefined();
312+
expect(envelope.questionId).toBe("ask-7");
313+
expect(
314+
store.attachToAsk("worker-9", "ask-8", envelope.requestId, 1_000_001),
302315
).toBeUndefined();
303316
expect(envelope.audit.map((event) => event.event)).toEqual([
304317
"denied",
@@ -332,12 +345,12 @@ describe("WorkerGrantStore lifecycle", () => {
332345
expect(store.peek(envelopeB.requestId)?.questionId).toBe("ask-B");
333346
});
334347

335-
test("unnamed ask keeps the legacy first-pending bind", () => {
348+
test("unnamed ask does not bind a pending denial", () => {
336349
const { store, envelopeA, envelopeB } = twoDenies();
337-
expect(store.attachToAsk("worker-1", "ask-x", undefined, 1_000_001)).toBe(
338-
envelopeA,
339-
);
340-
expect(envelopeA.questionId).toBe("ask-x");
350+
expect(
351+
store.attachToAsk("worker-1", "ask-x", undefined, 1_000_001),
352+
).toBeUndefined();
353+
expect(envelopeA.questionId).toBeUndefined();
341354
expect(envelopeB.questionId).toBeUndefined();
342355
});
343356

@@ -373,7 +386,7 @@ describe("WorkerGrantStore lifecycle", () => {
373386
test("audit trail orders deny → ask → consume", () => {
374387
const store = new WorkerGrantStore();
375388
const envelope = store.register(createDeniedCallEnvelope(descriptor()));
376-
store.attachToAsk("worker-1", "ask-1", undefined, 1_000_001);
389+
store.attachToAsk("worker-1", "ask-1", envelope.requestId, 1_000_001);
377390
store.consumeOnAllow({
378391
sessionId: "worker-1",
379392
canonicalTool: "run_shell",

‎src/permission/worker-grant.ts‎

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -281,32 +281,27 @@ export class WorkerGrantStore {
281281
return undefined;
282282
}
283283

284-
/** Stamp the questionId the parent's retry references. A named requestId
285-
* binds its exact denial (first-pending would join a question about B to
286-
* call A); an unnamed ask keeps the legacy first-pending bind. Unresolvable
287-
* ids fail closed; overdue envelopes sweep first. */
284+
/** Stamp the questionId the parent's retry references. Only the exact
285+
* requestId supplied by the worker can bind a denial: session-wide fallback
286+
* would attach a stale denial to an unrelated ask. Unresolvable or already
287+
* attached ids fail closed; overdue envelopes sweep first. */
288288
attachToAsk(
289289
sessionId: string,
290290
questionId: string,
291291
requestId?: string,
292292
now: number = Date.now(),
293293
): WorkerDeniedCallEnvelope | undefined {
294294
this.sweepExpired(now);
295-
const named = requestId?.trim() || undefined;
296-
if (named !== undefined) {
297-
const envelope = this.envelopes.get(named);
298-
if (
299-
envelope === undefined ||
300-
envelope.workerSessionId !== sessionId ||
301-
envelope.status !== "pending"
302-
)
303-
return undefined;
304-
envelope.questionId = questionId;
305-
audit(envelope, "asked", questionId);
306-
return envelope;
307-
}
308-
const envelope = this.pendingForSession(sessionId, now);
309-
if (envelope === undefined) return undefined;
295+
const named = requestId?.trim();
296+
if (named === undefined || named.length === 0) return undefined;
297+
const envelope = this.envelopes.get(named);
298+
if (
299+
envelope === undefined ||
300+
envelope.workerSessionId !== sessionId ||
301+
envelope.status !== "pending" ||
302+
envelope.questionId !== undefined
303+
)
304+
return undefined;
310305
envelope.questionId = questionId;
311306
audit(envelope, "asked", questionId);
312307
return envelope;

‎src/subagent/agent-fleet.test.ts‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1942,6 +1942,7 @@ describe("list_agents", () => {
19421942
});
19431943

19441944
test("projects a bound grant_request_id without relying on ask prose", async () => {
1945+
getProcessWorkerGrantStore().clear();
19451946
const sessions = createSubAgentSessionStore();
19461947
const fleetRecords = createFleetMailbox(sessions);
19471948
const id = "grant-projection";
@@ -1996,6 +1997,123 @@ describe("list_agents", () => {
19961997
expect(waitedAgent.grant_request_id).toBe(envelope.requestId);
19971998
});
19981999

2000+
test("does not rebind a resolved denial to a later generic ask", async () => {
2001+
getProcessWorkerGrantStore().clear();
2002+
const sessions = createSubAgentSessionStore();
2003+
const fleetRecords = createFleetMailbox(sessions);
2004+
const id = "grant-projection-no-rebind";
2005+
sessions.start({
2006+
id,
2007+
description: "need approval",
2008+
agentId: "a",
2009+
brief: "b",
2010+
});
2011+
sessions.markRunning(id);
2012+
fleetRecords.register(id);
2013+
const grantStore = getProcessWorkerGrantStore();
2014+
const envelopeA = grantStore.register(
2015+
createDeniedCallEnvelope({
2016+
callId: "denied-call-A",
2017+
tool: "run_shell",
2018+
action: "Run",
2019+
subject: "git status",
2020+
args: { command: "git status" },
2021+
cwd: process.cwd(),
2022+
workerSessionId: id,
2023+
}),
2024+
);
2025+
expect(
2026+
sessions.registerAsk(id, {
2027+
question: "Please decide how to proceed.",
2028+
questionId: "ask-1",
2029+
grantRequestId: envelopeA.requestId,
2030+
assessment: assessedAsk,
2031+
resolve: () => undefined,
2032+
reject: () => undefined,
2033+
}),
2034+
).toBe(true);
2035+
expect(
2036+
sessions.resolveEscalationAsk(id, {
2037+
kind: "minimum_grant_available",
2038+
answer: "continue",
2039+
questionId: "ask-1",
2040+
}),
2041+
).toBe(true);
2042+
expect(grantStore.peek(envelopeA.requestId)?.status).toBe("pending");
2043+
2044+
expect(
2045+
sessions.registerAsk(id, {
2046+
question: "What should I do next?",
2047+
questionId: "ask-2",
2048+
assessment: assessedAsk,
2049+
resolve: () => undefined,
2050+
reject: () => undefined,
2051+
}),
2052+
).toBe(true);
2053+
expect(sessions.peekAsk(id)?.deniedCall).toBeUndefined();
2054+
2055+
const list = createListAgentsTool({ sessions, fleetRecords });
2056+
const listed = parseFleetJson(
2057+
(await callFleetToolRaw(list, {})).content,
2058+
) as { agents: { grant_request_id?: string }[] };
2059+
expect(defined(listed.agents[0]).grant_request_id).toBeUndefined();
2060+
2061+
const wait = createWaitAgentsTool({ sessions, fleetRecords });
2062+
const waited = await callFleetTool(wait, {
2063+
targets: [id],
2064+
timeout_ms: 1,
2065+
});
2066+
expect(
2067+
defined((waited.results as { grant_request_id?: string }[])[0])
2068+
.grant_request_id,
2069+
).toBeUndefined();
2070+
2071+
expect(sessions.resolveAsk(id, "continue")).toBe(true);
2072+
const envelopeB = grantStore.register(
2073+
createDeniedCallEnvelope({
2074+
callId: "denied-call-B",
2075+
tool: "run_shell",
2076+
action: "Run",
2077+
subject: "git diff",
2078+
args: { command: "git diff" },
2079+
cwd: process.cwd(),
2080+
workerSessionId: id,
2081+
}),
2082+
);
2083+
expect(
2084+
sessions.registerAsk(id, {
2085+
question: "May I retry git diff?",
2086+
questionId: "ask-3",
2087+
grantRequestId: envelopeB.requestId,
2088+
assessment: assessedAsk,
2089+
resolve: () => undefined,
2090+
reject: () => undefined,
2091+
}),
2092+
).toBe(true);
2093+
expect(sessions.peekAsk(id)?.deniedCall?.requestId).toBe(
2094+
envelopeB.requestId,
2095+
);
2096+
expect(grantStore.peek(envelopeA.requestId)?.questionId).toBe("ask-1");
2097+
2098+
const listB = createListAgentsTool({ sessions, fleetRecords });
2099+
const listedB = parseFleetJson(
2100+
(await callFleetToolRaw(listB, {})).content,
2101+
) as { agents: { grant_request_id?: string }[] };
2102+
expect(defined(listedB.agents[0]).grant_request_id).toBe(
2103+
envelopeB.requestId,
2104+
);
2105+
2106+
const waitB = createWaitAgentsTool({ sessions, fleetRecords });
2107+
const waitedB = await callFleetTool(waitB, {
2108+
targets: [id],
2109+
timeout_ms: 1,
2110+
});
2111+
expect(
2112+
defined((waitedB.results as { grant_request_id?: string }[])[0])
2113+
.grant_request_id,
2114+
).toBe(envelopeB.requestId);
2115+
});
2116+
19992117
test("errors after wait_agents surfaces awaiting_director with a question", async () => {
20002118
const { gate, deps, list, id } = await spawnParkedAsk();
20012119
const wait = waitTool(deps);

‎src/subagent/session-store.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2094,8 +2094,8 @@ export function createSubAgentSessionStore(
20942094
if (pendingAsks.has(id)) return false;
20952095
// Attach the harness-owned denied-call envelope (exact ToolCall + hash)
20962096
// to the ask record, stamping its questionId for the parent's retry
2097-
// ref. A named grantRequestId binds that exact denial; an unnamed ask
2098-
// falls back to the session's first pending envelope.
2097+
// ref. Only a named grantRequestId binds its exact denial; generic asks
2098+
// must not inherit a pending envelope from an earlier question.
20992099
let deniedCall: WorkerDeniedCallEnvelope | undefined;
21002100
try {
21012101
deniedCall =

0 commit comments

Comments
 (0)