Skip to content

Commit af36a07

Browse files
fix(permission): forget cached denies on a new user turn (#1090)
* fix(permission): forget cached denies on a new user turn Same-turn reactor retries still short-circuit. A later message must re-ask the same URL instead of canned-replying Tool call rejected. * fix(permission): clear denials only on operator inbound Mailbox, fleet-dry, and background-shell wakes are also message.received and must not forget a deny the operator just made. * style(permission): oxfmt CL-8002 tests
1 parent e096f3b commit af36a07

11 files changed

Lines changed: 248 additions & 11 deletions

File tree

src/agent/director.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
type CompactionGovernor,
2323
} from "./compaction.js";
2424
import { onTurnBoundary } from "./reactor-events.js";
25+
import { isOperatorOriginated } from "./message-provenance.js";
2526
import { type } from "arktype";
2627
import {
2728
applyManageTasks,
@@ -559,6 +560,9 @@ class ChatDirectorImpl extends DefaultDirector {
559560
private currentSourceId: string | undefined;
560561
/** CL-7918 live replacement for the former getLiveFleetCount closure. */
561562
private allowIdleWithFleet: boolean;
563+
// Forget cached permission denies on the next inbound user message. Session
564+
// wiring points this at PermissionGate.clearDenials; unset in unit tests.
565+
private clearDenials: (() => void) | undefined;
562566
// Consecutive assistant turns that contain tool calls and no text. Reset on
563567
// any turn with text and on every fresh user message — a weak model that
564568
// spins in place on one thread of tool calls still converges to the
@@ -749,6 +753,10 @@ class ChatDirectorImpl extends DefaultDirector {
749753
this.allowIdleWithFleet = value;
750754
}
751755

756+
setClearDenials(clear: (() => void) | undefined): void {
757+
this.clearDenials = clear;
758+
}
759+
752760
updateToolDefinitions(toolDefinitions: ToolDefinition[]): void {
753761
const before = toolSetDigest(this._toolDefinitions);
754762
const after = toolSetDigest(toolDefinitions);
@@ -1012,6 +1020,15 @@ class ChatDirectorImpl extends DefaultDirector {
10121020
this.toolOnlyStreak = 0;
10131021
this.toolOnlyNudgeFired = false;
10141022
this.pendingToolOnlyNudge = false;
1023+
// Occupancy mailbox / fleet-dry / bg-shell inbounds are also
1024+
// message.received; only a human prompt may forget cached denies.
1025+
const inboundFlags =
1026+
"flags" in event.message && Array.isArray(event.message.flags)
1027+
? event.message.flags.filter(
1028+
(flag): flag is string => typeof flag === "string",
1029+
)
1030+
: undefined;
1031+
if (isOperatorOriginated(inboundFlags)) this.clearDenials?.();
10151032
}
10161033
if (onTurnBoundary(event)) this.inferenceRecoveries = 0;
10171034

@@ -1357,6 +1374,7 @@ export interface ChatDirector extends ReactorDirector {
13571374
updateToolDefinitions(toolDefinitions: ToolDefinition[]): void;
13581375
setWorkflowCoordinator(coordinator: WorkflowCoordinator | undefined): void;
13591376
setAllowIdleWithFleet(value: boolean): void;
1377+
setClearDenials(clear: (() => void) | undefined): void;
13601378
getTasks(): Task[];
13611379
restoreTasks(tasks: Task[]): void;
13621380
getContextEstimate(): { tokens: number; isEstimate: boolean };

src/director.test.ts

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -712,6 +712,77 @@ describe("open-task termination guard", () => {
712712
).toBe(true);
713713
expect(actions.some((a) => a.type === "infer")).toBe(false);
714714
});
715+
716+
test("a new user turn after a canned decline infers instead of canned-replying", async () => {
717+
const director = createChatDirector("base", [], {});
718+
let cleared = 0;
719+
director.setClearDenials(() => {
720+
cleared++;
721+
});
722+
const declinedTurn = actionsArray(
723+
await director.decide(
724+
makeToolErrorEvent("c", declined),
725+
mockState,
726+
mockCapabilities,
727+
),
728+
);
729+
expect(
730+
declinedTurn.some(
731+
(a) =>
732+
a.type === "reply" &&
733+
"content" in a &&
734+
a.content === "Tool call rejected by operator.",
735+
),
736+
).toBe(true);
737+
expect(cleared).toBe(0);
738+
739+
const next = actionsArray(
740+
await director.decide(
741+
{
742+
type: "message.received",
743+
message: {
744+
role: "user",
745+
content: "just talk",
746+
flags: ["operator-originated"],
747+
},
748+
} as unknown as ReactorInboundEvent,
749+
mockState,
750+
mockCapabilities,
751+
),
752+
);
753+
expect(
754+
next.some(
755+
(a) =>
756+
a.type === "reply" &&
757+
"content" in a &&
758+
a.content === "Tool call rejected by operator.",
759+
),
760+
).toBe(false);
761+
expect(next.some((a) => a.type === "infer")).toBe(true);
762+
expect(cleared).toBe(1);
763+
});
764+
765+
test("mailbox inbound does not clear cached denies", async () => {
766+
const director = createChatDirector("base", [], {});
767+
let cleared = 0;
768+
director.setClearDenials(() => {
769+
cleared++;
770+
});
771+
await director.decide(
772+
makeToolErrorEvent("c", declined),
773+
mockState,
774+
mockCapabilities,
775+
);
776+
await director.decide(
777+
{
778+
type: "message.received",
779+
message: { role: "user", content: "worker report" },
780+
} as unknown as ReactorInboundEvent,
781+
mockState,
782+
mockCapabilities,
783+
);
784+
expect(cleared).toBe(0);
785+
});
715786
});
716787

717788
describe("chatDirector compaction", () => {

src/exec/runner.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -856,6 +856,7 @@ export async function runExec(config: Config): Promise<ExecResult> {
856856
computeAdvertised,
857857
inactivityTimeoutMs: config.inactivityTimeoutMs ?? 750_000,
858858
totalTimeoutMs: config.totalTimeoutMs,
859+
clearDenials: () => permissionGate.clearDenials(),
859860
getProvider: () => config,
860861
getWorkdir: () => workdir,
861862
getSessionId: () => sessionId,

src/permission/gate.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,9 @@ export interface PermissionGate {
436436
getApprovals: () => readonly Approval[];
437437
// Forget every remembered approval so a fresh session re-prompts from scratch.
438438
reset: () => void;
439+
// Drop cached operator/headless denies so the next user turn re-asks. Same-turn
440+
// retries still short-circuit until this runs.
441+
clearDenials: () => void;
439442
// The approvals granted only for this session (not persisted to any store).
440443
getSessionApprovals: () => readonly Approval[];
441444
// Drop one approval from the gate's live list and from the session set so the
@@ -633,9 +636,10 @@ export function createPermissionGate(
633636

634637
// Same-turn denial memory: stable fingerprints of headless and
635638
// operator-declined denies so a retry with a fresh tool_call.id returns the
636-
// identical cached reason instead of re-evaluating. Cleared by reset() and
637-
// by every state change that can flip a deny to an allow (new grants,
638-
// re-seeded approvals, auto/skip toggles, provider-identity switches).
639+
// identical cached reason instead of re-evaluating. Cleared on inbound user
640+
// turns (clearDenials), by reset(), and by every state change that can flip a
641+
// deny to an allow (new grants, re-seeded approvals, auto/skip toggles,
642+
// provider-identity switches). Timeouts and aborts are never recorded.
639643
const denialMemory = new DenialMemory();
640644

641645
// Non-blocking policy decision for one tool call: everything the gate owns —
@@ -1158,6 +1162,7 @@ export function createPermissionGate(
11581162
isReactorGated: () => reactorGated,
11591163
getApprovals: () => approvals,
11601164
reset,
1165+
clearDenials: () => denialMemory.clear(),
11611166
getSessionApprovals,
11621167
removeSessionApproval,
11631168
setSeededApprovals,

src/permission/permission.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2375,6 +2375,46 @@ describe("createPermissionGate", () => {
23752375
expect(asked).toBe(1);
23762376
});
23772377

2378+
// A denied URL is remembered only for same-turn retries. An inbound user
2379+
// turn (clearDenials) must forget it so the same URL can be re-asked.
2380+
test("clearDenials drops cached denies so a later turn re-asks the same URL", async () => {
2381+
let asked = 0;
2382+
const gate = createPermissionGate({
2383+
approvals: [],
2384+
interactive: true,
2385+
skipPermissions: false,
2386+
reactorGated: false,
2387+
requestApproval: async () => {
2388+
asked++;
2389+
return { allow: false };
2390+
},
2391+
});
2392+
const args = { url: "https://example.com/docs", format: "markdown" };
2393+
const first = await gate.evaluate({
2394+
id: "call_0",
2395+
name: "web_fetch",
2396+
arguments: args,
2397+
});
2398+
if (first.allowed) throw new Error("expected the first call declined");
2399+
expect(asked).toBe(1);
2400+
const sameTurn = await gate.evaluate({
2401+
id: "call_1",
2402+
name: "web_fetch",
2403+
arguments: args,
2404+
});
2405+
if (sameTurn.allowed)
2406+
throw new Error("expected the same-turn retry denied");
2407+
expect(asked).toBe(1);
2408+
gate.clearDenials();
2409+
const later = await gate.evaluate({
2410+
id: "call_2",
2411+
name: "web_fetch",
2412+
arguments: args,
2413+
});
2414+
if (later.allowed) throw new Error("expected the later-turn call declined");
2415+
expect(asked).toBe(2);
2416+
});
2417+
23782418
// CL-8002: a reactor-path timeout is not an operator decision, so
23792419
// resolveSuspended must not cache it — the retry re-asks the operator.
23802420
test("reactor-path timeout is not cached: retry re-asks", async () => {

src/permission/reactor-authorize.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ export function workerPermissionGate(gate: PermissionGate): PermissionGate {
107107
isReactorGated: () => true,
108108
getApprovals: () => gate.getApprovals(),
109109
reset: () => gate.reset(),
110+
clearDenials: () => gate.clearDenials(),
110111
getSessionApprovals: () => gate.getSessionApprovals(),
111112
removeSessionApproval: (target) => gate.removeSessionApproval(target),
112113
setSeededApprovals: (seeded) => gate.setSeededApprovals(seeded),

src/session/assemble-runtime.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,11 @@ export interface ChatAgentWiring {
477477
* the TUI seeds it (fleet lanes may appear mid-session).
478478
*/
479479
allowIdleWithFleet?: boolean;
480+
/**
481+
* Bound to PermissionGate.clearDenials so a later user turn re-asks a URL
482+
* that was declined this turn. Same-turn reactor retries still short-circuit.
483+
*/
484+
clearDenials?: () => void;
480485
getProvider: () => { providerName: string; model: string };
481486
/** Pre-created holder so the workflow controller can close over it first. */
482487
directorHolder?: { instance?: ChatDirector };
@@ -535,6 +540,8 @@ export function assembleChatAgent(wiring: ChatAgentWiring): AssembledChatAgent {
535540
allowIdleWithFleet: wiring.allowIdleWithFleet,
536541
},
537542
);
543+
if (wiring.clearDenials !== undefined)
544+
d.setClearDenials(wiring.clearDenials);
538545
directorHolder.instance = d;
539546
return d;
540547
},

src/tui/runner/session.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -631,6 +631,7 @@ export async function assembleTUISession(
631631
// instead of a host closure; no getLiveFleetCount: the publisher drives
632632
// the allowance through setAllowIdleWithFleet.)
633633
allowIdleWithFleet: true,
634+
clearDenials: () => permissionGate.clearDenials(),
634635
getProvider: () => state.config,
635636
directorHolder,
636637
getWorkdir: () => state.workdir,

tests/integration/harness.ts

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import { type } from "arktype";
3232

3333
import { createAgentWithLiveToolDispatch } from "../../src/agent/live-tool-dispatch.js";
3434
import { createChatDirector } from "../../src/agent/director.js";
35+
import { OPERATOR_ORIGINATED_FLAG } from "../../src/agent/message-provenance.js";
3536
import {
3637
readSourceCredentialMaterial,
3738
registerSourceCredential,
@@ -129,10 +130,17 @@ export async function openIntegrationSession(
129130
const chatDirectorDef = defineDirector({
130131
id: `${ID_PREFIX}/chat`,
131132
configSchema: type({}),
132-
factory: (_config, _env, agentCtx) =>
133-
createChatDirector(agentCtx.systemPrompt, [...agentCtx.toolDefinitions], {
134-
inactivityTimeoutMs: 750_000,
135-
}),
133+
factory: (_config, _env, agentCtx) => {
134+
const d = createChatDirector(
135+
agentCtx.systemPrompt,
136+
[...agentCtx.toolDefinitions],
137+
{
138+
inactivityTimeoutMs: 750_000,
139+
},
140+
);
141+
d.setClearDenials(() => opts.permissionGate.clearDenials());
142+
return d;
143+
},
136144
});
137145

138146
const toolsFactory = defineTool({
@@ -297,10 +305,24 @@ export async function runUntilDone(
297305

298306
const collectTask = collect;
299307
const sendResult = await Promise.all([
300-
session.agent.send(message).then((result) => {
301-
turnComplete = true;
302-
return result;
303-
}),
308+
session.agent
309+
.send({
310+
ref: { uid: 1, mailbox: "INBOX" },
311+
headers: {
312+
from: "user@local",
313+
to: ["agent@local"],
314+
date: new Date().toISOString(),
315+
messageId: `<${crypto.randomUUID()}@local>`,
316+
interchangeType: "conversation.message",
317+
},
318+
flags: [OPERATOR_ORIGINATED_FLAG],
319+
content: message,
320+
signatureStatus: "missing",
321+
})
322+
.then((result) => {
323+
turnComplete = true;
324+
return result;
325+
}),
304326
session.harness.run({ wallClockBudgetMs: Infinity }),
305327
collectTask,
306328
]).then(([result]) => result);

tests/integration/reactor-permission-multi-turn.test.ts

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,4 +173,74 @@ describe("integration — reactor permission + multi-turn", () => {
173173
}
174174
},
175175
);
176+
177+
test.serial(
178+
"denied run_shell does not canned-reply a later prose turn and re-asks the same command",
179+
async () => {
180+
let asked = 0;
181+
const session = await openIntegrationSession({
182+
permissionGate: createPermissionGate({
183+
approvals: [],
184+
interactive: true,
185+
skipPermissions: false,
186+
reactorGated: false,
187+
requestApproval: async () => {
188+
asked++;
189+
return { allow: false };
190+
},
191+
}),
192+
});
193+
194+
try {
195+
session.harness.scenario.replyOnce("anthropic", {
196+
toolCalls: [
197+
{
198+
name: "run_shell",
199+
args: { command: "curl https://example.com/docs" },
200+
},
201+
],
202+
});
203+
204+
const denied = await runUntilDone(
205+
session,
206+
"Fetch https://example.com/docs.",
207+
);
208+
expect(asked).toBeGreaterThan(0);
209+
const askedAfterDeny = asked;
210+
expect(denied.events.some((e) => e.type === "reactor.error")).toBe(
211+
false,
212+
);
213+
214+
session.harness.scenario.replyOnce("anthropic", {
215+
text: "I'll continue without fetching.",
216+
});
217+
const prose = await runUntilDone(
218+
session,
219+
"Do not fetch anything. Confirm in prose.",
220+
);
221+
expect(prose.reply).toContain("I'll continue without fetching.");
222+
expect(prose.reply).not.toContain("Tool call rejected by operator.");
223+
expect(asked).toBe(askedAfterDeny);
224+
225+
session.harness.scenario.replyOnce("anthropic", {
226+
toolCalls: [
227+
{
228+
name: "run_shell",
229+
args: { command: "curl https://example.com/docs" },
230+
},
231+
],
232+
});
233+
const retried = await runUntilDone(
234+
session,
235+
"Try fetching https://example.com/docs again.",
236+
);
237+
expect(asked).toBe(askedAfterDeny + 1);
238+
expect(retried.events.some((e) => e.type === "reactor.error")).toBe(
239+
false,
240+
);
241+
} finally {
242+
await closeIntegrationSession(session);
243+
}
244+
},
245+
);
176246
});

0 commit comments

Comments
 (0)