From eedc84de429cc511be0a57f67beb34805f502ab0 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 14:51:40 +0300 Subject: [PATCH 01/36] test(skills): pin approval routing for delegated sub-agent turns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sub-agent executes under a synthetic `sub-agent:runtime:` endpoint while its responses route to the inherited requester origin. The approval gate asserts endpoint/origin equality, so every approval-gated tool call inside a sub-agent run is denied by construction. RED: "routes the approval callback to the inherited requester origin" fails with ok:false — the gate rejects the pair the sub-agent runner mints. The two failure-mode cases are guards for the new path: a delegated turn whose principal does not own the origin, or that runs in another tenant, must still fail closed. --- .../approval-request-context.test.ts | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/packages/skills/src/platform-tools/approval-request-context.test.ts b/packages/skills/src/platform-tools/approval-request-context.test.ts index 44f2c2c38..9c1ab9ab1 100644 --- a/packages/skills/src/platform-tools/approval-request-context.test.ts +++ b/packages/skills/src/platform-tools/approval-request-context.test.ts @@ -172,4 +172,79 @@ describe("resolveApprovalRequestContext", () => { expect(result.ok).toBe(false); }); + + describe("delegated execution turns", () => { + const DELEGATED_ENDPOINT = { + channelType: "sub-agent", + channelInstanceId: "runtime", + conversationId: "run-1", + conversationKind: "direct" as const, + }; + + function makeDelegatedTurnScope(principalId: string): ResolvedTurnScope { + return { + conversation: { + tenantId: "default", + agentId: "resolved-agent", + partition: { + kind: "endpoint-conversation-principal", + endpoint: DELEGATED_ENDPOINT, + principalId, + }, + }, + principal: { principalId }, + endpoint: DELEGATED_ENDPOINT, + }; + } + + it("routes the approval callback to the inherited requester origin", () => { + const result = runWithContext( + makeContext({ + sessionKey: "default:agent:resolved-agent:human-user:sub-agent:runtime:run-1:peer:human-user", + turnScope: makeDelegatedTurnScope("principal-human-user"), + }), + resolveApprovalRequestContext, + ); + + expect(result).toMatchObject({ + ok: true, + value: { + resolvingPrincipalId: "principal-human-user", + callbackOwner: { + tenantId: "default", + userId: "principal-human-user", + channelType: "telegram", + channelKey: "chat-1", + threadId: "thread-1", + }, + }, + }); + }); + + it("fails closed when the delegated turn principal does not own the delivery origin", () => { + const result = runWithContext( + makeContext({ + turnScope: makeDelegatedTurnScope("principal-other-user"), + }), + resolveApprovalRequestContext, + ); + + expect(result.ok).toBe(false); + }); + + it("fails closed when the delegated turn executes outside the delivery origin tenant", () => { + const foreignTenantScope = makeDelegatedTurnScope("principal-human-user"); + const result = runWithContext( + makeContext({ + turnScope: { + ...foreignTenantScope, + conversation: { ...foreignTenantScope.conversation, tenantId: "other-tenant" }, + }, + }), + resolveApprovalRequestContext, + ); + + expect(result.ok).toBe(false); + }); + }); }); From 4c30deeaabc7db4e1860c6b168aa32797bc05462 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 14:53:20 +0300 Subject: [PATCH 02/36] fix(skills): let delegated runs resolve an approval callback owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The approval gate required `turnScope.endpoint` to equal `deliveryOrigin` on every field. A delegated run executes under a synthetic `sub-agent:runtime:` endpoint while its responses route to the requester origin inherited at spawn, so the pair never matched and every approval-gated tool call inside a sub-agent run was denied — exec, terminal, orchestrate, pipeline, gateway, memory_manage, channels_manage and admin_manage all share this gate. The two routes are authenticated independently and are intentionally different: `DeliveryOrigin` is documented as surviving sub-agent spawns, and spawn admission already rejects an announcement route that differs from the inherited requester origin (`rejectAnnouncementRoute`). The gate was the layer contradicting that design. Split the check by what each half proves. The authority half — frozen origin, tenant agreement, agent agreement, and principal ownership — is unchanged and still applies to every turn, so an approval can never be routed to another tenant or to a principal that does not own the turn. The endpoint half now applies only to endpoints that are actually deliverable; `isDelegatedExecutionEndpoint` in core is the single discriminator. The two halves also fail with distinct messages so the denial names which invariant broke instead of a generic origin mismatch. Threat notes: no capability is widened. A delegated turn still cannot approve into a foreign tenant, a foreign principal, or an unfrozen model-supplied origin, and the non-delegated path keeps byte-identical checks. Redirecting the announce route is blocked upstream at spawn admission, so the origin a delegated turn sees is the caller's own. --- .../core/src/domain/conversation-scope.ts | 18 +++++++++++++++ packages/core/src/domain/index.ts | 2 ++ packages/core/src/exports/domain.ts | 2 ++ .../approval-request-context.ts | 22 +++++++++++++++---- 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/packages/core/src/domain/conversation-scope.ts b/packages/core/src/domain/conversation-scope.ts index b5f31bf5e..71ad4d932 100644 --- a/packages/core/src/domain/conversation-scope.ts +++ b/packages/core/src/domain/conversation-scope.ts @@ -13,6 +13,24 @@ export const ChannelEndpointSchema = z.strictObject({ }); export type ChannelEndpoint = z.infer; +/** Channel type minted for a delegated run's synthetic execution endpoint. */ +export const DELEGATED_EXECUTION_CHANNEL_TYPE = "sub-agent"; + +/** + * True when the endpoint is a delegated execution scope rather than a + * deliverable channel endpoint. + * + * A delegated run executes under a synthetic conversation of its own while its + * responses, announcements, and approval prompts route to the requester origin + * it inherited at spawn. The two routes are authenticated independently and are + * intentionally different, so endpoint-vs-origin equality does not hold for + * them — callers that compare the pair must exempt delegated endpoints or they + * reject every delegated turn. + */ +export function isDelegatedExecutionEndpoint(endpoint: ChannelEndpoint): boolean { + return endpoint.channelType === DELEGATED_EXECUTION_CHANNEL_TYPE; +} + export const PrincipalScopeSchema = z.strictObject({ principalId: z.string().min(1), }); diff --git a/packages/core/src/domain/index.ts b/packages/core/src/domain/index.ts index bfad25a1d..8e6929fda 100644 --- a/packages/core/src/domain/index.ts +++ b/packages/core/src/domain/index.ts @@ -292,10 +292,12 @@ export { ConversationRefSchema, ConversationLocatorSchema, ConversationScopeError, + DELEGATED_EXECUTION_CHANNEL_TYPE, encodeConversationScope, createConversationRef, createConversationLocator, conversationScopeToSessionKey, + isDelegatedExecutionEndpoint, } from "./conversation-scope.js"; export type { ChannelEndpoint, diff --git a/packages/core/src/exports/domain.ts b/packages/core/src/exports/domain.ts index 9688375d0..4ab2ef3c9 100644 --- a/packages/core/src/exports/domain.ts +++ b/packages/core/src/exports/domain.ts @@ -128,9 +128,11 @@ export { ResolvedTurnScopeSchema, ConversationRefSchema, ConversationLocatorSchema, + DELEGATED_EXECUTION_CHANNEL_TYPE, createConversationRef, createConversationLocator, conversationScopeToSessionKey, + isDelegatedExecutionEndpoint, CanonicalLocaleSchema, AgentExecutionFinishReasonSchema, AgentExecutionAbortReasonSchema, diff --git a/packages/skills/src/platform-tools/approval-request-context.ts b/packages/skills/src/platform-tools/approval-request-context.ts index de7ff462e..958c4e251 100644 --- a/packages/skills/src/platform-tools/approval-request-context.ts +++ b/packages/skills/src/platform-tools/approval-request-context.ts @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 import { createConversationRef, + isDelegatedExecutionEndpoint, tryGetContext, type ConversationRef, type ApprovalCallbackOwner, @@ -73,11 +74,24 @@ export function resolveApprovalRequestContext(): Result Date: Wed, 12 Aug 2026 14:54:18 +0300 Subject: [PATCH 03/36] refactor(agent,daemon): route delegated-endpoint checks through one predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `"sub-agent"` channel type was written literally at the mint site and at each of the three readers, so the discriminator could drift silently — which is what the mint site's own PITFALL comment warns about. Point them all at `DELEGATED_EXECUTION_CHANNEL_TYPE` / `isDelegatedExecutionEndpoint` so the constant has one definition. No behavior change. --- packages/agent/src/executor/prompt-assembly-runtime.ts | 3 ++- packages/agent/src/spawn/sub-agent-runner.ts | 3 ++- .../daemon/src/api/session-handlers/session-read-authority.ts | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/agent/src/executor/prompt-assembly-runtime.ts b/packages/agent/src/executor/prompt-assembly-runtime.ts index 94d276233..eb119a8b7 100644 --- a/packages/agent/src/executor/prompt-assembly-runtime.ts +++ b/packages/agent/src/executor/prompt-assembly-runtime.ts @@ -14,6 +14,7 @@ import type { TypedEventBus, WorkspaceFileName } from "@comis/core"; import { formatSessionKey, createMemoryRecallScope, + isDelegatedExecutionEndpoint, scriptTokenFactor, tryGetContext, systemNowMs, @@ -351,7 +352,7 @@ export async function assembleExecutionPrompt(params: PromptAssemblyParams): Pro const isSubagentTurn = ( partition?.kind === "endpoint-conversation" || partition?.kind === "endpoint-conversation-principal" - ) && partition.endpoint.channelType === "sub-agent"; + ) && isDelegatedExecutionEndpoint(partition.endpoint); const memoryScope = turnScope === undefined ? err(new Error("RAG recall requires resolved turn authority")) : createMemoryRecallScope(turnScope, !isSubagentTurn); diff --git a/packages/agent/src/spawn/sub-agent-runner.ts b/packages/agent/src/spawn/sub-agent-runner.ts index 1ff6a2794..2eeb5eab2 100644 --- a/packages/agent/src/spawn/sub-agent-runner.ts +++ b/packages/agent/src/spawn/sub-agent-runner.ts @@ -20,6 +20,7 @@ import { createResolvedRequestContext, runWithContext, tryGetContext, + DELEGATED_EXECUTION_CHANNEL_TYPE, type SessionKey, type ConversationLocator, type ConversationRef, @@ -163,7 +164,7 @@ function createSubAgentConversation( partition: { kind: "endpoint-conversation-principal", endpoint: { - channelType: "sub-agent", + channelType: DELEGATED_EXECUTION_CHANNEL_TYPE, channelInstanceId: "runtime", conversationId: runId, conversationKind: "direct", diff --git a/packages/daemon/src/api/session-handlers/session-read-authority.ts b/packages/daemon/src/api/session-handlers/session-read-authority.ts index 2209f82fb..55b7da5bf 100644 --- a/packages/daemon/src/api/session-handlers/session-read-authority.ts +++ b/packages/daemon/src/api/session-handlers/session-read-authority.ts @@ -6,6 +6,7 @@ import { ConversationScopeSchema, createConversationRef, emitObservationalEventSafely, + isDelegatedExecutionEndpoint, systemNowMs, type ConversationRef, } from "@comis/core"; @@ -174,7 +175,7 @@ export function resolveModelSessionCaller( const isSubagent = ( partition.kind === "endpoint-conversation" || partition.kind === "endpoint-conversation-principal" - ) && partition.endpoint.channelType === "sub-agent"; + ) && isDelegatedExecutionEndpoint(partition.endpoint); return { tenantId: parsed.data.tenantId, agentId: callerAgentId, From c9e57dfba7f49598852717f9dc3c60325f9609ae Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 14:57:34 +0300 Subject: [PATCH 04/36] test(daemon): pin the verdict for a turn that died without finalizing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live incident: a turn errored after its response-locale repair failed and its delivery-queue transition never enqueued. It ran no tools and painted no terminal activity pill, so `comis explain` root-caused it as `recall_miss` — "the turn ran with no memory context" — while the real cause was visible only in daemon.log. RED: expects `execution_terminal_failure`, receives `recall_miss`. `endedInTerminalExecutionFailure` already counts an `error` end reason as a death, but `terminalFailureKind` discards it when no failed finalize was recorded, so all three terminal verdicts return null and the incidental zero-hit recall wins. --- .../api/obs-handlers/obs-explain-heuristics.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/daemon/src/api/obs-handlers/obs-explain-heuristics.test.ts b/packages/daemon/src/api/obs-handlers/obs-explain-heuristics.test.ts index 6750bc7ed..b9c2db80d 100644 --- a/packages/daemon/src/api/obs-handlers/obs-explain-heuristics.test.ts +++ b/packages/daemon/src/api/obs-handlers/obs-explain-heuristics.test.ts @@ -1444,6 +1444,19 @@ describe("obs-explain-heuristics", () => { expect(rootCause(makeSignals({ recall: allMissRecall }))).toBeNull(); }); + it("names the terminal failure when a turn died without ever finalizing an activity surface", () => { + // Live incident: the turn errored after its response-locale repair failed and the + // delivery-queue transition never enqueued, so it ran no tools and painted no + // terminal activity pill. endedInTerminalExecutionFailure already counts that + // shape as a death, but terminalFailureKind discarded it for want of a failed + // finalize — so every named terminal verdict returned null and the incidental + // zero-hit recall beside it became the verdict. + const r = rootCause(makeSignals({ endReason: "error", degraded: true, recall: allMissRecall })); + + expect(r?.code).toBe("execution_terminal_failure"); + expect(r?.code).not.toBe("recall_miss"); + }); + it("a degraded session where SOME recalls hit does not fire recall_miss", () => { const r = rootCause( makeSignals({ From 82a303c41dd36d4903b91e8d0e16b9c3c6ce49d7 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 15:01:19 +0300 Subject: [PATCH 05/36] fix(daemon): stop recall_miss claiming turns that died before finalizing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `endedInTerminalExecutionFailure` counts an `error` end reason as a death on its own, but `terminalFailureKind` then discarded that case unless a failed activity finalize had also been recorded. So a turn that died before or inside its own delivery path — no tools run, no pill painted — returned null from all three terminal verdicts, and the incidental zero-hit recall below them became the root cause. The module doc already states the rule the code missed: "on a session that died in the execution lifecycle the recall evidence is incidental and must never become the verdict". Two predicates in one file disagreed about what counts as a death; they now share one. - `terminalFailureKind` names every session its own gate accepted, so suppression can never leave a death unrooted. - `recallMissVerdict` defers to that same gate instead of its narrower finalize check. - `execution_terminal_failure` stops claiming a finalize that never happened, and points at the locale-repair and delivery-queue seams — where a turn that never finalized usually died. Registry order is unchanged, so the drive/orchestrate verdicts keep their specific-over-generic ranking over the generic terminal cause. Three tests carried the old shape and are retargeted, not weakened: recall_miss's carrier becomes a turn that DELIVERED while degraded, which is what a zero-hit recall actually produces — it degrades an answer, it does not kill a turn. The third asserted `toBeNull()` on a died-with- partial-hit session; that session now gets named rather than going unrooted, and the test keeps its original intent on the surviving turn. --- .../obs-explain-heuristics.test.ts | 27 ++++++++------ .../obs-explain-recall-verdict.ts | 37 ++++++++++++++----- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/packages/daemon/src/api/obs-handlers/obs-explain-heuristics.test.ts b/packages/daemon/src/api/obs-handlers/obs-explain-heuristics.test.ts index b9c2db80d..8f0ca4aa8 100644 --- a/packages/daemon/src/api/obs-handlers/obs-explain-heuristics.test.ts +++ b/packages/daemon/src/api/obs-handlers/obs-explain-heuristics.test.ts @@ -1147,8 +1147,10 @@ describe("obs-explain-heuristics", () => { it("a DEGRADED session whose recalls ALL missed (no tool/context cause) → recall_miss", () => { // Grounded in live Hebrew-language runs where recall silently returned - // nothing and comis explain root-caused nothing. - const r = rootCause(makeSignals({ endReason: "error", degraded: true, recall: allMissRecall })); + // nothing and comis explain root-caused nothing. The carrier is a turn that + // DELIVERED while degraded — a zero-hit recall degrades an answer, it does + // not kill a turn, so a session that died names its death instead. + const r = rootCause(makeSignals({ endReason: "success", degraded: true, recall: allMissRecall })); expect(r).not.toBeNull(); expect(r!.code).toBe("recall_miss"); expect(r!.detail).toContain("all 2 recall"); @@ -1240,7 +1242,7 @@ describe("obs-explain-heuristics", () => { it("recall_miss still fires when no model call was rejected", () => { // Regression guard: the new gate must not swallow the genuine recall_miss. - const r = rootCause(makeSignals({ endReason: "error", degraded: true, recall: allMissRecall })); + const r = rootCause(makeSignals({ endReason: "success", degraded: true, recall: allMissRecall })); expect(r!.code).toBe("recall_miss"); }); @@ -1458,14 +1460,17 @@ describe("obs-explain-heuristics", () => { }); it("a degraded session where SOME recalls hit does not fire recall_miss", () => { - const r = rootCause( - makeSignals({ - endReason: "error", - degraded: true, - recall: { recalls: 3, zeroHits: 1, lastLanes: 4, lastFinalCount: 5, rerankerAvailable: true }, - }), - ); - expect(r).toBeNull(); + const partialHitRecall = { + recalls: 3, zeroHits: 1, lastLanes: 4, lastFinalCount: 5, rerankerAvailable: true, + }; + // A turn that DIED names its death; the partial-hit recall is incidental either way. + expect( + rootCause(makeSignals({ endReason: "error", degraded: true, recall: partialHitRecall }))?.code, + ).toBe("execution_terminal_failure"); + // A turn that survived has nothing to name at all — a partial hit is not a miss. + expect( + rootCause(makeSignals({ endReason: "success", degraded: true, recall: partialHitRecall })), + ).toBeNull(); }); it("recall_miss yields to the tool-failure catch-all (mutually exclusive — failures present)", () => { diff --git a/packages/daemon/src/api/obs-handlers/obs-explain-recall-verdict.ts b/packages/daemon/src/api/obs-handlers/obs-explain-recall-verdict.ts index f4ac55acf..e8efb0a65 100644 --- a/packages/daemon/src/api/obs-handlers/obs-explain-recall-verdict.ts +++ b/packages/daemon/src/api/obs-handlers/obs-explain-recall-verdict.ts @@ -85,7 +85,6 @@ const terminalFailureKind = (s: IncidentSignals): string | undefined => { s.turnFinalized?.outcome === "failure" ? s.turnFinalized.errorKind : undefined; if (finalizedKind !== undefined) return finalizedKind; if ((s.summaryTopErrorKinds?.auth ?? 0) > 0 || s.turnFinalized?.errorKind === "auth") return "auth"; - if (s.turnFinalized?.outcome !== "failure") return undefined; return "unclassified"; }; @@ -135,15 +134,29 @@ export const executionTerminalFailureVerdict = (s: IncidentSignals): RecallVerdi if (kind === undefined || kind === "auth" || kind === "dependency") return null; const reason = s.turnFinalized?.reason; const reasonClause = reason !== undefined && reason.length > 0 ? ` (${reason})` : ""; + // A death that never reached the activity surface leaves no pill to quote, so say + // that instead of claiming a finalize that never happened — the absent finalize is + // itself the lead: the turn died before or inside its own delivery path. + const finalized = s.turnFinalized?.outcome === "failure"; + const detail = finalized + ? `the turn finalized as a terminal ${kind} failure${reasonClause} — the execution died ` + + "with no tool failure to attribute it to" + : `the turn ended with ${s.endReason ?? "a terminal failure"} and never finalized an activity ` + + "surface — the execution died with no tool failure to attribute it to"; return { code: "execution_terminal_failure", - detail: - `the turn finalized as a terminal ${kind} failure${reasonClause} — the execution died ` - + "with no tool failure to attribute it to", - suggestedNextSteps: [ - `obs.explain depth=full for the terminal execution records behind the ${kind} failure`, - "run comis system-health --since 1 to see whether the same errorKind recurs across sessions", - ], + detail, + suggestedNextSteps: finalized + ? [ + `obs.explain depth=full for the terminal execution records behind the ${kind} failure`, + "run comis system-health --since 1 to see whether the same errorKind recurs across sessions", + ] + : [ + "inspect the response-locale repair and delivery-queue transition for this traceId — " + + "a turn that never finalized usually died in its own delivery path", + "obs.explain depth=full for the terminal execution records", + "run comis system-health --since 1 to see whether the same end reason recurs across sessions", + ], }; }; @@ -153,8 +166,12 @@ export const recallMissVerdict = (s: IncidentSignals): RecallVerdict | null => { if (s.recall.recalls === 0 || s.recall.zeroHits < s.recall.recalls) return null; if (s.failures.length > 0) return null; if (s.degraded !== true) return null; - // A terminal failure pill is the cause; the zero-hit recall beside it is incidental. - if (s.turnFinalized?.outcome === "failure") return null; + // A session that died in the execution lifecycle is the cause; the zero-hit recall + // beside it is incidental. Defer to the SAME predicate the terminal verdicts key on, + // so no death can be claimed here and named there — or, as the live incident showed, + // claimed here and named nowhere. A death that reaches the finalize surface and one + // that never gets there are the same death. + if (endedInTerminalExecutionFailure(s)) return null; return { code: "recall_miss", detail: From 8283ac5181d29024319b8552e58a4fb85d844cd0 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 15:04:49 +0300 Subject: [PATCH 06/36] test(daemon,orchestrator): pin the quarantine resolution trail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Investigating a quarantined announcement, the WARN hint pointed at /dead-letters.jsonl; no such file existed anywhere on the host, so the announcement looked lost. It was not — the entry had been dropped correctly because the outward ledger proved the user was already told, and the file is unlinked as soon as the queue drains to zero. Two gaps made a correct resolution read as data loss: - the hint names the path without its lifecycle, so absent reads as never written rather than already resolved - the resolution logs at DEBUG, so at the default level there is no trace of it at all — only the WARN that opened the condition RED: ANNOUNCEMENT_QUARANTINE_HINT is not exported; the drain records no INFO line. --- packages/daemon/src/health-metrics.test.ts | 15 +++++++++++ .../announcement-dead-letter.test.ts | 26 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/packages/daemon/src/health-metrics.test.ts b/packages/daemon/src/health-metrics.test.ts index 9862ea30d..9bc8101cf 100644 --- a/packages/daemon/src/health-metrics.test.ts +++ b/packages/daemon/src/health-metrics.test.ts @@ -21,6 +21,7 @@ import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { createMockEventBus } from "../../../test/support/mock-event-bus.js"; +import { ANNOUNCEMENT_QUARANTINE_HINT } from "./health-metrics.js"; // --------------------------------------------------------------------------- // Helpers @@ -114,6 +115,20 @@ describe("promptTimeoutsLast5m sliding window counter", () => { // 2. deadLetterQueue.size() returns correct count // --------------------------------------------------------------------------- +describe("ANNOUNCEMENT_QUARANTINE_HINT", () => { + // The hint sent operators to /dead-letters.jsonl, but the file is + // unlinked as soon as the queue drains to zero. Live, reading the WARN after a + // correct resolution meant finding no file at that path and concluding the + // user's announcement had been lost — the opposite of what had happened. + it("states that an absent dead-letter file means the quarantine already resolved", () => { + expect(ANNOUNCEMENT_QUARANTINE_HINT).toMatch(/dead-letters\.jsonl/); + // The lifecycle is the load-bearing half: naming the path without it is what + // turned a resolved quarantine into a phantom data-loss report. + expect(ANNOUNCEMENT_QUARANTINE_HINT).toMatch(/removed|absent|no longer|drain/i); + expect(ANNOUNCEMENT_QUARANTINE_HINT).toMatch(/resolved|already/i); + }); +}); + describe("deadLetterQueueSize metric", () => { let tempDir: string; diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter.test.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter.test.ts index 13a39b224..c492404a0 100644 --- a/packages/orchestrator/src/cross-session/announcement-dead-letter.test.ts +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter.test.ts @@ -751,6 +751,32 @@ describe("AnnouncementDeadLetterQueue drain marks recovered keys", () => { expect(sendToChannel).toHaveBeenCalledOnce(); expect(dlq.size()).toBe(0); }); + + // The quarantine WARN is emitted at the non-zero transition and the dead-letter + // file is unlinked once the queue drains, so an operator who reads the WARN + // later finds no file and no resolution line at the default log level — the + // resolution was DEBUG-only. Live, that combination read as "the announcement + // was lost" when the entry had in fact been dropped correctly because the + // outward ledger proved the user was already told. + it("records the resolution at INFO so a drained quarantine is visible without debug logging", async () => { + const logger = createMockLogger(); + const entry = makeFullEntry({ + runId: "run-resolution-visible", + idempotencyKey: "default:u1:c1::run-resolution-visible", + }); + await writeFile(filePath, JSON.stringify(entry) + "\n", "utf-8"); + const dlq = createAnnouncementDeadLetterQueue({ + filePath, eventBus: createMockEventBus(), logger, retryIntervalMs: 0, + }); + + await dlq.drain(vi.fn().mockResolvedValue(true)); + + const resolutions = (logger.info as unknown as { mock: { calls: [Record, string][] } }) + .mock.calls.filter(([, msg]) => /dead-letter/i.test(msg)); + expect(resolutions).toHaveLength(1); + expect(resolutions[0]?.[0]).toMatchObject({ runId: "run-resolution-visible" }); + expect(dlq.size()).toBe(0); + }); }); describe("AnnouncementDeadLetterQueue parent decision reservations", () => { From 74c1919bf4b0c716bd115b2288339a304625134a Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 15:05:59 +0300 Subject: [PATCH 07/36] fix(daemon,orchestrator): make a resolved announcement quarantine legible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The quarantine WARN named /dead-letters.jsonl but not the file's lifecycle, and the resolution logged at DEBUG. At the default log level a correctly-resolved quarantine therefore left a standing WARN, no file at the path it named, and no trace of the outcome — which reads as a lost user announcement. It is the opposite: the file is unlinked when the queue drains, and the usual cause of a drain is the outward ledger proving the user was already told. - the hint now states that an absent file means resolved, and points at the resolution line instead of leaving the absence unexplained - the per-entry resolution moves DEBUG -> INFO; volume stays bounded by the entries that actually clear - the docs described the file as append-only and only ever created. It is a snapshot: rewritten atomically on change, removed at zero. That drift is what made the absence look like evidence of loss Remaining gap, deliberately not folded in: system-health still shows the quarantine with no paired resolution, because info-severity diagnostic rows are excluded from findings by design. Pairing them means changing findings semantics, which is its own change. --- docs/agents/resilience.mdx | 6 +++++- docs/operations/data-directory.mdx | 4 +++- packages/daemon/src/health-metrics.ts | 19 ++++++++++++++++++- .../cross-session/announcement-dead-letter.ts | 8 +++++++- 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/docs/agents/resilience.mdx b/docs/agents/resilience.mdx index c431c35a3..3ff65a334 100644 --- a/docs/agents/resilience.mdx +++ b/docs/agents/resilience.mdx @@ -191,7 +191,11 @@ persistent dead-letter queue backed by a JSONL file at automatically, delivering all queued messages - **What the operator sees:** `announcement:dead_lettered` events when entries are queued, `announcement:dead_letter_delivered` events when they are - successfully delivered on retry + successfully delivered on retry, and an INFO log line per entry that clears +- **File lifetime:** the JSONL is a snapshot of the queue, not an append-only + log — it is rewritten on every change and removed once the queue drains. An + absent file means nothing is pending, so a quarantine WARN with no file means + that quarantine already resolved The dead-letter queue ensures that transient failures do not cause permanent message loss. Even during extended provider outages, announcements are preserved diff --git a/docs/operations/data-directory.mdx b/docs/operations/data-directory.mdx index 95388ccc8..52334dea1 100644 --- a/docs/operations/data-directory.mdx +++ b/docs/operations/data-directory.mdx @@ -489,7 +489,9 @@ Because it carries the gateway token, the handle is written with mode `0o600` (o ### `dead-letters.jsonl` -Append-only JSONL of announcement-delivery failures that exhausted in-process retries (channel down, bot blocked, etc.). The delivery system replays these on provider recovery and expires entries after one hour. Only created when at least one announcement has been dead-lettered. +A JSONL **snapshot** of announcement-delivery failures that exhausted in-process retries (channel down, bot blocked, etc.), plus any parked parent-decision reservations. The delivery system replays these on provider recovery and expires entries after one hour. + +The file tracks the queue rather than accumulating: every change rewrites it atomically, and it is **removed** as soon as the queue drains to zero. It therefore exists only while something is actually pending — if the daemon logged a quarantine WARN and this file is absent, the quarantine has since **resolved**, not vanished. Look for the matching resolution line (`Committed dead-letter operation removed without replay`, or `Dead-letter entry delivered successfully`) rather than treating the announcement as lost. **Written by:** the announcement delivery subsystem. **Safe to delete?** Yes. Pending re-deliveries are dropped; entries auto-expire after one hour anyway. diff --git a/packages/daemon/src/health-metrics.ts b/packages/daemon/src/health-metrics.ts index 06f6cc460..39f0a3a03 100644 --- a/packages/daemon/src/health-metrics.ts +++ b/packages/daemon/src/health-metrics.ts @@ -16,6 +16,23 @@ import { } from "./wiring/subagent-stuck-sweep.js"; import { systemNowMs } from "@comis/core"; +/** + * Operator guidance for a standing announcement quarantine. + * + * Names the file's LIFECYCLE, not just its path: the dead-letter file exists + * only while the queue is non-empty and is unlinked the moment it drains, so an + * operator reading this WARN after the fact finds nothing at that path. Without + * the lifecycle, that absence reads as "the announcement was lost" when the + * usual cause is the opposite — the entry was dropped because the outward ledger + * proved the user had already been told. + */ +export const ANNOUNCEMENT_QUARANTINE_HINT = + "Quarantined background-task announcements are awaiting an operator decision; nothing drains " + + "them automatically because retrying risks a duplicate delivery. Inspect " + + "/dead-letters.jsonl and decide whether the user was already informed. That file is " + + "removed once the queue drains, so if it is absent the quarantine has already resolved — look " + + "for the matching dead-letter resolution line rather than treating the announcement as lost."; + export function wireHealthLogging(deps: { container: BootContext["container"]; clock: BootContext["clock"]; @@ -109,7 +126,7 @@ export function wireHealthLogging(deps: { if (deadLetterQueueSize > 0 && deadLetterQueueSize !== lastDeadLetterQueueSize) { daemonLogger.warn({ deadLetterQueueSize, - hint: "Quarantined background-task announcements are awaiting an operator decision; nothing drains them automatically because retrying risks a duplicate delivery. Inspect /dead-letters.jsonl and decide whether the user was already informed.", + hint: ANNOUNCEMENT_QUARANTINE_HINT, errorKind: "internal" as const, }, "Announcements quarantined awaiting an operator decision"); // Also emit it, so the count reaches the system-health view. The WARN diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts index de46a4de6..c2f2e7d1c 100644 --- a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts @@ -884,7 +884,13 @@ export function createAnnouncementDeadLetterQueue( tryCatch(() => onDelivered(idempotencyKey)); } emitDelivered(entry, entry.attemptCount); - logger?.debug( + // INFO, not DEBUG: this is the resolution half of a condition whose opening + // half is a WARN. The dead-letter file is unlinked once the queue drains, so + // at the default level a resolved quarantine otherwise leaves the WARN + // standing with no trace of its outcome and no file to inspect — which reads + // as a lost announcement. Once per resolved entry, so the volume is bounded + // by the entries that actually cleared. + logger?.info( { runId: entry.runId, attemptCount: entry.attemptCount, From acc4f106f46c2f34d00650c8a22480f05eaf5beb Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 15:55:20 +0300 Subject: [PATCH 08/36] test(agent): drive the block-message round trip with a quote-bearing error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing no-nesting test hand-escapes its fixture, so it exercises the peel but never the round trip the builder actually produces. Live on comis-moshe, web_fetch block messages nested four deep and displaced the real error entirely: has failed 12 total times with the same error: "…has failed 11 total times with the same error: \"…has failed 10 total times…\"" The trigger is an inner error CONTAINING QUOTES — which every real web_fetch/exec failure has, since the envelope's inner text is itself JSON. buildBlockReason embeds lastError raw between quotes while peelEnvelope parses it as a JSON-escaped string, so the parse throws and nothing peels. `.slice(0, 150)` also cuts mid-string, leaving an unterminated quote even on the first nesting. RED: 3 `has failed` clauses where the invariant allows 1. --- .../src/safety/tool-retry-breaker.test.ts | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/packages/agent/src/safety/tool-retry-breaker.test.ts b/packages/agent/src/safety/tool-retry-breaker.test.ts index 890a14293..2d88e1f50 100644 --- a/packages/agent/src/safety/tool-retry-breaker.test.ts +++ b/packages/agent/src/safety/tool-retry-breaker.test.ts @@ -1060,6 +1060,41 @@ describe("tool retry breaker", () => { expect(reason).toContain("upstream 503 from provider"); }); + it("holds the no-nesting invariant when its OWN output is fed back, round after round", () => { + // The case above hand-escapes its fixture, so it exercises the peel but + // never the round trip: the builder embeds lastError RAW while the peeler + // parses it as a JSON-escaped string, so a real envelope (every exec and + // web_fetch failure is one) fails to peel and each retry adds a layer. + // Live on comis-moshe: "failed 13 total times with the same error: + // \"…failed 12 total times…\"" — 552 bytes with the real error buried. + // The inner text is itself JSON, so it CONTAINS QUOTES — which is what + // breaks the round trip. Every real web_fetch/exec failure looks like this. + const envelope = JSON.stringify({ + content: [{ + type: "text", + text: JSON.stringify({ + url: "https://www.tamir-group.co.il/solution/subaru-forester-2020", + error: "URL redirected to a different location. Redirects are blocked for security.", + }, null, 2), + }], + details: {}, + }); + + let reason = buildBlockReason("web_fetch", 11, envelope, [], "dependency", true); + // Each later turn feeds the PREVIOUS block message back in as lastError — + // exactly what the live retry loop does. + for (const count of [12, 13, 14]) { + reason = buildBlockReason("web_fetch", count, reason, [], "dependency", true); + } + + expect((reason.match(/has failed/g) ?? []).length).toBe(1); + expect(reason).not.toMatch(/same error:.*has failed/s); + expect((reason.match(/appears to be unavailable/g) ?? []).length).toBe(1); + // The innermost real error survives every round — it is the whole point + // of the clause, and it is what got buried. + expect(reason).toContain("Redirects are blocked for security"); + }); + it("collapses a raw serialized envelope lastError to its inner text", () => { const envelope = JSON.stringify({ content: [{ type: "text", text: "[permission_denied] EPERM: operation not permitted" }], From ffa4cc90f68c6674b40a3ee51532ce5b4f467475 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 15:56:55 +0300 Subject: [PATCH 09/36] fix(agent): stop the breaker block message nesting inside itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildBlockMessage embedded lastError raw between quotes while peelEnvelope parsed it back as a JSON string literal. Real error text contains quotes (the envelope's inner text is itself JSON), so the parse threw, nothing peeled, and the next round quoted the whole prior block message. Live on comis-moshe web_fetch reached four levels, and because the clause is capped at 150 chars each round kept only the outer prose — by round 9 the message carried no real error at all, just recursive counters. Three changes, each closing one link: - the clause is now written with JSON.stringify, so quotes inside real error text are escaped and the round trip is defined - peelEnvelope scans to the closing UNESCAPED quote instead of the first `". ` run, which cut mid-literal on any error containing a JSON body, an HTML attribute, or a sentence - a structural backstop drops a clause that still reads as a block message after peeling, so the documented invariant holds even when a parse fails rather than depending on it succeeding The existing test hand-escaped its fixture and so exercised the peel but never the builder's own output; the new one feeds buildBlockReason its own result four rounds running, which is what the retry loop does. --- .../src/safety/tool-retry-breaker.test.ts | 8 ++- .../agent/src/safety/tool-retry-breaker.ts | 62 ++++++++++++++----- 2 files changed, 52 insertions(+), 18 deletions(-) diff --git a/packages/agent/src/safety/tool-retry-breaker.test.ts b/packages/agent/src/safety/tool-retry-breaker.test.ts index 2d88e1f50..62faab7cd 100644 --- a/packages/agent/src/safety/tool-retry-breaker.test.ts +++ b/packages/agent/src/safety/tool-retry-breaker.test.ts @@ -1091,8 +1091,12 @@ describe("tool retry breaker", () => { expect(reason).not.toMatch(/same error:.*has failed/s); expect((reason.match(/appears to be unavailable/g) ?? []).length).toBe(1); // The innermost real error survives every round — it is the whole point - // of the clause, and it is what got buried. - expect(reason).toContain("Redirects are blocked for security"); + // of the clause, and it is what got displaced. (The clause is capped at + // 150 chars, and a long URL eats most of that, so assert on the head of + // the real error rather than a phrase past the cut.) + expect(reason).toContain("URL redirected"); + // …and it is the ERROR that survives, never the prior block's prose. + expect(reason).not.toContain("DO NOT retry this tool. Instead:\\n"); }); it("collapses a raw serialized envelope lastError to its inner text", () => { diff --git a/packages/agent/src/safety/tool-retry-breaker.ts b/packages/agent/src/safety/tool-retry-breaker.ts index 098c45661..c03b454bc 100644 --- a/packages/agent/src/safety/tool-retry-breaker.ts +++ b/packages/agent/src/safety/tool-retry-breaker.ts @@ -158,6 +158,14 @@ export function extractErrorTag(errorText: string): string { * Handles both raw JSON envelopes and the breaker's own serialized block * message (which starts with prose then embeds the next envelope in quotes). */ +/** + * Peel depth bound. One nesting level is two layers (the block prose, then the + * tool-result envelope inside it), so this leaves headroom for a value that + * arrived already nested while still terminating on adversarial input. The loop + * breaks at the fixpoint, so the bound is a backstop, not the usual exit. + */ +const MAX_ENVELOPE_PEEL_DEPTH = 6; + function peelEnvelope(text: string): string { const external = unwrapExternalContent(text); if (external !== null) { @@ -183,22 +191,34 @@ function peelEnvelope(text: string): string { } // Shape B: breaker block message — starts with a prose prefix that - // embeds the next envelope in quotes: + // embeds the next envelope as a JSON string literal: // `Tool "exec" has failed 2 consecutive times with the same error: // "{\"content\":[...]}". This tool appears to be unavailable. ...` - // Peel the quoted JSON substring, if present. - const quotedStart = text.indexOf('same error: "'); - const contentStart = quotedStart === -1 ? -1 : quotedStart + 'same error: "'.length; - const quotedEnd = contentStart === -1 ? -1 : text.indexOf('". ', contentStart); - if (quotedEnd !== -1) { - // The captured group is JSON with escaped quotes. Unescape by parsing - // the outer quoted string as JSON (wrap in extra quotes so JSON.parse - // handles the escapes). - try { - const inner = JSON.parse(`"${text.slice(contentStart, quotedEnd)}"`) as string; - return inner; - } catch { - // Fall through — return prefix + match unchanged. + // Scan to the closing UNESCAPED quote rather than the first `". ` run: + // the embedded error is real error text, so it routinely contains both + // (a JSON body, an HTML attribute, a sentence). Terminating on the first + // `". ` cut mid-literal, the parse below then threw, nothing peeled, and + // the next round embedded this whole message — the recursive nesting. + const marker = 'same error: "'; + const markerAt = text.indexOf(marker); + if (markerAt !== -1) { + const contentStart = markerAt + marker.length; + let cursor = contentStart; + while (cursor < text.length) { + const ch = text[cursor]; + if (ch === "\\") { + cursor += 2; + continue; + } + if (ch === '"') break; + cursor += 1; + } + if (cursor < text.length) { + try { + return JSON.parse(`"${text.slice(contentStart, cursor)}"`) as string; + } catch { + // Fall through — return the text unchanged. + } } } @@ -384,14 +404,24 @@ export function buildBlockReason( // never embeds a prior `appears to be unavailable` clause. let peeledError = lastError; if (peeledError !== undefined) { - for (let depth = 0; depth < 2; depth++) { + for (let depth = 0; depth < MAX_ENVELOPE_PEEL_DEPTH; depth++) { const peeled = peelEnvelope(peeledError); if (peeled === peeledError) break; peeledError = peeled; } + // Structural backstop for the INVARIANT. Peeling is a parse and a parse can + // fail; when it does, embedding the value nests the whole prior block + // message instead of the error it was meant to quote — and because the + // clause is truncated, four rounds of that displaced the real error + // completely, leaving only recursive prose. A clause we cannot collapse is + // worth less than no clause: the count and the tool name still carry. + if (isBreakerBlockMessage(peeledError)) peeledError = undefined; } const errorClause = peeledError - ? ` with the same error: "${peeledError.slice(0, 150)}"` + // Embed as a JSON string literal so the quotes inside real error text are + // escaped — peelEnvelope parses this back, and the raw embed it replaces + // is what made that parse throw. + ? ` with the same error: ${JSON.stringify(peeledError.slice(0, 150))}` : ""; const header = errorTag && isParameterValidationTag(errorTag) ? `Tool "${toolName}" failed parameter validation ${count} times (same args). Fix the arguments before retrying.` From 036ac1409d1c6fc9fcda63b085ec306390ad79d3 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 15:58:24 +0300 Subject: [PATCH 10/36] test(agent): pin one coherent re-spawn directive for unreachable tools RequiredToolsUnreachableError concatenates per-tool hints, and each hint recommends a group computed for its own tool alone. Live on comis-moshe a spawn needing web_search + web_fetch was told to re-spawn with ['cron-minimal'] AND with ['full'] in one message; web_search is in 'cron-minimal', web_fetch is in no profile, so only 'full' satisfies the pair. The caller followed the guidance and failed identically twice (12:33:53, 12:34:18). RED: 2 directives where 1 is correct; and a denylisted requirement still emits a re-spawn directive that cannot possibly work. --- .../agent/src/spawn/sub-agent-runner.test.ts | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/packages/agent/src/spawn/sub-agent-runner.test.ts b/packages/agent/src/spawn/sub-agent-runner.test.ts index f96d75e0d..b9156a1c3 100644 --- a/packages/agent/src/spawn/sub-agent-runner.test.ts +++ b/packages/agent/src/spawn/sub-agent-runner.test.ts @@ -5677,6 +5677,67 @@ describe("spawn required_tools gate", () => { expect(runner.listRuns(60)).toHaveLength(0); }); + it("gives ONE re-spawn group that reaches every unreachable tool, not one per tool", () => { + // Live on comis-moshe: a spawn needing web_search + web_fetch was told + // "Re-spawn with tool_groups:['cron-minimal']" AND "Re-spawn with + // tool_groups:['full']" in the same message. web_search is in + // 'cron-minimal', web_fetch is in no profile at all, so 'cron-minimal' + // cannot satisfy the pair — the caller obeyed and failed identically twice. + const runner = createSubAgentRunner(deps); + + let caughtErr: unknown; + try { + runner.spawn({ + task: "test", + agentId: "default", + toolGroups: ["minimal"], + requiredTools: ["web_search", "web_fetch"], + reachableToolNames: new Set(["read", "write"]), + }); + } catch (e) { + caughtErr = e; + } + + expect(caughtErr).toBeInstanceOf(RequiredToolsUnreachableError); + const message = (caughtErr as RequiredToolsUnreachableError).message; + + // Exactly one actionable re-spawn directive, and it must name a group that + // reaches BOTH tools. Only 'full' does. + const directives = message.match(/Re-spawn with tool_groups:\[[^\]]*\]/g) ?? []; + expect(directives).toHaveLength(1); + expect(directives[0]).toContain("full"); + expect(directives[0]).not.toContain("cron-minimal"); + // Both tool names are still named, so the caller knows what drove it. + expect(message).toContain("web_search"); + expect(message).toContain("web_fetch"); + }); + + it("says no re-spawn can help when a required tool is denied to all sub-agents", () => { + // A denylisted tool is unfixable by any group. Pairing it with a merely + // out-of-profile tool must not emit a re-spawn directive that would fail + // again on the denied one. + const runner = createSubAgentRunner(deps); + + let caughtErr: unknown; + try { + runner.spawn({ + task: "test", + agentId: "default", + toolGroups: ["minimal"], + requiredTools: ["gateway", "web_search"], + reachableToolNames: new Set(["read", "write"]), + }); + } catch (e) { + caughtErr = e; + } + + expect(caughtErr).toBeInstanceOf(RequiredToolsUnreachableError); + const message = (caughtErr as RequiredToolsUnreachableError).message; + expect(message).toMatch(/denied to ALL sub-agents/i); + // No re-spawn directive can satisfy a denylisted requirement. + expect(message.match(/Re-spawn with tool_groups:\[[^\]]*\]/g) ?? []).toHaveLength(0); + }); + it("spawn with requiredTools=['gateway'] throws RequiredToolsUnreachableError with denylist reason", () => { // 'gateway' is in SUB_AGENT_TOOL_DENYLIST — denied to ALL sub-agents. const runner = createSubAgentRunner(deps); From f563f36412d46ba41363a40f29507370c1cf0bee Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 16:00:18 +0300 Subject: [PATCH 11/36] fix(core): emit one coherent re-spawn directive for unreachable tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rejection joined per-tool hints, each carrying a group computed for its own tool in isolation. A caller passes ONE group list, so a spawn needing web_search + web_fetch received two contradictory directives — ['cron-minimal'] and ['full'] — and obeying either failed again on the other tool. The error is the only place that sees the whole set, so the directive is derived there now. `groupsReachingAll` intersects the profiles across every unreachable tool and the message names that single group, falling back to 'full' when no profile covers them all (web_fetch is in none). A denylisted requirement is unfixable by any group, so when one is present no re-spawn directive is emitted at all — it now says to drop the tool or keep that step in the parent. Telling a caller to retry a spawn that cannot succeed is what produced the identical back-to-back failures. Per-tool `hint` values are unchanged on `unreachableTools`, so structured consumers keep the per-tool detail; only the rendered message changed. --- .../src/domain/sub-agent-tool-denylist.ts | 79 ++++++++++++++++++- 1 file changed, 75 insertions(+), 4 deletions(-) diff --git a/packages/core/src/domain/sub-agent-tool-denylist.ts b/packages/core/src/domain/sub-agent-tool-denylist.ts index 41792f005..1f0446eea 100644 --- a/packages/core/src/domain/sub-agent-tool-denylist.ts +++ b/packages/core/src/domain/sub-agent-tool-denylist.ts @@ -204,6 +204,26 @@ export function toolReachableGroups(toolName: string): string[] { return result; } +/** + * Profile names that reach EVERY named tool — the groups a single re-spawn + * could actually use. + * + * A per-tool answer is not composable: recommending the groups for each tool + * separately produces a set of directives that contradict each other whenever + * the tools do not share a profile, and a caller can only pass one group list. + * `'full'` is not a profile key (it is the "no ceiling" sentinel), so it is + * never returned here; callers fall back to it when this returns empty. + * + * @param toolNames - Tools that must all be reachable from the same group + * @returns Profile names containing every tool (empty if no single profile does) + */ +export function groupsReachingAll(toolNames: readonly string[]): string[] { + if (toolNames.length === 0) return []; + return Object.entries(SUB_AGENT_TOOL_PROFILES) + .filter(([, tools]) => toolNames.every((name) => tools.includes(name))) + .map(([profileName]) => profileName); +} + /** * Classification for a single tool that is unreachable by the sub-agent's * profile/group ceiling at spawn time. @@ -225,15 +245,66 @@ export interface UnreachableToolEntry { * (@allow-throw boundary in sub-agent-runner.ts). rpc-dispatch.ts converts * this to a JSON-RPC error response. */ +/** + * Render the rejection as ONE coherent instruction. + * + * Per-tool hints are computed per tool and cannot see each other, so joining + * them emits a separate `Re-spawn with tool_groups:[…]` directive for every + * tool. A caller passes one group list, so two directives are a contradiction, + * and obeying either fails again on the other tool. This is the only place that + * sees the whole set, so the combined directive is derived here. + * + * A denylisted requirement is unfixable by any group, so when one is present no + * re-spawn directive is emitted at all — telling a caller to retry a spawn that + * cannot succeed is worse than telling it to change the request. + */ +function buildUnreachableToolsMessage(tools: readonly UnreachableToolEntry[]): string { + const named = tools.map((t) => t.toolName).join(", "); + const denied = tools.filter((t) => t.reason === "denylist"); + const outside = tools.filter((t) => t.reason === "outside_profile"); + const parts = [`Required tools unreachable: ${named}.`]; + + if (denied.length > 0) { + parts.push(denied.map((t) => t.hint).join(" ")); + if (outside.length > 0) { + parts.push( + `No re-spawn can satisfy this request while ${denied.map((t) => `'${t.toolName}'`).join(", ")} ` + + `${denied.length === 1 ? "is" : "are"} required — drop ` + + `${denied.length === 1 ? "it" : "them"} or perform that step in the parent.`, + ); + } + return parts.join(" "); + } + + const outsideNames = outside.map((t) => t.toolName); + const shared = groupsReachingAll(outsideNames); + // 'full' is the only ceiling that reaches a tool no profile lists (MCP tools, + // and generic tools absent from every profile), so it is the fallback. + const suggestion = shared.length > 0 ? shared.join("' | '") : "full"; + const validGroups = [...Object.keys(SUB_AGENT_TOOL_PROFILES), "full"].join("' | '"); + parts.push( + outsideNames.length === 1 + ? `Tool '${outsideNames[0]}' is outside this sub-agent's profile.` + : `Tools ${outsideNames.map((n) => `'${n}'`).join(", ")} are outside this sub-agent's profile; ` + + `one re-spawn must reach all of them.`, + ); + parts.push(`Re-spawn with tool_groups:['${suggestion}'].`); + parts.push(`Valid groups are '${validGroups}' — any other value is ignored.`); + if (outsideNames.some((n) => n.startsWith("mcp__"))) { + parts.push( + "MCP tool names are resolved from connected servers at runtime, so no narrow profile " + + "lists them and 'full' is the only group that reaches them.", + ); + } + return parts.join(" "); +} + export class RequiredToolsUnreachableError extends Error { readonly kind = "required_tools_unreachable" as const; readonly unreachableTools: UnreachableToolEntry[]; constructor(tools: UnreachableToolEntry[]) { - super( - `Required tools unreachable: ${tools.map((t) => t.toolName).join(", ")}. ` + - tools.map((t) => t.hint).join(" "), - ); + super(buildUnreachableToolsMessage(tools)); this.unreachableTools = tools; this.name = "RequiredToolsUnreachableError"; } From eba3462c0f56c631ef0c8d1a83bcbf1a8738076f Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 16:04:14 +0300 Subject: [PATCH 12/36] test(memory): pin hard-over-soft precedence in the session rollup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `aggregateSessionsInWindow` makes `degraded` sticky but resolves the cause last-degraded-wins, so a soft cause overwrites a hard one. Live on comis-moshe: an 11:22 turn died (endReason "error") and a 12:36 turn in the same chat finished completed_with_tool_errors. system-health computes hardDegraded = degradedCount - deliveredWithToolErrorsCount so the session was then counted as "the user still got a reply" and the report moved from "1 hard-degraded, 50%" to "0 hard-degraded, 0%" with nothing fixed. The failure was downgraded, not merely hidden — and the traceId an operator needs went with it. RED: endReason is 'completed_with_tool_errors', expected 'error'. The second case pins the precedence as hard-over-soft rather than first-wins, so a session that ends up dying still reports the death. --- .../observability-store.test.ts | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/packages/memory/src/observability-store/observability-store.test.ts b/packages/memory/src/observability-store/observability-store.test.ts index e594e0ea1..f0ca7edd8 100644 --- a/packages/memory/src/observability-store/observability-store.test.ts +++ b/packages/memory/src/observability-store/observability-store.test.ts @@ -267,6 +267,70 @@ describe("ObservabilityStore — aggregateSessionsInWindow (A1)", () => { expect(r.lastTs).toBe(3_000); }); + it("a later delivered-with-tool-errors turn does not reclassify an earlier hard failure", () => { + // Live on comis-moshe: an 11:22 turn died (endReason "error"), and a 12:36 + // turn in the SAME chat finished completed_with_tool_errors. `degraded` is + // sticky, but the cause is last-degraded-wins, so the soft cause overwrote + // the hard one. system-health computes + // hardDegraded = degradedCount - deliveredWithToolErrorsCount + // so the session then counted as "the user still got a reply" and the + // report went from "1 hard-degraded, 50%" to "0 hard-degraded, 0%" with + // nothing fixed — the hard failure was downgraded, not merely hidden. + store.insertDiagnostic({ + timestamp: 1_000, + category: "session_summary", + severity: "warning", + sessionKey: "s1", + traceId: "trace-hard-failure", + message: "session:summary", + details: summaryDetails({ degraded: true, endReason: "error" }), + }); + store.insertDiagnostic({ + timestamp: 2_000, + category: "session_summary", + severity: "warning", + sessionKey: "s1", + traceId: "trace-soft-degradation", + message: "session:summary", + details: summaryDetails({ degraded: true, endReason: "completed_with_tool_errors" }), + }); + + const rollup = store.aggregateSessionsInWindow(0)[0]!; + + expect(rollup.degraded).toBe(true); + // The hard cause survives, and with it the trace an operator must open. + expect(rollup.endReason).toBe("error"); + expect(rollup).toHaveProperty("traceId", "trace-hard-failure"); + }); + + it("a later hard failure still replaces an earlier soft degradation", () => { + // The precedence is hard-over-soft, not first-wins: a session that ends up + // dying must report the death even if it merely limped earlier. + store.insertDiagnostic({ + timestamp: 1_000, + category: "session_summary", + severity: "warning", + sessionKey: "s1", + traceId: "trace-soft-degradation", + message: "session:summary", + details: summaryDetails({ degraded: true, endReason: "completed_with_tool_errors" }), + }); + store.insertDiagnostic({ + timestamp: 2_000, + category: "session_summary", + severity: "warning", + sessionKey: "s1", + traceId: "trace-hard-failure", + message: "session:summary", + details: summaryDetails({ degraded: true, endReason: "error" }), + }); + + const rollup = store.aggregateSessionsInWindow(0)[0]!; + + expect(rollup.endReason).toBe("error"); + expect(rollup).toHaveProperty("traceId", "trace-hard-failure"); + }); + it("retains the trace id of the sticky degraded execution after a clean continuation", () => { store.insertDiagnostic({ timestamp: 1_000, From 7275995e11afc414f82cf6bec5f974313bdcb05e Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 16:05:40 +0300 Subject: [PATCH 13/36] fix(memory): stop a soft degradation downgrading a hard failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-session rollup made `degraded` sticky but resolved the cause last-degraded-wins. A later `completed_with_tool_errors` turn in the same conversation therefore overwrote an earlier `error`, and since the system detector computes hardDegraded as degradedCount minus the delivered-with-tool-errors bucket, the death was subtracted out of the daemon-wide view entirely — taking its traceId with it. Live on comis-moshe the report moved from "1 hard-degraded, 50%" to "0 hard-degraded, 0%" across a window in which nothing had been fixed, which is the worst possible reading for a health surface to offer. A soft cause no longer overwrites a hard one already recorded, mirroring the `background_pending` rule beside it: a cause describing a lesser state does not mask a worse one. Precedence is hard-over-soft, not first-wins, so a session that ends up dying still reports the death. DELIVERED_WITH_TOOL_ERRORS_CAUSE is now exported and shared: the rule that subtracts this bucket and the rule that must not downgrade into it are one invariant, and they must name one string. --- .../observability-queries.ts | 17 ++++++++++++++++- .../observability-store/system-window-rollup.ts | 11 ++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/packages/memory/src/observability-store/observability-queries.ts b/packages/memory/src/observability-store/observability-queries.ts index 3e79379a7..0e3e3b5dc 100644 --- a/packages/memory/src/observability-store/observability-queries.ts +++ b/packages/memory/src/observability-store/observability-queries.ts @@ -7,6 +7,7 @@ import type Database from "better-sqlite3"; import { z } from "zod"; import { createRowMapper } from "../row-mapper.js"; +import { DELIVERED_WITH_TOOL_ERRORS_CAUSE } from "./system-window-rollup.js"; import { diagnosticMapper, channelSnapshotMapper, @@ -336,7 +337,21 @@ export function bindQueries(db: Database.Database): ObservabilityQueries { typeof d.endReason === "string" && d.endReason.length > 0 ? d.endReason : "unknown"; if (rowDegraded) { const isPendingContinuation = rowEndReason === "background_pending"; - if (!isPendingContinuation || !acc.degraded) { + // A SOFT cause must never overwrite a HARD one. `degraded` is sticky, + // but the cause was last-degraded-wins, so a later + // `completed_with_tool_errors` turn in the same conversation + // reclassified an earlier death as "the user still got a reply" — and + // the system detector subtracts exactly that bucket out of its hard + // count, so the failure disappeared from the daemon-wide view along + // with the traceId needed to open it. Same shape as the + // `background_pending` rule beside it: a cause that describes a lesser + // state does not get to mask a worse one already recorded. + const wouldDowngrade = + rowEndReason === DELIVERED_WITH_TOOL_ERRORS_CAUSE + && acc.degraded + && acc.endReason !== DELIVERED_WITH_TOOL_ERRORS_CAUSE + && acc.endReason !== "background_pending"; + if ((!isPendingContinuation || !acc.degraded) && !wouldDowngrade) { acc.endReason = rowEndReason; if (r.trace_id.length > 0) acc.traceId = r.trace_id; } diff --git a/packages/memory/src/observability-store/system-window-rollup.ts b/packages/memory/src/observability-store/system-window-rollup.ts index de0523e56..03340275d 100644 --- a/packages/memory/src/observability-store/system-window-rollup.ts +++ b/packages/memory/src/observability-store/system-window-rollup.ts @@ -59,7 +59,16 @@ const SYSTEM_DEGRADED_BY_CAUSE_CAP = 10; * hard degraded rate. Mirrors the `completed_with_tool_errors` literal in * `END_REASON_MAP` (executor-post-execution.ts); a rename there must update this. */ -const DELIVERED_WITH_TOOL_ERRORS_CAUSE = "completed_with_tool_errors"; +/** + * The one SOFT degradation cause: the turn errored somewhere but still + * delivered a reply. {@link reduceSystemWindow} counts it separately so the + * system detector can subtract it out of the hard-degraded rate. + * + * Exported because the per-session aggregator must not let this cause overwrite + * a hard one it already recorded — a downgrade there deletes the session from + * the hard count here. The two rules are one invariant and must name one string. + */ +export const DELIVERED_WITH_TOOL_ERRORS_CAUSE = "completed_with_tool_errors"; /** The stable bucket for a degraded row whose endReason is missing/blank. */ const UNKNOWN_CAUSE = "unknown"; From 4feb7bb38de1a8f7a0790dc3410d8c55c534a458 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 16:21:51 +0300 Subject: [PATCH 14/36] test(agent): pin a delivered sub-agent answer as delivered, not failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sub-agent searched used-car listings, hit bot protection on 3 of many web_fetch calls across 21 turns, and returned a complete answer naming the sources it could not verify. Its finish reason was `completed_with_tool_errors` — not "stop"/"end_turn" — so the outcome resolved to `model_halted`, and the parent announced Status: Failed Result: Error: Two independent defects, one RED pair each: - the classification calls a delivered run a halt - the failed branch puts the child's own response in the `error` slot, so a deliverable is relabelled an error and invites the user to discard it `modelStoppedCleanly` is renamed `modelDelivered`: the field decides whether the model got to deliver, and a name asserting "cleanly" is what made `completed_with_tool_errors` look like it did not belong. The contract gate stays independent — delivering prose is still not writing the files a child promised. --- .../sub-agent-announcement-content.test.ts | 38 ++++++++++++++ .../agent/src/spawn/sub-agent-outcome.test.ts | 51 ++++++++++++++++--- 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/packages/agent/src/spawn/sub-agent-announcement-content.test.ts b/packages/agent/src/spawn/sub-agent-announcement-content.test.ts index c6e55c4f1..10dd36e51 100644 --- a/packages/agent/src/spawn/sub-agent-announcement-content.test.ts +++ b/packages/agent/src/spawn/sub-agent-announcement-content.test.ts @@ -74,3 +74,41 @@ describe("sub-agent announcement content", () => { expect(disclosure.text?.match(/governor limit of 6/gu)).toHaveLength(1); }); }); + +describe("buildAnnouncementMessage — a real response is never rendered as an error", () => { + // Second half of the same live incident. Even once classification is right, + // the failed branch put the child's own response into the `error` slot, so + // the reader saw `Result: Error: `. A deliverable + // relabelled as an error invites the user to discard good work. + it("shows the response as the result when a degraded run still produced one", () => { + const message = buildAnnouncementMessage({ + task: "find listings", + status: "failed", + response: "Found 3 active listings; yad2 was behind a bot challenge.", + runtimeMs: 1000, + tokensUsed: 10, + cost: 0.1, + finishReason: "max_steps", + sessionKey: "s1", + }); + + expect(message).toContain("Found 3 active listings"); + expect(message).not.toContain("Error: Found 3 active listings"); + expect(message).not.toContain("Result: Error:"); + }); + + it("still reports an error when the run produced no response at all", () => { + const message = buildAnnouncementMessage({ + task: "find listings", + status: "failed", + error: "provider refused the request", + runtimeMs: 1000, + tokensUsed: 10, + cost: 0.1, + finishReason: "error", + sessionKey: "s1", + }); + + expect(message).toContain("Error: provider refused the request"); + }); +}); diff --git a/packages/agent/src/spawn/sub-agent-outcome.test.ts b/packages/agent/src/spawn/sub-agent-outcome.test.ts index 54d3efc27..37e828eb6 100644 --- a/packages/agent/src/spawn/sub-agent-outcome.test.ts +++ b/packages/agent/src/spawn/sub-agent-outcome.test.ts @@ -1,11 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 import { describe, expect, it } from "vitest"; -import { resolveSubAgentOutcome } from "./sub-agent-outcome.js"; +import { isDeliveredFinishReason, resolveSubAgentOutcome } from "./sub-agent-outcome.js"; describe("resolveSubAgentOutcome", () => { it("reports completed when the model stopped cleanly and the contract is satisfied", () => { - expect(resolveSubAgentOutcome({ modelStoppedCleanly: true, missingContractedOutputs: [] })) + expect(resolveSubAgentOutcome({ modelDelivered: true, missingContractedOutputs: [] })) .toEqual({ success: true, reason: "completed", missingOutputs: [] }); }); @@ -14,7 +14,7 @@ describe("resolveSubAgentOutcome", () => { // "completed". The parent then discarded it and started over. it("does not report completed when a contracted output is missing", () => { const outcome = resolveSubAgentOutcome({ - modelStoppedCleanly: true, + modelDelivered: true, missingContractedOutputs: ["reports/activity.xlsx"], }); expect(outcome.success).toBe(false); @@ -24,7 +24,7 @@ describe("resolveSubAgentOutcome", () => { it("reports every missing path, not just the first", () => { const outcome = resolveSubAgentOutcome({ - modelStoppedCleanly: true, + modelDelivered: true, missingContractedOutputs: ["a.xlsx", "b.csv"], }); expect(outcome.missingOutputs).toEqual(["a.xlsx", "b.csv"]); @@ -34,7 +34,7 @@ describe("resolveSubAgentOutcome", () => { // "contract_unsatisfied" would bury the actual reason the run stopped. it("keeps the model halt as the reason when the model did not stop cleanly", () => { const outcome = resolveSubAgentOutcome({ - modelStoppedCleanly: false, + modelDelivered: false, missingContractedOutputs: ["reports/activity.xlsx"], }); expect(outcome.success).toBe(false); @@ -42,13 +42,50 @@ describe("resolveSubAgentOutcome", () => { }); it("reports a halted model even when no contract was declared", () => { - expect(resolveSubAgentOutcome({ modelStoppedCleanly: false, missingContractedOutputs: [] })) + expect(resolveSubAgentOutcome({ modelDelivered: false, missingContractedOutputs: [] })) .toEqual({ success: false, reason: "model_halted", missingOutputs: [] }); }); it("leaves a run with no declared contract unaffected", () => { // No expected_outputs ⇒ nothing to be missing ⇒ behaviour is unchanged. - expect(resolveSubAgentOutcome({ modelStoppedCleanly: true, missingContractedOutputs: [] }).success) + expect(resolveSubAgentOutcome({ modelDelivered: true, missingContractedOutputs: [] }).success) .toBe(true); }); }); + +describe("isDeliveredFinishReason", () => { + // Live incident: a sub-agent searched for used-car listings, hit bot + // protection on 3 of many web_fetch calls across 21 turns, and returned a + // complete answer naming the sources it could not verify. Its finish reason + // was `completed_with_tool_errors`, which is not "stop"/"end_turn", so the + // outcome resolved to `model_halted` and the parent announced + // "Status: Failed" over a perfectly good result. + it("treats a completed-with-tool-errors finish as delivered rather than halted", () => { + expect(isDeliveredFinishReason("completed_with_tool_errors")).toBe(true); + + expect(resolveSubAgentOutcome({ + modelDelivered: isDeliveredFinishReason("completed_with_tool_errors"), + missingContractedOutputs: [], + })).toEqual({ success: true, reason: "completed", missingOutputs: [] }); + }); + + it("accepts the two clean stops and nothing else", () => { + expect(isDeliveredFinishReason("stop")).toBe(true); + expect(isDeliveredFinishReason("end_turn")).toBe(true); + // Genuine halts stay halts — the ceiling, the loop guard, and a hard error + // all mean the model did NOT get to deliver. + for (const halted of ["max_steps", "error", "context_exhausted", "loop_detected", "budget_exceeded"]) { + expect(isDeliveredFinishReason(halted)).toBe(false); + } + expect(isDeliveredFinishReason(undefined)).toBe(false); + }); + + it("does not let a delivered finish paper over an unsatisfied output contract", () => { + // The contract gate is independent: delivering prose is not writing the + // files the child's own prompt promised. + expect(resolveSubAgentOutcome({ + modelDelivered: isDeliveredFinishReason("completed_with_tool_errors"), + missingContractedOutputs: ["report.xlsx"], + })).toEqual({ success: false, reason: "contract_unsatisfied", missingOutputs: ["report.xlsx"] }); + }); +}); From ad6485bdbc6f3bd08c7e02446cb1d6cd6f561e3e Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 16:25:26 +0300 Subject: [PATCH 15/36] fix(agent): announce a delivered sub-agent answer as delivered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A run that finished `completed_with_tool_errors` was resolved as `model_halted`, so a complete answer reached the user as Status: Failed Result: Error: Two independent links, both closed: `isDeliveredFinishReason` now decides the question, and admits `completed_with_tool_errors` — a name that says COMPLETED, for a run whose tool errors are already carried as degradation elsewhere. The genuine halts stay out. This makes the existing `Completed ()` branch reachable, which is what should have rendered all along; it was never dead code, just unreachable behind the wrong classification. The announcement no longer renders a response as an error under any status. The failed branch handed `error` whatever it was given, and its caller fell back to the child's own response when it had no failure string. The caller now passes the output as `response` and reserves `error` for the one case where the response cannot be trusted as the result — a child that reported done while background work it launched was still running. `modelStoppedCleanly` is renamed `modelDelivered`. The field decides whether the model got to deliver; a name asserting "cleanly" is what made a delivered-with-tool-errors run look like it did not qualify. The output contract is untouched: delivering prose is still not writing the files a child promised, and that gate fails the run independently. --- docs/agents/subagent-lifecycle.mdx | 14 ++++++++ .../spawn/sub-agent-announcement-content.ts | 15 ++++++-- packages/agent/src/spawn/sub-agent-outcome.ts | 35 +++++++++++++++++-- packages/agent/src/spawn/sub-agent-runner.ts | 25 +++++++------ 4 files changed, 72 insertions(+), 17 deletions(-) diff --git a/docs/agents/subagent-lifecycle.mdx b/docs/agents/subagent-lifecycle.mdx index 897faac2e..39b3b1d59 100644 --- a/docs/agents/subagent-lifecycle.mdx +++ b/docs/agents/subagent-lifecycle.mdx @@ -216,6 +216,20 @@ classification. For example, an error-classified failed run is rendered as `Failed`, so delivery does not erase the reason recorded by status and observability surfaces. +A run that reached the end of its work counts as **delivered**, not halted — +including one that finished `completed_with_tool_errors`, where some tool call +failed but the sub-agent still produced an answer. Such a run is announced as +`Completed (completed_with_tool_errors)`, and its tool failures remain visible +as degradation on the observability surfaces. Only the genuine halts (the step +ceiling, the no-progress loop guard, context exhaustion, a budget stop, a hard +error) are announced as failures. + +The sub-agent's output is always rendered as the **result**, never as an error. +A degraded run that still produced an answer shows that answer, with the +degradation carried by the status label. The one exception is a run that +abandoned background work it had launched: there the response cannot be trusted +as the result, so the failure is reported instead. + ## Objective reinforcement When a sub-agent's conversation grows long enough to trigger the context diff --git a/packages/agent/src/spawn/sub-agent-announcement-content.ts b/packages/agent/src/spawn/sub-agent-announcement-content.ts index 5b1c22c0a..34db112fd 100644 --- a/packages/agent/src/spawn/sub-agent-announcement-content.ts +++ b/packages/agent/src/spawn/sub-agent-announcement-content.ts @@ -150,9 +150,18 @@ export function buildAnnouncementMessage(params: { announcementVerb = "completed"; } - const resultText = params.status === "completed" - ? (params.response ?? "No output") - : `Error: ${params.error ?? "Unknown error"}`; + // A response is the run's OUTPUT, never its error — even on the failed path. + // The failed branch used to render whatever it was handed as `Error: …`, and + // its caller falls back to the child's own response when it has no failure + // string, so a complete answer was published to the user prefixed "Error:". + // Prefer a real response wherever one exists; the status label above already + // carries the degradation, so nothing is hidden by showing the work. + const response = params.response?.trim(); + const resultText = response !== undefined && response.length > 0 + ? params.response ?? "" + : params.status === "completed" + ? "No output" + : `Error: ${params.error ?? "Unknown error"}`; let validationLine = ""; if (params.validation && params.validation.length > 0) { const verified = params.validation.filter((result) => result.exists).length; diff --git a/packages/agent/src/spawn/sub-agent-outcome.ts b/packages/agent/src/spawn/sub-agent-outcome.ts index 620f400f4..b1296b490 100644 --- a/packages/agent/src/spawn/sub-agent-outcome.ts +++ b/packages/agent/src/spawn/sub-agent-outcome.ts @@ -42,16 +42,45 @@ export interface SubAgentOutcome { readonly missingOutputs: readonly string[]; } +/** + * Finish reasons under which the model reached the end of its work and produced + * an answer. + * + * `completed_with_tool_errors` belongs here: it names a run that COMPLETED, with + * some tool call along the way having failed. A child that loses a few fetches + * to bot protection, works around them, and reports what it could not verify has + * done its job — and the tool errors are already carried as degradation on the + * outcome surfaces. Excluding it announced a delivered answer as a halt. + * + * The genuine halts — the step ceiling, the loop guard, context exhaustion, a + * budget stop, a hard error — mean the model never got to deliver, and stay out. + */ +const DELIVERED_FINISH_REASONS: ReadonlySet = new Set([ + "stop", + "end_turn", + "completed_with_tool_errors", +]); + +/** Whether this finish reason means the model delivered rather than halted. */ +export function isDeliveredFinishReason(finishReason: string | undefined): boolean { + return finishReason !== undefined && DELIVERED_FINISH_REASONS.has(finishReason); +} + export interface ResolveSubAgentOutcomeInput { - /** Whether the model's finish reason was a clean stop. */ - readonly modelStoppedCleanly: boolean; + /** + * Whether the model reached the end of its work (see + * {@link isDeliveredFinishReason}). Named for DELIVERY, not cleanliness: a run + * can deliver a complete answer with tool errors behind it, and a field + * asserting "stopped cleanly" made that case look like it did not qualify. + */ + readonly modelDelivered: boolean; /** Declared `expected_outputs` that post-run validation did not find. */ readonly missingContractedOutputs: readonly string[]; } /** Resolve the terminal outcome from the model's stop and the output contract. */ export function resolveSubAgentOutcome(input: ResolveSubAgentOutcomeInput): SubAgentOutcome { - if (!input.modelStoppedCleanly) { + if (!input.modelDelivered) { return { success: false, reason: "model_halted", missingOutputs: [] }; } if (input.missingContractedOutputs.length > 0) { diff --git a/packages/agent/src/spawn/sub-agent-runner.ts b/packages/agent/src/spawn/sub-agent-runner.ts index 2eeb5eab2..904bdca87 100644 --- a/packages/agent/src/spawn/sub-agent-runner.ts +++ b/packages/agent/src/spawn/sub-agent-runner.ts @@ -89,7 +89,7 @@ import { type ValidationResult, } from "./sub-agent-result-processor.js"; import { buildHaltedAccount } from "./halted-account.js"; -import { resolveSubAgentOutcome } from "./sub-agent-outcome.js"; +import { isDeliveredFinishReason, resolveSubAgentOutcome } from "./sub-agent-outcome.js"; import { comparePosture, SandboxDowngradeError, type SandboxPosture } from "./sandbox-posture.js"; import { steerRun as steerRunHelper, type SteerRunDeps, type SteerableRun } from "./steer-run.js"; import type { RunHandle } from "../executor/active-run-registry.js"; @@ -3191,8 +3191,7 @@ function classifyCompletionErrorKind( if (deliverySuppressedRunIds.has(runId)) return; const providerCompletedAt = clock.now(); - const modelStoppedCleanly = - result.finishReason === "stop" || result.finishReason === "end_turn"; + const modelDelivered = isDeliveredFinishReason(result.finishReason); // Output-contract validation. Hoisted ABOVE the outcome derivation (it used to run // several hundred lines below, purely as a WARN) because a clean model stop is not the @@ -3259,7 +3258,7 @@ function classifyCompletionErrorKind( } const subAgentOutcome = resolveSubAgentOutcome({ - modelStoppedCleanly, + modelDelivered, missingContractedOutputs, }); // Two INDEPENDENT reasons a run is not a success, and a run can hit @@ -3609,16 +3608,20 @@ function classifyCompletionErrorKind( announcementText = buildAnnouncementMessage({ task: params.task, status: isSuccess ? "completed" : "failed", - ...(isSuccess - ? { + // The child's output is its RESPONSE on both paths. Only a + // genuine failure string belongs in `error`: abandoned + // background work is the one case where the response cannot be + // trusted as the result, because the child reported done while + // work it launched was still running. Everywhere else the run + // produced something and the reader must see it as output — + // routing it through `error` published a complete answer to the + // user prefixed "Error:". + ...(backgroundProcessFailure !== undefined + ? { error: backgroundProcessFailure } + : { response: condensedResult ? `${condensedResult.result.summary}${fullResultLine}` : sanitizeAssistantResponse(result.response), - } - : { - error: backgroundProcessFailure ?? (condensedResult - ? `${condensedResult.result.summary}${fullResultLine}` - : sanitizeAssistantResponse(result.response)), }), runtimeMs, stepsExecuted: result.stepsExecuted, From cd7c0becae363ebb27383b278a3bc4191c842b9c Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 16:47:03 +0300 Subject: [PATCH 16/36] test(agent): pin a wrong-script reply as delivered-degraded, not terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When bounded locale repair cannot reach the enforced script, the branch discarded the model's answer, substituted a canned "locale unavailable" line, and set finishReason=error. Live on comis-moshe that killed two turns in one day (11:22 telegram, 12:39 sub-agent) and was the sole cause of every hard failure in the window — the user received a runtime-generated reply instead of a usable answer. It contradicts the documented contract. docs/operations/multilingual.mdx states "every non-Latin capability has a working, visible, lower-fidelity floor — nothing hard-fails", and that Comis "preserves the original response so a locale rewrite cannot reverse the observed outcome". The three sibling branches in this same function agree: repair-errored, literals-dropped and repair-succeeded all preserve the response and return. This branch is the outlier. RED: expects the Hebrew answer preserved with finishReason "stop"; receives the canned line with finishReason "error". The existing case that pinned the terminal behaviour is retargeted rather than deleted — the scenario it covers is still exercised, only its expected outcome changes. --- .../response-locale-enforcement.test.ts | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts b/packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts index 887ef79d4..b6592409e 100644 --- a/packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts +++ b/packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts @@ -442,7 +442,7 @@ describe("applyResponseLocaleEnforcement", () => { expect(result).not.toHaveProperty("errorContext"); }); - it("fails visibly when the bounded repair still violates the enforced current-request script", async () => { + it("delivers the answer, degraded, when the bounded repair still violates the enforced script", async () => { const eventBus = new TypedEventBus(); const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), @@ -476,16 +476,18 @@ describe("applyResponseLocaleEnforcement", () => { await applyResponseLocaleEnforcement(params); - expect(result).toMatchObject({ - response: - "I couldn't produce a response in the language and writing system requested for this message. Please retry or select a model that supports it.", - finishReason: "error", - terminalErrorKind: "validation", - errorContext: { - errorType: "ResponseLocaleMismatch", - retryable: true, - }, - }); + // A wrong SCRIPT is a presentation defect, not an execution failure. The + // multilingual contract is that "every non-Latin capability has a working, + // visible, lower-fidelity floor — nothing hard-fails", and the three sibling + // branches here (repair errored, repair dropped literals, repair succeeded) + // all preserve the response and return. Only this one discarded the model's + // answer for a canned line and marked the turn terminal. Live on + // comis-moshe it killed two turns in one day and was the sole cause of every + // hard failure in the window; the user got a runtime-generated reply in + // place of a usable answer. + expect(result).toMatchObject({ response: hebrew, finishReason: "stop" }); + expect(result).not.toHaveProperty("terminalErrorKind"); + expect(result).not.toHaveProperty("errorContext"); expect(session.prompt).not.toHaveBeenCalled(); expect(JSON.stringify(session.streamFunction.mock.calls[0]?.[1])) .toContain("what model are u actually using now"); From 6a4bde38480774e29409a11f8c8c1275a137a014 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 16:50:57 +0300 Subject: [PATCH 17/36] fix(agent): deliver a wrong-script answer instead of killing the turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When bounded locale repair could not reach the enforced script, the branch discarded the model's answer for a canned "locale unavailable" line and set finishReason=error. Live on comis-moshe that killed two turns in one day and was the sole cause of every hard failure in the window: the user asked a question, the model answered it, and the runtime replaced the answer with an apology. A wrong writing system is a presentation defect, not an execution failure. The documented contract agrees — "every non-Latin capability has a working, visible, lower-fidelity floor, nothing hard-fails" — and so do the three sibling branches in this same function, which preserve the response and return when the repair errors, drops literals, or succeeds. This branch was the outlier, and a canned refusal is not a floor. The response now stays. Nothing is hidden by keeping it: the WARN above already names the resolver tier that set the target, and `execution:recovery_attempted` (locale_fidelity, succeeded false) already carries the signal to the observability surfaces. `recoverFinalResponseLocaleFailure` is removed with its caller and its two tests. It existed only to clear this terminal state after a later guard happened to satisfy the policy; with the state never set, its precondition can no longer hold. It was the only consumer of the only producer, so leaving it would strand a function that can never fire. --- docs/operations/multilingual.mdx | 7 +++ .../executor/executor-post-execution.test.ts | 11 ---- .../src/executor/executor-post-execution.ts | 23 -------- .../response-locale-enforcement.test.ts | 29 ---------- .../response-locale-enforcement.ts | 58 +++++-------------- 5 files changed, 20 insertions(+), 108 deletions(-) diff --git a/docs/operations/multilingual.mdx b/docs/operations/multilingual.mdx index 1c68fb946..afaa93786 100644 --- a/docs/operations/multilingual.mdx +++ b/docs/operations/multilingual.mdx @@ -173,6 +173,13 @@ Three config keys carry the multilingual surface; all are documented in the URLs, and tool results preserved), and streaming consumers receive only the finalized response while enforcement is active. + If the repair still cannot reach the expected script, **the answer is delivered anyway** — in + the script the model produced. A wrong writing system is a presentation defect, not an execution + failure: the turn answered the question, so discarding it would trade a cosmetic problem for a + total loss, and the degradation matrix below applies here too. The mismatch stays visible as a + WARN naming the resolver tier that set the target, and as an + `execution:recovery_attempted` event with `reason: "locale_fidelity"` and `succeeded: false`. + A contradicting script is detected two ways. **Bulk** foreign prose has to clear a share and a unit floor, so that legitimately quoting a foreign name or term is not treated as a violation. Separately, a single **token that welds two different non-Latin scripts** counts on its own, with diff --git a/packages/agent/src/executor/executor-post-execution.test.ts b/packages/agent/src/executor/executor-post-execution.test.ts index 6d1291930..27440f806 100644 --- a/packages/agent/src/executor/executor-post-execution.test.ts +++ b/packages/agent/src/executor/executor-post-execution.test.ts @@ -1228,17 +1228,6 @@ describe("tool-failure endReason and notice", () => { .toBeLessThan(stripped.indexOf("synchronizeFinalAssistantResponse(")); }); - it("source-grep — final model-status grounding reconciles locale failure before terminal classification", () => { - const stripped = readPostExecStripped(); - const guardIndex = stripped.indexOf("enforceActiveModelSelfStatus("); - const reconcileIndex = stripped.indexOf("recoverFinalResponseLocaleFailure("); - const terminalIndex = stripped.indexOf("const finishReasonStr"); - - expect(guardIndex).toBeGreaterThanOrEqual(0); - expect(reconcileIndex).toBeGreaterThan(guardIndex); - expect(terminalIndex).toBeGreaterThan(reconcileIndex); - }); - it("source-grep — the failure notice is built through the locale seam, not a literal", () => { const stripped = readPostExecStripped(); // It used to be a bare English `[tool failure] reported an error` diff --git a/packages/agent/src/executor/executor-post-execution.ts b/packages/agent/src/executor/executor-post-execution.ts index da3747a6a..56259b8e4 100644 --- a/packages/agent/src/executor/executor-post-execution.ts +++ b/packages/agent/src/executor/executor-post-execution.ts @@ -153,7 +153,6 @@ import { } from "./executor-response-filter.js"; import { BACKGROUND_POLLER_TOOL } from "../safety/background-failure-attribution.js"; import { parseContextExhaustionCause } from "../context-engine/errors.js"; -import { recoverFinalResponseLocaleFailure } from "./prompt-runner/response-locale-enforcement.js"; import { buildSyntheticCriticDeps } from "./verification-gate-synth-deps.js"; import { resolveScaffoldDefaults } from "./scaffold-defaults.js"; import { generateCanaryToken } from "@comis/core"; @@ -1397,28 +1396,6 @@ export async function postExecution(params: PostExecutionParams): Promise }, }); } - if ( - activeModelSelfStatus.corrected - && recoverFinalResponseLocaleFailure(result, params.responseLocalePolicy) - ) { - deps.logger.info( - { - step: "response-locale-recovery", - provider: params.provider, - modelId: params.modelId, - durationMs: 0, - }, - "Final response guard satisfied the captured locale policy", - ); - deps.eventBus.emit("execution:recovery_attempted", { - agentId: effectiveAgentId, - sessionKey: formattedKey, - reason: "locale_fidelity", - succeeded: true, - timestamp: deps.clock.now(), - }); - } - // Derive effectiveFinishReason BEFORE the bookend log so it is visible there. // The bookend must log effectiveFinishReason (not result.finishReason) so that // an output_starved turn — which carries result.finishReason="stop" until promoted here — diff --git a/packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts b/packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts index b6592409e..a8e27bc9d 100644 --- a/packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts +++ b/packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts @@ -413,35 +413,6 @@ describe("applyResponseLocaleEnforcement", () => { expect(providerOptions[Symbol.for("comis.auxiliary-stream-call")]).toBe(true); }); - it("recovers the locale terminal error when a later deterministic guard satisfies the policy", () => { - const candidate = ( - responseLocaleEnforcement as Record - ).recoverFinalResponseLocaleFailure; - expect(candidate).toBeTypeOf("function"); - const result: Record = { - response: "openai / gpt-4.1-nano", - finishReason: "error", - terminalErrorKind: "validation", - errorContext: { - errorType: "ResponseLocaleMismatch", - retryable: true, - }, - }; - - const recovered = (candidate as ( - result: Record, - policy: ResponseLocalePolicy, - ) => boolean)(result, LATIN_POLICY); - - expect(recovered).toBe(true); - expect(result).toMatchObject({ - response: "openai / gpt-4.1-nano", - finishReason: "stop", - }); - expect(result).not.toHaveProperty("terminalErrorKind"); - expect(result).not.toHaveProperty("errorContext"); - }); - it("delivers the answer, degraded, when the bounded repair still violates the enforced script", async () => { const eventBus = new TypedEventBus(); const logger = { diff --git a/packages/agent/src/executor/prompt-runner/response-locale-enforcement.ts b/packages/agent/src/executor/prompt-runner/response-locale-enforcement.ts index d6ec43e6c..925159227 100644 --- a/packages/agent/src/executor/prompt-runner/response-locale-enforcement.ts +++ b/packages/agent/src/executor/prompt-runner/response-locale-enforcement.ts @@ -20,12 +20,7 @@ import { evaluateResponseLocale, type ResponseLocaleQualityFinding, } from "../resolve-response-locale-policy.js"; -import { - buildResponseLocaleUnavailableReply, - catalogFromLocalePacks, -} from "../degraded-reply.js"; import type { RunPromptParams } from "./prompt-runner-types.js"; -import type { ExecutionResult } from "../types.js"; import { classifyToolFailureRecovery } from "../../bridge/tool-failure-recovery.js"; import { unrepairedMismatchHint } from "./locale-mismatch-hint.js"; import { markAuxiliaryStreamCall } from "../stream-wrappers/auxiliary-stream-call.js"; @@ -325,33 +320,6 @@ function emitLocaleRecovery(params: RunPromptParams, succeeded: boolean): void { ); } -/** - * Clear a locale-only terminal error when a later deterministic response guard - * produced a final response that satisfies the same captured policy. - */ -export function recoverFinalResponseLocaleFailure( - result: ExecutionResult, - policy: ResponseLocalePolicy, -): boolean { - if ( - result.finishReason !== "error" - || result.terminalErrorKind !== "validation" - || result.errorContext?.errorType !== "ResponseLocaleMismatch" - || evaluateResponseLocale(policy, result.response) !== undefined - ) { - return false; - } - const mutableResult = result as unknown as { - finishReason: string; - terminalErrorKind?: unknown; - errorContext?: unknown; - }; - mutableResult.finishReason = "stop"; - delete mutableResult.terminalErrorKind; - delete mutableResult.errorContext; - return true; -} - /** Apply locale enforcement at the success-path egress boundary. */ export async function applyResponseLocaleEnforcement(params: RunPromptParams): Promise { if (params.responseLocalePolicy === undefined) return; @@ -481,17 +449,17 @@ export async function applyResponseLocaleEnforcement(params: RunPromptParams): P }, "Response locale remained mismatched after repair", ); - params.result.response = buildResponseLocaleUnavailableReply( - params.responseLocalePolicy.locale, - catalogFromLocalePacks(params.config.localePacks), - ); - params.result.finishReason = "error"; - params.result.terminalErrorKind = "validation"; - params.result.errorContext = { - errorType: "ResponseLocaleMismatch", - retryable: true, - originalError: - `Expected ${outcome.value.finalFinding?.expectedScript ?? "requested"} script ` - + `but repair produced ${outcome.value.finalFinding?.actualScript ?? "an incompatible"} script`, - }; + // The response stays. A wrong script is a presentation defect, not an + // execution failure: the model answered the question, in the wrong writing + // system. Discarding it for a canned line converted a cosmetic problem into a + // total loss of the turn — and the multilingual contract is the opposite, + // that every capability degrades to "a working, visible, lower-fidelity + // floor" and nothing hard-fails. The three sibling branches above agree; this + // one was the outlier. `params.result.response` already carries the best + // available text (assigned from the repair outcome), the WARN above names the + // resolver tier to check, and `execution:recovery_attempted` + // (reason "locale_fidelity", succeeded false) already carries the signal to + // the observability surfaces — so nothing is hidden by keeping the answer. + params.result.localeQualityFinding = outcome.value.finalFinding + ?? params.result.localeQualityFinding; } From 5337f82b7fa054a9e92bc5f2175694686c8f7655 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 17:37:40 +0300 Subject: [PATCH 18/36] test(orchestrator): pin an operator lever for quarantined announcements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A quarantined announcement is held by design — nothing drains it, because retrying risks a duplicate delivery. But the runtime offered no way to act on it. Live on comis-moshe a governed entry sat unresolved for 45 minutes, re-warning every 5, and clearing it required stopping the daemon and deleting the JSONL by hand: the in-memory queue is authoritative and rewrites the file on the next persist, so editing it under a running daemon is silently undone. RED: listQuarantined and release do not exist. The list row carries ids, route, timing and the failure reason, plus the announcement LENGTH — never its text. The row rides an admin RPC and a terminal, and the whole point of the quarantine is that this content was not delivered; an operator deciding its fate does not need to read it. --- .../announcement-dead-letter.test.ts | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter.test.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter.test.ts index c492404a0..fad95b711 100644 --- a/packages/orchestrator/src/cross-session/announcement-dead-letter.test.ts +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter.test.ts @@ -1685,3 +1685,78 @@ describe("AnnouncementDeadLetterQueue drain consults the outward ledger", () => expect(dlq.size()).toBe(0); }); }); + +// --------------------------------------------------------------------------- +// The operator lever. A quarantined announcement is held BY DESIGN — nothing +// drains it, because retrying risks a duplicate delivery. Live on comis-moshe a +// governed entry sat unresolved for 45 minutes, re-warning every 5, and the +// only way to clear it was to stop the daemon and delete the JSONL by hand: the +// in-memory queue is authoritative and rewrites the file on the next persist, +// so editing it under a running daemon is silently undone. A condition the +// runtime knows about and offers no lever for is not finished. +// --------------------------------------------------------------------------- +describe("AnnouncementDeadLetterQueue operator lever", () => { + let tmpDir: string; + let filePath: string; + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), "dlq-operator-")); + filePath = join(tmpDir, "dead-letters.jsonl"); + }); + + afterEach(async () => { + await rm(tmpDir, { recursive: true, force: true }); + }); + + it("lists a quarantined announcement by id without exposing its text", async () => { + const queue = createAnnouncementDeadLetterQueue({ filePath, eventBus: createMockEventBus() }); + await queue.enqueue(makeEntry({ + runId: "run-stuck", + channelType: "telegram", + channelId: "678314278", + announcementText: "the answer the user never saw", + lastError: "outward_operation_unresolved", + })); + + const rows = queue.listQuarantined(); + + expect(rows).toHaveLength(1); + const row = rows[0]!; + expect(row.runId).toBe("run-stuck"); + expect(row.channelType).toBe("telegram"); + expect(row.channelId).toBe("678314278"); + expect(row.lastError).toBe("outward_operation_unresolved"); + expect(row.kind).toBe("entry"); + expect(typeof row.id).toBe("string"); + // The operator needs to know there IS content and how much, never the + // content itself: this row rides an admin RPC and a terminal. + expect(row.announcementChars).toBe("the answer the user never saw".length); + expect(JSON.stringify(row)).not.toContain("the answer the user never saw"); + }); + + it("releases a quarantined announcement by id and persists the removal", async () => { + const queue = createAnnouncementDeadLetterQueue({ filePath, eventBus: createMockEventBus() }); + await queue.enqueue(makeEntry({ runId: "run-stuck" })); + const id = queue.listQuarantined()[0]!.id; + + const released = await queue.release(id, "discarded"); + + expect(released).toMatchObject({ ok: true, value: true }); + expect(queue.listQuarantined()).toHaveLength(0); + expect(queue.size()).toBe(0); + // Durable: a fresh queue over the same file must not resurrect it. + const reloaded = createAnnouncementDeadLetterQueue({ filePath, eventBus: createMockEventBus() }); + await reloaded.drain(vi.fn().mockResolvedValue(true)); + expect(reloaded.size()).toBe(0); + }); + + it("reports an unknown id as not released rather than failing the call", async () => { + const queue = createAnnouncementDeadLetterQueue({ filePath, eventBus: createMockEventBus() }); + await queue.enqueue(makeEntry({ runId: "run-stuck" })); + + const released = await queue.release("no-such-id", "discarded"); + + expect(released).toMatchObject({ ok: true, value: false }); + expect(queue.size()).toBe(1); + }); +}); From 494e322fe2421435372120a4e6fe36f34d7260aa Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 17:39:24 +0300 Subject: [PATCH 19/36] feat(orchestrator): give the dead-letter queue list and release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The queue could report a COUNT and nothing else, so a quarantined announcement was visible and unactionable: the only way to clear one was to stop the daemon and delete the JSONL, because the in-memory queue is authoritative and rewrites the file on the next persist. `listQuarantined()` returns every parked item — entries and parked parent-decision reservations alike — with ids, route, timing and the failure reason, plus the announcement LENGTH. Never its text: these rows ride an admin RPC and a terminal, and an operator deciding the fate of an undelivered message does not need to read it. Ordered oldest-first so the longest-stuck item leads. `release(id, outcome)` records the decision and drops the item. Both outcomes remove it: the queue exists to hold an UNDECIDED announcement, so `delivered` and `discarded` are equally finished, and the distinction belongs on the audit trail rather than in the queue. It is serialized with the drain, so a release cannot interleave with a sweep and resurrect the item from a stale snapshot, and it persists BEFORE mutating memory so a storage failure leaves the announcement parked rather than dropping it. An unknown id resolves false rather than failing — releasing the same id twice is an operator retrying, not an error. --- .../cross-session/announcement-dead-letter.ts | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts index c2f2e7d1c..551afe1f6 100644 --- a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts @@ -37,6 +37,36 @@ export interface AnnouncementLogger { debug(obj: Record, msg: string): void; } +/** + * One quarantined announcement, as an operator sees it. + * + * Content-free by construction: the announcement's LENGTH is carried, never its + * text. These rows ride an admin RPC and a terminal, and the announcement is + * quarantined precisely because it was NOT delivered to its intended reader — + * an operator deciding its fate needs the route and the reason, not the message. + */ +export interface QuarantinedAnnouncement { + readonly id: string; + /** `entry` — a failed delivery. `parent_decision` — a parked adjudication. */ + readonly kind: "entry" | "parent_decision"; + readonly runId: string; + readonly agentId?: string; + readonly channelType: ChannelType; + readonly channelId: string; + readonly threadId?: string; + readonly failedAt: number; + readonly attemptCount: number; + readonly lastAttemptAt?: number; + /** Why it is parked (e.g. `outward_operation_unresolved`). */ + readonly lastError?: string; + readonly idempotencyKey?: string; + /** Size of the withheld announcement text, in characters. */ + readonly announcementChars: number; +} + +/** What an operator decided about a quarantined announcement. */ +export type QuarantineReleaseOutcome = "delivered" | "discarded"; + /** Dead-letter queue interface for announcement retry management. */ export interface AnnouncementDeadLetterQueue { /** @@ -76,6 +106,28 @@ export interface AnnouncementDeadLetterQueue { ): Promise; /** Return the current number of entries in the queue. */ size(): number; + /** + * Every parked announcement, content-free, for operator review. Reads the + * in-memory state — which is authoritative, since a persist rewrites the file + * from it. Ordered oldest-first so the longest-stuck item leads. + */ + listQuarantined(): readonly QuarantinedAnnouncement[]; + /** + * Record an operator's decision about one parked announcement and drop it. + * + * `delivered` — the reader already has it (verified out of band); `discarded` + * — it is not worth sending. Both remove the item: the queue's job is to hold + * an undecided announcement, and a decided one is finished either way. The + * distinction rides the audit trail, not the queue. + * + * Resolves `false` for an unknown id rather than failing — releasing the same + * id twice is an operator retrying, not an error. Persists before mutating + * the in-memory state, so a failed write leaves the item parked. + */ + release( + id: string, + outcome: QuarantineReleaseOutcome, + ): Promise>; } /** Configuration options for the dead-letter queue factory. */ @@ -907,6 +959,49 @@ export function createAnnouncementDeadLetterQueue( } } + /** + * Apply an operator decision. Serialized with the drain so a release cannot + * interleave with a sweep and resurrect the item from a stale snapshot. + */ + async function releaseSerialized( + id: string, + outcome: QuarantineReleaseOutcome, + ): Promise> { + const loaded = await loadFromDisk(); + if (!loaded.ok) return loaded; + const entry = entries.find((candidate) => candidate.id === id); + const reservation = decisionReservations.find((candidate) => candidate.id === id); + if (entry === undefined && reservation === undefined) return ok(false); + + const nextEntries = entries.filter((candidate) => candidate.id !== id); + const nextReservations = decisionReservations.filter((candidate) => candidate.id !== id); + // Persist BEFORE mutating memory: a failed write must leave the item parked + // rather than dropping an undelivered announcement on a storage error. + const persisted = await persist(nextEntries, nextReservations); + if (!persisted.ok) { + logger?.error( + { + errorKind: "resource" as const, + hint: "restore dead-letter storage before releasing; the announcement is still quarantined", + }, + "Quarantined announcement release was not durably persisted", + ); + return persisted; + } + entries = nextEntries; + decisionReservations = nextReservations; + logger?.info( + { + runId: entry?.runId ?? reservation?.runId, + kind: entry !== undefined ? "entry" : "parent_decision", + outcome, + remaining: entries.length + decisionReservations.length, + }, + "Quarantined announcement released by operator decision", + ); + return ok(true); + } + return { enqueue: (entry) => serialize(() => enqueueDurably(entry)), reserveDecision: (entry) => serialize(() => decisionStore.reserve(entry)), @@ -917,5 +1012,38 @@ export function createAnnouncementDeadLetterQueue( drain: (sendToChannel, onDelivered) => serialize(() => drainSerialized(sendToChannel, onDelivered)), size: () => entries.length + decisionReservations.length, + listQuarantined: () => [ + ...entries.map((entry): QuarantinedAnnouncement => ({ + id: entry.id, + kind: "entry" as const, + runId: entry.runId, + ...(entry.agentId === undefined ? {} : { agentId: entry.agentId }), + channelType: entry.channelType, + channelId: entry.channelId, + ...(entry.threadId === undefined ? {} : { threadId: entry.threadId }), + failedAt: entry.failedAt, + attemptCount: entry.attemptCount, + lastAttemptAt: entry.lastAttemptAt, + ...(entry.lastError === undefined ? {} : { lastError: entry.lastError }), + ...(entry.idempotencyKey === undefined ? {} : { idempotencyKey: entry.idempotencyKey }), + announcementChars: entry.announcementText.length, + })), + ...decisionReservations.map((record): QuarantinedAnnouncement => ({ + id: record.id, + kind: "parent_decision" as const, + runId: record.runId, + agentId: record.agentId, + channelType: record.channelType, + channelId: record.channelId, + ...(record.threadId === undefined ? {} : { threadId: record.threadId }), + failedAt: record.failedAt, + // A reservation is parked awaiting adjudication, never retried, so it + // has no attempt history to report. + attemptCount: 0, + idempotencyKey: record.idempotencyKey, + announcementChars: record.announcementText.length, + })), + ].sort((left, right) => left.failedAt - right.failedAt || left.id.localeCompare(right.id)), + release: (id, outcome) => serialize(() => releaseSerialized(id, outcome)), }; } From 543fcc5974cd82ecd4416277a7b5a500d51b7493 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 17:51:25 +0300 Subject: [PATCH 20/36] =?UTF-8?q?feat(daemon,cli):=20add=20`comis=20quaran?= =?UTF-8?q?tine`=20=E2=80=94=20list=20and=20release=20parked=20announcemen?= =?UTF-8?q?ts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the operator lever. The port from the previous commit was unreachable; this exposes it as two admin-only RPCs and a CLI group. `obs.quarantine.list` / `obs.quarantine.release` carry no `rpc` route, so they sit in the deny-by-origin control plane: an agent turn — including a prompt-injected one — can never reach them. Deciding the fate of a message a user was meant to receive is an operator's call. It is daemon-only by design, with no `--offline` mode. While the daemon is up it is the only authority: the queue lives in memory and rewrites dead-letters.jsonl from it on the next persist, so an offline write is silently undone. That is the trap this command exists to remove, and an offline path would reintroduce it. The listing is content-free — ids, route, timing, attempt count, failure reason, and the announcement's LENGTH. Never its text. Gate work this required, each deliberate rather than waved through: - announcement-dead-letter.ts crossed the 1000-line cap, so the projection moved to a sibling module (the announcement-dead-letter-file.ts precedent). Allowlists are shrink-only; adding an entry is not an option. - the contract codegen budget had 202B of headroom and these two contracts need 859B. Raised to 198,650 per the budget's own documented rule — reviewed addition, gzipped still at 20,896B against 38,912B (46% spare). The new headroom stays deliberately tight; the cap is a ratchet. - the CLI command-count pin, the docs command-group count, and the OBSERVABILITY_CONTRACTS count/method list all moved with the addition. --- docs/reference/cli.mdx | 28 +++- packages/cli/src/cli.test.ts | 2 +- packages/cli/src/cli.ts | 2 + packages/cli/src/commands/quarantine.ts | 131 ++++++++++++++++++ .../api-contracts/announcement-quarantine.ts | 88 ++++++++++++ .../src/api-contracts/observability.test.ts | 11 +- .../core/src/api-contracts/observability.ts | 14 ++ packages/daemon/src/api/obs-handlers/index.ts | 3 + .../src/api/obs-handlers/obs-quarantine.ts | 71 ++++++++++ packages/daemon/src/api/types.ts | 5 + packages/daemon/src/daemon.ts | 1 + .../announcement-dead-letter-quarantine.ts | 87 ++++++++++++ .../cross-session/announcement-dead-letter.ts | 72 ++-------- packages/web/src/api/contracts.generated.json | 79 +++++++++++ .../web/src/api/contracts.generated.size.json | 8 +- packages/web/src/api/contracts.generated.ts | 79 +++++++++++ scripts/contracts/size-budget.ts | 12 +- 17 files changed, 619 insertions(+), 74 deletions(-) create mode 100644 packages/cli/src/commands/quarantine.ts create mode 100644 packages/core/src/api-contracts/announcement-quarantine.ts create mode 100644 packages/daemon/src/api/obs-handlers/obs-quarantine.ts create mode 100644 packages/orchestrator/src/cross-session/announcement-dead-letter-quarantine.ts diff --git a/docs/reference/cli.mdx b/docs/reference/cli.mdx index bd6b905aa..3b4e408b2 100644 --- a/docs/reference/cli.mdx +++ b/docs/reference/cli.mdx @@ -6,7 +6,7 @@ icon: "terminal" **What this is for:** the `comis` command is how you set up your daemon, manage agents and channels, browse memory, audit security, and operate the running system from the terminal. **Who it's for:** operators and developers running Comis on their own machine or a server. -Comis currently ships **32 top-level command groups**. Some commands call the +Comis currently ships **33 top-level command groups**. Some commands call the running daemon over JSON-RPC; others inspect or update local state. The built-in `--help` output is authoritative for the installed version. @@ -1645,6 +1645,32 @@ comis cost export --provider openai --model gpt-4o-mini --- +### `comis quarantine` + +Inspect and release **quarantined background-task announcements** — a completed task's outcome that the runtime could not prove reached its reader. + +Nothing drains a quarantined announcement automatically: re-sending one whose delivery is unproven risks telling a user the same thing twice, so the runtime holds it and waits for a human. This command is that decision point. Backed by `ObsQuarantineListContract` (`obs.quarantine.list`) and `ObsQuarantineReleaseContract` (`obs.quarantine.release`), both admin-only. + +```bash +# What is waiting, oldest first +comis quarantine list +comis quarantine list --format json + +# Decide one +comis quarantine release --outcome delivered +comis quarantine release --outcome discarded +``` + +Use `delivered` when you have confirmed out of band that the reader already has the message, and `discarded` when it is not worth sending. Both remove the item — the queue exists to hold an *undecided* announcement — and the distinction is kept for the audit trail. Releasing an id that is already gone reports that plainly instead of failing, so re-running the command is safe. + +The listing is **content-free**: it shows ids, route, timing, attempt count, the failure reason and the announcement's *length*, never its text. An operator deciding whether a reader was already informed needs the route and the reason, not the message body. + + +There is deliberately no offline mode. While the daemon is running it is the only authority over the queue: it holds the state in memory and rewrites `~/.comis/dead-letters.jsonl` from it on the next persist, so editing that file under a live daemon is silently undone. Always go through this command; see [`dead-letters.jsonl`](/operations/data-directory#dead-letters-jsonl). + + +--- + ### `comis explain` Assemble an `IncidentReport` for a single agent session — a bounded, causal post-mortem (outcome, cost, per-tool stats, normalized failures, circuit-breaker timeline, large-result offloads, recall health — including degraded/failed recall lanes — session-wide activity-finalize tallies, and a deterministic likely root cause). The report is derived from log evidence only; no LLM is invoked, so the same session always yields the same verdict. Backed by `ObsExplainContract` (`obs.explain`). diff --git a/packages/cli/src/cli.test.ts b/packages/cli/src/cli.test.ts index 13aed3a85..7c09a7913 100644 --- a/packages/cli/src/cli.test.ts +++ b/packages/cli/src/cli.test.ts @@ -110,7 +110,7 @@ describe("CLI entry point", () => { ] as const; it("registers exactly 32 commands", () => { - expect(program.commands).toHaveLength(32); + expect(program.commands).toHaveLength(33); }); it.each(expectedCommands)("registers the '%s' command", (name) => { diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index aa5f52a7c..66606c688 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -39,6 +39,7 @@ import { registerExplainCommand } from "./commands/explain.js"; import { registerOrchestrateCommand } from "./commands/orchestrate.js"; import { registerWhoamiCommand } from "./commands/whoami.js"; import { registerCostExportCommand } from "./commands/cost-export.js"; +import { registerQuarantineCommand } from "./commands/quarantine.js"; import { registerCronCommand } from "./commands/cron.js"; import { registerTasksCommand } from "./commands/tasks.js"; import { registerSystemHealthCommand } from "./commands/system-health.js"; @@ -80,6 +81,7 @@ registerExplainCommand(program); registerOrchestrateCommand(program); registerWhoamiCommand(program); registerCostExportCommand(program); +registerQuarantineCommand(program); registerCronCommand(program); registerTasksCommand(program); registerSystemHealthCommand(program); diff --git a/packages/cli/src/commands/quarantine.ts b/packages/cli/src/commands/quarantine.ts new file mode 100644 index 000000000..ca61e507e --- /dev/null +++ b/packages/cli/src/commands/quarantine.ts @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: Apache-2.0 +// @allow-throw: CLI command module — Commander.js boundary catches throws and surfaces user-readable messages. The catch blocks below convert them to error()/process.exit(1) directly. +/** + * `comis quarantine` — the operator lever over quarantined background-task + * announcements. + * + * A quarantined announcement is a completed task's outcome that the runtime + * could not prove was delivered. Nothing drains it automatically, because + * re-sending risks telling a user the same thing twice — so it waits for a + * human. Before this command the wait was unbounded: clearing one meant + * stopping the daemon and editing `dead-letters.jsonl` by hand, since the + * running queue holds its state in memory and rewrites that file on the next + * persist, silently undoing an edit made under a live daemon. + * + * DAEMON-ONLY on purpose. There is deliberately no `--offline` mode: while the + * daemon is up it is the only authority, and an offline write would be + * overwritten without warning — the exact trap this command exists to remove. + * + * Content-free: the listing shows ids, route, timing, the failure reason and + * the announcement's LENGTH — never its text. The announcement is quarantined + * precisely because it was not delivered to its intended reader; an operator + * deciding its fate needs the route and the reason, not the message. + * + * Usage: + * comis quarantine list [--format table|json] + * comis quarantine release --outcome delivered|discarded + * + * @module + */ + +import type { Command } from "commander"; +import { + ObsQuarantineListContract, + ObsQuarantineReleaseContract, + type QuarantinedAnnouncementWire, +} from "@comis/core"; +import { callTyped, withClient } from "../client/rpc-client.js"; +import { error, json } from "../output/format.js"; +import { withSpinner } from "../output/spinner.js"; + +/** Render one parked announcement as an operator-readable block. */ +function renderRow(row: QuarantinedAnnouncementWire, nowMs: number): string { + const ageMin = Math.max(0, Math.round((nowMs - row.failedAt) / 60_000)); + const lines = [ + ` ${row.id}`, + ` kind : ${row.kind}`, + ` run : ${row.runId}`, + ` route : ${row.channelType}/${row.channelId}${row.threadId ? `/${row.threadId}` : ""}`, + ` parked : ${ageMin} min ago (attempts: ${row.attemptCount})`, + ` announcement: ${row.announcementChars} chars (withheld)`, + ]; + if (row.lastError !== undefined) lines.push(` reason : ${row.lastError}`); + if (row.agentId !== undefined) lines.push(` agent : ${row.agentId}`); + return lines.join("\n"); +} + +/** Register the `quarantine` command group. */ +export function registerQuarantineCommand(program: Command): void { + const group = program + .command("quarantine") + .description("Inspect and release quarantined background-task announcements"); + + group + .command("list") + .description("List announcements awaiting an operator decision") + .option("--format ", "Output format: table or json", "table") + .action(async (options: { format?: string }) => { + try { + const result = await withSpinner( + "Reading quarantined announcements...", + () => withClient((client) => callTyped(client, ObsQuarantineListContract, {})), + ); + const rows = result.rows as unknown as QuarantinedAnnouncementWire[]; + if (options.format === "json") { + json(result); + return; + } + if (rows.length === 0) { + process.stdout.write("No quarantined announcements.\n"); + return; + } + const nowMs = Date.now(); + process.stdout.write( + `${result.total} quarantined announcement(s) awaiting a decision:\n\n` + + rows.map((row) => renderRow(row, nowMs)).join("\n\n") + + "\n\nRelease one with:\n" + + " comis quarantine release --outcome delivered|discarded\n" + + "Use 'delivered' when you have confirmed the reader already has it, " + + "'discarded' when it is not worth sending.\n", + ); + } catch (cause) { + error(cause instanceof Error ? cause.message : String(cause)); + process.exit(1); + } + }); + + group + .command("release ") + .description("Record a decision about one quarantined announcement and drop it") + .requiredOption( + "--outcome ", + "delivered (the reader already has it) or discarded (not worth sending)", + ) + .action(async (id: string, options: { outcome: string }) => { + try { + if (options.outcome !== "delivered" && options.outcome !== "discarded") { + error("--outcome must be 'delivered' or 'discarded'"); + process.exit(1); + return; + } + const result = await withSpinner( + "Releasing quarantined announcement...", + () => withClient((client) => callTyped(client, ObsQuarantineReleaseContract, { + id, + outcome: options.outcome as "delivered" | "discarded", + })), + ); + // A false `released` is not an error: the id is already gone, which is + // the state the operator wanted. Say so rather than implying a failure. + process.stdout.write( + result.released + ? `Released ${id} as ${options.outcome}. ${result.remaining} announcement(s) still quarantined.\n` + : `No quarantined announcement carries id ${id} — it is already gone. ` + + `${result.remaining} still quarantined.\n`, + ); + } catch (cause) { + error(cause instanceof Error ? cause.message : String(cause)); + process.exit(1); + } + }); +} diff --git a/packages/core/src/api-contracts/announcement-quarantine.ts b/packages/core/src/api-contracts/announcement-quarantine.ts new file mode 100644 index 000000000..c2b80f6a0 --- /dev/null +++ b/packages/core/src/api-contracts/announcement-quarantine.ts @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: Apache-2.0 +/** + * The `obs.quarantine.list` / `obs.quarantine.release` wire shapes — the + * operator lever over quarantined background-task announcements. + * + * A quarantined announcement is held ON PURPOSE: nothing drains it, because + * re-sending an announcement whose delivery could not be PROVEN risks telling a + * user the same thing twice. The runtime therefore surfaces the condition (a + * WARN, a `health_signal`) and waits for a human to decide. Until these + * contracts existed it waited forever — the only way to clear one was to stop + * the daemon and edit `dead-letters.jsonl` by hand, because the in-memory queue + * is authoritative and rewrites that file on the next persist, so an edit under + * a running daemon is silently undone. + * + * Both are `admin`-only and carry no `rpc` route, which puts them in the + * deny-by-origin control plane: an agent turn — including a prompt-injected one + * — can never reach them. Releasing decides the fate of a message a user was + * supposed to receive, which is an operator's call, not an agent's. + * + * The list rows are content-free: ids, route, timing, failure reason, and the + * announcement's LENGTH. Never its text. The rows ride a terminal and an admin + * RPC, and an operator deciding whether a reader was already informed needs the + * route and the reason, not the message body. + * + * @module + */ +import { z } from "zod"; +import { defineContract } from "./types.js"; + +/** + * One parked announcement as the wire carries it. Mirrors the orchestrator's + * `QuarantinedAnnouncement` field-for-field; there is no announcement-text + * field, structurally — only `announcementChars`. + */ +export interface QuarantinedAnnouncementWire { + id: string; + kind: "entry" | "parent_decision"; + runId: string; + agentId?: string; + channelType: string; + channelId: string; + threadId?: string; + failedAt: number; + attemptCount: number; + lastAttemptAt?: number; + lastError?: string; + idempotencyKey?: string; + announcementChars: number; +} + +/** List every quarantined announcement awaiting an operator decision. */ +export const ObsQuarantineListContract = defineContract({ + method: "obs.quarantine.list", + request: z.object({}), + response: z.object({ + /** Oldest-first, so the longest-stuck item leads. */ + rows: z.array(z.record(z.string(), z.unknown())), + /** Total parked items — equals `rows.length`; explicit so a caller that + * renders nothing still reports the count it was told. */ + total: z.number(), + }), + scopes: ["admin"] as const, +}); + +/** + * Record an operator's decision about one parked announcement and drop it. + * + * `delivered` — the reader already has it, verified out of band. `discarded` — + * it is not worth sending. Both remove the item; the queue exists to hold an + * UNDECIDED announcement, so either decision finishes it. The distinction is + * kept for the audit trail, not for the queue. + */ +export const ObsQuarantineReleaseContract = defineContract({ + method: "obs.quarantine.release", + request: z.object({ + /** The `id` from `obs.quarantine.list`. */ + id: z.string().min(1), + outcome: z.enum(["delivered", "discarded"]), + }), + response: z.object({ + /** False when no parked item carries that id — a repeat release, not an + * error, so the caller can report "already gone" rather than fail. */ + released: z.boolean(), + /** Items still parked after this decision. */ + remaining: z.number(), + }), + scopes: ["admin"] as const, +}); diff --git a/packages/core/src/api-contracts/observability.test.ts b/packages/core/src/api-contracts/observability.test.ts index bdeba2d2a..58b1ffc6a 100644 --- a/packages/core/src/api-contracts/observability.test.ts +++ b/packages/core/src/api-contracts/observability.test.ts @@ -46,8 +46,8 @@ describe("observability-domain contracts", () => { // Aggregator sanity // ------------------------------------------------------------------------- - it("OBSERVABILITY_CONTRACTS has exactly 29 entries", () => { - expect(OBSERVABILITY_CONTRACTS.length).toBe(29); + it("OBSERVABILITY_CONTRACTS has exactly 31 entries", () => { + expect(OBSERVABILITY_CONTRACTS.length).toBe(31); }); it("all contracts are admin-scoped EXCEPT the agent self-observability pair", () => { @@ -90,6 +90,9 @@ describe("observability-domain contracts", () => { // Incident-report assembler. "obs.explain", "obs.getCacheStats", + // The operator lever over quarantined announcements (admin-only). + "obs.quarantine.list", + "obs.quarantine.release", "obs.reset", "obs.reset.table", // Live spend snapshot the kill-switch enforces. @@ -914,8 +917,8 @@ describe("ObsTrace contracts", () => { ).not.toThrow(); }); - it("OBSERVABILITY_CONTRACTS has exactly 29 entries", () => { - expect(OBSERVABILITY_CONTRACTS.length).toBe(29); + it("OBSERVABILITY_CONTRACTS has exactly 31 entries", () => { + expect(OBSERVABILITY_CONTRACTS.length).toBe(31); }); it("OBSERVABILITY_CONTRACTS includes each obs.trace contract exactly once by method name", () => { diff --git a/packages/core/src/api-contracts/observability.ts b/packages/core/src/api-contracts/observability.ts index b474b5e1c..77feaf77d 100644 --- a/packages/core/src/api-contracts/observability.ts +++ b/packages/core/src/api-contracts/observability.ts @@ -56,8 +56,20 @@ export type { SystemHealthReport } from "./system-health-report.js"; // re-export the contract + schema so the `@comis/core` public surface + the // registered RPC set carry them (the ObsSystemHealthContract precedent). import { ObsAuditQueryContract } from "./audit-query.js"; +import { + ObsQuarantineListContract, + ObsQuarantineReleaseContract, +} from "./announcement-quarantine.js"; export { ObsAuditQueryContract } from "./audit-query.js"; export type { AuditEventRowWire, AuditQueryResponse } from "./audit-query.js"; +// The quarantined-announcement operator lever (list + release) lives in the +// sibling `announcement-quarantine.ts`. Same admin-only, deny-by-origin shape +// as the audit read above. +export { + ObsQuarantineListContract, + ObsQuarantineReleaseContract, +} from "./announcement-quarantine.js"; +export type { QuarantinedAnnouncementWire } from "./announcement-quarantine.js"; // The five obs.billing.* contracts (+ their BillingSnapshot response schema) // live in the sibling `observability-billing.ts` (file-size split). Import for // the OBSERVABILITY_CONTRACTS array below; re-export so the `@comis/core` @@ -755,6 +767,8 @@ export const OBSERVABILITY_CONTRACTS = [ ObsBillingTotalContract, ObsBillingUsage24hContract, ObsAuditQueryContract, + ObsQuarantineListContract, + ObsQuarantineReleaseContract, ObsCacheBreaksByReasonContract, ObsCacheStatsWindowContract, ObsChannelsAllContract, diff --git a/packages/daemon/src/api/obs-handlers/index.ts b/packages/daemon/src/api/obs-handlers/index.ts index beb8f5b03..21db32205 100644 --- a/packages/daemon/src/api/obs-handlers/index.ts +++ b/packages/daemon/src/api/obs-handlers/index.ts @@ -43,6 +43,7 @@ export { assembleSystemHealthReport } from "./system-health.js"; // createObsHandlers below (no separate MCP closure — the audit query is an // admin-RPC-only read surface, not an operator-allowlisted MCP tool). export { bindObsAuditHandlers } from "./obs-audit.js"; +export { bindObsQuarantineHandlers } from "./obs-quarantine.js"; // The obs.cacheBreaks.byReason binder. Re-exported for symmetry // with the other obs-handler slices; the daemon composition root spreads it into @@ -64,6 +65,7 @@ import { bindObsTraceHandlers } from "./obs-trace.js"; import { bindObsExplainHandlers } from "./obs-explain.js"; import { bindSystemHealthHandlers } from "./system-health.js"; import { bindObsAuditHandlers } from "./obs-audit.js"; +import { bindObsQuarantineHandlers } from "./obs-quarantine.js"; import { bindObsCacheBreaksHandlers } from "./obs-cache-breaks.js"; import { bindObsSpendHandlers } from "./obs-spend.js"; @@ -85,6 +87,7 @@ export function createObsHandlers(deps: ObsHandlerDeps): Record)._trustLevel as string | undefined; + if (trustLevel !== "admin") throw new AuthorizationError("Admin access required"); +} + +/** Bind both quarantine handlers. Computed-key form is required by the parity tests. */ +export function bindObsQuarantineHandlers(deps: ObsHandlerDeps): Record { + return { + [ObsQuarantineListContract.method]: async (rawParams) => { + requireAdmin(rawParams); + ObsQuarantineListContract.request.parse(stripInternalFields(rawParams)); + + const rows = deps.deadLetterQueue?.listQuarantined() ?? []; + // The rows are content-free by construction (the port carries + // `announcementChars`, never the text), so they ride the loose-record + // wire projection directly — the same narrowing the sibling obs.* reads use. + const result = { rows: rows.map((row) => ({ ...row })), total: rows.length }; + if (IS_DEV) ObsQuarantineListContract.response.parse(result); + return result; + }, + + [ObsQuarantineReleaseContract.method]: async (rawParams) => { + requireAdmin(rawParams); + const params = ObsQuarantineReleaseContract.request.parse(stripInternalFields(rawParams)); + + const queue = deps.deadLetterQueue; + if (queue === undefined) { + const empty = { released: false, remaining: 0 }; + if (IS_DEV) ObsQuarantineReleaseContract.response.parse(empty); + return empty; + } + + const released = await queue.release(params.id, params.outcome); + // A storage failure must surface: reporting `released:false` here would be + // indistinguishable from an unknown id, and the operator would believe the + // announcement was already gone while it is still parked. + if (!released.ok) throw released.error; + + const result = { released: released.value, remaining: queue.size() }; + if (IS_DEV) ObsQuarantineReleaseContract.response.parse(result); + return result; + }, + }; +} diff --git a/packages/daemon/src/api/types.ts b/packages/daemon/src/api/types.ts index be150a70e..ede3a56af 100644 --- a/packages/daemon/src/api/types.ts +++ b/packages/daemon/src/api/types.ts @@ -765,6 +765,11 @@ export interface ObservabilityApiDeps { perAgent: ReadonlyMap; perTenant: ReadonlyMap; global: number; ceilings: { perAgentUsd: number | null; perTenantUsd: number | null; daemonGlobalUsd: number | null }; }; + /** The live announcement dead-letter queue — the operator lever surface for + * `obs.quarantine.*`. The RUNNING queue is authoritative: it rewrites the + * JSONL from memory on each persist, so the daemon is the only place a + * release can be applied while it is up. Absent ⇒ honest empty. @optional-field */ + deadLetterQueue?: import("@comis/orchestrator").AnnouncementDeadLetterQueue; // Observability persistence deps obsStore?: import("@comis/memory").ObservabilityStore; obsPersistence?: Pick< diff --git a/packages/daemon/src/daemon.ts b/packages/daemon/src/daemon.ts index 25c332927..bd84ec8d1 100644 --- a/packages/daemon/src/daemon.ts +++ b/packages/daemon/src/daemon.ts @@ -848,6 +848,7 @@ function buildRpcDispatchDeps(deps: { approvalGate: c.approvalGate, suspendedAgents: c.suspendedAgents, hotAdd: g.hotAdd, hotRemove: g.hotRemove, diagnosticCollector: c.diagnosticCollector, billingEstimator: c.billingEstimator, + deadLetterQueue: c.deadLetterQueue, channelActivityTracker: c.channelActivityTracker, deliveryTracer: c.deliveryTracer, budgetGuards: c.budgetGuards, // Thread the LIVE spend snapshot the kill-switch enforces // (getSnapshot(), NOT the lagging SQL read) + the configured ceilings, so diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter-quarantine.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter-quarantine.ts new file mode 100644 index 000000000..84bde71a2 --- /dev/null +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter-quarantine.ts @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 +/** + * The quarantined-announcement operator projection. + * + * Split from `announcement-dead-letter.ts` (the `announcement-dead-letter-file.ts` + * discipline) to keep that file under the production line cap. PURE: the + * projection is a total function of the queue's two in-memory lists, so the + * operator view can never disagree with what a drain would act on. + * + * @module + */ + +import type { ChannelType, DeadLetterEntry, ParentDecisionReservationRecord } from "./announcement-dead-letter-file.js"; + +/** + * One quarantined announcement, as an operator sees it. + * + * Content-free by construction: the announcement's LENGTH is carried, never its + * text. These rows ride an admin RPC and a terminal, and the announcement is + * quarantined precisely because it was NOT delivered to its intended reader — + * an operator deciding its fate needs the route and the reason, not the message. + */ +export interface QuarantinedAnnouncement { + readonly id: string; + /** `entry` — a failed delivery. `parent_decision` — a parked adjudication. */ + readonly kind: "entry" | "parent_decision"; + readonly runId: string; + readonly agentId?: string; + readonly channelType: ChannelType; + readonly channelId: string; + readonly threadId?: string; + readonly failedAt: number; + readonly attemptCount: number; + readonly lastAttemptAt?: number; + /** Why it is parked (e.g. `outward_operation_unresolved`). */ + readonly lastError?: string; + readonly idempotencyKey?: string; + /** Size of the withheld announcement text, in characters. */ + readonly announcementChars: number; +} + +/** What an operator decided about a quarantined announcement. */ +export type QuarantineReleaseOutcome = "delivered" | "discarded"; + + +/** + * Project the live queue into the operator view: entries and parked + * parent-decision reservations in one list, oldest-first so the longest-stuck + * item leads, with the announcement's LENGTH standing in for its text. + */ +export function projectQuarantined( + entries: readonly DeadLetterEntry[], + reservations: readonly ParentDecisionReservationRecord[], +): readonly QuarantinedAnnouncement[] { + return [ + ...entries.map((entry): QuarantinedAnnouncement => ({ + id: entry.id, + kind: "entry" as const, + runId: entry.runId, + ...(entry.agentId === undefined ? {} : { agentId: entry.agentId }), + channelType: entry.channelType, + channelId: entry.channelId, + ...(entry.threadId === undefined ? {} : { threadId: entry.threadId }), + failedAt: entry.failedAt, + attemptCount: entry.attemptCount, + lastAttemptAt: entry.lastAttemptAt, + ...(entry.lastError === undefined ? {} : { lastError: entry.lastError }), + ...(entry.idempotencyKey === undefined ? {} : { idempotencyKey: entry.idempotencyKey }), + announcementChars: entry.announcementText.length, + })), + ...reservations.map((record): QuarantinedAnnouncement => ({ + id: record.id, + kind: "parent_decision" as const, + runId: record.runId, + agentId: record.agentId, + channelType: record.channelType, + channelId: record.channelId, + ...(record.threadId === undefined ? {} : { threadId: record.threadId }), + failedAt: record.failedAt, + // A reservation is parked awaiting adjudication, never retried, so it has + // no attempt history to report. + attemptCount: 0, + idempotencyKey: record.idempotencyKey, + announcementChars: record.announcementText.length, + })), + ].sort((left, right) => left.failedAt - right.failedAt || left.id.localeCompare(right.id)); +} diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts index 551afe1f6..b66abc935 100644 --- a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts @@ -28,6 +28,15 @@ export type { DeadLetterEntry, ParentDecisionReservation, } from "./announcement-dead-letter-file.js"; +import { projectQuarantined } from "./announcement-dead-letter-quarantine.js"; +import type { + QuarantinedAnnouncement, + QuarantineReleaseOutcome, +} from "./announcement-dead-letter-quarantine.js"; +export type { + QuarantinedAnnouncement, + QuarantineReleaseOutcome, +} from "./announcement-dead-letter-quarantine.js"; /** Minimal structural logger accepted from the daemon composition root. */ export interface AnnouncementLogger { @@ -37,36 +46,6 @@ export interface AnnouncementLogger { debug(obj: Record, msg: string): void; } -/** - * One quarantined announcement, as an operator sees it. - * - * Content-free by construction: the announcement's LENGTH is carried, never its - * text. These rows ride an admin RPC and a terminal, and the announcement is - * quarantined precisely because it was NOT delivered to its intended reader — - * an operator deciding its fate needs the route and the reason, not the message. - */ -export interface QuarantinedAnnouncement { - readonly id: string; - /** `entry` — a failed delivery. `parent_decision` — a parked adjudication. */ - readonly kind: "entry" | "parent_decision"; - readonly runId: string; - readonly agentId?: string; - readonly channelType: ChannelType; - readonly channelId: string; - readonly threadId?: string; - readonly failedAt: number; - readonly attemptCount: number; - readonly lastAttemptAt?: number; - /** Why it is parked (e.g. `outward_operation_unresolved`). */ - readonly lastError?: string; - readonly idempotencyKey?: string; - /** Size of the withheld announcement text, in characters. */ - readonly announcementChars: number; -} - -/** What an operator decided about a quarantined announcement. */ -export type QuarantineReleaseOutcome = "delivered" | "discarded"; - /** Dead-letter queue interface for announcement retry management. */ export interface AnnouncementDeadLetterQueue { /** @@ -1012,38 +991,7 @@ export function createAnnouncementDeadLetterQueue( drain: (sendToChannel, onDelivered) => serialize(() => drainSerialized(sendToChannel, onDelivered)), size: () => entries.length + decisionReservations.length, - listQuarantined: () => [ - ...entries.map((entry): QuarantinedAnnouncement => ({ - id: entry.id, - kind: "entry" as const, - runId: entry.runId, - ...(entry.agentId === undefined ? {} : { agentId: entry.agentId }), - channelType: entry.channelType, - channelId: entry.channelId, - ...(entry.threadId === undefined ? {} : { threadId: entry.threadId }), - failedAt: entry.failedAt, - attemptCount: entry.attemptCount, - lastAttemptAt: entry.lastAttemptAt, - ...(entry.lastError === undefined ? {} : { lastError: entry.lastError }), - ...(entry.idempotencyKey === undefined ? {} : { idempotencyKey: entry.idempotencyKey }), - announcementChars: entry.announcementText.length, - })), - ...decisionReservations.map((record): QuarantinedAnnouncement => ({ - id: record.id, - kind: "parent_decision" as const, - runId: record.runId, - agentId: record.agentId, - channelType: record.channelType, - channelId: record.channelId, - ...(record.threadId === undefined ? {} : { threadId: record.threadId }), - failedAt: record.failedAt, - // A reservation is parked awaiting adjudication, never retried, so it - // has no attempt history to report. - attemptCount: 0, - idempotencyKey: record.idempotencyKey, - announcementChars: record.announcementText.length, - })), - ].sort((left, right) => left.failedAt - right.failedAt || left.id.localeCompare(right.id)), + listQuarantined: () => projectQuarantined(entries, decisionReservations), release: (id, outcome) => serialize(() => releaseSerialized(id, outcome)), }; } diff --git a/packages/web/src/api/contracts.generated.json b/packages/web/src/api/contracts.generated.json index 786c2e906..ba0030f24 100644 --- a/packages/web/src/api/contracts.generated.json +++ b/packages/web/src/api/contracts.generated.json @@ -13299,6 +13299,85 @@ "admin" ] }, + "obs.quarantine.list": { + "request": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "properties": {}, + "additionalProperties": false + }, + "response": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "properties": { + "rows": { + "type": "array", + "items": { + "type": "object", + "propertyNames": { + "type": "string" + }, + "additionalProperties": {} + } + }, + "total": { + "type": "number" + } + }, + "required": [ + "rows", + "total" + ], + "additionalProperties": false + }, + "scopes": [ + "admin" + ] + }, + "obs.quarantine.release": { + "request": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "properties": { + "id": { + "type": "string", + "minLength": 1 + }, + "outcome": { + "type": "string", + "enum": [ + "delivered", + "discarded" + ] + } + }, + "required": [ + "id", + "outcome" + ], + "additionalProperties": false + }, + "response": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "properties": { + "released": { + "type": "boolean" + }, + "remaining": { + "type": "number" + } + }, + "required": [ + "released", + "remaining" + ], + "additionalProperties": false + }, + "scopes": [ + "admin" + ] + }, "obs.reset": { "request": { "$schema": "https://json-schema.org/draft/2020-12/schema", diff --git a/packages/web/src/api/contracts.generated.size.json b/packages/web/src/api/contracts.generated.size.json index 33e2bcea9..0f0b5798d 100644 --- a/packages/web/src/api/contracts.generated.size.json +++ b/packages/web/src/api/contracts.generated.size.json @@ -1,8 +1,8 @@ { - "totalMinified": 197513, - "totalGzipped": 20821, + "totalMinified": 198407, + "totalGzipped": 20896, "budget": { - "minified": 197750, + "minified": 198650, "gzipped": 38912 }, "overBudget": false, @@ -159,6 +159,8 @@ "obs.diagnostics": 596, "obs.explain": 23833, "obs.getCacheStats": 358, + "obs.quarantine.list": 401, + "obs.quarantine.release": 458, "obs.reset": 571, "obs.reset.table": 483, "obs.spend.snapshot": 397, diff --git a/packages/web/src/api/contracts.generated.ts b/packages/web/src/api/contracts.generated.ts index d79ab6ab3..914e74a55 100644 --- a/packages/web/src/api/contracts.generated.ts +++ b/packages/web/src/api/contracts.generated.ts @@ -13475,6 +13475,85 @@ export const CONTRACTS = { "admin" ] }, + "obs.quarantine.list": { + "request": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "properties": {}, + "additionalProperties": false + }, + "response": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "properties": { + "rows": { + "type": "array", + "items": { + "type": "object", + "propertyNames": { + "type": "string" + }, + "additionalProperties": {} + } + }, + "total": { + "type": "number" + } + }, + "required": [ + "rows", + "total" + ], + "additionalProperties": false + }, + "scopes": [ + "admin" + ] + }, + "obs.quarantine.release": { + "request": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "properties": { + "id": { + "type": "string", + "minLength": 1 + }, + "outcome": { + "type": "string", + "enum": [ + "delivered", + "discarded" + ] + } + }, + "required": [ + "id", + "outcome" + ], + "additionalProperties": false + }, + "response": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "properties": { + "released": { + "type": "boolean" + }, + "remaining": { + "type": "number" + } + }, + "required": [ + "released", + "remaining" + ], + "additionalProperties": false + }, + "scopes": [ + "admin" + ] + }, "obs.reset": { "request": { "$schema": "https://json-schema.org/draft/2020-12/schema", diff --git a/scripts/contracts/size-budget.ts b/scripts/contracts/size-budget.ts index 6cf814fc5..745d225a9 100644 --- a/scripts/contracts/size-budget.ts +++ b/scripts/contracts/size-budget.ts @@ -3,7 +3,7 @@ * Bundle-size measurement for `packages/web/src/api/contracts.generated.ts`. * * Budget: - * - 197,750 bytes minified (`BUDGET_MINIFIED_BYTES`) + * - 198,650 bytes minified (`BUDGET_MINIFIED_BYTES`) * - 38 KB gzipped (`BUDGET_GZIPPED_BYTES`) * * Measurement architecture: @@ -24,14 +24,20 @@ import { transformSync } from "esbuild"; import { gzipSync } from "node:zlib"; /** - * Budget: 197,750 bytes minified. + * Budget: 198,650 bytes minified. * * This cap permits bounded additive RPC-schema growth while keeping accidental * validator expansion visible in CI. Raise it only for reviewed contract * additions after confirming that the independently enforced gzipped wire-size * budget still has ample headroom. + * + * Raised from 197,750 for the two `obs.quarantine.*` contracts (859B combined): + * the operator lever over quarantined announcements. Gzipped sat at 20,896B + * against its 38,912B budget — 46% headroom — so the wire cost is unaffected. + * The ~240B of remaining minified headroom is deliberate: this cap is a ratchet, + * and keeping it close to the total is what makes unreviewed growth fail. */ -export const BUDGET_MINIFIED_BYTES = 197_750; +export const BUDGET_MINIFIED_BYTES = 198_650; /** Budget: 38 KB gzipped. */ export const BUDGET_GZIPPED_BYTES = 38_912; From d10890fda76fd8456f3cea59013bf5596c269c4f Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 18:09:41 +0300 Subject: [PATCH 21/36] fix(orchestrator): load from disk before listing quarantined announcements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `listQuarantined()` projected the in-memory lists, which are empty until some operation faults the file in — and the queue loads lazily, inside the serialized operations. A freshly-started daemon has not drained yet, so an operator running `comis quarantine list` right after a restart saw an empty queue while the JSONL held a stuck item. That is exactly the state the command exists to surface, so the failure landed on its primary use. Caught only by driving the deployed command against a real parked announcement: the unit tests all enqueued first, which loads as a side effect, so every one of them passed over the gap. The new test seeds the file through one queue instance and lists through a second, which is what a restart actually looks like. The method is now async and loads before projecting. A read failure warns and returns what is known rather than throwing, so a storage problem degrades the listing instead of hiding the command. --- .../src/api/obs-handlers/obs-quarantine.ts | 2 +- .../announcement-dead-letter.test.ts | 23 +++++++++++++-- .../cross-session/announcement-dead-letter.ts | 29 +++++++++++++++---- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/packages/daemon/src/api/obs-handlers/obs-quarantine.ts b/packages/daemon/src/api/obs-handlers/obs-quarantine.ts index 295af944f..2e33ab67c 100644 --- a/packages/daemon/src/api/obs-handlers/obs-quarantine.ts +++ b/packages/daemon/src/api/obs-handlers/obs-quarantine.ts @@ -37,7 +37,7 @@ export function bindObsQuarantineHandlers(deps: ObsHandlerDeps): Record { lastError: "outward_operation_unresolved", })); - const rows = queue.listQuarantined(); + const rows = await queue.listQuarantined(); expect(rows).toHaveLength(1); const row = rows[0]!; @@ -1734,15 +1734,32 @@ describe("AnnouncementDeadLetterQueue operator lever", () => { expect(JSON.stringify(row)).not.toContain("the answer the user never saw"); }); + it("lists entries written by a PREVIOUS process, before any drain has run", async () => { + // The queue loads from disk lazily, inside the serialized operations. A + // fresh daemon has not drained yet, so an operator running `list` right + // after a restart saw an empty queue while the JSONL held a stuck item — + // the exact state the command exists to surface. Reproduced live on + // comis-moshe against a real parked announcement. + const seeded = createAnnouncementDeadLetterQueue({ filePath, eventBus: createMockEventBus() }); + await seeded.enqueue(makeEntry({ runId: "run-from-a-previous-boot" })); + + // A brand-new queue over the same file: nothing has loaded it yet. + const fresh = createAnnouncementDeadLetterQueue({ filePath, eventBus: createMockEventBus() }); + const rows = await fresh.listQuarantined(); + + expect(rows).toHaveLength(1); + expect(rows[0]!.runId).toBe("run-from-a-previous-boot"); + }); + it("releases a quarantined announcement by id and persists the removal", async () => { const queue = createAnnouncementDeadLetterQueue({ filePath, eventBus: createMockEventBus() }); await queue.enqueue(makeEntry({ runId: "run-stuck" })); - const id = queue.listQuarantined()[0]!.id; + const id = (await queue.listQuarantined())[0]!.id; const released = await queue.release(id, "discarded"); expect(released).toMatchObject({ ok: true, value: true }); - expect(queue.listQuarantined()).toHaveLength(0); + expect(await queue.listQuarantined()).toHaveLength(0); expect(queue.size()).toBe(0); // Durable: a fresh queue over the same file must not resurrect it. const reloaded = createAnnouncementDeadLetterQueue({ filePath, eventBus: createMockEventBus() }); diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts index b66abc935..d762d0f15 100644 --- a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts @@ -86,11 +86,15 @@ export interface AnnouncementDeadLetterQueue { /** Return the current number of entries in the queue. */ size(): number; /** - * Every parked announcement, content-free, for operator review. Reads the - * in-memory state — which is authoritative, since a persist rewrites the file - * from it. Ordered oldest-first so the longest-stuck item leads. + * Every parked announcement, content-free, for operator review. Ordered + * oldest-first so the longest-stuck item leads. + * + * ASYNC because the queue loads from disk lazily: a freshly-started daemon + * has not drained yet, so a synchronous read of the in-memory lists reports + * an empty queue while the JSONL holds a stuck item — precisely the state an + * operator runs this to discover. It loads first, then projects. */ - listQuarantined(): readonly QuarantinedAnnouncement[]; + listQuarantined(): Promise; /** * Record an operator's decision about one parked announcement and drop it. * @@ -991,7 +995,22 @@ export function createAnnouncementDeadLetterQueue( drain: (sendToChannel, onDelivered) => serialize(() => drainSerialized(sendToChannel, onDelivered)), size: () => entries.length + decisionReservations.length, - listQuarantined: () => projectQuarantined(entries, decisionReservations), + listQuarantined: () => serialize(async () => { + // Load before projecting: the in-memory lists are empty until some + // operation has faulted the file in, and `list` is usually the FIRST + // thing an operator runs after a restart. + const loadedFromDisk = await loadFromDisk(); + if (!loadedFromDisk.ok) { + logger?.warn( + { + errorKind: "resource" as const, + hint: "restore dead-letter storage; the quarantine listing is incomplete", + }, + "Quarantined announcement listing could not read the dead-letter file", + ); + } + return projectQuarantined(entries, decisionReservations); + }), release: (id, outcome) => serialize(() => releaseSerialized(id, outcome)), }; } From d8f2fdc70f6e840f051f7f950d99bfa44da8b5b6 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 19:00:32 +0300 Subject: [PATCH 22/36] test(agent): pin the step-limit message to the knob that actually bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live: a deep-research sub-agent ran 18 searches and 21 fetches over 51 steps, hit the ceiling, and told the operator: "I stopped after 51 tool-execution steps because agents.default.maxSteps=50. Simplify the workflow or increase agents.default.maxSteps before retrying." `agents.default.maxSteps` defaults to 150 and does not govern sub-agents. The ceiling that bound was `security.agentToAgent.subAgentMaxSteps` (default 50, sub-agent-runner.ts:3037), or the caller's own `max_steps` if the spawn passed one. An operator following that guidance would raise a setting with no effect and hit the identical wall — `comis explain` repeats the same wrong key in its `execution_step_limit_reached` verdict. Naming the wrong knob is worse than naming none: it converts a one-line config fix into a hunt. RED: expected 'security.agentToAgent.subAgentMaxSteps', received 'agents.default.maxSteps'. --- .../src/bridge/bridge-safety-controls.test.ts | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/packages/agent/src/bridge/bridge-safety-controls.test.ts b/packages/agent/src/bridge/bridge-safety-controls.test.ts index ca65967a9..05695ff88 100644 --- a/packages/agent/src/bridge/bridge-safety-controls.test.ts +++ b/packages/agent/src/bridge/bridge-safety-controls.test.ts @@ -11,7 +11,8 @@ import { describe, it, expect, vi } from "vitest"; import type { SessionKey, TypedEventBus, ComisLogger } from "@comis/core"; -import { checkLoopLimit, emitLoopAbort, emitStepLimitAbort, buildAbortRedirectMessage, checkSpendLimit, emitSpendAbort } from "./bridge-safety-controls.js"; +import { checkLoopLimit, emitLoopAbort, emitStepLimitAbort, buildAbortRedirectMessage, checkSpendLimit, emitSpendAbort, resolveStepLimitDetails } from "./bridge-safety-controls.js"; +import { createStepCounter } from "../executor/step-counter.js"; import type { ExecutionPlan } from "../planner/types.js"; import type { SpendGateOutcome } from "../budget/budget-guard.js"; import { SpendError, type SpendWarn } from "../budget/spend-accumulator.js"; @@ -437,3 +438,43 @@ describe("emitSpendAbort", () => { expect((logger.warn as ReturnType).mock.calls[0][0].hint).toMatch(/observability\.spend/); }); }); + +describe("resolveStepLimitDetails — the knob it names must be the one that bound", () => { + // Live: a deep-research sub-agent ran 18 searches and 21 fetches, hit the + // ceiling at step 51, and told the operator: + // "I stopped after 51 tool-execution steps because agents.default.maxSteps=50. + // Simplify the workflow or increase agents.default.maxSteps before retrying." + // `agents.default.maxSteps` defaults to 150 and does not govern sub-agents at + // all. The ceiling that actually bound was + // `security.agentToAgent.subAgentMaxSteps` (default 50), so an operator who + // followed the guidance would raise a setting with no effect and hit the same + // wall. Naming the WRONG knob is worse than naming none. + it("names the sub-agent ceiling when a delegated run is what hit the limit", () => { + const counter = createStepCounter(50); + for (let i = 0; i < 50; i++) counter.increment(); + + const details = resolveStepLimitDetails(counter, "default", { + bindingKnob: "security.agentToAgent.subAgentMaxSteps", + }); + + expect(details.bindingKnob).toBe("security.agentToAgent.subAgentMaxSteps"); + expect(details.cap).toBe(50); + expect(details.stepsExecuted).toBe(50); + }); + + it("names the caller's own override when the spawn passed max_steps", () => { + const counter = createStepCounter(20); + const details = resolveStepLimitDetails(counter, "default", { + bindingKnob: "sessions_spawn(max_steps)", + }); + + expect(details.bindingKnob).toBe("sessions_spawn(max_steps)"); + }); + + it("still names the agent setting for an ordinary top-level turn", () => { + // Regression guard: the default path is unchanged. + const counter = createStepCounter(150); + expect(resolveStepLimitDetails(counter, "researcher").bindingKnob) + .toBe("agents.researcher.maxSteps"); + }); +}); From 5e764a10da174c0bbd8e2f8ff02fc5164e97d9cc Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 19:09:48 +0300 Subject: [PATCH 23/36] fix(agent,daemon): name the step ceiling that actually bound the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A halted deep-research sub-agent told its operator to raise `agents.default.maxSteps`. That key defaults to 150 and does not govern sub-agents; `comis explain` repeated it in the `execution_step_limit_reached` verdict. The ceiling that bound was `security.agentToAgent.subAgentMaxSteps` (default 50). Following the guidance would have changed a setting with no effect and hit the same wall — the run had already spent $1.15 and 1.1M tokens across 18 searches and 21 fetches, and returned nothing. The second hint was wrong in the same way: "Increase max_steps in sessions_spawn" points at a parameter that setup-cross-session-graph.ts:83 clamps with `Math.min(maxSteps, configMaxSteps)`. A caller can only ever LOWER the ceiling, so raising it is silently ignored. Both messages sent operators somewhere that could not work. The knob now travels with the counter, mirroring `describeTimeoutKnob`, which already threads a timeout's source for this exact reason — only the creator knows which ceiling won. Sub-agent spawns label it `sessions_spawn(max_steps)` when the caller's value is the lower of the two and therefore binding, `security.agentToAgent.subAgentMaxSteps` otherwise; top-level turns keep `agents..maxSteps`, now passed explicitly rather than inferred from the agent id. The abort hint names the config key and states the clamp. The reference docs described the value as a "Default", which reads as raisable per spawn; they now say it is a ceiling, that `max_steps` clamps to it, that research-style delegations will hit 50, and that it is not `agents..maxSteps`. The four wiring tests that pinned the step count now pin the knob with it, so the provenance cannot regress silently. `deadLetterQueue` also gains its required AUDIT-observability.md row. --- docs/reference/config-yaml.mdx | 2 +- docs/reference/security-model.mdx | 2 +- .../src/bridge/bridge-safety-controls.ts | 21 ++++++++++++++++++- packages/agent/src/executor/step-counter.ts | 18 +++++++++++++++- .../src/spawn/sub-agent-result-processor.ts | 7 ++++++- packages/daemon/AUDIT-observability.md | 1 + .../setup-agents/setup-agents-runtime.ts | 2 +- .../setup-cross-session-graph.ts | 9 +++++++- .../setup-cross-session-runtime.test.ts | 8 +++---- 9 files changed, 59 insertions(+), 11 deletions(-) diff --git a/docs/reference/config-yaml.mdx b/docs/reference/config-yaml.mdx index 8289a9836..1755ab3f8 100644 --- a/docs/reference/config-yaml.mdx +++ b/docs/reference/config-yaml.mdx @@ -1582,7 +1582,7 @@ Security configuration for log redaction, audit logging, permissions, action con | `allowAgents` | `string[]` | `[]` | Allowed agent IDs for sub-agents (empty = all) | | `subAgentRetentionMs` | `number` | `3600000` | Retention for completed sub-agent sessions (1 hour) | | `waitTimeoutMs` | `number` | `60000` | Default timeout for wait mode (60 seconds) | -| `subAgentMaxSteps` | `number` | `50` | Default max steps for sub-agent execution | +| `subAgentMaxSteps` | `number` | `50` | Ceiling on tool-execution steps for a sub-agent run. A spawn's own `max_steps` is **clamped to this value** and cannot exceed it, so this is the only setting that raises the limit. Research-style delegations that search and fetch repeatedly will hit 50 — raise it for those deployments. Not to be confused with `agents..maxSteps`, which governs top-level turns only | | `subAgentToolGroups` | `enum[]` | `["coding"]` | Default tool profile groups: `minimal`, `coding`, `messaging`, `supervisor`, `full` | | `subAgentMcpTools` | `enum` | `"inherit"` | MCP tool inheritance: `inherit` or `none` | | `tokenBudget` | `number \| null` | `null` | Per-spawn token budget for graph sub-agents. `null` (default) means inherit the graph share -- `budget.max_tokens` divided by the total node count -- but **only when the graph sets a token budget**; otherwise sub-agents are unbounded (today's behavior, unchanged). A positive integer caps every graph node's sub-agent at that many tokens. A graph node's own per-node budget overrides this default. A breach fails that node (honoring the graph's `on_failure`). See [Execution Graphs: Token budgets](/agents/execution-graphs#token-budgets). | diff --git a/docs/reference/security-model.mdx b/docs/reference/security-model.mdx index 111af4b36..0b9a6db6e 100644 --- a/docs/reference/security-model.mdx +++ b/docs/reference/security-model.mdx @@ -881,7 +881,7 @@ Top-level `security` configuration. | `agentToAgent.allowAgents` | `string[]` | `[]` | Allowed agent IDs for sub-agent spawning (empty = allow all) | | `agentToAgent.subAgentRetentionMs` | `number` | `3600000` (1 hour) | Retention period for completed sub-agent sessions | | `agentToAgent.waitTimeoutMs` | `number` | `60000` (60s) | Default timeout for wait mode | -| `agentToAgent.subAgentMaxSteps` | `number` | `50` | Default max steps for sub-agent execution | +| `agentToAgent.subAgentMaxSteps` | `number` | `50` | Ceiling on tool-execution steps for a sub-agent run. A spawn's own `max_steps` is **clamped to this value** and cannot exceed it, so this is the only setting that raises the limit. Research-style delegations that search and fetch repeatedly will hit 50 — raise it for those deployments. Not to be confused with `agents..maxSteps`, which governs top-level turns only | | `agentToAgent.subAgentToolGroups` | `string[]` | `["coding"]` | Default tool profile groups for sub-agents | | `agentToAgent.subAgentMcpTools` | `"inherit"` \| `"none"` | `"inherit"` | MCP tool inheritance policy for sub-agents | diff --git a/packages/agent/src/bridge/bridge-safety-controls.ts b/packages/agent/src/bridge/bridge-safety-controls.ts index 1ce1a3746..a6b752598 100644 --- a/packages/agent/src/bridge/bridge-safety-controls.ts +++ b/packages/agent/src/bridge/bridge-safety-controls.ts @@ -46,12 +46,31 @@ export interface AbortRedirectDetails { stepLimit?: StepLimitDetails; } +/** + * Where a run's step ceiling came from, when it is not the agent's own setting. + * + * A delegated run is governed by `security.agentToAgent.subAgentMaxSteps` (or by + * the `max_steps` its caller passed), NOT by `agents..maxSteps` — those are + * different keys with different defaults. Naming the agent setting for a + * sub-agent sends an operator to raise a knob that has no effect on the run that + * stopped, so the provenance travels with the counter rather than being guessed + * from the agent id. Mirrors `describeTimeoutKnob`, which already threads the + * timeout's source for exactly this reason. + */ +export interface StepLimitProvenance { + /** The config key or tool parameter that set the ceiling. */ + readonly bindingKnob: string; +} + export function resolveStepLimitDetails( stepCounter: StepCounter, agentId: string, + provenance?: StepLimitProvenance, ): StepLimitDetails { return { - bindingKnob: `agents.${agentId}.maxSteps`, + bindingKnob: provenance?.bindingKnob + ?? stepCounter.getBindingKnob?.() + ?? `agents.${agentId}.maxSteps`, stepsExecuted: stepCounter.getCount(), cap: stepCounter.getLimit?.() ?? stepCounter.getCount(), }; diff --git a/packages/agent/src/executor/step-counter.ts b/packages/agent/src/executor/step-counter.ts index 22e2376f9..2c7d270fd 100644 --- a/packages/agent/src/executor/step-counter.ts +++ b/packages/agent/src/executor/step-counter.ts @@ -11,6 +11,15 @@ export interface StepCounter { getCount(): number; /** Return the configured step ceiling. */ getLimit?(): number; + /** + * The config key or tool parameter that set this ceiling. + * + * Travels with the counter because only its creator knows which knob won: a + * delegated run is bounded by `security.agentToAgent.subAgentMaxSteps`, a + * top-level turn by `agents..maxSteps`. Guessing from the agent id names + * the wrong key and sends operators to a setting with no effect. + */ + getBindingKnob?(): string; } /** Default maximum steps if not specified */ @@ -24,8 +33,13 @@ const DEFAULT_MAX_STEPS = 50; * should stop processing. * * @param maxSteps - Maximum allowed steps before halting (default: 50) + * @param bindingKnob - The config key/parameter that set `maxSteps`, reported + * verbatim when the ceiling stops a run. */ -export function createStepCounter(maxSteps: number = DEFAULT_MAX_STEPS): StepCounter { +export function createStepCounter( + maxSteps: number = DEFAULT_MAX_STEPS, + bindingKnob?: string, +): StepCounter { let count = 0; return { @@ -42,6 +56,8 @@ export function createStepCounter(maxSteps: number = DEFAULT_MAX_STEPS): StepCou count = 0; }, + ...(bindingKnob === undefined ? {} : { getBindingKnob: (): string => bindingKnob }), + getCount(): number { return count; }, diff --git a/packages/agent/src/spawn/sub-agent-result-processor.ts b/packages/agent/src/spawn/sub-agent-result-processor.ts index 04dd44083..6cdf71561 100644 --- a/packages/agent/src/spawn/sub-agent-result-processor.ts +++ b/packages/agent/src/spawn/sub-agent-result-processor.ts @@ -110,7 +110,12 @@ export function classifyAbortReason( case "max_steps": return { category: "step_limit", - hint: "Increase max_steps in sessions_spawn or simplify the task", + // `max_steps` is clamped to security.agentToAgent.subAgentMaxSteps at + // spawn, so recommending it alone sends a caller to a parameter that + // cannot raise the ceiling it just hit. Name the config key that can. + hint: + "Raise security.agentToAgent.subAgentMaxSteps (a spawn's own max_steps is " + + "clamped to it and cannot exceed it), or simplify the task", severity: "actionable", }; case "loop_detected": diff --git a/packages/daemon/AUDIT-observability.md b/packages/daemon/AUDIT-observability.md index b0a70a998..a909dd23f 100644 --- a/packages/daemon/AUDIT-observability.md +++ b/packages/daemon/AUDIT-observability.md @@ -18,6 +18,7 @@ The table below uses a tight Markdown shape — `| | { const sessionKey = { channelId: "chan-1", userId: "user-1", tenantId: "t-1" }; await executeAgent("agent-2", sessionKey, "task", 10); - expect(mockCreateStepCounter).toHaveBeenCalledWith(MIN_SUB_AGENT_STEPS); + expect(mockCreateStepCounter).toHaveBeenCalledWith(MIN_SUB_AGENT_STEPS, "sessions_spawn(max_steps)"); }); it("preserves max_steps at or above MIN_SUB_AGENT_STEPS", async () => { @@ -1700,7 +1700,7 @@ describe("setupCrossSession", () => { const sessionKey = { channelId: "chan-1", userId: "user-1", tenantId: "t-1" }; await executeAgent("agent-2", sessionKey, "task", 40); - expect(mockCreateStepCounter).toHaveBeenCalledWith(40); + expect(mockCreateStepCounter).toHaveBeenCalledWith(40, "sessions_spawn(max_steps)"); }); it("uses config default when no max_steps provided (still subject to floor)", async () => { @@ -1727,7 +1727,7 @@ describe("setupCrossSession", () => { await executeAgent("agent-2", sessionKey, "task"); // Config default is 50, which is above floor of 30 - expect(mockCreateStepCounter).toHaveBeenCalledWith(50); + expect(mockCreateStepCounter).toHaveBeenCalledWith(50, "security.agentToAgent.subAgentMaxSteps"); }); it("floor applies even when config default is below MIN_SUB_AGENT_STEPS", async () => { @@ -1774,7 +1774,7 @@ describe("setupCrossSession", () => { await executeAgent("agent-2", sessionKey, "task"); // Config default is 15, below floor -- should be raised to MIN_SUB_AGENT_STEPS (30) - expect(mockCreateStepCounter).toHaveBeenCalledWith(MIN_SUB_AGENT_STEPS); + expect(mockCreateStepCounter).toHaveBeenCalledWith(MIN_SUB_AGENT_STEPS, "security.agentToAgent.subAgentMaxSteps"); }); }); From a07f4a9c95dc48a3687f04eb766919e5e0879f7a Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 19:12:36 +0300 Subject: [PATCH 24/36] test(core): pin a sub-agent step ceiling that research can finish under RED: expected 300, received 50. A spawn's own `max_steps` is clamped to security.agentToAgent.subAgentMaxSteps, so this schema default is the only thing that sets the reachable ceiling for a default deployment. At 50 a research delegation cannot finish: a live run spent 18 web_search and 21 web_fetch calls, hit the limit at step 51, and returned nothing after $1.15 and 1.1M tokens. --- .../core/src/config/schema-security.test.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/core/src/config/schema-security.test.ts b/packages/core/src/config/schema-security.test.ts index 5ca7062d0..1c0758e20 100644 --- a/packages/core/src/config/schema-security.test.ts +++ b/packages/core/src/config/schema-security.test.ts @@ -340,3 +340,27 @@ describe("AgentToAgentConfigSchema.steerInject (gated-off by default)", () => { expect(registrySrc).not.toMatch(/steerInject/); }); }); + +// --------------------------------------------------------------------------- +// AgentToAgentConfigSchema.subAgentMaxSteps — the sub-agent step ceiling. +// --------------------------------------------------------------------------- +describe("AgentToAgentConfigSchema.subAgentMaxSteps", () => { + // The previous default of 50 could not carry a research-style delegation: a + // live run spent 18 web_search + 21 web_fetch calls, hit the ceiling at step + // 51, and returned nothing after $1.15 and 1.1M tokens. A spawn's own + // `max_steps` is clamped to this value, so this schema default is the only + // thing that sets the reachable ceiling for a default deployment. + it("defaults to a ceiling that can carry a multi-source research delegation", () => { + const result = AgentToAgentConfigSchema.safeParse({}); + expect(result.success).toBe(true); + if (result.success) expect(result.data.subAgentMaxSteps).toBe(300); + }); + + it("still accepts an explicit operator value in either direction", () => { + for (const value of [25, 1000]) { + const result = AgentToAgentConfigSchema.safeParse({ subAgentMaxSteps: value }); + expect(result.success).toBe(true); + if (result.success) expect(result.data.subAgentMaxSteps).toBe(value); + } + }); +}); From 19d4f1579721a48ff967bcfd534bc52d1325c621 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 19:18:44 +0300 Subject: [PATCH 25/36] feat(core): raise the sub-agent step ceiling default to 300 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `security.agentToAgent.subAgentMaxSteps` defaulted to 50, and a spawn's own `max_steps` is clamped to it, so 50 was the reachable ceiling for any default deployment. Delegated research does not fit in it: a live run spent 18 web_search and 21 web_fetch calls, hit the limit at step 51, and returned nothing after $1.15 and 1.1M tokens. The work was well-formed — the ceiling was simply sized for single-answer delegations. 300 is sized for the step-hungry case. The ceiling exists to bound runaway loops, not to control cost — `observability.spend` and the per-spawn token budget do that, and both still apply — so it belongs where honest work fits rather than at the cheapest value that usually suffices. Deployments wanting a tighter loop bound can lower it; the value was always operator -settable and still is. Raises the reachable ceiling for every deployment by 6x. That is the intended effect: at 50 the runtime silently could not complete a whole class of delegation, and the two hints it offered both pointed at knobs that could not raise it (fixed in the preceding commit). Snapshot, docs (config-yaml, security-model, subagent-lifecycle), and the two assertions that pinned 50 all move with it. The docs also stop describing the value as a "Default" that a spawn can raise, and now say it is a clamp. --- docs/agents/subagent-lifecycle.mdx | 2 +- docs/reference/config-yaml.mdx | 2 +- docs/reference/security-model.mdx | 2 +- .../section-registry-parity.test.ts.snap | 20 +++++++++---------- packages/core/src/config/schema-security.ts | 15 ++++++++++++-- .../src/event-bus/events-messaging.test.ts | 2 +- 6 files changed, 27 insertions(+), 16 deletions(-) diff --git a/docs/agents/subagent-lifecycle.mdx b/docs/agents/subagent-lifecycle.mdx index 39b3b1d59..397d7cd25 100644 --- a/docs/agents/subagent-lifecycle.mdx +++ b/docs/agents/subagent-lifecycle.mdx @@ -287,7 +287,7 @@ security: agentToAgent: enabled: true allowAgents: ["coder", "researcher"] - subAgentMaxSteps: 50 + subAgentMaxSteps: 300 subAgentToolGroups: ["coding"] subagentContext: # -- Spawn limits -- diff --git a/docs/reference/config-yaml.mdx b/docs/reference/config-yaml.mdx index 1755ab3f8..1415596df 100644 --- a/docs/reference/config-yaml.mdx +++ b/docs/reference/config-yaml.mdx @@ -1582,7 +1582,7 @@ Security configuration for log redaction, audit logging, permissions, action con | `allowAgents` | `string[]` | `[]` | Allowed agent IDs for sub-agents (empty = all) | | `subAgentRetentionMs` | `number` | `3600000` | Retention for completed sub-agent sessions (1 hour) | | `waitTimeoutMs` | `number` | `60000` | Default timeout for wait mode (60 seconds) | -| `subAgentMaxSteps` | `number` | `50` | Ceiling on tool-execution steps for a sub-agent run. A spawn's own `max_steps` is **clamped to this value** and cannot exceed it, so this is the only setting that raises the limit. Research-style delegations that search and fetch repeatedly will hit 50 — raise it for those deployments. Not to be confused with `agents..maxSteps`, which governs top-level turns only | +| `subAgentMaxSteps` | `number` | `300` | Ceiling on tool-execution steps for a sub-agent run. A spawn's own `max_steps` is **clamped to this value** and cannot exceed it, so this is the only setting that raises the limit. Sized for delegated research, which spends a step per search and per fetch; lower it to tighten the runaway-loop bound. This is not a cost control — see `observability.spend` and the per-spawn token budget. Not to be confused with `agents..maxSteps`, which governs top-level turns only | | `subAgentToolGroups` | `enum[]` | `["coding"]` | Default tool profile groups: `minimal`, `coding`, `messaging`, `supervisor`, `full` | | `subAgentMcpTools` | `enum` | `"inherit"` | MCP tool inheritance: `inherit` or `none` | | `tokenBudget` | `number \| null` | `null` | Per-spawn token budget for graph sub-agents. `null` (default) means inherit the graph share -- `budget.max_tokens` divided by the total node count -- but **only when the graph sets a token budget**; otherwise sub-agents are unbounded (today's behavior, unchanged). A positive integer caps every graph node's sub-agent at that many tokens. A graph node's own per-node budget overrides this default. A breach fails that node (honoring the graph's `on_failure`). See [Execution Graphs: Token budgets](/agents/execution-graphs#token-budgets). | diff --git a/docs/reference/security-model.mdx b/docs/reference/security-model.mdx index 0b9a6db6e..39e71d6d4 100644 --- a/docs/reference/security-model.mdx +++ b/docs/reference/security-model.mdx @@ -881,7 +881,7 @@ Top-level `security` configuration. | `agentToAgent.allowAgents` | `string[]` | `[]` | Allowed agent IDs for sub-agent spawning (empty = allow all) | | `agentToAgent.subAgentRetentionMs` | `number` | `3600000` (1 hour) | Retention period for completed sub-agent sessions | | `agentToAgent.waitTimeoutMs` | `number` | `60000` (60s) | Default timeout for wait mode | -| `agentToAgent.subAgentMaxSteps` | `number` | `50` | Ceiling on tool-execution steps for a sub-agent run. A spawn's own `max_steps` is **clamped to this value** and cannot exceed it, so this is the only setting that raises the limit. Research-style delegations that search and fetch repeatedly will hit 50 — raise it for those deployments. Not to be confused with `agents..maxSteps`, which governs top-level turns only | +| `agentToAgent.subAgentMaxSteps` | `number` | `300` | Ceiling on tool-execution steps for a sub-agent run. A spawn's own `max_steps` is **clamped to this value** and cannot exceed it, so this is the only setting that raises the limit. Sized for delegated research, which spends a step per search and per fetch; lower it to tighten the runaway-loop bound. This is not a cost control — see `observability.spend` and the per-spawn token budget. Not to be confused with `agents..maxSteps`, which governs top-level turns only | | `agentToAgent.subAgentToolGroups` | `string[]` | `["coding"]` | Default tool profile groups for sub-agents | | `agentToAgent.subAgentMcpTools` | `"inherit"` \| `"none"` | `"inherit"` | MCP tool inheritance policy for sub-agents | diff --git a/packages/core/src/config/__snapshots__/section-registry-parity.test.ts.snap b/packages/core/src/config/__snapshots__/section-registry-parity.test.ts.snap index 5936dd778..61774b8c1 100644 --- a/packages/core/src/config/__snapshots__/section-registry-parity.test.ts.snap +++ b/packages/core/src/config/__snapshots__/section-registry-parity.test.ts.snap @@ -7822,7 +7822,7 @@ exports[`section-registry parity > field-metadata view > getFieldMetadata("secur "maxPingPongTurns": 3, "sandboxNoDowngrade": true, "steerInject": false, - "subAgentMaxSteps": 50, + "subAgentMaxSteps": 300, "subAgentMcpTools": "inherit", "subAgentRetentionMs": 3600000, "subAgentSessionPersistence": true, @@ -7917,7 +7917,7 @@ exports[`section-registry parity > field-metadata view > getFieldMetadata("secur "type": "boolean" }, { - "default": 50, + "default": 300, "immutable": true, "path": "security.agentToAgent.subAgentMaxSteps", "type": "integer" @@ -14803,7 +14803,7 @@ exports[`section-registry parity > field-metadata view > getFieldMetadata() — "maxPingPongTurns": 3, "sandboxNoDowngrade": true, "steerInject": false, - "subAgentMaxSteps": 50, + "subAgentMaxSteps": 300, "subAgentMcpTools": "inherit", "subAgentRetentionMs": 3600000, "subAgentSessionPersistence": true, @@ -14886,7 +14886,7 @@ exports[`section-registry parity > field-metadata view > getFieldMetadata() — "maxPingPongTurns": 3, "sandboxNoDowngrade": true, "steerInject": false, - "subAgentMaxSteps": 50, + "subAgentMaxSteps": 300, "subAgentMcpTools": "inherit", "subAgentRetentionMs": 3600000, "subAgentSessionPersistence": true, @@ -14981,7 +14981,7 @@ exports[`section-registry parity > field-metadata view > getFieldMetadata() — "type": "boolean" }, { - "default": 50, + "default": 300, "immutable": true, "path": "security.agentToAgent.subAgentMaxSteps", "type": "integer" @@ -26442,7 +26442,7 @@ exports[`section-registry parity > schema-serializer view > getConfigSchema("sec "maxPingPongTurns": 3, "sandboxNoDowngrade": true, "steerInject": false, - "subAgentMaxSteps": 50, + "subAgentMaxSteps": 300, "subAgentMcpTools": "inherit", "subAgentRetentionMs": 3600000, "subAgentSessionPersistence": true, @@ -26534,7 +26534,7 @@ exports[`section-registry parity > schema-serializer view > getConfigSchema("sec "type": "boolean" }, "subAgentMaxSteps": { - "default": 50, + "default": 300, "exclusiveMinimum": 0, "maximum": 9007199254740991, "type": "integer" @@ -40220,7 +40220,7 @@ exports[`section-registry parity > schema-serializer view > getConfigSchema() "maxPingPongTurns": 3, "sandboxNoDowngrade": true, "steerInject": false, - "subAgentMaxSteps": 50, + "subAgentMaxSteps": 300, "subAgentMcpTools": "inherit", "subAgentRetentionMs": 3600000, "subAgentSessionPersistence": true, @@ -40304,7 +40304,7 @@ exports[`section-registry parity > schema-serializer view > getConfigSchema() "maxPingPongTurns": 3, "sandboxNoDowngrade": true, "steerInject": false, - "subAgentMaxSteps": 50, + "subAgentMaxSteps": 300, "subAgentMcpTools": "inherit", "subAgentRetentionMs": 3600000, "subAgentSessionPersistence": true, @@ -40396,7 +40396,7 @@ exports[`section-registry parity > schema-serializer view > getConfigSchema() "type": "boolean" }, "subAgentMaxSteps": { - "default": 50, + "default": 300, "exclusiveMinimum": 0, "maximum": 9007199254740991, "type": "integer" diff --git a/packages/core/src/config/schema-security.ts b/packages/core/src/config/schema-security.ts index cce597cce..98d636b22 100644 --- a/packages/core/src/config/schema-security.ts +++ b/packages/core/src/config/schema-security.ts @@ -49,8 +49,19 @@ const AgentToAgentBaseSchema = z.strictObject({ subAgentRetentionMs: z.number().int().positive().default(3_600_000), /** Default timeout for wait mode in ms (default 60 seconds) */ waitTimeoutMs: z.number().int().positive().default(60_000), - /** Default max steps for sub-agent execution (hard cap per-spawn overrides cannot exceed) */ - subAgentMaxSteps: z.number().int().positive().default(50), + /** + * Tool-execution step ceiling for a sub-agent run, and a hard cap: a spawn's + * own `max_steps` is clamped to this value and can only lower it. + * + * 300 because delegated research is step-hungry in a way single-answer work + * is not — a multi-source investigation spends a step per search and per + * fetch, and the previous ceiling of 50 stopped one mid-flight after 18 + * searches and 21 fetches, discarding the whole run. This bounds runaway + * loops; it is not a cost control (`observability.spend` and the token + * budget are), so it is set where honest work fits rather than at the + * cheapest value that usually suffices. + */ + subAgentMaxSteps: z.number().int().positive().default(300), /** Default tool profile groups for sub-agent tool assembly */ subAgentToolGroups: z.array(z.enum(["minimal", "coding", "messaging", "supervisor", "full"])).default(["coding"]), /** MCP tool inheritance policy for sub-agents: "inherit" passes MCP tools, "none" excludes them */ diff --git a/packages/core/src/event-bus/events-messaging.test.ts b/packages/core/src/event-bus/events-messaging.test.ts index 3a1f47baf..735568571 100644 --- a/packages/core/src/event-bus/events-messaging.test.ts +++ b/packages/core/src/event-bus/events-messaging.test.ts @@ -975,7 +975,7 @@ describe("Config nesting integration", () => { const config = SecurityConfigSchema.parse({}); expect(config.agentToAgent.enabled).toBe(true); expect(config.agentToAgent.maxPingPongTurns).toBe(3); - expect(config.agentToAgent.subAgentMaxSteps).toBe(50); + expect(config.agentToAgent.subAgentMaxSteps).toBe(300); expect(config.agentToAgent.subAgentMcpTools).toBe("inherit"); }); From 656ecd8321b8dc538436b0bca7bc438c54439c50 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 19:33:19 +0300 Subject: [PATCH 26/36] refactor(orchestrator,agent): keep two files under the production line cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pnpm validate` failed the file-size gate: announcement-dead-letter.ts (1017) and sub-agent-result-processor.ts (1005) both crossed the 1000-line cap as the quarantine work and the step-limit hint landed. Allowlists are shrink-only, so both shrink. The release logic moves to announcement-dead-letter-quarantine.ts, which is where it belonged — that sibling already holds the operator projection, and `releaseQuarantined` is the operator mutation. It takes the two lists and an injected `persist` that commits both the write and the in-memory swap, so the ordering guarantee is unchanged: persist first, mutate only on success, leaving a failed write with the announcement still parked. The step-limit hint loses its explanatory comment; the string states the clamp itself, so the comment repeated it. No behavior change — 2645 orchestrator/spawn/obs tests and all 935 architecture gates pass. --- .../src/spawn/sub-agent-result-processor.ts | 7 +-- .../announcement-dead-letter-quarantine.ts | 56 +++++++++++++++++ .../cross-session/announcement-dead-letter.ts | 61 +++++-------------- 3 files changed, 73 insertions(+), 51 deletions(-) diff --git a/packages/agent/src/spawn/sub-agent-result-processor.ts b/packages/agent/src/spawn/sub-agent-result-processor.ts index 6cdf71561..f057fcefa 100644 --- a/packages/agent/src/spawn/sub-agent-result-processor.ts +++ b/packages/agent/src/spawn/sub-agent-result-processor.ts @@ -110,12 +110,7 @@ export function classifyAbortReason( case "max_steps": return { category: "step_limit", - // `max_steps` is clamped to security.agentToAgent.subAgentMaxSteps at - // spawn, so recommending it alone sends a caller to a parameter that - // cannot raise the ceiling it just hit. Name the config key that can. - hint: - "Raise security.agentToAgent.subAgentMaxSteps (a spawn's own max_steps is " - + "clamped to it and cannot exceed it), or simplify the task", + hint: "Raise security.agentToAgent.subAgentMaxSteps (a spawn's own max_steps is clamped to it), or simplify the task", severity: "actionable", }; case "loop_detected": diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter-quarantine.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter-quarantine.ts index 84bde71a2..059426240 100644 --- a/packages/orchestrator/src/cross-session/announcement-dead-letter-quarantine.ts +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter-quarantine.ts @@ -10,6 +10,7 @@ * @module */ +import { ok, type Result } from "@comis/shared"; import type { ChannelType, DeadLetterEntry, ParentDecisionReservationRecord } from "./announcement-dead-letter-file.js"; /** @@ -85,3 +86,58 @@ export function projectQuarantined( })), ].sort((left, right) => left.failedAt - right.failedAt || left.id.localeCompare(right.id)); } + +/** Minimal structural logger accepted by the release path. */ +interface QuarantineLogger { + info(obj: Record, msg: string): void; + error(obj: Record, msg: string): void; +} + +/** + * Apply an operator decision to one parked announcement. + * + * Persists BEFORE the caller mutates its in-memory state (the injected + * `persist` commits both), so a storage failure leaves the announcement parked + * rather than dropping an undelivered message on a bad write. An unknown id + * resolves `false` rather than failing — releasing the same id twice is an + * operator retrying, not an error. + */ +export async function releaseQuarantined(input: { + readonly id: string; + readonly outcome: QuarantineReleaseOutcome; + readonly entries: readonly DeadLetterEntry[]; + readonly reservations: readonly ParentDecisionReservationRecord[]; + readonly logger?: QuarantineLogger; + readonly persist: ( + entries: readonly DeadLetterEntry[], + reservations: readonly ParentDecisionReservationRecord[], + ) => Promise>; +}): Promise> { + const entry = input.entries.find((candidate) => candidate.id === input.id); + const reservation = input.reservations.find((candidate) => candidate.id === input.id); + if (entry === undefined && reservation === undefined) return ok(false); + + const nextEntries = input.entries.filter((candidate) => candidate.id !== input.id); + const nextReservations = input.reservations.filter((candidate) => candidate.id !== input.id); + const persisted = await input.persist(nextEntries, nextReservations); + if (!persisted.ok) { + input.logger?.error( + { + errorKind: "resource" as const, + hint: "restore dead-letter storage before releasing; the announcement is still quarantined", + }, + "Quarantined announcement release was not durably persisted", + ); + return persisted; + } + input.logger?.info( + { + runId: entry?.runId ?? reservation?.runId, + kind: entry !== undefined ? "entry" : "parent_decision", + outcome: input.outcome, + remaining: nextEntries.length + nextReservations.length, + }, + "Quarantined announcement released by operator decision", + ); + return ok(true); +} diff --git a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts index d762d0f15..9e29803e8 100644 --- a/packages/orchestrator/src/cross-session/announcement-dead-letter.ts +++ b/packages/orchestrator/src/cross-session/announcement-dead-letter.ts @@ -28,7 +28,7 @@ export type { DeadLetterEntry, ParentDecisionReservation, } from "./announcement-dead-letter-file.js"; -import { projectQuarantined } from "./announcement-dead-letter-quarantine.js"; +import { projectQuarantined, releaseQuarantined } from "./announcement-dead-letter-quarantine.js"; import type { QuarantinedAnnouncement, QuarantineReleaseOutcome, @@ -942,49 +942,6 @@ export function createAnnouncementDeadLetterQueue( } } - /** - * Apply an operator decision. Serialized with the drain so a release cannot - * interleave with a sweep and resurrect the item from a stale snapshot. - */ - async function releaseSerialized( - id: string, - outcome: QuarantineReleaseOutcome, - ): Promise> { - const loaded = await loadFromDisk(); - if (!loaded.ok) return loaded; - const entry = entries.find((candidate) => candidate.id === id); - const reservation = decisionReservations.find((candidate) => candidate.id === id); - if (entry === undefined && reservation === undefined) return ok(false); - - const nextEntries = entries.filter((candidate) => candidate.id !== id); - const nextReservations = decisionReservations.filter((candidate) => candidate.id !== id); - // Persist BEFORE mutating memory: a failed write must leave the item parked - // rather than dropping an undelivered announcement on a storage error. - const persisted = await persist(nextEntries, nextReservations); - if (!persisted.ok) { - logger?.error( - { - errorKind: "resource" as const, - hint: "restore dead-letter storage before releasing; the announcement is still quarantined", - }, - "Quarantined announcement release was not durably persisted", - ); - return persisted; - } - entries = nextEntries; - decisionReservations = nextReservations; - logger?.info( - { - runId: entry?.runId ?? reservation?.runId, - kind: entry !== undefined ? "entry" : "parent_decision", - outcome, - remaining: entries.length + decisionReservations.length, - }, - "Quarantined announcement released by operator decision", - ); - return ok(true); - } - return { enqueue: (entry) => serialize(() => enqueueDurably(entry)), reserveDecision: (entry) => serialize(() => decisionStore.reserve(entry)), @@ -1011,6 +968,20 @@ export function createAnnouncementDeadLetterQueue( } return projectQuarantined(entries, decisionReservations); }), - release: (id, outcome) => serialize(() => releaseSerialized(id, outcome)), + release: (id, outcome) => serialize(async () => { + const loaded = await loadFromDisk(); + if (!loaded.ok) return loaded; + return releaseQuarantined({ + id, outcome, entries, reservations: decisionReservations, logger, + persist: async (nextEntries, nextReservations) => { + const written = await persist(nextEntries, nextReservations); + if (written.ok) { + entries = [...nextEntries]; + decisionReservations = [...nextReservations]; + } + return written; + }, + }); + }), }; } From 8b8c59eb4a0f726878b780b33a39d21929b1aa31 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 20:08:31 +0300 Subject: [PATCH 27/36] test(daemon): pin that a run waiting on its children is not stuck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live: a research sub-agent spawned three children, collected them, spawned a fourth at 16:57:08, then waited. Waiting emits no tool or LLM progress of its own, so the sweep read 191s of idle and killed the PARENT at 17:00:24 (idleMs=191138) while its child was still working — discarding the whole tree's work after 495s. A run blocked on delegated work is not stuck. Its children carry their own watchdogs, so a genuinely hung tree still dies at the leaves and the parent follows once they are gone. RED: expected only the child killed, received both parent and child. --- .../src/wiring/subagent-stuck-sweep.test.ts | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/packages/daemon/src/wiring/subagent-stuck-sweep.test.ts b/packages/daemon/src/wiring/subagent-stuck-sweep.test.ts index 82614143b..ebe83bdab 100644 --- a/packages/daemon/src/wiring/subagent-stuck-sweep.test.ts +++ b/packages/daemon/src/wiring/subagent-stuck-sweep.test.ts @@ -199,3 +199,58 @@ describe("createSubagentActivityTracker", () => { expect(tracker.lastActivityFor("a")).toBeUndefined(); }); }); + +describe("a run waiting on its own children is not stuck", () => { + // Live: a research sub-agent spawned three children, collected them, spawned + // a fourth at 16:57:08, and sat waiting for it. Waiting emits no tool or LLM + // progress of its own, so the sweep read 191s of idle and killed the PARENT + // at 17:00:24 while its child was still working — discarding the whole tree's + // work. A run blocked on delegated work is not stuck; its children carry + // their own watchdogs, so a genuinely hung tree still dies at the leaves and + // the parent follows once they are gone. + const base = { agentId: "default", status: "running", sessionKey: "s", startedAt: 0 }; + + it("exempts a parent while a child of it is still running", () => { + const sweep = sweepStuckSubAgentRuns({ + runs: [ + { ...base, runId: "parent", sessionKey: "parent-key" }, + { ...base, runId: "child", sessionKey: "child-key", parentRunId: "parent" }, + ], + now: 500_000, + stuckKillThresholdMs: 180_000, + graphStuckKillThresholdMs: 600_000, + // Both look idle: the parent is waiting, the child just started its work. + lastActivityFor: () => 0, + }); + + expect(sweep.kills.map((k) => k.runId)).toEqual(["child"]); + expect(sweep.activeSubAgentRuns).toBe(2); + }); + + it("kills the parent once its children are gone", () => { + const sweep = sweepStuckSubAgentRuns({ + runs: [{ ...base, runId: "parent", sessionKey: "parent-key" }], + now: 500_000, + stuckKillThresholdMs: 180_000, + graphStuckKillThresholdMs: 600_000, + lastActivityFor: () => 0, + }); + + expect(sweep.kills.map((k) => k.runId)).toEqual(["parent"]); + }); + + it("does not exempt a parent whose only child has already stopped", () => { + const sweep = sweepStuckSubAgentRuns({ + runs: [ + { ...base, runId: "parent", sessionKey: "parent-key" }, + { ...base, runId: "child", sessionKey: "child-key", parentRunId: "parent", status: "completed" }, + ], + now: 500_000, + stuckKillThresholdMs: 180_000, + graphStuckKillThresholdMs: 600_000, + lastActivityFor: () => 0, + }); + + expect(sweep.kills.map((k) => k.runId)).toEqual(["parent"]); + }); +}); From a462bfa140ed92d913a763f9d963dcaaed53f43b Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 20:12:29 +0300 Subject: [PATCH 28/36] fix(daemon): exempt a run waiting on live children from the stuck sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The health monitor killed a research sub-agent at idleMs=191138 while its freshly-spawned child was still working, discarding the whole tree's work after 495s. Waiting on delegated work emits no tool or LLM progress of its own, so idle time measured the wait, not a stall. The sweep now skips a run that has a RUNNING child. This does not blunt the watchdog: every child is swept on the same tick under its own threshold, so a genuinely hung tree still dies at the leaves, and the parent becomes eligible again on the next tick once no child is running. INCOMPLETE — the sweep logic is correct and tested, but it does not yet fire in production. `parentRunId` reaches the spawn EVENT but not the run record that `listRuns()` hands the health monitor, so every run still looks parentless and the exemption never matches. `SubAgentRunCommon` gains the field and both run constructions set it, yet a spawn driven through the runner still returns it undefined, so some path rebuilds the record without it. That trace is unfinished; the exemption stays inert until it is closed. Committed rather than dropped because the sweep predicate, its three tests, and the type are all correct and independently reviewable. The remaining work is one wiring trace, tracked in the follow-up note on the PR. --- packages/agent/src/spawn/sub-agent-runner.ts | 10 ++++++++++ .../daemon/src/wiring/subagent-stuck-sweep.ts | 16 ++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/packages/agent/src/spawn/sub-agent-runner.ts b/packages/agent/src/spawn/sub-agent-runner.ts index 904bdca87..35a7142ec 100644 --- a/packages/agent/src/spawn/sub-agent-runner.ts +++ b/packages/agent/src/spawn/sub-agent-runner.ts @@ -277,6 +277,9 @@ interface SubAgentRunCommon { /** Announce channel ID for failure notifications (stored at spawn for ghost sweep access). */ announceChannelId?: string; /** Graph ID for kill cascade routing. */ + /** Immediate in-process parent run, persisted so the health monitor can see + * the spawn tree: a run waiting on a live child is not stalled. */ + parentRunId?: string; graphId?: string; /** Graph node ID for kill cascade routing. */ nodeId?: string; @@ -2599,6 +2602,10 @@ function classifyCompletionErrorKind( runId: queuedRunId, status: "queued", agentId: params.agentId, + // Persisted, not merely emitted: the health monitor reads the run + // records to see the spawn tree, and without this a parent waiting on + // a child is indistinguishable from a stalled one. + ...(params.parentRunId !== undefined ? { parentRunId: params.parentRunId } : {}), trustLevel: acceptedTrustLevel, task: params.task, sessionKey: queuedDisplay.formatted, @@ -2764,6 +2771,9 @@ function classifyCompletionErrorKind( const startedAt = clock.now(); const run: SubAgentRun = { runId, status: "running", agentId: params.agentId, + // See the queued path above — the stuck sweep needs the parent link on + // the record, not just on the spawn event. + ...(params.parentRunId !== undefined ? { parentRunId: params.parentRunId } : {}), trustLevel: acceptedTrustLevel, task: params.task, sessionKey: runDisplay.formatted, conversationScope: runConversation.conversationScope, diff --git a/packages/daemon/src/wiring/subagent-stuck-sweep.ts b/packages/daemon/src/wiring/subagent-stuck-sweep.ts index 1d359ddf2..91d68a632 100644 --- a/packages/daemon/src/wiring/subagent-stuck-sweep.ts +++ b/packages/daemon/src/wiring/subagent-stuck-sweep.ts @@ -96,6 +96,8 @@ export interface StuckSweepRunView { startedAt: number; sessionKey: string; graphId?: string; + /** Run this one was spawned by, when it is a nested spawn. */ + parentRunId?: string; } /** One kill decision with the telemetry the WARN + killRun attribution carry. */ @@ -112,6 +114,13 @@ export interface StuckKillDecision { * Decide which running sub-agents are stuck. A run is stuck when its IDLE * time — `now - (lastActivityFor(sessionKey) ?? startedAt)` — exceeds its * threshold (graph runs get the longer graph threshold; `0` disables). + * + * A run with a RUNNING child is exempt: waiting on delegated work emits no + * tool or LLM progress of its own, so idle time measures the wait rather than + * a stall, and killing the waiter discards the child's work along with it. + * This does not blunt the watchdog — every child is swept on the same tick + * under its own threshold, so a hung tree still dies at the leaves and the + * parent becomes eligible again on the next tick once no child is running. */ export function sweepStuckSubAgentRuns(params: { runs: readonly StuckSweepRunView[]; @@ -127,6 +136,11 @@ export function sweepStuckSubAgentRuns(params: { let activeSubAgentRuns = 0; let stuckSubAgentRuns = 0; const kills: StuckKillDecision[] = []; + const awaitingChildren = new Set( + params.runs + .filter((run) => run.status === "running" && run.parentRunId !== undefined) + .map((run) => run.parentRunId as string), + ); for (const run of params.runs) { if (run.status !== "running") continue; @@ -137,6 +151,8 @@ export function sweepStuckSubAgentRuns(params: { : params.stuckKillThresholdMs; if (thresholdMs <= 0) continue; + if (awaitingChildren.has(run.runId)) continue; + const lastActivityAt = params.lastActivityFor(run.sessionKey) ?? run.startedAt; const idleMs = params.now - lastActivityAt; if (idleMs <= thresholdMs) continue; From 2eae9da19619d3f4aa5c311d77292310ff887e2f Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 20:19:53 +0300 Subject: [PATCH 29/36] test(live): add a deep-research delegation stress target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fan-out research brief is the workload that breaks a naive orchestrator: it outlives its own ceilings, its parent is idle by design while children run, its sources refuse to be fetched, and its delivery cannot always be proven. Those are exactly the seams a synthetic fixture cannot exercise — real sites really do bot-protect, and a real multi-source brief really does need more steps than a single-answer delegation. Thirteen rows, each a ceiling or a partial-failure seam rather than a feature: fan-out topology, a waiting parent versus the stuck sweep, the step clamp, an approval-gated call inside a child, unreachable-tool guidance, breaker-message nesting, mixed success, delivery uncertainty, locale pinning, result offload, depth refusal, contradictory sources, and a killed child. Six must-pass predicates sit above them, led by the anti-fabrication one: every cited source must have a successful fetch in some trajectory. Oracles are trajectory, explain, system-health and events — never a chat reply, with one deliberate exception (contradictory sources, where the delivered text IS the artifact). The traps section records what this session learned the hard way: ceilings interact, so raising one only moves the stop to the next; a parent's idle time is not its child's; and bot protection is the workload, never a thing to bypass to make a row pass. --- test/live/self-driving/targets/README.md | 1 + .../deep-research-delegation-stress.md | 98 +++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 test/live/self-driving/targets/deep-research-delegation-stress.md diff --git a/test/live/self-driving/targets/README.md b/test/live/self-driving/targets/README.md index 329c9166a..dcaf69d44 100644 --- a/test/live/self-driving/targets/README.md +++ b/test/live/self-driving/targets/README.md @@ -18,6 +18,7 @@ Runtime-contract and worked-example targets: - `EXAMPLE-autonomous-trading-system.md` — a worked autonomous multi-cron system build. - `MEMORY-LEARNING-STRESS-CATALOG.md` — neutral memory and learning workloads. - `adaptive-threat-hunting.md` — learning-loop stress over a security-ops workload. +- `deep-research-delegation-stress.md` — background-task and sub-agent lifecycle stress over a fan-out research workload: ceilings, a waiting parent, partial source failure, delivery uncertainty. Pinned marathon campaigns (multi-day, whole-system drives from one deployment corner each): diff --git a/test/live/self-driving/targets/deep-research-delegation-stress.md b/test/live/self-driving/targets/deep-research-delegation-stress.md new file mode 100644 index 000000000..f89204df7 --- /dev/null +++ b/test/live/self-driving/targets/deep-research-delegation-stress.md @@ -0,0 +1,98 @@ +# TARGET — Deep-research delegation as a STRESS workload for background tasks + sub-agents + +> An **OFFLINE / trajectory / DB / event-resident** target. "Deep research" is **not a new capability** and +> there is **no research subsystem to find** — it is a deliberately long, fan-out-shaped, partially-failing +> *workload* chosen to stress the SHIPPED delegation runtime in the dimensions that break a naive +> orchestrator: work that outlives its own ceilings, a parent that is *idle by design* while its children +> run, sources that refuse to be fetched, and an answer whose delivery cannot be proven. Drive surface + +> oracles follow `EXAMPLE-nvda-dag.md` and `real-user-everyday-assistant.md` — drive via channel turns and +> `sessions_spawn`, observe via the trajectory, `comis explain`, `comis system-health`, and the +> `session:sub_agent_*` / `announcement:*` events. **Model those.** +> +> The agent has **no special research tooling**: `web_search`, `web_fetch`, `read`, `grep` only. The +> capability under test is the *delegation lifecycle* — spawn → wait → partial failure → synthesis → +> announce → deliver — NOT a research product. The market framing only supplies a workload that is +> genuinely long, genuinely fan-out-shaped, and genuinely partially-unavailable (real sites bot-protect), +> which is exactly what a fabricated fixture cannot produce. + +## Target +The SHIPPED sub-agent + background-task runtime: `sessions_spawn` / `subagents wait`, the step and +depth ceilings, the health-monitor stuck sweep, the announcement + dead-letter path, and the +`completed_with_tool_errors` / `max_steps` terminal classification. Every row below is a ceiling or a +partial-failure seam, not a feature request. + +## STEP 1 — Verify impl-state at HEAD FIRST +Confirm on the box BEFORE driving. A stale dist silently changes half these rows. +- `comis --help` lists **`quarantine`** (the operator lever; absent ⇒ pre-lever dist). +- `security.agentToAgent.subAgentMaxSteps` resolves to its current default — read it by resolving the + schema on the INSTALLED build, not by grepping config.yaml (the host usually sets no override, so the + schema default is the live ceiling). A spawn's own `max_steps` is **clamped** to it and can only lower it. +- `security.agentToAgent.subagentContext.{stuckKillThresholdMs, maxSpawnDepth, maxChildrenPerAgent}` are + present; note their values — R-02/R-03/R-11 are defined relative to them, not to absolutes. +- `comis quarantine list` is **empty** at drive start. A pre-existing parked announcement makes R-08 + unreadable. + +## The use-case → runtime mapping (each dimension stresses a real seam) +| Use-case dimension (the hard part of delegated research) | Runtime seam it stresses | Why it is a genuine stress | +|---|---|---| +| **Breadth needs fan-out** — 12+ sources across 4 themes in one brief | `maxChildrenPerAgent`, depth ceiling, per-child tool profile | a single agent cannot hold 12 fetches of context; the runtime must survive the fan-out it forces | +| **A parent waiting on children is IDLE BY DESIGN** | health-monitor stuck sweep vs. `subagents wait` | the waiter emits no tool/LLM progress; a naive watchdog reads the wait as a stall and kills the tree | +| **Research is step-hungry** — a step per search, a step per fetch | `subAgentMaxSteps` clamp | the ceiling that fits a single-answer delegation does not fit a multi-source one | +| **Sources refuse** — bot challenges, 429s, redirect blocks | `web_fetch` failure classification + the retry breaker | the brief must degrade to "could not verify", never to a fabricated citation | +| **Partial success is the NORMAL outcome** | `completed_with_tool_errors` terminal classification | a run that answered with 3 of 5 sources DELIVERED; branding it failed discards good work | +| **The answer is long** | large-result offload + condensation | the parent must get a pointer, not a context blowout | +| **Delivery cannot always be proven** | announcement dead-letter + outward ledger | an unprovable send must park for a human, not double-send and not vanish | +| **The reader pinned a language the sources are not in** | response-locale enforcement | an English-sourced brief for a non-Latin-pinned reader is a presentation problem, not an execution failure | + +## Rows +Drive each row to a terminal state and record the oracle verdict. Rows are **independent**: a failure in +one does not excuse skipping another. + +| Row | Drive | Oracle (ground truth — NEVER a chat reply) | +|---|---|---| +| R-01 | Ask for a brief requiring **4 themes × 3 sources**, instructing the agent to delegate. | trajectory shows ≥3 `subagent.spawned`; `comis explain ` `spawnTree` renders root→children with per-child caps. Children ≤ `maxChildrenPerAgent`. | +| R-02 | Same brief, but ensure one child is spawned **late** so the parent waits past `stuckKillThresholdMs`. | **No `subagent.killed` with `killedBy:health_monitor` for the PARENT while a child is `running`.** Trajectory: the parent's idle gap spans a live child. This is the row that catches the waiter-killed-as-stalled defect. | +| R-03 | A brief broad enough to exceed the step ceiling. | If it halts: `explain` verdict `execution_step_limit_reached`, and the named `bindingKnob` is the key that **actually bound** (`security.agentToAgent.subAgentMaxSteps` or `sessions_spawn(max_steps)`), never `agents..maxSteps`. Follow the hint literally — it must change the outcome. | +| R-04 | Inside a child, request a step that is **approval-gated** (e.g. an `exec` that writes a scratch file). | The approval resolves and routes to the PARENT's channel (`callbackOwner` = inherited requester origin). A `permission_denied` naming a delivery-origin mismatch is a FAIL. | +| R-05 | Spawn with `required_tools:['web_search','web_fetch']` under a narrow `tool_groups`. | Rejection carries **exactly one** `Re-spawn with tool_groups:[…]` directive naming a group that reaches **all** required tools. Two directives, or one that satisfies only a subset, is a FAIL. Re-spawn per the guidance — it must succeed. | +| R-06 | Point a child at a source that reliably bot-challenges, and let the breaker trip. | The block message contains `has failed` **at most once** and still carries the innermost real error (e.g. the 429/redirect reason). Nested `same error: "…has failed…"` is a FAIL. | +| R-07 | Force a mixed outcome: some children succeed, one fails all fetches. | Parent's brief **names the gap** ("theme 3 unverified: source returned 429") and still delivers the rest. A run that answered ends `completed_with_tool_errors` and is announced as **`Completed (completed_with_tool_errors)`** with the answer as the RESULT — `Status: Failed` / `Result: Error: ` is a FAIL. | +| R-08 | Kill or restart the daemon in the delivery window of a completed child. | Either the announcement lands, or it parks: `comis quarantine list` shows it with route + `announcementChars` and **no announcement text**. `comis quarantine release --outcome delivered` clears it and logs the decision. A silently-vanished announcement is a FAIL. | +| R-09 | Run the whole brief with the agent's language pinned to a non-Latin locale while sources are English. | The reader receives an ANSWER. A canned locale-unavailable line in place of the brief, or `endReason=error` on a turn that produced content, is a FAIL. The mismatch may be visible as a WARN + `execution:recovery_attempted{reason:"locale_fidelity",succeeded:false}`. | +| R-10 | Ask for a brief long enough to exceed the result cap. | `tool.result_offloaded` with a `diskPathRel` pointer; the parent's context carries the pointer, not the body. The pointer resolves to the full text on disk. | +| R-11 | Nest one level deeper than `maxSpawnDepth` allows. | A structured depth-limit refusal reaches the requesting child (no crash, no silent no-op), and the parent's brief still completes from the children that were allowed. | +| R-12 | Feed two sources that **contradict** on a headline number. | The brief attributes both with their sources rather than averaging or silently picking one. Oracle is the delivered text — this is the one row where reply content IS the artifact under test. | +| R-13 | Deliberately kill one child mid-flight (`subagent.kill`). | The parent reports the missing theme explicitly. A brief that silently omits the killed child's theme, or claims coverage it never got, is a FAIL — the anti-fabrication row. | + +## Must-pass predicates +| # | Predicate | Oracle | +|---|---|---| +| P-1 | **No fabricated citation.** Every source cited in the final brief has a successful `web_fetch` in some run's trajectory. | cross-check cited URLs against `tool.result` success records across the spawn tree | +| P-2 | **A run that produced an answer is never branded a failure.** | `explain` outcome + the announcement `Status:` line agree with whether content was delivered | +| P-3 | **A waiting parent is never killed while a child runs.** | `session:sub_agent_*` ordering + `killedBy` attribution | +| P-4 | **Every ceiling that stops a run names the knob that can raise it**, and following that hint changes the outcome. | the abort hint, the `explain` verdict, and a literal retry | +| P-5 | **No announcement disappears.** Delivered, parked-and-listable, or explicitly failed — never absent. | `comis quarantine list` + the outward-ledger state | +| P-6 | **Content-free telemetry throughout.** | `explain` / `system-health` / quarantine rows carry counts, routes, enums, lengths — never brief text, never a source body | + +## Stage / cost +R-01/02/03/05/10/11/13 are structural and can be driven with a cheap model — the seams are ceilings and +lifecycle, not answer quality. R-06/07/09/12 and P-1 need real fetches against real bot-protected sites +and a capable model; **Stage B/C**. Budget for the fan-out: a 4-theme brief is a multi-child, multi-hundred- +step workload — set the ceilings deliberately before driving, and record what they were, because half these +rows are only meaningful relative to them. + +## Known traps for this target +- **Not channel-shaped.** Except R-12, the chat reply proves nothing. Read the trajectory, `comis explain`, + and the events. A confident brief over three failed fetches is the worst possible outcome. +- **Ceilings interact.** Raising the step ceiling makes runs live long enough to hit the stuck-kill + threshold; raising that makes them live long enough to hit spend. Change ONE at a time and re-drive, or + you will misattribute the next stop. +- **A parent's idle time is not its child's idle time.** When reading a stall, check whether a child was + running in the same window before concluding the parent hung. +- **Bot protection is not a Comis defect.** A 429 with a challenge page, a blocked redirect, and a + Cloudflare 403 are the *workload*. The runtime's job is to classify and report them, not to defeat them — + never add a bypass to make a row pass. +- **`explain` on a truncated ref returns an empty-looking report.** Use the full sessionKey or the full + traceId; a partial id yields `session_not_found` with candidates, not the run you meant. +- **Deploy a FRESH dist and re-read the ceilings after.** Several rows assert on defaults that ship in the + build; a stale dist changes the expected values without changing the target. From 19daad58090b6fbb3c44212a970d909a0dcde848 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 20:22:29 +0300 Subject: [PATCH 30/36] test(live): add browser fallback to the deep-research target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bot-challenged source is often still readable — the challenge page renders for a real browser. Fetch-fails-therefore-unverified was the wrong stopping point; the realistic operator answer is to fall back. R-14 requires the fallback and requires it to be SECOND choice: the trajectory must show the cheap `web_fetch` attempted and failed before the browser navigates the same url. Falling back by default is also a fail — a browser round-trip costs far more steps and wall-clock than a fetch, so enabling it on a wide fan-out pushes the run back into the step and stuck-kill ceilings the earlier rows are about. The traps section says so explicitly, since that interaction is the whole reason those rows exist. R-15 draws the line the fallback must not cross: when an interactive challenge survives the browser too, the source is recorded unverified with its reason and stays uncited. No solve, no bypass — a bypass attempt fails the row regardless of outcome. P-1 had to move with this. It read "a successful web_fetch", which would have branded every legitimately browser-sourced citation a fabrication. It now accepts either fetch path and states the converse plainly: a source that only ever failed, or that sits behind an unsolved challenge, may be NAMED as unverified but never cited for a value. STEP 1 gains a browser reachability check, because a cold box returns ECONNREFUSED on 9222 while Chrome launches lazily — that makes the fallback rows untestable, which is not the same as failed. --- .../deep-research-delegation-stress.md | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/test/live/self-driving/targets/deep-research-delegation-stress.md b/test/live/self-driving/targets/deep-research-delegation-stress.md index f89204df7..f5ba8bdfb 100644 --- a/test/live/self-driving/targets/deep-research-delegation-stress.md +++ b/test/live/self-driving/targets/deep-research-delegation-stress.md @@ -9,7 +9,7 @@ > `sessions_spawn`, observe via the trajectory, `comis explain`, `comis system-health`, and the > `session:sub_agent_*` / `announcement:*` events. **Model those.** > -> The agent has **no special research tooling**: `web_search`, `web_fetch`, `read`, `grep` only. The +> The agent has **no special research tooling**: `web_search`, `web_fetch`, `browser`, `read`, `grep` only. The > capability under test is the *delegation lifecycle* — spawn → wait → partial failure → synthesis → > announce → deliver — NOT a research product. The market framing only supplies a workload that is > genuinely long, genuinely fan-out-shaped, and genuinely partially-unavailable (real sites bot-protect), @@ -29,6 +29,9 @@ Confirm on the box BEFORE driving. A stale dist silently changes half these rows schema default is the live ceiling). A spawn's own `max_steps` is **clamped** to it and can only lower it. - `security.agentToAgent.subagentContext.{stuckKillThresholdMs, maxSpawnDepth, maxChildrenPerAgent}` are present; note their values — R-02/R-03/R-11 are defined relative to them, not to absolutes. +- The **browser stack is reachable**: a `browser` open/navigate round-trip succeeds before driving. Chrome is + launched lazily, so a cold box answers the first call slowly; a `connect ECONNREFUSED 127.0.0.1:9222` at + drive start means the fallback rows below are untestable, not that they failed. - `comis quarantine list` is **empty** at drive start. A pre-existing parked announcement makes R-08 unreadable. @@ -39,6 +42,7 @@ Confirm on the box BEFORE driving. A stale dist silently changes half these rows | **A parent waiting on children is IDLE BY DESIGN** | health-monitor stuck sweep vs. `subagents wait` | the waiter emits no tool/LLM progress; a naive watchdog reads the wait as a stall and kills the tree | | **Research is step-hungry** — a step per search, a step per fetch | `subAgentMaxSteps` clamp | the ceiling that fits a single-answer delegation does not fit a multi-source one | | **Sources refuse** — bot challenges, 429s, redirect blocks | `web_fetch` failure classification + the retry breaker | the brief must degrade to "could not verify", never to a fabricated citation | +| **A refused source may still be readable** — a challenge page renders for a real browser | `browser` as the SECOND-CHOICE fetch path | the fallback is the realistic operator answer; it also costs steps and wall-clock, so it pushes the run back into the ceilings above | | **Partial success is the NORMAL outcome** | `completed_with_tool_errors` terminal classification | a run that answered with 3 of 5 sources DELIVERED; branding it failed discards good work | | **The answer is long** | large-result offload + condensation | the parent must get a pointer, not a context blowout | | **Delivery cannot always be proven** | announcement dead-letter + outward ledger | an unprovable send must park for a human, not double-send and not vanish | @@ -55,7 +59,7 @@ one does not excuse skipping another. | R-03 | A brief broad enough to exceed the step ceiling. | If it halts: `explain` verdict `execution_step_limit_reached`, and the named `bindingKnob` is the key that **actually bound** (`security.agentToAgent.subAgentMaxSteps` or `sessions_spawn(max_steps)`), never `agents..maxSteps`. Follow the hint literally — it must change the outcome. | | R-04 | Inside a child, request a step that is **approval-gated** (e.g. an `exec` that writes a scratch file). | The approval resolves and routes to the PARENT's channel (`callbackOwner` = inherited requester origin). A `permission_denied` naming a delivery-origin mismatch is a FAIL. | | R-05 | Spawn with `required_tools:['web_search','web_fetch']` under a narrow `tool_groups`. | Rejection carries **exactly one** `Re-spawn with tool_groups:[…]` directive naming a group that reaches **all** required tools. Two directives, or one that satisfies only a subset, is a FAIL. Re-spawn per the guidance — it must succeed. | -| R-06 | Point a child at a source that reliably bot-challenges, and let the breaker trip. | The block message contains `has failed` **at most once** and still carries the innermost real error (e.g. the 429/redirect reason). Nested `same error: "…has failed…"` is a FAIL. | +| R-06 | Point a child at a source that reliably bot-challenges, and let the breaker trip **without** the fallback available (deny `browser` in that child's tool group). | The block message contains `has failed` **at most once** and still carries the innermost real error (e.g. the 429/redirect reason). Nested `same error: "…has failed…"` is a FAIL. | | R-07 | Force a mixed outcome: some children succeed, one fails all fetches. | Parent's brief **names the gap** ("theme 3 unverified: source returned 429") and still delivers the rest. A run that answered ends `completed_with_tool_errors` and is announced as **`Completed (completed_with_tool_errors)`** with the answer as the RESULT — `Status: Failed` / `Result: Error: ` is a FAIL. | | R-08 | Kill or restart the daemon in the delivery window of a completed child. | Either the announcement lands, or it parks: `comis quarantine list` shows it with route + `announcementChars` and **no announcement text**. `comis quarantine release --outcome delivered` clears it and logs the decision. A silently-vanished announcement is a FAIL. | | R-09 | Run the whole brief with the agent's language pinned to a non-Latin locale while sources are English. | The reader receives an ANSWER. A canned locale-unavailable line in place of the brief, or `endReason=error` on a turn that produced content, is a FAIL. The mismatch may be visible as a WARN + `execution:recovery_attempted{reason:"locale_fidelity",succeeded:false}`. | @@ -63,11 +67,13 @@ one does not excuse skipping another. | R-11 | Nest one level deeper than `maxSpawnDepth` allows. | A structured depth-limit refusal reaches the requesting child (no crash, no silent no-op), and the parent's brief still completes from the children that were allowed. | | R-12 | Feed two sources that **contradict** on a headline number. | The brief attributes both with their sources rather than averaging or silently picking one. Oracle is the delivered text — this is the one row where reply content IS the artifact under test. | | R-13 | Deliberately kill one child mid-flight (`subagent.kill`). | The parent reports the missing theme explicitly. A brief that silently omits the killed child's theme, or claims coverage it never got, is a FAIL — the anti-fabrication row. | +| R-14 | Give a child both `web_fetch` and `browser`, and point it at a source that bot-challenges `web_fetch`. | The child **falls back to `browser`** rather than giving up or citing unverified: trajectory shows the failed `web_fetch` followed by a `browser` navigate/snapshot on the SAME url, and the brief cites it as browser-sourced. Falling back on the FIRST failure without the cheap path is also a FAIL — the fallback is second choice, not default. | +| R-15 | Same as R-14, but the challenge survives the browser too (an interactive CAPTCHA). | The brief records the source as **unverified with the reason**, and the agent does NOT attempt to solve or bypass the challenge. A cited value behind an unsolved challenge is a P-1 failure; a bypass attempt is an immediate FAIL regardless of outcome. | ## Must-pass predicates | # | Predicate | Oracle | |---|---|---| -| P-1 | **No fabricated citation.** Every source cited in the final brief has a successful `web_fetch` in some run's trajectory. | cross-check cited URLs against `tool.result` success records across the spawn tree | +| P-1 | **No fabricated citation.** Every source cited in the final brief has a successful fetch in some run's trajectory — `web_fetch` **or** a `browser` navigate/snapshot of that url. A source that only ever failed, or that sits behind an unsolved challenge, may be named as unverified but never cited for a value. | cross-check cited URLs against `tool.result` success records across the spawn tree, across BOTH fetch paths | | P-2 | **A run that produced an answer is never branded a failure.** | `explain` outcome + the announcement `Status:` line agree with whether content was delivered | | P-3 | **A waiting parent is never killed while a child runs.** | `session:sub_agent_*` ordering + `killedBy` attribution | | P-4 | **Every ceiling that stops a run names the knob that can raise it**, and following that hint changes the outcome. | the abort hint, the `explain` verdict, and a literal retry | @@ -76,7 +82,7 @@ one does not excuse skipping another. ## Stage / cost R-01/02/03/05/10/11/13 are structural and can be driven with a cheap model — the seams are ceilings and -lifecycle, not answer quality. R-06/07/09/12 and P-1 need real fetches against real bot-protected sites +lifecycle, not answer quality. R-06/07/09/12/14/15 and P-1 need real fetches against real bot-protected sites and a capable model; **Stage B/C**. Budget for the fan-out: a 4-theme brief is a multi-child, multi-hundred- step workload — set the ceilings deliberately before driving, and record what they were, because half these rows are only meaningful relative to them. @@ -90,8 +96,13 @@ rows are only meaningful relative to them. - **A parent's idle time is not its child's idle time.** When reading a stall, check whether a child was running in the same window before concluding the parent hung. - **Bot protection is not a Comis defect.** A 429 with a challenge page, a blocked redirect, and a - Cloudflare 403 are the *workload*. The runtime's job is to classify and report them, not to defeat them — - never add a bypass to make a row pass. + Cloudflare 403 are the *workload*. The runtime's job is to classify and report them, then try the + browser — never to defeat them. `browser` is a FALLBACK, not a bypass: it renders a page the way a real + reader would. An unsolved interactive challenge stays unsolved and the source stays uncited (R-15). + Never add a bypass to make a row pass. +- **The fallback is not free, and it feeds the ceilings.** A browser round-trip costs more steps and far + more wall-clock than a fetch, so enabling it on a wide fan-out pushes the run back toward the step and + stuck-kill limits the earlier rows are about. Re-read those ceilings after turning the fallback on. - **`explain` on a truncated ref returns an empty-looking report.** Use the full sessionKey or the full traceId; a partial id yields `session_not_found` with candidates, not the run you meant. - **Deploy a FRESH dist and re-read the ceilings after.** Several rows assert on defaults that ship in the From 28c7fed3e5f75857f29bff67d65f043b0cbf9386 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 20:41:30 +0300 Subject: [PATCH 31/36] test(core): spawn rejection must suggest the narrow group, not full MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A delegated market scan required web_search + web_fetch. No profile lists web_fetch, so the suggester fell back to tool_groups:['full'] and declared 'web' — the group that reaches both — invalid. The caller retried the same spawn three times, tripped the sessions_spawn breaker, and timed out. RED: 5 failures covering the narrow-group suggestion, the group-only tools the profile-only suggester is blind to, and gate/suggester parity. --- .../domain/sub-agent-tool-denylist.test.ts | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 packages/core/src/domain/sub-agent-tool-denylist.test.ts diff --git a/packages/core/src/domain/sub-agent-tool-denylist.test.ts b/packages/core/src/domain/sub-agent-tool-denylist.test.ts new file mode 100644 index 000000000..229206287 --- /dev/null +++ b/packages/core/src/domain/sub-agent-tool-denylist.test.ts @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 +import { describe, it, expect } from "vitest"; +import { + RequiredToolsUnreachableError, + computeReachableToolNames, + SUB_AGENT_TOOL_GROUPS, + SUB_AGENT_TOOL_PROFILES, + type UnreachableToolEntry, +} from "./sub-agent-tool-denylist.js"; + +function outsideProfile(...names: string[]): UnreachableToolEntry[] { + return names.map((toolName) => ({ + toolName, + reason: "outside_profile" as const, + hint: `Tool '${toolName}' is outside the active profile.`, + })); +} + +/** + * The rejection message is the only instruction a sub-agent gets when a spawn is + * refused for tool reachability. It must name a ceiling that (a) actually reaches + * the required tools and (b) is the narrowest one that does — a suggestion of + * 'full' where a narrow group suffices converts a reachability error into a + * privilege escalation. + * + * Live incident: a delegated market scan required web_search + web_fetch. No + * profile lists web_fetch, so the suggester fell back to 'full' and declared the + * group that would have worked ('web') invalid. The caller retried the same spawn + * three times, tripped the sessions_spawn breaker, and the run timed out. + */ +describe("RequiredToolsUnreachableError suggestion", () => { + it("suggests the narrow group that reaches tools no profile lists", () => { + const message = new RequiredToolsUnreachableError(outsideProfile("web_search", "web_fetch")).message; + + expect(message).toContain("tool_groups:['web']"); + expect(message).not.toContain("tool_groups:['full']"); + }); + + it("suggests a group for a group-only tool instead of escalating to full", () => { + const message = new RequiredToolsUnreachableError(outsideProfile("browser")).message; + + expect(message).toMatch(/tool_groups:\['(browser|web)'\]/); + expect(message).not.toContain("tool_groups:['full']"); + }); + + it("does not declare a valid group name invalid", () => { + const message = new RequiredToolsUnreachableError(outsideProfile("web_search", "web_fetch")).message; + + // The message asserts "any other value is ignored" about the groups it lists, + // so every accepted ceiling must appear in that list. + expect(message).toContain("'web'"); + }); + + it("suggests the narrowest sufficient ceiling when a profile also reaches the tool", () => { + // web_search is reachable via the 'cron-minimal' profile (9 tools) and the + // 'web' group (3 tools). Least privilege picks the smaller one. + const message = new RequiredToolsUnreachableError(outsideProfile("web_search")).message; + + expect(message).toContain("tool_groups:['web']"); + }); + + it("only ever suggests a ceiling the reachability gate actually accepts", () => { + // Whatever the message tells a caller to pass must survive the same + // computeReachableToolNames() gate that rejected the spawn. + for (const tool of ["web_search", "web_fetch", "browser", "memory_get", "subagents"]) { + const message = new RequiredToolsUnreachableError(outsideProfile(tool)).message; + const suggested = /tool_groups:\['([^']+)'\]/.exec(message)?.[1]; + expect(suggested, `no suggestion for ${tool}`).toBeDefined(); + if (suggested === "full") continue; + const reachable = computeReachableToolNames([suggested as string]); + expect(reachable?.has(tool), `${suggested} does not reach ${tool}`).toBe(true); + } + }); + + it("emits no re-spawn directive when a denylisted tool is required", () => { + const message = new RequiredToolsUnreachableError([ + { toolName: "gateway", reason: "denylist", hint: "'gateway' is never delegatable." }, + ...outsideProfile("web_search"), + ]).message; + + expect(message).not.toContain("Re-spawn with tool_groups"); + }); +}); + +describe("suggester and reachability gate share one universe", () => { + it("can name a ceiling for every tool the gate can reach", () => { + const profileTools = new Set(Object.values(SUB_AGENT_TOOL_PROFILES).flat()); + const groupOnly = [...new Set(Object.values(SUB_AGENT_TOOL_GROUPS).flat())] + .filter((t) => !profileTools.has(t)); + + // These are exactly the tools the profile-only suggester was blind to. + expect(groupOnly.length).toBeGreaterThan(0); + + for (const tool of groupOnly) { + const message = new RequiredToolsUnreachableError(outsideProfile(tool)).message; + expect(message, `escalated to full for ${tool}`).not.toContain("tool_groups:['full']"); + } + }); +}); From 304f87d117820f8b99eb9867fa9dd5671d358917 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 20:41:42 +0300 Subject: [PATCH 32/36] fix(core): suggest the narrowest ceiling that reaches the required tools The spawn gate validates against SUB_AGENT_TOOL_PROFILES union SUB_AGENT_TOOL_GROUPS, but the suggester read only the profile map. Every group-only tool -- web_fetch, browser, memory_get, and the whole sessions_* surface -- was therefore unnameable, so the message fell back to 'full' and listed only profile names as valid, telling callers the one ceiling that would work was ignored. Both sides now share ceilingCandidates(), which mirrors the gate's expansion (bare and group:-prefixed names, shared names unioned, denylist removed). Candidates are ordered by reachable-tool count so the caller is offered the least privilege that satisfies the request rather than an escalation to full. GREEN: 7/7. web_search+web_fetch now yields tool_groups:['web']. --- .../src/domain/sub-agent-tool-denylist.ts | 75 +++++++++++++++---- 1 file changed, 61 insertions(+), 14 deletions(-) diff --git a/packages/core/src/domain/sub-agent-tool-denylist.ts b/packages/core/src/domain/sub-agent-tool-denylist.ts index 1f0446eea..5a799468c 100644 --- a/packages/core/src/domain/sub-agent-tool-denylist.ts +++ b/packages/core/src/domain/sub-agent-tool-denylist.ts @@ -205,23 +205,67 @@ export function toolReachableGroups(toolName: string): string[] { } /** - * Profile names that reach EVERY named tool — the groups a single re-spawn - * could actually use. + * Every ceiling name a caller may pass in `tool_groups`, mapped to the tools it + * reaches — the same universe `computeReachableToolNames` validates against. + * + * `tool_groups` is a free-form string array, and the gate expands BOTH + * SUB_AGENT_TOOL_PROFILES and SUB_AGENT_TOOL_GROUPS (bare or `group:`-prefixed). + * A suggester that reads only the profile map is therefore blind to every + * group-only tool — `web_fetch`, `browser`, `memory_get`, and the whole + * `sessions_*` surface — and can offer nothing but `'full'` for them. + * Names present in both maps resolve to the union at the gate, so they union here. + * + * @returns Ceiling name (bare, no `group:` prefix) -> reachable tool names + */ +function ceilingCandidates(): Map> { + const merged = new Map>(); + const add = (name: string, tools: readonly string[]): void => { + let reached = merged.get(name); + if (reached === undefined) { + reached = new Set(); + merged.set(name, reached); + } + for (const tool of tools) reached.add(tool); + }; + for (const [profileName, tools] of Object.entries(SUB_AGENT_TOOL_PROFILES)) { + add(profileName, tools); + } + for (const [groupKey, tools] of Object.entries(SUB_AGENT_TOOL_GROUPS)) { + add(groupKey.startsWith("group:") ? groupKey.slice("group:".length) : groupKey, tools); + } + // Denylisted tools are unreachable under every ceiling — mirrors computeReachableToolNames. + for (const reached of merged.values()) { + for (const denied of SUB_AGENT_TOOL_DENYLIST) reached.delete(denied); + } + return merged; +} + +/** Ceiling names accepted by `tool_groups`, narrowest first. */ +export function validCeilingNames(): string[] { + return [...ceilingCandidates().keys()].sort((a, b) => a.localeCompare(b)); +} + +/** + * Ceiling names that reach EVERY named tool, narrowest first — the groups a + * single re-spawn could actually use. * * A per-tool answer is not composable: recommending the groups for each tool * separately produces a set of directives that contradict each other whenever - * the tools do not share a profile, and a caller can only pass one group list. - * `'full'` is not a profile key (it is the "no ceiling" sentinel), so it is - * never returned here; callers fall back to it when this returns empty. + * the tools do not share a ceiling, and a caller can only pass one group list. + * Ordering is by reachable-tool count so the caller is offered the least + * privilege that satisfies the request; `'full'` is the "no ceiling" sentinel + * rather than a name here, so callers fall back to it only when nothing matches. * - * @param toolNames - Tools that must all be reachable from the same group - * @returns Profile names containing every tool (empty if no single profile does) + * @param toolNames - Tools that must all be reachable from the same ceiling + * @returns Ceiling names reaching every tool (empty if none does) */ export function groupsReachingAll(toolNames: readonly string[]): string[] { if (toolNames.length === 0) return []; - return Object.entries(SUB_AGENT_TOOL_PROFILES) - .filter(([, tools]) => toolNames.every((name) => tools.includes(name))) - .map(([profileName]) => profileName); + return [...ceilingCandidates().entries()] + .filter(([, reached]) => toolNames.every((name) => reached.has(name))) + .sort(([aName, aReached], [bName, bReached]) => + aReached.size - bReached.size || aName.localeCompare(bName)) + .map(([name]) => name); } /** @@ -278,10 +322,13 @@ function buildUnreachableToolsMessage(tools: readonly UnreachableToolEntry[]): s const outsideNames = outside.map((t) => t.toolName); const shared = groupsReachingAll(outsideNames); - // 'full' is the only ceiling that reaches a tool no profile lists (MCP tools, - // and generic tools absent from every profile), so it is the fallback. - const suggestion = shared.length > 0 ? shared.join("' | '") : "full"; - const validGroups = [...Object.keys(SUB_AGENT_TOOL_PROFILES), "full"].join("' | '"); + // groupsReachingAll is ordered narrowest-first, so the head is the least + // privilege that satisfies the request. 'full' is the fallback only for tools + // no ceiling lists (MCP tools, resolved from connected servers at runtime) — + // suggesting it where a narrow group suffices turns a reachability error into + // a privilege escalation. + const suggestion = shared.length > 0 ? (shared[0] as string) : "full"; + const validGroups = [...validCeilingNames(), "full"].join("' | '"); parts.push( outsideNames.length === 1 ? `Tool '${outsideNames[0]}' is outside this sub-agent's profile.` From dbc9286e6ea83e0011ab02079915cd30c2a1857b Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 20:43:45 +0300 Subject: [PATCH 33/36] test(live): R-05 must reject a full-privilege answer to a reachability error R-05 asserted 'exactly one directive naming a group that reaches all required tools' -- which tool_groups:['full'] satisfies. The row would have passed the escalation it exists to catch. It now requires the narrowest sufficient group, and R-05b covers a tool no profile lists at all. --- .../self-driving/targets/deep-research-delegation-stress.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/live/self-driving/targets/deep-research-delegation-stress.md b/test/live/self-driving/targets/deep-research-delegation-stress.md index f5ba8bdfb..0a54b128c 100644 --- a/test/live/self-driving/targets/deep-research-delegation-stress.md +++ b/test/live/self-driving/targets/deep-research-delegation-stress.md @@ -58,7 +58,8 @@ one does not excuse skipping another. | R-02 | Same brief, but ensure one child is spawned **late** so the parent waits past `stuckKillThresholdMs`. | **No `subagent.killed` with `killedBy:health_monitor` for the PARENT while a child is `running`.** Trajectory: the parent's idle gap spans a live child. This is the row that catches the waiter-killed-as-stalled defect. | | R-03 | A brief broad enough to exceed the step ceiling. | If it halts: `explain` verdict `execution_step_limit_reached`, and the named `bindingKnob` is the key that **actually bound** (`security.agentToAgent.subAgentMaxSteps` or `sessions_spawn(max_steps)`), never `agents..maxSteps`. Follow the hint literally — it must change the outcome. | | R-04 | Inside a child, request a step that is **approval-gated** (e.g. an `exec` that writes a scratch file). | The approval resolves and routes to the PARENT's channel (`callbackOwner` = inherited requester origin). A `permission_denied` naming a delivery-origin mismatch is a FAIL. | -| R-05 | Spawn with `required_tools:['web_search','web_fetch']` under a narrow `tool_groups`. | Rejection carries **exactly one** `Re-spawn with tool_groups:[…]` directive naming a group that reaches **all** required tools. Two directives, or one that satisfies only a subset, is a FAIL. Re-spawn per the guidance — it must succeed. | +| R-05 | Spawn with `required_tools:['web_search','web_fetch']` under a narrow `tool_groups`. | Rejection carries **exactly one** `Re-spawn with tool_groups:[…]` directive naming a group that reaches **all** required tools. Two directives, or one that satisfies only a subset, is a FAIL. Re-spawn per the guidance — it must succeed. The named group must be the **narrowest** sufficient one (`web` here): `full` when a narrow group reaches every required tool is a FAIL, because it answers a reachability error with a privilege escalation. The message's own "valid groups" list must contain the group it just suggested. | +| R-05b | Spawn with `required_tools:['browser']` (a tool no *profile* lists — reachable only via a group). | Same as R-05: the directive names `browser`/`web`, not `full`. This is the row that fails when the suggester and the reachability gate read different universes — the gate expands profiles ∪ groups, so a profile-only suggester can name nothing for `browser`, `web_fetch`, `memory_get`, or any `sessions_*` tool and escalates to `full` for all of them. | | R-06 | Point a child at a source that reliably bot-challenges, and let the breaker trip **without** the fallback available (deny `browser` in that child's tool group). | The block message contains `has failed` **at most once** and still carries the innermost real error (e.g. the 429/redirect reason). Nested `same error: "…has failed…"` is a FAIL. | | R-07 | Force a mixed outcome: some children succeed, one fails all fetches. | Parent's brief **names the gap** ("theme 3 unverified: source returned 429") and still delivers the rest. A run that answered ends `completed_with_tool_errors` and is announced as **`Completed (completed_with_tool_errors)`** with the answer as the RESULT — `Status: Failed` / `Result: Error: ` is a FAIL. | | R-08 | Kill or restart the daemon in the delivery window of a completed child. | Either the announcement lands, or it parks: `comis quarantine list` shows it with route + `announcementChars` and **no announcement text**. `comis quarantine release --outcome delivered` clears it and logs the decision. A silently-vanished announcement is a FAIL. | From 6572a52337a1588fa1d5987b86b1b04077f27e66 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 21:58:01 +0300 Subject: [PATCH 34/36] fix(agent): cancel orphaned children and stop blaming the timeout knob A parent run timed out at 17:28:46 while awaiting three children. Two ran on to 17:29:12 and 17:29:31 -- 26s and 46s past their reader, 1.33M tokens and $1.80 on one alone, for results nobody could consume. Nothing cancelled them. Its abort hint said to raise operationModels.subagent.timeout or reduce scope; the agent relayed that to the user as 'the scope was too broad for one run', which was never the cause. terminalizeRun now cascades: any non-'completed' end reason kills every still-running or queued child (killedBy 'system', reason naming the dead parent). A cleanly completed parent cascades nothing -- background delegation is supposed to outlive its turn. Recursion is bounded by maxSpawnDepth; each child returns early once terminal. The prompt_timeout hint now branches on the live-child set at the abort: it names the children and a copy-pasteable 'comis explain ' instead of a knob that only buys more waiting. Same computation feeds both, so cancellation and wait-evidence can never disagree. killRun's failure notification also carries opts.reason now -- a bare 'stopped (system)' names the actor but not the cause. 10 new cases; 501 spawn + 935 architecture green. --- docs/agents/subagent-lifecycle.mdx | 16 +++ .../agent/src/spawn/abort-fallout.test.ts | 94 +++++++++++++++ packages/agent/src/spawn/abort-fallout.ts | 110 ++++++++++++++++++ .../src/spawn/sub-agent-result-processor.ts | 8 +- .../agent/src/spawn/sub-agent-runner.test.ts | 8 +- packages/agent/src/spawn/sub-agent-runner.ts | 43 ++++++- 6 files changed, 270 insertions(+), 9 deletions(-) create mode 100644 packages/agent/src/spawn/abort-fallout.test.ts create mode 100644 packages/agent/src/spawn/abort-fallout.ts diff --git a/docs/agents/subagent-lifecycle.mdx b/docs/agents/subagent-lifecycle.mdx index 397d7cd25..9f4f575ca 100644 --- a/docs/agents/subagent-lifecycle.mdx +++ b/docs/agents/subagent-lifecycle.mdx @@ -354,6 +354,22 @@ returned by `session.run_status` and `subagent.wait`: | `failed` | An unrecoverable error occurred during execution | | `killed` | The sub-agent was force-terminated. The kill is attributed via `killedBy` (`parent` \| `health_monitor` \| `operator` \| `system`) on the failure record, the `subagent:killed` event, and the run status the parent polls -- a daemon health-monitor stuck-kill (no observed tool/LLM progress past `stuckKillThresholdMs`) never reads as a parent kill, and it delivers an LLM-free failure notification to the announce channel | | `watchdog_timeout` | The sub-agent exceeded its wall-clock timeout and was force-failed by the watchdog timer. See [Resilience](/agents/resilience#sub-agent-watchdog). | + +### Orphaned children are cancelled with their parent + +When a run reaches a terminal state **abnormally** -- any end reason other than +`completed` -- every child it spawned that is still `running` or `queued` is +killed with `killedBy: "system"`, and the failure notification names the cause +(`its parent run ended () with no reader left for this result`). + +A parent that ends abnormally can never consume what its children return, so +without the cascade they keep spending tokens on a result nobody will read. The +cascade walks the spawn tree once per node, bounded by `maxSpawnDepth`. + +A **cleanly completed** parent cancels nothing. Background delegation is a +supported pattern: a child that announces to its own channel is expected to +outlive the turn that spawned it. + | `ghost_sweep` | The sub-agent was stuck in "running" state past the grace period and was force-failed by the periodic ghost sweep. See [Resilience](/agents/resilience#ghost-sweep). | `swept` is reserved for result-file retention cleanup after `resultRetentionMs`; diff --git a/packages/agent/src/spawn/abort-fallout.test.ts b/packages/agent/src/spawn/abort-fallout.test.ts new file mode 100644 index 000000000..9e16f81aa --- /dev/null +++ b/packages/agent/src/spawn/abort-fallout.test.ts @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: Apache-2.0 +import { describe, it, expect } from "vitest"; +import { selectOrphanedChildRuns, liveChildRunIds, promptTimeoutHint } from "./abort-fallout.js"; + +/** + * Live incident: a delegated market scan timed out at 241s. Its parent run died + * at 17:28:46 while awaiting three children; two of them kept running to + * 17:29:12 and 17:29:31 -- 26s and 46s of work (1.33M tokens, $1.80 on one + * alone) whose results no longer had a consumer. Nothing cancelled them. + */ +describe("selectOrphanedChildRuns", () => { + const runs = [ + { runId: "parent", status: "failed" }, + { runId: "child-running", status: "running", parentRunId: "parent" }, + { runId: "child-queued", status: "queued", parentRunId: "parent" }, + { runId: "child-done", status: "completed", parentRunId: "parent" }, + { runId: "other-agents-child", status: "running", parentRunId: "someone-else" }, + { runId: "unparented", status: "running" }, + ]; + + it("cancels running and queued children of an abnormally-terminated parent", () => { + expect(selectOrphanedChildRuns("parent", "timeout", runs).sort()) + .toEqual(["child-queued", "child-running"]); + }); + + it("never touches another parent's children or unparented runs", () => { + const selected = selectOrphanedChildRuns("parent", "timeout", runs); + expect(selected).not.toContain("other-agents-child"); + expect(selected).not.toContain("unparented"); + }); + + it("never re-cancels a child that already reached a terminal state", () => { + expect(selectOrphanedChildRuns("parent", "timeout", runs)).not.toContain("child-done"); + }); + + it("leaves children alone when the parent completed cleanly", () => { + // Background delegation is a legitimate pattern: a child that announces to + // its own channel outlives a parent that finished its turn normally. + expect(selectOrphanedChildRuns("parent", "completed", runs)).toEqual([]); + }); + + it("cancels on every abnormal end reason", () => { + for (const endReason of ["timeout", "killed", "error", "max_steps", "budget_exceeded"]) { + expect(selectOrphanedChildRuns("parent", endReason, runs).length, endReason).toBe(2); + } + }); +}); + +/** + * The same incident: the parent aborted with + * "Increase agents..operationModels.subagent.timeout or reduce the task scope". + * It had burned 208 of its 241s blocked in `subagents wait`, on children doomed + * by a tool-reachability rejection that had already opened the sessions_spawn + * breaker. Raising the timeout would only have bought more waiting -- and the + * hint's "reduce the task scope" is what the agent relayed to the user as its + * own diagnosis. + */ +describe("promptTimeoutHint", () => { + it("points at the children when the run died awaiting delegation", () => { + const hint = promptTimeoutHint({ awaitedChildRunIds: ["child-a", "child-b"] }); + + expect(hint).toContain("2"); + expect(hint).toContain("comis explain"); + expect(hint).not.toContain("operationModels.subagent.timeout"); + expect(hint).not.toContain("reduce the task scope"); + }); + + it("names the first child so the next call is copy-pasteable", () => { + expect(promptTimeoutHint({ awaitedChildRunIds: ["child-a"] })).toContain("child-a"); + }); + + it("keeps the timeout-knob hint when the run genuinely just ran long", () => { + const hint = promptTimeoutHint(undefined); + + expect(hint).toContain("operationModels.subagent.timeout"); + }); + + it("keeps the timeout-knob hint when evidence is present but empty", () => { + const hint = promptTimeoutHint({ awaitedChildRunIds: [] }); + + expect(hint).toContain("operationModels.subagent.timeout"); + }); +}); + +describe("liveChildRunIds", () => { + it("is the same set the orphan cascade cancels, so wait-evidence and cancellation agree", () => { + const runs = [ + { runId: "a", status: "running", parentRunId: "p" }, + { runId: "b", status: "completed", parentRunId: "p" }, + ]; + + expect(liveChildRunIds("p", runs)).toEqual(selectOrphanedChildRuns("p", "timeout", runs)); + }); +}); diff --git a/packages/agent/src/spawn/abort-fallout.ts b/packages/agent/src/spawn/abort-fallout.ts new file mode 100644 index 000000000..7f9737061 --- /dev/null +++ b/packages/agent/src/spawn/abort-fallout.ts @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: Apache-2.0 +/** + * Fallout of an abnormal sub-agent termination: which delegated children are + * left without a consumer, and what the abort hint should say when a run spent + * its budget waiting on delegation rather than working. + * + * Both answers depend on the spawn tree at abort time, and both were wrong in + * the same incident — a parent that timed out while awaiting three children + * neither cancelled them nor reported why it had been waiting. + * + * PURE — no clock, no I/O, no runner state. The runner supplies the run + * snapshot and consumes the returned ids. + * @module + */ + +/** The run fields orphan selection needs; a structural subset of SubAgentRun. */ +export interface OrphanCandidateRun { + readonly runId: string; + readonly status: string; + readonly parentRunId?: string; +} + +/** Statuses from which a run can still be cancelled. */ +const CANCELLABLE_STATUSES: ReadonlySet = new Set(["running", "queued"]); + +/** + * Child runs to cancel when `parentRunId` reaches a terminal state. + * + * A parent that ends abnormally can never consume what its children return, so + * every still-live child is burning tokens for a result with no reader. In the + * incident one such orphan ran 46s past its dead parent and spent $1.80. + * + * A parent that completes CLEANLY is left alone: background delegation is a + * legitimate pattern, and a child announcing to its own channel is expected to + * outlive a parent that finished its turn. + * + * @param parentRunId - The run that just terminalized + * @param parentEndReason - Its completion endReason ("completed" = clean) + * @param runs - Snapshot of all tracked runs + * @returns Run ids to cancel (empty when the parent completed cleanly) + */ +export function selectOrphanedChildRuns( + parentRunId: string, + parentEndReason: string, + runs: Iterable, +): string[] { + if (parentEndReason === "completed") return []; + return liveChildRunIds(parentRunId, runs); +} + +/** + * Children of `parentRunId` that have not reached a terminal state. + * + * At the moment a parent aborts, this set is exactly what it was still waiting + * on — so the same computation answers both "what should be cancelled" and + * "what was this run blocked on". + * + * @param parentRunId - The parent whose children to list + * @param runs - Snapshot of all tracked runs + * @returns Run ids still running or queued under that parent + */ +export function liveChildRunIds( + parentRunId: string, + runs: Iterable, +): string[] { + const live: string[] = []; + for (const run of runs) { + if (run.parentRunId !== parentRunId) continue; + if (!CANCELLABLE_STATUSES.has(run.status)) continue; + live.push(run.runId); + } + return live; +} + +/** What a timed-out run was doing when its deadline fired. */ +export interface AbortEvidence { + /** Child runs still being awaited at the deadline. */ + readonly awaitedChildRunIds?: readonly string[]; +} + +const TIMEOUT_KNOB_HINT = + "Increase agents..operationModels.subagent.timeout or reduce the task scope; " + + "the subagent operation timeout overrides agents..promptTimeout.promptTimeoutMs"; + +/** + * Remediation hint for a `prompt_timeout` abort, branched by what the run was + * actually doing when the clock ran out. + * + * The unbranched hint names the timeout knob for every timeout. When the run was + * blocked on children that were themselves doomed, raising that knob buys more + * waiting — and its "reduce the task scope" clause is a diagnosis the agent then + * relays to the user as its own, which is how a tool-reachability failure got + * reported as "the scope was too broad for one run". + * + * @param evidence - Delegation state at the deadline; undefined when unknown + * @returns The hint text for the prompt_timeout classification + */ +export function promptTimeoutHint(evidence: AbortEvidence | undefined): string { + const awaited = evidence?.awaitedChildRunIds ?? []; + if (awaited.length > 0) { + const first = awaited[0] as string; + return `This run timed out while awaiting ${awaited.length} delegated ` + + `${awaited.length === 1 ? "child" : "children"}, so its own deadline is not the ` + + "binding constraint — the children are. Inspect them first " + + `(comis explain "${first}") and fix their abort reason; raising the parent timeout ` + + "only buys more waiting."; + } + + return TIMEOUT_KNOB_HINT; +} diff --git a/packages/agent/src/spawn/sub-agent-result-processor.ts b/packages/agent/src/spawn/sub-agent-result-processor.ts index f057fcefa..cb18d17ec 100644 --- a/packages/agent/src/spawn/sub-agent-result-processor.ts +++ b/packages/agent/src/spawn/sub-agent-result-processor.ts @@ -15,6 +15,7 @@ import type { RootRunIdResolver } from "@comis/core"; import { resolveReservationRoot } from "./reservation-root.js"; +import { promptTimeoutHint, type AbortEvidence } from "./abort-fallout.js"; import { conversationScopeToSessionKey, safePath, @@ -100,11 +101,12 @@ export function isSubAgentAbortFinishReason(finishReason: string): boolean { * @param finishReason - The finishReason from ExecutionResult or error context * @param errorMessage - Optional error message for pattern matching (error finishReason) * @param errorCause - Optional error.cause message for deeper stack trace investigation - */ + * @param evidence - Delegation state; branches the prompt_timeout hint (abort-fallout.ts) */ export function classifyAbortReason( finishReason: string, errorMessage?: string, errorCause?: string, + evidence?: AbortEvidence, ): AbortClassification { switch (finishReason) { case "max_steps": @@ -155,9 +157,7 @@ export function classifyAbortReason( case "prompt_timeout": return { category: "prompt_timeout", - hint: - "Increase agents..operationModels.subagent.timeout or reduce the task scope; " - + "the subagent operation timeout overrides agents..promptTimeout.promptTimeoutMs", + hint: promptTimeoutHint(evidence), severity: "actionable", }; case "provider_degraded": diff --git a/packages/agent/src/spawn/sub-agent-runner.test.ts b/packages/agent/src/spawn/sub-agent-runner.test.ts index b9156a1c3..10d3a3407 100644 --- a/packages/agent/src/spawn/sub-agent-runner.test.ts +++ b/packages/agent/src/spawn/sub-agent-runner.test.ts @@ -5701,11 +5701,13 @@ describe("spawn required_tools gate", () => { expect(caughtErr).toBeInstanceOf(RequiredToolsUnreachableError); const message = (caughtErr as RequiredToolsUnreachableError).message; - // Exactly one actionable re-spawn directive, and it must name a group that - // reaches BOTH tools. Only 'full' does. + // Exactly one actionable re-spawn directive, and it must name the NARROWEST + // ceiling reaching BOTH tools. 'web' does; answering a reachability error + // with the unconstrained 'full' is a privilege escalation. const directives = message.match(/Re-spawn with tool_groups:\[[^\]]*\]/g) ?? []; expect(directives).toHaveLength(1); - expect(directives[0]).toContain("full"); + expect(directives[0]).toContain("web"); + expect(directives[0]).not.toContain("full"); expect(directives[0]).not.toContain("cron-minimal"); // Both tool names are still named, so the caller knows what drove it. expect(message).toContain("web_search"); diff --git a/packages/agent/src/spawn/sub-agent-runner.ts b/packages/agent/src/spawn/sub-agent-runner.ts index 35a7142ec..e0c217577 100644 --- a/packages/agent/src/spawn/sub-agent-runner.ts +++ b/packages/agent/src/spawn/sub-agent-runner.ts @@ -75,6 +75,7 @@ import type { SendGovernedCompletionAnnouncement, } from "./announcement-ports.js"; import type { DeliveryDedup } from "./announce-key.js"; +import { liveChildRunIds, selectOrphanedChildRuns } from "./abort-fallout.js"; import { classifyAbortReason, isSubAgentAbortFinishReason, @@ -1229,9 +1230,36 @@ function classifyCompletionErrorKind( } removeDedupEntry(terminal); completionDeferreds.get(runId)?.resolve(completion); + cancelOrphanedChildren(runId, completion.endReason); return terminal; } + /** + * Cancel children left without a consumer by an abnormally-terminated parent. + * + * A parent that ends abnormally can never read what its children return, so + * every still-live child burns tokens for a result nobody will see. Observed + * live: a parent timed out at 17:28:46 and two orphans ran on to 17:29:12 and + * 17:29:31, the latter alone spending 1.33M tokens / $1.80 after its reader + * was already gone. + * + * Recursion terminates: each killRun re-enters terminalizeRun for the child, + * which returns early once that child is terminal, so the cascade walks the + * spawn tree at most once per node and is bounded by maxSpawnDepth. + * + * A cleanly-completed parent cascades nothing — background delegation is a + * legitimate pattern and its children are expected to outlive the turn. + */ + function cancelOrphanedChildren(parentRunId: string, parentEndReason: string): void { + const orphaned = selectOrphanedChildRuns(parentRunId, parentEndReason, runs.values()); + for (const childRunId of orphaned) { + killRun(childRunId, { + killedBy: "system", + reason: `its parent run ended (${parentEndReason}) with no reader left for this result`, + }); + } + } + function waitForCompletion(runId: string): Promise | undefined { const run = runs.get(runId); if (!run) return undefined; @@ -3292,7 +3320,14 @@ function classifyCompletionErrorKind( let abortClassification: AbortClassification | undefined; if (isSubAgentAbortFinishReason(result.finishReason)) { try { - abortClassification = classifyAbortReason(result.finishReason); + // Live children at the abort are exactly what the run was awaiting, + // so a timeout hint can name them instead of the timeout knob. + abortClassification = classifyAbortReason( + result.finishReason, + undefined, + undefined, + { awaitedChildRunIds: liveChildRunIds(runId, runs.values()) }, + ); } catch { /* classification must never block */ } } @@ -4350,9 +4385,13 @@ function classifyCompletionErrorKind( callerSessionKey: run.callerSessionKey, // shared dedup key callerConversation: run.callerConversation, destinationEndpoint: run.callerEndpoint, + // A bare "(system)" names the actor but not the cause; when the caller + // supplied a reason it is the only thing here that tells the reader why. detail: killedBy === "health_monitor" ? `The background task was stopped by the daemon health monitor${opts?.idleMs !== undefined ? ` after ${Math.round(opts.idleMs / 1000)}s without progress` : ""}${opts?.thresholdMs !== undefined ? ` (security.agentToAgent.subagentContext.stuckKillThresholdMs=${opts.thresholdMs})` : ""}.` - : `The background task was stopped (${killedBy}).`, + : opts?.reason !== undefined + ? `The background task was stopped: ${opts.reason}.` + : `The background task was stopped (${killedBy}).`, }, deps)); } From 003305757828871202d14919f4973295339fc494 Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Wed, 12 Aug 2026 22:10:49 +0300 Subject: [PATCH 35/36] fix(agent): kill a tree deepest-first so the cascade cannot steal attribution killByRootRun killed in map order. Once a parent kill cascades to its children, a later direct killRun on an already-cascaded child returns {killed:false}, so an explicit tree-kill under-reported (1 instead of 3) and -- worse -- children of an operator kill reached the failure record as 'system' cascades. The killedBy union exists precisely to stop a kill masquerading as another actor's. Targets are now snapshotted, ordered deepest-first so each child takes THIS call's attribution before its parent's cascade can reach it, and counted by what actually left the live set. --- packages/agent/src/spawn/abort-fallout.ts | 6 +++ packages/agent/src/spawn/sub-agent-runner.ts | 40 +++++++++++++++---- .../session-spawn-ceiling.integration.test.ts | 13 +++++- 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/packages/agent/src/spawn/abort-fallout.ts b/packages/agent/src/spawn/abort-fallout.ts index 7f9737061..a995b8c7e 100644 --- a/packages/agent/src/spawn/abort-fallout.ts +++ b/packages/agent/src/spawn/abort-fallout.ts @@ -108,3 +108,9 @@ export function promptTimeoutHint(evidence: AbortEvidence | undefined): string { return TIMEOUT_KNOB_HINT; } + +/** + * Hard bound on parent-chain walks. Depth is already limited by maxSpawnDepth; + * this is the backstop that keeps a corrupt or cyclic parent link from spinning. + */ +export const MAX_SPAWN_TREE_WALK = 64; diff --git a/packages/agent/src/spawn/sub-agent-runner.ts b/packages/agent/src/spawn/sub-agent-runner.ts index e0c217577..b2ce37875 100644 --- a/packages/agent/src/spawn/sub-agent-runner.ts +++ b/packages/agent/src/spawn/sub-agent-runner.ts @@ -75,7 +75,7 @@ import type { SendGovernedCompletionAnnouncement, } from "./announcement-ports.js"; import type { DeliveryDedup } from "./announce-key.js"; -import { liveChildRunIds, selectOrphanedChildRuns } from "./abort-fallout.js"; +import { liveChildRunIds, selectOrphanedChildRuns, MAX_SPAWN_TREE_WALK } from "./abort-fallout.js"; import { classifyAbortReason, isSubAgentAbortFinishReason, @@ -4438,14 +4438,38 @@ function classifyCompletionErrorKind( * drives; this helper itself raises nothing (the raw-throw.test.ts gate). */ function killByRootRun(rootRunId: string, opts?: Parameters[1]): { killed: number } { - let killed = 0; - for (const run of runs.values()) { - if ( - run.rootRunId === rootRunId && - (run.status === "running" || run.status === "queued") - ) { - if (killRun(run.runId, opts).killed) killed++; + // Snapshot the live tree before killing anything: killing a parent cascades + // to its children (cancelOrphanedChildren), so a direct killRun reaching an + // already-cascaded child returns false and would under-report a tree this + // call did in fact terminate. + const targets = [...runs.values()] + .filter((run) => + run.rootRunId === rootRunId + && (run.status === "running" || run.status === "queued")) + .map((run) => run.runId); + + // Deepest-first, so every child is killed with THIS call's attribution + // before its parent's cascade can claim it as a "system" kill. An explicit + // operator/parent tree-kill must not reach the failure record as a cascade. + const depthOf = (runId: string): number => { + let depth = 0; + let cursor = runs.get(runId)?.parentRunId; + while (cursor !== undefined && depth < MAX_SPAWN_TREE_WALK) { + depth++; + cursor = runs.get(cursor)?.parentRunId; } + return depth; + }; + const byDepth = new Map(targets.map((runId) => [runId, depthOf(runId)])); + targets.sort((a, b) => (byDepth.get(b) ?? 0) - (byDepth.get(a) ?? 0)); + + for (const runId of targets) killRun(runId, opts); + + // Synchronous throughout, so any target no longer live was terminated here. + let killed = 0; + for (const runId of targets) { + const status = runs.get(runId)?.status; + if (status !== "running" && status !== "queued") killed++; } return { killed }; } diff --git a/packages/daemon/src/api/session-spawn-ceiling.integration.test.ts b/packages/daemon/src/api/session-spawn-ceiling.integration.test.ts index fad796bb5..e4f25d2c1 100644 --- a/packages/daemon/src/api/session-spawn-ceiling.integration.test.ts +++ b/packages/daemon/src/api/session-spawn-ceiling.integration.test.ts @@ -334,13 +334,24 @@ describe("tree-wide spawn ceiling — driven through the REAL session.spawn path const c1 = await h.spawnViaHandler({ task: "c1", callerConversationScope: parentRun.conversationScope }); const c2 = await h.spawnViaHandler({ task: "c2", callerConversationScope: parentRun.conversationScope }); - const killed = h.subAgentRunner.killByRootRun(parentRoot); + const killed = h.subAgentRunner.killByRootRun(parentRoot, { killedBy: "operator" }); // The whole tree (parent + 2 children) is reached — not just the parent. expect(killed.killed).toBe(3); expect(h.subAgentRunner.getRunStatus(parent.runId)?.status).toBe("failed"); expect(h.subAgentRunner.getRunStatus(c1.runId)?.status).toBe("failed"); expect(h.subAgentRunner.getRunStatus(c2.runId)?.status).toBe("failed"); + + // An explicit tree-kill owns its attribution end to end. Killing the parent + // first would let the orphan cascade claim the children as "system" kills, + // which is the misattribution the killedBy union exists to prevent — so the + // tree is killed deepest-first. + for (const child of [c1, c2]) { + const summary = h.subAgentRunner.getRunStatus(child.runId)?.completion?.summary; + expect(summary, `child ${child.runId} was cascaded, not operator-killed`) + .toContain("operator"); + expect(summary).not.toContain("its parent run ended"); + } }); it("(d) a completed run RELEASES its slot so a later spawn on the same root is re-admitted", async () => { From 717c5d3b3b64e15d751ee446ecf8fdedc86f9bbd Mon Sep 17 00:00:00 2001 From: Moshe Anconina Date: Thu, 13 Aug 2026 00:06:44 +0300 Subject: [PATCH 36/36] test(integration): DLQ success is INFO, not DEBUG The resolution log was deliberately promoted to INFO so a drained quarantine is visible without debug logging -- a DEBUG-only resolution left a standing WARN and no visible outcome, which reads as a lost user announcement when it is the opposite. This integration test still pinned DEBUG. The runtime change and its asserting test belong in one commit; this one was missed because the integration tier runs in a separate config that pnpm validate does not execute, so it stayed green locally and failed only on CI shard 4. Its real intent -- a successful delivery must never be an ERROR -- is unchanged and still asserted. Verified against the real module: enqueue -> drain delivers (1 send, size 0) and logs 'Dead-letter entry delivered successfully' at INFO. --- test/integration/resilience-e2e-dead-letter.test.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/integration/resilience-e2e-dead-letter.test.ts b/test/integration/resilience-e2e-dead-letter.test.ts index a654b5bf4..2b2ba1f40 100644 --- a/test/integration/resilience-e2e-dead-letter.test.ts +++ b/test/integration/resilience-e2e-dead-letter.test.ts @@ -351,7 +351,7 @@ describe("resilience E2E: dead-letter queue retry pipeline", () => { // Log level verification for DLQ delivery // ------------------------------------------------------------------------- - it("successful DLQ delivery logs at DEBUG level (not ERROR)", async () => { + it("successful DLQ delivery logs at INFO level (not ERROR)", async () => { const eventBus = new TypedEventBus(); const logger = createMockLogger(); @@ -382,9 +382,13 @@ describe("resilience E2E: dead-letter queue retry pipeline", () => { expect(dlq.size()).toBe(0); - // Verify DEBUG log for successful delivery (not ERROR). - // The DLQ uses logger.debug for successful delivery. - expect(logger.debug).toHaveBeenCalledWith( + // Verify the successful delivery is reported, and is NOT an ERROR — that is + // what this test guards. The level is INFO, not DEBUG: a drained quarantine + // logged only at DEBUG left a standing WARN with no visible resolution at + // the default level, which reads as a lost user announcement when it is the + // opposite. Diagnosability must not depend on debug logging having been + // enabled before the incident. + expect(logger.info).toHaveBeenCalledWith( expect.objectContaining({ runId: "run-4" }), expect.stringContaining("delivered successfully"), );