fix: fourteen production defects found by log review, plus the quarantine operator lever - #498
Merged
Merged
Conversation
A sub-agent executes under a synthetic `sub-agent:runtime:<runId>` 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.
The approval gate required `turnScope.endpoint` to equal `deliveryOrigin` on every field. A delegated run executes under a synthetic `sub-agent:runtime:<runId>` 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.
…redicate 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.
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.
`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.
Investigating a quarantined announcement, the WARN hint pointed at <dataDir>/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.
…ible The quarantine WARN named <dataDir>/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.
…error 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.
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.
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.
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.
`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.
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.
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: <the actual answer, in full> 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.
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: <the answer> 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 (<finishReason>)` 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.
…inal 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.
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.
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.
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.
…nouncements 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.
…ments `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.
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'.
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.<id>.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.<id>.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.
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.
`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.
…e cap `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.
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.
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.
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.
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.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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.
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'].
…y 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.
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 <childRunId>' 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.
…ribution
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.
anconina
changed the title
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fourteen defects found and fixed while reviewing production logs on a live deployment, plus the operator lever the investigation showed was missing. Every fix is deployed and verified against the real host.
They share one theme: a message or a lens that misleads its reader. Several were not wrong code so much as code that told an operator — or a user — something untrue.
Delivery and execution
Sub-agents could never obtain an approval.
resolveApprovalRequestContextrequiredturnScope.endpointto equaldeliveryOriginon every field. A delegated run executes under a syntheticsub-agent:runtime:<runId>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 was denied — exec, terminal, orchestrate, pipeline, gateway, memory_manage, channels_manage, admin_manage all share the gate. The two routes are authenticated independently and are intentionally different (DeliveryOriginis documented as surviving sub-agent spawns; spawn admission already rejects a mismatched announce route). Split the check by what each half proves: the authority half is unchanged, the endpoint half now applies only to deliverable endpoints.A wrong-script reply killed the turn. When bounded locale repair could not reach the enforced script, the branch discarded the model's answer for a canned line and set
finishReason=error. This was the sole cause of every hard failure in the observed window. It contradicts the documented contract — "every non-Latin capability has a working, visible, lower-fidelity floor, nothing hard-fails" — and the three sibling branches in the same function, which all preserve the response. A wrong writing system is a presentation defect, not an execution failure.A delivered answer was announced as
Status: Failed/Result: Error: <the answer>.completed_with_tool_errors— a finish reason whose name says completed — was missing from the clean-stop check, so a run that answered was resolved asmodel_halted; and the failed branch routed the child's own response into theerrorslot. Fixing the classification made the pre-existingCompleted (<finishReason>)branch reachable: it was never dead code, only unreachable behind the wrong verdict.Messages that misled
The breaker block message nested inside itself. The builder embedded
lastErrorraw between quotes while the peeler parsed it as a JSON-escaped string; real error text contains quotes, so the parse threw and each retry wrapped the previous message. Observed four levels deep, and because the clause is capped at 150 chars, by round nine the message carried no real error at all — just recursive counters. A documentedINVARIANT: 'has failed' at most oncewas live-violated.sessions_spawngave contradictory re-spawn guidance. Per-tool hints each recommended a group computed for their own tool, so a spawn needingweb_search+web_fetchwas told to use['cron-minimal']and['full']in one message. A caller passes one list; obeying either fails on the other tool. Now one directive, intersected across all unreachable tools, and a denylisted requirement emits none at all.The quarantine hint sent operators to a file that had been deleted.
dead-letters.jsonlis a snapshot, unlinked when the queue drains, and the resolution logged at DEBUG — so at the default level a resolved quarantine left a standing WARN, no file, and no trace of the outcome. That reads as a lost user announcement; it is the opposite. Hint now states the lifecycle, resolution promoted to INFO, and the docs corrected (they described the file as append-only and only-ever-created, which is what created the wrong mental model).Observability lenses
explainblamedrecall_misson turns that died.endedInTerminalExecutionFailurecounts anerrorend reason as a death, butterminalFailureKinddiscarded that case without a failed finalize, so all three terminal verdicts returned null and the incidental zero-hit recall won. Two predicates in one file disagreed about what a death is; they now share one.A soft degradation downgraded a hard failure out of
system-health.degradedwas sticky but the cause was last-degraded-wins, so a latercompleted_with_tool_errorsoverwrote an earliererror. SincehardDegraded = degradedCount − deliveredWithToolErrorsCount, the death was subtracted out of the daemon-wide view along with its traceId — the report moved from "1 hard-degraded, 50%" to "0 hard-degraded, 0%" across a window in which nothing had been fixed.The missing lever
comis quarantine list/release. A quarantined announcement is held by design, but the runtime offered no way to act on one: clearing it meant stopping the daemon and editing the JSONL, because the running queue is authoritative and rewrites that file from memory on the next persist. Two admin-only RPCs and a CLI group close it. Daemon-only on purpose — an offline path would reintroduce the exact trap.Step limits
The step-limit message named a knob that could not raise the limit. A halted deep-research sub-agent told its operator to raise
agents.default.maxSteps— a key that defaults to 150 and does not govern sub-agents;comis explainrepeated it in theexecution_step_limit_reachedverdict. The ceiling that bound wassecurity.agentToAgent.subAgentMaxSteps. The second hint was wrong the same way: "Increase max_steps in sessions_spawn" points at a parameter thatsetup-cross-session-graph.ts:83clamps withMath.min(maxSteps, configMaxSteps), so a caller can only ever lower it. Both messages sent operators somewhere that could not work, after the run had already spent $1.15 and 1.1M tokens across 18 searches and 21 fetches and returned nothing. The knob now travels with the counter, mirroringdescribeTimeoutKnob, which already threads a timeout's source for exactly this reason.security.agentToAgent.subAgentMaxStepsdefaulted to 50, which no research delegation fits in. Since a spawn'smax_stepsis clamped to it, 50 was the reachable ceiling for any default deployment — the runtime silently could not complete a whole class of delegation. Now 300. This ceiling bounds runaway loops; it is not a cost control (observability.spendand the per-spawn token budget are, and both still apply), so it is set where honest work fits. This raises the reachable ceiling 6x for every deployment — reviewers should confirm they accept that.Delegation reachability and fallout (a second incident, investigated after the first eleven landed)
A browser-based market scan timed out at 241s having produced nothing. The agent's own report said "the scope was too broad for one run; the practical next step is three smaller parallel browser scans" — it had already spawned three children. That self-diagnosis was invention, and it reached the user as if it were a finding. Ground truth from
comis explainplus the daemon log: the parent aborted at step 16 of 50, having spent 208 of its 241s blocked insubagents wait.A reachability rejection could only ever answer "use
full".computeReachableToolNames(the gate) validates againstSUB_AGENT_TOOL_PROFILES∪SUB_AGENT_TOOL_GROUPS, andtool_groupsisz.array(z.string())— free-form.groupsReachingAll(the suggester) searched profiles only. Fourteen tools live in a group and no profile —web_fetch,browser,memory_get,pipeline, the wholesessions_*surface — so for any of them the suggester could name nothing and fell back to'full', while the message's own "Valid groups are … — any other value is ignored" clause declaredweb, the one ceiling that would have worked, invalid.sessions_spawnwas rejected three times, the breaker opened, and the three children that did spawn inherited thecodingdefault: market research with no web tools, each burning its 40-step cap.docs/agent-tools/sessions.mdx:154already documented"web"as valid — the docs were right and the runtime string contradicted them. Both sides now shareceilingCandidates(), ordered by reachable-tool count so a reachability error yields least privilege instead of an escalation to unconstrained.Orphaned children outlived the parent that could no longer read them. The parent died at 17:28:46; two children 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. Corroborated at restart by two
Completion announcement delivery route mismatchERRORs at 17:29:18 and 17:29:37: the orphans finished and their results could not even be routed, because the parent's route was gone.terminalizeRunnow cascades on any non-completedend reason. A cleanly completed parent cascades nothing — background delegation is supposed to outlive its turn.The timeout hint named a knob that could not help.
prompt_timeoutemitted "Increaseagents.<id>.operationModels.subagent.timeoutor reduce the task scope" for every timeout — and "reduce the task scope" is verbatim what the agent relayed to the user as its own diagnosis. When a run died waiting on doomed children, raising that knob buys more waiting. The hint now branches on the live-child set at the abort and hands over a copy-pasteablecomis explain "<childRunId>". The same computation feeds both cancellation and wait-evidence, so they can never disagree.Landed alongside: an explicit tree-kill no longer loses its attribution.
killByRootRunkilled in map order, so once the new cascade fired, an operator tree-kill reached the failure record as asystemcascade — the exact misattribution thekilledByunion exists to prevent — and under-reported 1 killed instead of 3. Trees are now killed deepest-first. Nothing asserted that invariant before; it now has a test.Related Issue
N/A: found by reviewing production logs on a live deployment; there is no pre-existing tracking issue. Each commit cites the observed incident (traceIds and timestamps) in place of an issue link.
Type of Change
Checklist
pnpm validate)N/AreasonThe focus box is deliberately unchecked: this is eleven concerns from one investigation, landed as twelve RED→GREEN pairs plus three refactors. Each pair is a self-contained commit and reviewable in isolation; splitting them into separate PRs was possible but they were verified together against one host.
RED Test Proof
Every production change landed as a test-first pair. Each RED below failed on the pre-patch code:
Evidence and Residual Risk
explain: the misdiagnosed session now returnsexecution_terminal_failureand names the locale/delivery seams it took a raw log grep to findsystem-health: hard-degraded went 0 → 2, correct because the earlier death is no longer downgraded — the number got worse because it got honestStatus: Completed (completed_with_tool_errors)with the answer as the resultcomis quarantine: listed and released the actual stuck announcement, ending with an empty queue, the snapshot file removed, and an INFO audit linesubAgentMaxSteps = 300from the schema (the host sets no override), verified by resolving the config on the installed build rather than grepping the filepnpm validate. Telegram channel,openai-codexprovider, sub-agent and channel turn scopes, existing state (a real~/.comiswith live history).pnpm validateis the cross-platform floor. The Docker image build,*.linux.test.ts, and the integration tier did not run for this change set.web_fetchredirect policy versus site bot-protection, and pairing a quarantine with its resolution insystem-healthfindings (info-severity rows are excluded from findings by design).Additional Notes
Landed as stacked branches, merged here from the tip; the commit sequence is readable in order. Findings 12-14 come from a second incident investigated after the first eleven were already deployed.
Two process notes worth a reviewer's attention, both self-inflicted and both caught only by testing against reality:
tsc --noEmitand never rebuilt, then shipped a staledist. The deployed command failed withInternal server error; invoking the handler directly gaverows.map is not a function.One correction carried through the commits: I initially reported
dead-letters.jsonlas never written. It is written — it is a snapshot, unlinked when the queue empties. The docs said "append-only" and "only created when…", which is what produced the wrong reading, and they are fixed here.Two further self-inflicted traps from the second investigation, both caught before shipping:
@comis/coredist masked a broken test. I reported "1193 passing, no regressions" for finding 12 — measured beforepnpm build. Once core's dist refreshed, a test written earlier in the same session failed: it assertedtool_groups:['full'], encoding the escalation as correct. It is retargeted here, and reviewers should read that diff specifically.session-mutate.ts:320inherits the announce channel fromrequesterOrigin, so every child has one and that gate would have made the cascade a no-op in production — a third inert fix, avoided by reading the resolving code instead of assuming the field's meaning.One tradeoff reviewers should confirm they accept: the cascade cancels all live children of an abnormally-terminated parent, including any that might have announced a usable partial result. The alternative is the status quo that spent $1.80 on results with no reader, and a cancelled child still delivers a failure notification naming the cause, so nothing vanishes silently. Reversing it is a one-line predicate change.