From 83f7c03a8c3fec64ee604a0f7aa6ce054d8bc48b Mon Sep 17 00:00:00 2001 From: suzuke Date: Mon, 27 Apr 2026 11:21:46 +0800 Subject: [PATCH] =?UTF-8?q?fix(broadcast):=20close=20cost-guard=20gate=20g?= =?UTF-8?q?ap=20(#24=20follow-up=20=E2=80=94=20broadcast=20surface)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #57 (#24 fix) gated `sendToInstance` and (transitively via wrapAsSend) `delegate_task` and `request_information`, but `broadcast` runs its own per-target send loop without funnelling through `sendToInstance` — so a limited instance could still be flooded via broadcast with task/query/ update kinds. ts-reviewer flagged this as Out-of-scope Observation 1 on PR #57. This PR closes the gap. Behaviour: per-target skip with structured warning, never short-circuits the whole broadcast. Other targets still receive normally. - broadcast loop: when `ctx.costGuard?.isLimited(target)` is true, push the target onto a new `costLimited` array with the same warning string format used by sendToInstance and continue the loop. - broadcast result gains a `cost_limited: [{ target, warning }]` field alongside `sent_to` and `failed`. Callers see exactly which targets were skipped and why. - BroadcastRequestKind already excludes `"report"` (broadcasts don't carry correlation_id), so no kind-bypass is needed here. - Session-routing edge case (Out-of-scope Observation 2) explicitly deferred per ts-lead — gate keys by `targetName` exactly like the rest of the broadcast loop. Tests cover the operator-mandated three states for broadcast: - (a) mixed → limited targets warned + skipped, others delivered - (b) all limited → every target warned, zero IPC sends - (c) no targets limited → identical to pre-fix, cost_limited empty `npx tsc --noEmit` clean. `npx vitest run` 63 files / 527 tests pass (+3 over post-#57 baseline; no regressions). Closes #24 issue intent (now covers all outbound-dispatch surfaces). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/outbound-handlers.ts | 15 ++- tests/outbound-cost-guard-precheck.test.ts | 133 +++++++++++++++++++++ 2 files changed, 147 insertions(+), 1 deletion(-) diff --git a/src/outbound-handlers.ts b/src/outbound-handlers.ts index 57d89d3c..a9117a45 100644 --- a/src/outbound-handlers.ts +++ b/src/outbound-handlers.ts @@ -396,7 +396,20 @@ const broadcast: Handler = (ctx, rawArgs, respond, meta) => { const sentTo: string[] = []; const failed: string[] = []; + // #24 follow-up: per-target cost-guard skip. Unlike sendToInstance (which + // short-circuits the whole call), broadcast must keep dispatching to other + // targets — only the over-limit ones drop out. BroadcastRequestKind already + // excludes "report", so no kind-bypass needed here. + const costLimited: { target: string; warning: string }[] = []; for (const targetName of targetNames) { + if (ctx.costGuard?.isLimited(targetName)) { + const limitUsd = (ctx.costGuard.getLimitCents() / 100).toFixed(2); + costLimited.push({ + target: targetName, + warning: `cost-guard: instance '${targetName}' has reached its daily cost limit ($${limitUsd}). Message not delivered — target is paused.`, + }); + continue; + } const targetIpc = ctx.instanceIpcClients.get(targetName) ?? ctx.instanceIpcClients.get(ctx.sessionRegistry.get(targetName) ?? ""); if (!targetIpc) { failed.push(targetName); continue; } @@ -420,7 +433,7 @@ const broadcast: Handler = (ctx, rawArgs, respond, meta) => { ctx.eventLog?.logActivity("message", senderLabel, summary, target); } ctx.queueMirrorMessage?.(`📢 ${senderLabel} → [${sentTo.join(", ")}]: ${message.slice(0, 500)}${message.length > 500 ? " […]" : ""}`); - respond({ sent_to: sentTo, failed, count: sentTo.length }); + respond({ sent_to: sentTo, failed, cost_limited: costLimited, count: sentTo.length }); }; // ── Teams ──────────────────────────────────────────────────────────────── diff --git a/tests/outbound-cost-guard-precheck.test.ts b/tests/outbound-cost-guard-precheck.test.ts index 05cda866..cd3d3694 100644 --- a/tests/outbound-cost-guard-precheck.test.ts +++ b/tests/outbound-cost-guard-precheck.test.ts @@ -182,3 +182,136 @@ describe("Feature #24 — cost-guard pre-check at outbound dispatch", () => { }); }); }); + +// ── #24 follow-up: broadcast handler gap ──────────────────────────────────── +// +// `broadcast` runs its own per-target send loop without funnelling through +// sendToInstance, so PR #57's gate did not cover it. ts-reviewer flagged the +// gap on PR #57 (Out-of-scope Observation 1). This block verifies the gate +// in three states, scoped per ts-lead's follow-up dispatch: +// (a) mixed — limited targets warn + skip, others receive +// (b) all limited — every target warns, zero IPC sends +// (c) all not limited — behaves identically to pre-fix +// +// BroadcastRequestKind already excludes "report", so no kind-bypass is needed. + +interface MockTargetSet { + ipcs: Record; + ctx: any; // OutboundContext; uses vitest mocks so cast for brevity +} + +function makeBroadcastCtx(targetNames: string[], costGuard: CostGuardMock | null): MockTargetSet { + const ipcs: Record = {}; + const instanceIpcClients = new Map void }>(); + // sender's own ipc — broadcast filters it out by name match + const senderIpc = mockIpc(); + ipcs.sender = senderIpc; + instanceIpcClients.set("sender", senderIpc.ipc); + for (const name of targetNames) { + const ch = mockIpc(); + ipcs[name] = ch; + instanceIpcClients.set(name, ch.ipc); + } + return { + ipcs, + ctx: { + fleetConfig: { instances: Object.fromEntries(["sender", ...targetNames].map(n => [n, { working_directory: `/tmp/${n}` }])) }, + adapter: null, + logger: { info: vi.fn(), warn: vi.fn(), debug: vi.fn(), error: vi.fn() }, + routing: { resolve: () => undefined }, + instanceIpcClients, + lifecycle: { daemons: new Map() }, + sessionRegistry: new Map(), + eventLog: null, + costGuard, + lastActivityMs: () => 0, + startInstance: vi.fn(), + connectIpcToInstance: vi.fn(), + saveFleetConfig: vi.fn(), + }, + }; +} + +describe("#24 follow-up — cost-guard pre-check at broadcast dispatch", () => { + // Per-target isLimited mock: only the names in `limitedSet` are over budget. + function partialCostGuard(limitedSet: Set, limitUsd = 5): CostGuardMock { + const limitCents = limitUsd * 100; + return { + isLimited: vi.fn((name: string) => limitedSet.has(name)), + getLimitCents: vi.fn(() => limitCents), + getDailyCostCents: vi.fn((name: string) => limitedSet.has(name) ? limitCents + 100 : 0), + }; + } + + it("(a) mixed targets — limited skipped with warning, others delivered", async () => { + const cg = partialCostGuard(new Set(["target-b"])); + const { ctx, ipcs } = makeBroadcastCtx(["target-a", "target-b", "target-c"], cg); + const handler = outboundHandlers.get("broadcast")!; + const respond = vi.fn(); + + await handler(ctx, { message: "hello fleet", targets: ["target-a", "target-b", "target-c"] }, respond, meta); + + expect(ipcs["target-a"].messages).toHaveLength(1); + expect(ipcs["target-b"].messages).toHaveLength(0); + expect(ipcs["target-c"].messages).toHaveLength(1); + + expect(respond).toHaveBeenCalledTimes(1); + const result = respond.mock.calls[0][0] as { + sent_to: string[]; + failed: string[]; + cost_limited: { target: string; warning: string }[]; + count: number; + }; + expect(result.sent_to).toEqual(["target-a", "target-c"]); + expect(result.cost_limited).toHaveLength(1); + expect(result.cost_limited[0].target).toBe("target-b"); + expect(result.cost_limited[0].warning).toContain("cost-guard"); + expect(result.cost_limited[0].warning).toContain("'target-b'"); + expect(result.cost_limited[0].warning).toContain("$5.00"); + expect(result.failed).toEqual([]); + expect(result.count).toBe(2); + }); + + it("(b) all targets limited — every target warns, zero IPC sends", async () => { + const cg = partialCostGuard(new Set(["target-a", "target-b", "target-c"])); + const { ctx, ipcs } = makeBroadcastCtx(["target-a", "target-b", "target-c"], cg); + const handler = outboundHandlers.get("broadcast")!; + const respond = vi.fn(); + + await handler(ctx, { message: "hello fleet", targets: ["target-a", "target-b", "target-c"] }, respond, meta); + + expect(ipcs["target-a"].messages).toHaveLength(0); + expect(ipcs["target-b"].messages).toHaveLength(0); + expect(ipcs["target-c"].messages).toHaveLength(0); + + const result = respond.mock.calls[0][0] as { + sent_to: string[]; + cost_limited: { target: string }[]; + count: number; + }; + expect(result.sent_to).toEqual([]); + expect(result.cost_limited.map(e => e.target).sort()).toEqual(["target-a", "target-b", "target-c"]); + expect(result.count).toBe(0); + }); + + it("(c) no targets limited — behaves identically to pre-fix (cost_limited empty)", async () => { + const cg = partialCostGuard(new Set()); + const { ctx, ipcs } = makeBroadcastCtx(["target-a", "target-b"], cg); + const handler = outboundHandlers.get("broadcast")!; + const respond = vi.fn(); + + await handler(ctx, { message: "hello fleet", targets: ["target-a", "target-b"] }, respond, meta); + + expect(ipcs["target-a"].messages).toHaveLength(1); + expect(ipcs["target-b"].messages).toHaveLength(1); + + const result = respond.mock.calls[0][0] as { + sent_to: string[]; + cost_limited: unknown[]; + count: number; + }; + expect(result.sent_to).toEqual(["target-a", "target-b"]); + expect(result.cost_limited).toEqual([]); + expect(result.count).toBe(2); + }); +});