Skip to content

Commit d4833e2

Browse files
nxmxbbdnxmxbbd
authored
fix(cron): refuse keyless implicit isolated cron delivery inherited from shared agent-main bucket (#91685)
Summary: - The PR changes isolated cron delivery resolution to reject keyless implicit delivery inherited from the shar ... targets into delivery context resolution, and cleans up direct cron sessions on unresolved delivery exits. - PR surface: Source +57, Tests +496. Total +553 across 8 files. - Reproducibility: yes. from source inspection: current resolver can inherit the shared agent-main last target ... ls or sends based on that resolved target; I did not run live Matrix reproduction in this read-only review. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(cron): clean up deleteAfterRun session when keyless cron delivery… - PR branch already contained follow-up commit before automerge: Merge remote-tracking branch 'upstream/main' into fix/91613-isolated-… - PR branch already contained follow-up commit before automerge: Merge upstream main into fix/91613-isolated-cron-delivery-identity - PR branch already contained follow-up commit before automerge: chore: retrigger PR CI after upstream base fix Validation: - ClawSweeper review passed for head f129375. - Required merge gates passed before the squash merge. Prepared head SHA: f129375 Review: #91685 (comment) Co-authored-by: nxmxbbd <[email protected]>
1 parent d1bb2d5 commit d4833e2

8 files changed

Lines changed: 561 additions & 8 deletions

‎src/cron/isolated-agent.delivery-awareness.test.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,15 @@ describe("runCronIsolatedAgentTurn cron delivery awareness", () => {
106106
});
107107
});
108108

109-
it("does not queue main-session awareness for implicit last-target delivery", async () => {
109+
it("refuses keyless implicit last-target delivery inherited from the shared main bucket, queuing no awareness", async () => {
110+
// #91613: a keyless implicit cron (sessionTarget "isolated", delivery.channel "last", no `to`)
111+
// would inherit the SHARED agent-main bucket's lastTo. In a multi-conversation agent that room
112+
// belongs to whichever conversation last wrote main — the wrong room — and the durable queue
113+
// replays it after a restart. It is now refused at the delivery dispatch !ok gate (errorKind
114+
// delivery-target) — the agent turn still runs, but delivery is refused, so nothing reaches the
115+
// wrong room or the durable queue, and no main-session awareness event is queued. (This is the
116+
// single-conversation behavior change called out for the maintainer: a keyless cron must now
117+
// pin delivery.to / delivery.channel, or run from a session that carries its own context.)
110118
await withTempCronHome(async (home) => {
111119
const storePath = await writeDefaultAgentSessionStoreEntries({
112120
"agent:main:main": {
@@ -131,8 +139,8 @@ describe("runCronIsolatedAgentTurn cron delivery awareness", () => {
131139
},
132140
});
133141

134-
expect(result.status).toBe("ok");
135-
expect(result.delivered).toBe(true);
142+
expect(result.status).toBe("error");
143+
expect(result.delivered).toBeFalsy();
136144
expect(peekSystemEvents("agent:main:main")).toStrictEqual([]);
137145
});
138146
});

‎src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1175,6 +1175,79 @@ describe("dispatchCronDelivery — double-announce guard", () => {
11751175
expect(retireSessionMcpRuntime).not.toHaveBeenCalled();
11761176
});
11771177

1178+
it("cleans up the direct cron session when delivery target resolution is refused (deleteAfterRun)", async () => {
1179+
// A keyless implicit cron whose inherited shared-bucket target is refused
1180+
// (resolvedDelivery.ok=false, issue #91613 fail-closed path) must still
1181+
// retire its session/transcript when deleteAfterRun is enabled — otherwise
1182+
// the one-shot session leaks.
1183+
const params = makeBaseParams({ synthesizedText: "refused report" });
1184+
params.resolvedDelivery = {
1185+
ok: false,
1186+
channel: "telegram",
1187+
to: undefined,
1188+
accountId: undefined,
1189+
threadId: undefined,
1190+
mode: "implicit",
1191+
error: new Error("refusing inherited shared-bucket delivery target"),
1192+
};
1193+
params.agentSessionKey = "agent:main:cron:test-job";
1194+
(params.job as { deleteAfterRun?: boolean }).deleteAfterRun = true;
1195+
1196+
const state = await dispatchCronDelivery(params);
1197+
1198+
expect(deliverOutboundPayloads).not.toHaveBeenCalled();
1199+
expectResultFields(state.result, {
1200+
status: "error",
1201+
errorKind: "delivery-target",
1202+
});
1203+
expect(callGateway).toHaveBeenCalledWith({
1204+
method: "sessions.delete",
1205+
params: {
1206+
key: "agent:main:cron:test-job",
1207+
deleteTranscript: true,
1208+
emitLifecycleHooks: false,
1209+
},
1210+
timeoutMs: 10_000,
1211+
});
1212+
});
1213+
1214+
it("cleans up the direct cron session when refused delivery is best-effort (deleteAfterRun)", async () => {
1215+
// Same fail-closed refusal, best-effort variant: dispatch returns status:ok
1216+
// (warn-logs instead of failing the run) but the deleteAfterRun session must
1217+
// still be retired.
1218+
const params = makeBaseParams({
1219+
synthesizedText: "refused report",
1220+
deliveryBestEffort: true,
1221+
});
1222+
params.resolvedDelivery = {
1223+
ok: false,
1224+
channel: "telegram",
1225+
to: undefined,
1226+
accountId: undefined,
1227+
threadId: undefined,
1228+
mode: "implicit",
1229+
error: new Error("refusing inherited shared-bucket delivery target"),
1230+
};
1231+
params.agentSessionKey = "agent:main:cron:test-job";
1232+
(params.job as { deleteAfterRun?: boolean }).deleteAfterRun = true;
1233+
1234+
const state = await dispatchCronDelivery(params);
1235+
1236+
expect(deliverOutboundPayloads).not.toHaveBeenCalled();
1237+
expectResultFields(state.result, {
1238+
status: "ok",
1239+
});
1240+
expect(callGateway).toHaveBeenCalledWith({
1241+
method: "sessions.delete",
1242+
params: {
1243+
key: "agent:main:cron:test-job",
1244+
deleteTranscript: true,
1245+
emitLifecycleHooks: false,
1246+
},
1247+
timeoutMs: 10_000,
1248+
});
1249+
});
1250+
11781251
it("text delivery fires exactly once (no double-deliver)", async () => {
11791252
vi.mocked(deliverOutboundPayloads).mockResolvedValue([{ ok: true } as never]);
11801253

‎src/cron/isolated-agent/delivery-dispatch.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1219,6 +1219,12 @@ export async function dispatchCronDelivery(
12191219

12201220
if (params.deliveryRequested && !params.skipHeartbeatDelivery && !sourceDeliverySatisfied) {
12211221
if (!params.resolvedDelivery.ok) {
1222+
// The target could not be resolved (e.g. a keyless implicit cron whose
1223+
// inherited shared-bucket target was refused). We never send here, so a
1224+
// deleteAfterRun cron must still retire its session/transcript before
1225+
// returning — otherwise the one-shot session leaks. Safe no-op for
1226+
// non-deleteAfterRun / non-cron sessions (see cleanupDirectCronSession).
1227+
await cleanupDirectCronSessionIfNeeded();
12221228
if (!params.deliveryBestEffort) {
12231229
return {
12241230
result: failDeliveryTarget(params.resolvedDelivery.error.message),

0 commit comments

Comments
 (0)