fix: harden agent execution and incident observability - #364
Merged
Conversation
added 30 commits
July 28, 2026 00:37
…, name every failure Three silent behaviour bugs and twelve diagnosability gaps, found by reviewing a production session and then reproducing it end-to-end through the Telegram emulator on the same provider/model. Behaviour fixes * Fresh-tail clamp was a constant. resolveClampedFreshTailTurns estimated a step at W/20, which cancels against the 30% budget: floor(0.3W / (W/20)) = 6 for EVERY finite window and every configured value, so contextEngine.freshTailTurns (1..50, default 8) was unreachable above 6. On a long tool loop the user's originating request slid out of the verbatim tail while the window sat 91% empty, and the agent then answered as though it had never been asked. Replaced with an absolute per-step estimate calibrated so the change can only ever keep MORE context: at the smallest viable window (8192) it reproduces the previous 6 exactly, and every larger window now reaches its configured value. The two existing auto-estimate tests could not catch this — both only asserted 1 <= result <= configured, which a constant 6 satisfies. * Daemon crash-looped on a supported config. constructCapabilityLayer returns no capability endpoint when no agent has autonomy enabled; setupProactiveSchedulers treated it as mandatory and the daemon threw, completing its whole boot and then exiting 1 — forever, while systemctl reported active. Omitting the autonomy block defaults to enabled, but setting any autonomy sub-key without enabled: true resolves to disabled, so adding one documented knob bricked the box. Now boots with an ERROR naming what is off and the knob; any other missing dependency still aborts. * Response locale is pinned, not switched per message. Only the explicit agents.<id>.language pin enforces the reply language; the channel-supplied and script-derived tiers are advisory. Inferring from the current message alone let a single message switch a whole conversation and then burn a repair round-trip fighting a model that was already correct. * range_token was redacted as a secret. The .*token field-name wildcard rewrote a plain enum argument to the redaction placeholder, which the model then read back from its own replay context and sent to the MCP server. Narrow, audited exact-name exception set — never a pattern loosening — with an exhaustive negative matrix. * A failure with no attributable event no longer renders a bare errorKind pill to the user (UNATTRIBUTED_FAILURE_REASON). * Approval-gated env_set no longer tells the caller to re-read a value the redaction pass has already removed — which could only be satisfied by asking the user to re-send a credential through the chat. Both messages now name the deadlock, forbid re-transmission, and give the out-of-band path. Diagnosability * explain: an unresolvable sessionKey yields session_not_found instead of an all-zero report indistinguishable from a healthy session. * explain: contextBudget carries freshTailSteps/freshTailStepsConfigured, plus a fresh_tail_clamped verdict — the fresh-tail slide was previously invisible behind a clean "fits". * One failure produced up to 17 trajectory records; a dispatch redelivery is now marked and skipped by the bridge. * Cache breaks were logged twice under one message, double-counting every lens. * system-health reports the hard-degraded split it already fired findings on, so the table and its own JSON can no longer disagree. * totalTokens declares its counting convention (tokenBasis) — the two lenses count different things and neither said so. * Cache-churn attribution parses the block count and names block-count-changed instead of "unknown". * Hints that pointed the wrong way now name the knob: the lookback cache break no longer claims "no action needed" on a priced break; an MCP timeout names integrations.mcp.callToolTimeoutMs and its value and says an identical retry re-expires it; the locale repair names the resolver tier rather than blaming the model; a dropped memory distinguishes a real credential from a heuristic match. * The CLI no longer suggests re-running init when the gateway token is in the encrypted store — following that advice rotates a live token. Also fixes a latent two-sources-of-truth drift: the MCP client fell back to a 60s call timeout while the schema defaulted to 120s, silently halving the deadline on any un-threaded wiring path. Both now reference one constant, pinned by a parity gate. Docs updated in the same change for every behaviour above, including two new troubleshooting runbooks (cron/heartbeat never firing, and gateway-token recovery under encrypted storage).
Both cost a cycle while standing up a live-test box. A HEAD-only symbol grep run as root returns a FALSE NEGATIVE: /home/comis is 0700, so the Permission-denied goes to stderr while `grep -rl ... | wc -l` prints 0 on stdout. That is indistinguishable from "the deploy did not land" and sends you chasing a packaging bug that does not exist. Run the proof as the service user, and cross-check the packed tarball when in doubt. `sudo -u comis -i bash -lc "..."` mangles a multi-line loop into `syntax error near unexpected token 'do'` — use a `bash -s` heredoc instead. Same trap family as the existing `su - comis -c` quoting note.
…cident run
Second pass over the same live incident: the four ledgered findings plus the small
frictions, each fixed test-first and the headline one re-proven on the wire.
Backgrounded tools tell the truth end to end. A background hand-off returns a
non-error placeholder, so the activity card closed a still-running tool as
"completed" — four reports that later timed out were each shown as succeeded, and
the turn then finalized failure with nothing to name, pinning a failure pill above
a delivered answer. Four coupled layers:
* tool:executed carries `backgrounded`; the activity stream leaves the card
running on a hand-off instead of closing it.
* background_task:{completed,failed} carry the promote-time toolCallId/
sessionKey/traceId (captured in the middleware, persisted on the task), so the
terminal event can close the card it belongs to — previously impossible: the
events had only taskId while cards are keyed on toolCallId.
* The stream closes the card on the real terminal (redeliveries skipped;
a pre-upgrade record without correlation is a no-op).
* A failure whose answer WAS fully delivered reclassifies to
success_with_recovered_failures when the failure is attributable to observed
events — never plain success (no events ⇒ the truthful failure stands), and
never for a resource abort (a stopped run renders as stopped).
Live re-proof on the rig, same turn shape as the incident: in-flight failures
shown WITH the tool name, artifact delivered, card ends "✓ done" and is removed,
finalize reads success_with_recovered_failures with failedEventCount 2 — where
the incident showed a kept "❌ dependency" over the user's file.
Secret field names match on keyword BOUNDARIES, pairs-only. The suffix-anchored
pattern missed any credential keyword followed by more name
(AWS_BEARER_TOKEN_BEDROCK, SECRETS_MASTER_KEY). The fix matches qualifier+keyword
segment pairs (bearer+token, master+key, client+secret, db+password, …) rather
than bare keywords: a first cut with bare `secret` false-flagged the stock
`security.writeSecretGuard` boolean, which made persistToConfig's plaintext-secret
guard abort — every config write would have failed. The default config now scans
clean by construction, and meta names (secretName, passwordResetRequired) are
pinned negative alongside the counting vocabulary.
A gated secret write survives its own confirmation. The approval gate needs a
second call, and session redaction rewrites the secret in context between the two
— the only recovery was asking the user to re-paste a credential into the chat.
env_set now stashes the request server-side and returns a pending_action_id; the
confirm passes the id back and the daemon replays the original value (one-shot,
5-minute TTL), even when the model's context re-supplies the redaction
placeholder. The secret never has to survive in conversation history.
Audit rows are joinable. secret:accessed carries optional sessionKey/traceId; the
sink prefers them over AsyncLocalStorage (empty for boot reads — why every live
row had traceId null) and tags refs.origin boot|request so no-context-by-design is
distinguishable from context-lost. PATH-class environment names no longer dilute
the trail.
Also: the LLM-input debug line carries a digest + executionId so it joins to the
trajectory's prompt.submitted record; URL extraction strips the quote family
(a config snippet's closing quote rode into the fetch and 404'd); a PEP 668
pip refusal gets a recovery hint naming the venv and --break-system-packages paths
up front; the learning_health finding points at the outcome-judge no-op when
coverage starves; the live-test env template uses default-assign so an inline
override can no longer be clobbered into targeting the wrong box.
Empty-turn recovery exists to stop a SILENT turn — a tool batch that finished
with no assistant text and told the user nothing. It fired on a turn that had
already answered.
Live shape: an onboarding turn sent its question with
message({action:"send"}) rather than returning it as the final assistant text.
The final message was therefore empty, recovery treated the turn as silent, and
the user received a second bubble on top of the real answer:
[comis: tool-call summary recovered from successful operations — the
assistant's final message was empty]
Completed 1 tool call in this batch:
• message({action: "send"})
The work was done; the assistant did not summarize. Please ask "what did you
do?" if details are needed.
User actions: 9b1ba054
That is internal scaffolding — it names the tool protocol, instructs the user to
interrogate the assistant, and trails a raw artifact id.
Suppress the synthesis when the batch itself put words on the user's channel: a
message send/reply carrying non-empty text. Everything that delivers NO words
still synthesizes, so the silent-turn safety net is unchanged — a react, an
attach with no text, and any non-message tool batch all keep the summary.
The turn that took the tool path is left with an empty final response rather
than a fabricated one; the INFO line records the suppression with the tool names
so the underlying behaviour (a reply routed through the message tool instead of
the response path) stays diagnosable.
…ol-loop The LCD assembler concatenates store-backed history with a fresh tail sliced from the live array at a step-count boundary. Those two indices advance on different clocks: the store is written at afterTurn, so its horizon is frozen for the whole turn while the step boundary marches forward one step per LLM call. Once a turn ran more steps than freshTailTurns the boundary overtook the horizon, and every live message in between was assembled into neither segment. The first casualty is always the user message that STARTED the turn -- it sits exactly at the horizon, unpersisted until the turn ends. A long tool loop therefore dropped the request it was still serving, and the model answered as though it had never been asked, contradicting the user about what they wanted. Nothing reported it. The loss is not an eviction, so verdict stayed "fits", droppedCount stayed 0, and the window sat 91% idle. Ground truth from a live session: the store horizon held at 56 across a 7.5-minute turn while tailStart walked 41 -> 57; the final assembly shipped 72 of 73 live messages, and the one missing was the user's own request. Raising freshTailTurns cannot fix this -- it only moves which step opens the hole. Clamp the fresh-tail start to the persisted horizon, so freshTailTurns is a FLOOR on verbatim recency rather than a ceiling and anything the store cannot supply rides the verbatim tail instead. Gated on a complete read scope: countMessages answers 0 for an empty store, an incomplete (fail-closed) scope and a corrupt count alike, and clamping to 0 under fail-closed would make the whole live array unconditional and overflow a tight window. Clamping also stops the tail start sliding mid-turn, so the tail grows by pure suffix-append. Reconcile the concat seam against the live array on the INFO line that already rides every LLM call, netting out both deliberate removals (budget eviction and the residual trim) and measuring before transcript repair -- so a future regression here announces itself instead of needing a two-log-line hand-join. Separately, the MCP call-timeout hint closed by telling the caller to raise integrations.mcp.callToolTimeoutMs, which is an immutable config path. The agent followed the advice, the gateway rejected the patch, and the rejection surfaced to the user as a bare "[tool failure] gateway reported an error" stacked on top of an otherwise correct answer. The hint now names the knob for the operator and leaves the caller only the remedy it can actually apply. Also renames the fresh-tail-bound log's stepCount/stepSizes to tailSegmentCount/segmentSizes: that grouping opens a segment at every non-toolResult message, so it counts user messages too, unlike freshTailSteps which counts assistants. Reading one as the other inverts the diagnosis -- a segment count of steps+1 is the healthy shape, not an over-bound turn.
…et call An approval-gated secret write returns `requiresConfirmation: true` with an opaque `pending_action_id`, and instructs the model to re-call with that id and `_confirmed: true` while OMITTING the value -- the value is replayed server-side precisely because it is about to be redacted out of the model's context. That handle is the whole mechanism for completing a gated write without asking the user to send the credential twice. scrubRedactedToolCalls threw it away. The scrub keys on the tool_use ARGS carrying the redaction placeholder, which is correct -- the args hold the secret. But it then also replaced the matching tool_RESULT, which holds no secret at all, with an opaque "(prior secret operation -- no output shown)", and summarised the assistant turn as "The action completed; do not retry" for a call that had never run. Live consequence, both halves visible in one session: the user pasted MCP credentials, three env_set calls each returned a pending_action_id, the user replied "yes" -- and the agent had no handle to confirm with and believed the secrets were already stored. It listed secrets, found zero, and asked the user to resend the username and password in cleartext over the chat channel. That is the exact deadlock the pending_action_id was introduced to break, defeated one layer below where it was implemented. Index the gated results by toolCallId before the scrub runs, then carry ONLY the opaque id through: the result replacement names the id and the confirm shape, and the assistant summary states the action is pending confirmation rather than complete. Nothing else from the result survives -- the id is extracted, never the surrounding payload -- so the secret still never re-enters replay. Ungated redacted calls keep their existing text byte-for-byte.
The response-locale resolver has three tiers: an operator pin (`agents.<id>.language`), a channel-supplied request locale, and a script inferred from the current message. `docs/operations/multilingual.mdx` already specifies the contract -- "the two inferred tiers ... are advisory: they inform the prompt but never trigger a repair", and only the operator pin enforces. The code did not match its own spec. The request tier was built with enforceLocale: true, which made a DEVICE SETTING the strongest signal in the system -- stronger than the language the conversation was actually being held in, whose script-derived tier is advisory by design. Live: a Hebrew conversation, correctly answered in Hebrew from the advisory und-Hebr tier. The next message was an English technical instruction, which does not contradict a Telegram client language_code of "en", so the transport tier engaged, outranked the conversation, and the agent switched to English and stayed there. The resolver's own comment already called this tier "TRANSPORT metadata (a client UI language_code, a caller-supplied field) -- a device setting, not the conversation's language"; it just was not treated that way. Mark the request tier advisory, matching the script tier beside it and the documented contract. An operator who needs a guaranteed reply language sets `agents.<id>.language`, which still enforces and still outranks the device locale. The script-contradiction guard is unchanged. Updates the five assertions that encoded the old enforcing behaviour, and makes the config reference state plainly that only the explicit key enforces -- it previously read as though any resolved locale could trigger a repair turn.
…failed
The per-tool retry breaker is recorded against the tool that RETURNED the error.
Auto-backgrounding invalidates that assumption: it converts a slow tool into a
non-error "moved to the background" result, so the originating tool reports
SUCCESS on every launch and its consecutive-failure counter resets each time. It
can never trip. The real failure surfaces later on the `background_tasks` POLLER,
which relays it -- and the poller trips instead.
The effect is the exact inverse of what a breaker is for: the agent loses its
ability to OBSERVE outcomes while the tool that is actually failing stays
completely unthrottled.
Live: a heavy report tool was launched 20+ times between 19:45:02 and 19:53:54,
each burning the full 120s MCP call deadline, against a 600s execution budget.
The breaker did fire -- `tool.breaker_opened {toolName:"background_tasks",
consecutiveFailures:2, errorTag:"conflict"}` -- but on the poller, so nothing
slowed the launches. The turn died at 599s on `execution.aborted
{reason:"pipeline_timeout"}` and the user got a canned error after ten minutes
of waiting.
Both halves are needed; either alone makes it worse. Stop blaming the poller for
relaying a failure it merely reported (its OWN failures -- a bad taskId, a
storage error -- still count, so this is not a blanket amnesty), and count the
failure against the originating tool. `background_task:failed` already carries
that tool name, stamped by the manager from the task record, so nothing has to be
parsed out of the poller's prose error text, which names the tool only in
free-form English.
The subscription's lifetime matches the per-execution breaker it feeds and is
torn down in the same finally as `unsubscribeCacheTrace`, so no listener outlives
the execution that owns it.
…imeout reply Two defects put the deterministic platform replies permanently in English no matter what `agents.<id>.language` was pinned to. Live: a Hebrew deployment whose agent answered in Hebrew all turn, then delivered an English canned line when the turn hit the wall-clock ceiling. 1. The pipeline-timeout reply was a hard-coded literal at the orchestrator send site, outside the degraded-reply mechanism entirely — the one message a stuck turn is guaranteed to produce was the only one no pack could reach. It is now the `pipeline_timeout` member of `LocaleMessageId`, built by `buildPipelineTimeoutReply` and carrying the incident ref like its siblings. Its text also no longer claims a transient "try again in a moment" for a turn that burned the whole ceiling and will do so again unless the ask is narrowed; it names the real cause and the actual remedy. 2. `createLocaleCatalog` was an injection seam with no production caller. The only real call site, `executor-post-execution.ts`, passed no catalog, so `DEFAULT_LOCALE_CATALOG` (no packs) resolved English for every reply — and the config reference's claim that `language` is "consumed by the deterministic degraded replies" had nothing behind it. The fix wires the seam to operator config via a new `agents.<id>.localePacks` (locale tag -> message id -> text), threaded to both send sites. The engine still ships exactly one pack, English: shipping a curated set of languages would be the runtime choosing a preferred human language, which the generic- runtime boundary forbids, and would leave every unshipped language silently English anyway. `catalogFromLocalePacks` validates ids at the boundary and logs an unknown one at WARN naming the exact config path, so a typo in a pack is never silently dead config. `PlatformReplyLocale` lives in its own leaf module: both `ExecutionPipelineDeps` and `execution-execute.ts` need the shape, and declaring it in either creates a .d.ts back-edge the cycles gate rejects. Docs corrected in the same change — both the config reference and the multilingual guide asserted `language` localized these replies. They now state that it does not, and that `localePacks` is what does.
…op the false "running"
Four defects found by replaying an operator's real message sequence against a
live MCP server. The heavy report turn they all landed on went from a 603s
wall-clock timeout to a 24s honest answer.
**The call deadline was not a deadline.** `resetTimeoutOnProgress` restarts the
timeout on every progress notification and the SDK applies no ceiling unless
`maxTotalTimeout` is passed ("If not specified, there is no maximum total
timeout"). So `integrations.mcp.callToolTimeoutMs` silently degraded into a
per-progress-GAP timeout while the config docs, the key's name, and the expiry
hint's promise that an identical retry "re-expires the same deadline" all
called it the total. Ground truth: a 120000ms cap with recorded call durations
of 200877ms and 106246ms, and one call free to consume an entire turn budget.
Passing `maxTotalTimeout` makes the configured value mean what it says.
**Every JSON-Schema constraint was stripped from MCP tool schemas.**
`jsonSchemaToTypeBox` mapped `type:"number"` to a bare `Type.Number()`, dropping
`minimum`/`maximum`/`enum`/`pattern`/`description`/`default` — and that
converted schema IS the model-facing `parameters`. The model could not see
bounds the server had published, so it guessed and took an MCP -32602 rejection
per guess: four of eight tool failures in one live turn were exactly this
(`page_size` below a minimum of 10, `top_n` at 200 against a maximum of 100, an
out-of-range enum). Constraint keywords are now carried through verbatim,
including for a typeless `{enum:[...]}` that previously collapsed to Any.
**A finished frame kept claiming to be running.** The "(running N s)" suffix was
appended whenever the caller supplied `elapsedMs`, with no reference to whether
anything was in flight — so a failed tool rendered "❌ … (running 0 s)", an
outcome and a running claim on one line, and a finished turn left a pill
reading "(running 475 s)" ticking after the work was over. Gated on an event
actually being in flight; an empty frame still opens with (running 0 s).
**A config parse error crash-looped with no reason.** The parser's own message —
which carries the line and column — went only into `details`, while `message`
is the single string the boot FATAL prints. A broken edit produced "Failed to
parse config file: <path>" and nothing else, on a systemd restart loop that
repeated it 40 times. The reason now rides the message; only its first line is
taken, so a YAML source excerpt never reaches a log.
…e timeout advice
Two observability defects from the same live incident as 2f1fd8b92.
**A breaker refusal was reported as a 3ms timeout.** `buildBlockReason` quotes
the original failure verbatim (`…with the same error: "…timed out…"`), and the
bridge fed that text to the MCP error classifier, which read the QUOTATION as a
fresh timeout. The blocked call was then published as
`tool.timeout {timeoutMs: 3}` and `errorKind:"timeout"` — 3ms being how long the
breaker took to refuse, presented as an expired 120000ms deadline. That is a
false report at the exact surface an operator reads to find a real one, and it
inflates the timeout population every timeout heuristic keys on.
A blocked call never reached the server, so it has no transport outcome to
classify at all. It is now recognized before the classifier and recorded as
`precondition` / `runtime_guard` / `transportOk:false`, so no `tool:timeout` is
emitted for it. The recognizer lives with the producer (`isBreakerBlockMessage`)
and requires BOTH structural invariants `buildBlockReason` guarantees, so a tool
that merely echoes one phrase is not mistaken for a block.
**`provider_timeout` led its advice with the one step that makes things worse.**
"raise the per-call timeout" named no knob, and a turn's wall-clock budget is
fixed — a longer per-call deadline just buys a longer burn before the same
abort. Narrowing now leads. The deadline is still named, but by its exact key
and with the fact that it is an immutable path an agent cannot patch, so the
reader knows the step needs an operator and a daemon restart.
… authoritative Two corrections to 2f1fd8b92 and c777c5501, both found by re-verifying them against the live box instead of trusting the unit tests. **The absolute ceiling was scoped to the tracing branch.** `maxTotalTimeout` was added inside the `requestTraceId` conditional next to `resetTimeoutOnProgress`, so it applied only when a trace context happened to exist — and the untraced paths, which is exactly where a long-running background call runs, kept no ceiling at all. A deadline enforced only when tracing is on is not a deadline. It is now passed unconditionally, alongside (not inside) the progress reset. The two exact-equality assertions on the SDK options object are updated: the ceiling being present on every call IS the contract. **A breaker refusal now overrides rather than falls back.** The first cut gated it on `toolErrorKind === undefined`, which makes the classification depend on whether some upstream detector had already assigned a kind by sniffing the same quoted text. A refusal is authoritative — the call never ran, so nothing in the text is evidence of a transport outcome — and it must not be order-dependent. The bridge test locks the contract: a refusal quoting a timeout is `precondition` and emits no `tool:timeout`. Honesty note on the second one: I have not observed the ordering actually flip in production, and the regression test passes against the previous code too. This is defense-in-depth on a real semantic error, not a fix for a reproduced failure.
…ifespan, not the call Read as a call duration and compared against `integrations.mcp.callToolTimeoutMs`, this field manufactures a phantom deadline breach: the terminal state is committed on a poll, so the span carries launch and polling latency on top of the call. Live, a correctly-capped 120000ms call surfaced here as 138841ms and was twice mistaken for an unenforced deadline — once in a written findings ledger, once in a fix aimed at the wrong layer. The declaration now names the span and points at `tool.result`'s durationMs for the call itself.
…kground poller Found by re-running the operator replay end to end. A Hebrew reply came back with `[tool failure] background_tasks reported an error` stapled to it — a bare, bracket-tagged English internal string appended to an otherwise complete answer in another language. Two defects in one line. **It was outside the locale seam.** Every other deterministic platform string now resolves through `LocaleMessageId`; this one was a literal at its append site, so no operator pack could reach it. It is now `tool_failure_notice`. The tool NAME still rides along verbatim — identifiers are never translated (the no-translation principle in docs/operations/multilingual.mdx); only the sentence around it is localized. **It named `background_tasks`.** That is the POLLER, which relays other tools' failures — so the notice pointed the reader at the one tool that was working. The same mis-attribution the retry breaker had in 463c55646, surfacing at the user-facing layer instead of the breaker. The notice now prefers a real failing tool and falls back to a nameless notice rather than blaming the messenger. The two source-grep contract tests that pinned the old literal are updated to pin the new contract: built through the seam, and never naming the poller.
… read it The "## Task Delegation" policy — the >30s rule, the media/multi-file/deep- research criteria, the parallel fan-out instruction — reaches the model through exactly one path: SYSTEM_PROMPT_GUIDES["sessions_spawn"], which jit-guide-injector appends to a tool result only after sessions_spawn has been called successfully. That ordering is circular. The policy exists to make the model reach for the tool, and it is unreachable until the model has already reached for the tool. buildTaskDelegationSection() holds the same text with no production caller, so nothing puts any of it in the system prompt. The only always-present text was the lean description, "Start a background sub-agent and return its run ID immediately." — what the tool does, never when to use it. Observed live: an agent with sessions_spawn on its surface ran ten consecutive turns of heavy report work inline, worst turn 205s, and spawned zero sub-agents. Auto-backgrounding fired three times, which is the runtime rescuing work the model should have delegated. Move the trigger into the lean description, where it is present before the first call and costs one line instead of the ~2000-token guide. The full guide keeps its JIT delivery.
SENSITIVE_ARG_NAMES matches the bare word `token` on any component boundary, so `range_token` — a fixed date-range enum on an MCP tool — was rewritten to "[REDACTED]" when the session was persisted. The matcher is name-based and value-blind, so nothing about the actual value could save it. That placeholder is not inert. It is replayed into the model's context on the next turn and copied forward as a literal argument, which is the exact hazard scrub-redacted-tool-calls.ts already documents for `env_value`. Live drive of ten prompts: every one of the five tool failures was this, across two different tools, with the pagination pattern making it unmistakable — pages 1, 2 and 3 rejected identically as the model re-sent "[REDACTED]". Split the matcher. Unambiguous credential names (api_key, secret, password, and qualified forms like access_token / refresh_token) stay value-blind, as does a parameter named exactly `token`. The ambiguous `*_token` family is redacted only when the value could plausibly be a credential; a short all-lowercase identifier cannot be one. looksLikeApiKey still runs afterwards as a second line of defence. Also name the accepted values on an enum rejection. AJV's "must be equal to one of the allowed values" lists nothing, so a model that sent a wrong value has nothing to correct toward and retries the same argument — which is why the same call failed three times rather than once. The values are already in the tool's schema at the format site, so the formatter now takes a resolver and interpolates them. Callers that pass no resolver keep the previous wording byte-identical.
An Anthropic cached prefix is system -> tools -> messages. The trace fingerprinted the first and third components and not the second, so a tool added, removed, or re-described mid-session invalidated every cached block after the system prompt while the record built to diagnose prefix collapse showed nothing moving. That gap blocked a live investigation: 37 calls on one drive held a single stable systemDigest and still burned 10.8M cache-creation tokens against 4.0M reads, creating a full ~350-450k prefix on nearly every call and reading it back eight times. System stable plus messages growing normally leaves the tool array as the remaining suspect, and the trace could not speak to it either way. Digest the full tool definitions rather than the name list, so a schema or description edit that leaves names intact still moves the digest. toolCount rides along for the cheap case where the array simply grew.
The 1-hour cache TTL is an Anthropic-API beta. Bedrock serves Anthropic models but does not accept Anthropic beta headers — request-body/factory.ts already gates the 1M-context beta on the direct provider for exactly this reason, with the comment "direct Anthropic only -- NOT Bedrock/Vertex". The body-level `ttl: "1h"` carried no equivalent gate, so every cached block went out asking for an hour on a provider that cannot grant it. The write is billed; the read never matches. Measured on amazon-bedrock/claude-opus-4-8 with the new toolsDigest in place: a single stable systemDigest, a single stable toolsDigest over a constant 193-tool array, ~459k cache-creation tokens on every call, and zero cache reads across the entire drive. Nothing in the prefix was moving — the same two prefixes were re-created call after call and never once matched. Cap retention at the one point where it is resolved, so all four downstream sites that write `ttl: "1h"` follow from the same decision rather than each growing its own provider guard. supportsExtendedCacheTtl is deliberately NOT the family check: Bedrock IS in the Anthropic family, which is why the family predicate cannot express this. getCacheProviderInfo has the same literal-string gap but no production consumers, and an existing test deliberately pins Bedrock to catalog-driven eligibility. Left alone rather than fought for no runtime effect.
…ion cap Capping retention at the resolution point was not enough. Three sites emit ttl: "1h" directly without reading the resolved retention — the UNTRUSTED_ anchor, adaptive zone promotion, and the safety-net sweep itself. The sweep runs last, sees one of those 1h markers, and upgrades every earlier 5m marker to match, silently reversing the cap on a provider that cannot honor it. Live on amazon-bedrock after the cap landed, this fired on EVERY turn, logging "upgraded out-of-order 5m markers to 1h" with errorKind:"internal" and a hint pointing at an upstream placement bug — while itself being the thing re-breaking the request. Make the sweep provider-aware. Where extended TTL is unavailable it now normalizes DOWNWARD, stripping every ttl so all markers are the plain 5m default: monotonicity holds trivially, every marker is honorable, and any future site that emits 1h directly is corrected rather than propagated. Demoted to DEBUG for that path — it is the expected steady state there, not a defect worth a WARN. Upgrade behaviour is unchanged and remains the default when the flag is omitted, so direct Anthropic keeps its 1h coordination byte-for-byte.
…e trace messageFingerprints is replaced wholesale by a bounded-payload sentinel once the array passes the persistence limit — at 247 messages it is already gone. That is exactly the regime where cache economics matter, so "where did the cached prefix diverge?" becomes unanswerable from the record built to answer it. Live case it blocks: a session whose cross-turn hit ratio fell from 98% at a ~120k-token prefix to 33% and then 0% as the conversation grew, with a stable systemDigest and a stable toolsDigest throughout. Something in the message array stops matching, and the per-message fingerprints needed to locate it are stripped. Record cumulative hashes over the leading fingerprints at exponentially spaced depths plus the full length. Comparing two calls localizes the first divergence to a range in ~log(n) values, and the ladder is fixed-size so the bound never touches it.
The DEBUG line read "lcd history evicted under budget" on every assembly, including when droppedCount was 0 and nothing had been dropped. During a live cache investigation that reads as a statement that the message array was rewritten — the first thing to rule out when a cached prefix stops matching — and it cost a full detour down the wrong layer before droppedCount turned out to be 0 on every call of the run. State what happened: eviction only when something was evicted, otherwise that the history fit. The structured fields are unchanged.
The tool-failure notice ends in an em-dash because the caller appends the failing tool's name verbatim. When the ONLY unrecovered failure is the background poller — which must never be named, since it merely relays another tool's failure — the lookup returns undefined and appended an empty string, so the user's reply ended: "...so part of this may be incomplete — " Observed at the end of a Hebrew answer about a report tool that hit the MCP call deadline: a truncated sentence trailing off into nothing, in the one place the turn was trying to be honest about being degraded. Add a complete-sentence variant to the locale set and select on whether a name exists. Regression introduced when the poller was excluded from blame.
The prompt_timeout userMessage was a hard-coded English literal in error-classifier.ts, delivered verbatim into conversations in any language. It is the same gap already closed for pipeline_timeout, and it matters for the same reason: a stalled turn is one of the few messages a user is guaranteed to see, so it is exactly the one that must sit inside the localizable platform-reply set rather than beside it. Observed live: a Hebrew conversation stalled for 404s and the user received "The request took too long to process. Please try again with a simpler message." Add prompt_timeout to the locale message set and select it at the delivery site in failure-path.ts. Other classifier categories keep their literal text until they are given ids too — this changes only the category that was observed.
anyOf / oneOf / allOf reached the Type.Any() fallback, which removes the type entirely: the model is told nothing about the parameter's shape AND local validation accepts any value, so a wrong type travels to the MCP server and returns as an opaque -32602. Same silent-information-loss class as the numeric bounds and enums that were being stripped. A composed schema frequently carries no top-level `type`, so the branch has to run BEFORE the type checks or it is unreachable. Observed live: a model passed an object-typed parameter as a string, local validation waved it through, and the server rejected it twice in one turn. Whether that specific parameter arrives as a composed schema is not confirmed — the fidelity gap is real and worth closing on its own terms, so this is not claimed as the fix for that turn. $ref still falls back to Type.Any(): resolving it needs the document root, which this function does not receive.
The SDK only accepts a progress notification when the request registered a handler for it. `onprogress` was gated on a trace context, so on any UNTRACED path — which is exactly where a backgrounded call runs — a server that reports progress produced: MCP client error: Received a progress notification for an unknown token and the client CLOSED THE CONNECTION, failing the tool with -32000 and forcing a reconnect mid-turn. This is the same mistake, in the same options object, that the ceiling above was already fixed for: `maxTotalTimeout` had been scoped to the trace branch and was made unconditional because a deadline that applies only when tracing happens to be on is not a deadline. The progress handler sat in that branch untouched, and the identical reasoning applies — the untraced background path is precisely where long, progress-reporting calls live. Observed live: a progress-reporting MCP tool failed with -32000 Connection closed, followed by "MCP server reconnect started".
…ss it SYSTEM_PROMPT_GUIDES["sessions_spawn"] holds the "## Task Delegation" policy — the >30s rule, the media/multi-file/deep-research criteria, the parallel fan-out instruction — and jit-guide-injector delivered it ONLY after a successful sessions_spawn call. That is circular: the policy exists to make the model reach for the tool, and it was unreachable until the model had already reached for it. Moving the trigger into the always-present lean description was not enough. Across 119 live prompts on two different model families the agent delegated ZERO times, including an eleven-tool, multi-minute, file-producing turn that matches three of the criteria outright. A direct request proved the tool is present and works, so this was never missing plumbing — the model simply never learned the policy. Deliver it on the first tool result of ANY kind when sessions_spawn is on the surface. Keyed to the policy rather than the tool that carried it, so a later spawn does not repeat the whole section, and gated on availability so an agent without the tool is never told to use it. Callers passing no options are byte-identical to before.
The gate added in the previous commit read `contextTools` at the discovery re-injection call site. That array holds only NEWLY DISCOVERED tools, so sessions_spawn was never in it and delegationAvailable was always false — the policy still never shipped. Verified live: the guide log showed only "tool:read", never "section:sessions_spawn". createJitGuideWrapper is the site that matters. It wraps the session's initial tools and is handed the full active tool array, so the availability check belongs there and is computed once per session rather than per tool call. Also settles what was suspected for a long stretch: sessions_spawn is NOT deferred. The only deferred tools on this deployment are the three channel actions (discord/slack/whatsapp). The tool has been on the surface the whole time, and a direct request spawns a run successfully — the model simply never learned the policy, because the policy was keyed on having already used it.
A spawned sub-agent gets a RESTRICTED default profile: the parent's MCP tools and `message` are outside it. The Task Delegation policy told the model to spawn and said nothing about tool_groups, so the first spawn of any real task failed with Required tools unreachable: mcp__…, message. … outside this sub-agent's profile. Re-spawn with tool_groups:['full']. before doing any work, degrading the turn on the way to recovering. Observed on the first delegation this runtime ever performed — the failure was invisible until the preceding fix made delegation reachable at all. State it in "How to Delegate", where the model reads the rest of the spawn recipe: which two surfaces sit outside the child profile, and what to pass when the child needs them.
… the decision The delegation policy rode a TOOL RESULT, so it landed AFTER the model had read the request, chosen an approach and issued its first call. Measured on a real Telegram turn with Opus 4.8: the policy arrived on the first MCP tool result while the model was already working inline, and it simply carried on — the user got no "running, I'll send it when ready" and no window to keep interacting. That also explains why the same code behaved differently per model. gpt-5.6-sol happened to re-plan after the first tool result and spawned; Opus committed to its plan and did not. A directive that must shape the FIRST decision cannot be delivered as a consequence of that decision. Move it into the engine kernel, gated on sessions_spawn being on the surface — telling an agent without the tool to delegate is worse than silence. Sourced from the CACHE-STABLE tool snapshot, not the live list, so the cached system prefix does not flip between turns when MCP servers connect or disconnect. A universal orchestration mechanism with no domain assumptions, which is what the engine prompt is allowed to carry. The JIT guide stays as the detailed recipe.
… text The turn's final assistant text is delivered to the channel automatically. When the agent ALSO delivers user-facing content with the `message` tool, both go out. Observed on real Telegram: a file arrived with its caption, immediately followed by a second message restating it. The trajectory showed message x2 against THREE dispatched deliveries — one extra per exchange. The runtime already honours a silent sentinel (isSilentResponse) for exactly this case; the model was simply never told to use it after sending. State the contract where it decides: deliver via `message`, then reply NO_REPLY. Prompt-level, so it carries the same caveat the delegation directive just proved — a model may not comply. The durable fix is for the runtime to suppress the final delivery when the agent already delivered to the same channel this turn, which needs the message tool's target channel threaded to post-execution to avoid suppressing a legitimate reply after a cross-channel send.
added 2 commits
July 28, 2026 01:23
Combine regression tests and production changes in one review commit because the round includes secret-handling corrections.
PR description incompletePlease fill in all required sections before this PR can be reviewed. Required sections: Description, Related Issue, Type of Change, Checklist, RED Test Proof. For code changes in See CONTRIBUTING.md for the full contribution bar. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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.
Intent
Validate the production-incident hardening work on this branch. The objective is to make long tool-driven agent turns retain the user's originating request; keep tool, background-task, delivery, timeout, breaker, and activity outcomes truthful; preserve secret-safe approval replay; enforce the intended locale and MCP timeout/schema/progress behavior; improve cache-prefix stability and diagnostics; give models accurate delegation and message-delivery guidance; and make daemon/session observability reconstruct these failures with actionable, internally consistent evidence. Preserve Comis's domain-neutral hexagonal runtime boundaries, Result/security/logging conventions, test-first coverage, and documentation, without weakening approvals or secret handling.
What Changed
Risk Assessment
🚨 High: Two reachable source paths can still convert failed or entirely undelivered turns into successful delivered outcomes, contradicting the hardening objective.
Testing
No baseline results were supplied. The focused automated matrix and end-to-end API/state checks succeeded after correcting execution-only module-resolution issues in the evidence harness; the worktree remained clean. No UI surface changed, so reviewer-visible JSON is the appropriate evidence.
Evidence: End-to-end product evidence
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
🔧 **Rebase** - 1 issue found → auto-fixed ✅
packages/daemon/src/api/obs-handlers/obs-explain.ts- merge conflict rebasing onto origin/main🔧 Fix applied.
✅ Re-checked - no issues remain.
packages/agent/src/context-engine/lcd-assembler.ts:268- Intent requires “make long tool-driven agent turns retain the user's originating request.” The new store-horizon clamp initially retains that request, but the later residual bound drops the oldest non-toolResult segment—which is the originating user message—once a finite-window tool loop exceeds the residual. Pin the in-flight request atboundProtectedFreshTail, trimming completed tool segments or returning explicit exhaustion instead of dispatching without it.packages/agent/src/executor/executor-response-filter.ts:356- Intent requires delivery outcomes to remain truthful.deliveredUserFacingTextdeclares delivery solely from amessagesend/reply call's arguments; it never verifies the correlated tool result. If the platform send fails and the final response is empty, this suppresses recovery, logs that a reply was delivered, and leaves the user silent. Require a successful tool result or authoritative delivery receipt.packages/orchestrator/src/execution/execution-pipeline.ts:720- Intent requires tool and delivery outcomes to remain truthful. A successful platform delivery only proves that some text arrived;withDeliveredEvidenceattaches that receipt to every unattributed execution failure. Any observed failed tool then converts a terminal failure—such as a delivered “I couldn't complete this” response—intosuccess_with_recovered_failures. Require explicit recovery/task-success evidence rather than inferring recovery from transport delivery.packages/agent/src/safety/background-failure-attribution.ts:93- Intent requires breaker outcomes to remain truthful. This per-execution listener accepts every same-agent background failure without checkingsessionKeyordispatchRedelivery. Concurrent sessions therefore charge one session's breaker for another's failure, and dispatch redrives repeatedly count one terminal failure until a breaker trips. Scope the listener to the originating session and ignore redelivery events.packages/agent/src/background/background-task-types.ts:156- Intent requires background-task/activity outcomes and session observability to survive accurately. The new fields are documented as persisted, butPersistedTaskStateSchema,PersistedTaskState, andtoPersistedStateomit all three. After restart, recovery emits notoolCallId/sessionKey/traceId; activity cannot close the card and the trajectory bridge cannot scope the event, potentially recording it in unrelated active session recorders. Persist and restore these correlation fields.packages/observability/src/activity/activity-stream.ts:527- Intent requires timeout and activity outcomes to remain truthful. Every failed background task is labeleddependency, including the manager's explicit hard-timeout path. Add a closederrorKindto the terminal background event/persisted state at the failure boundary and propagatetimeoutinto activity and trajectory consumers.packages/skills/src/platform-tools/tools/gateway-tool.ts:415- Intent requires secret-safe approval replay.takePendingEnvSetdeletes the only retained secret before key validation, store preflight, andenv.set. A wrong-key retry or transient RPC/store failure therefore destroys the approved action and makes a correct replay impossible after session redaction. Validate through a non-destructive lookup and consume only after the write succeeds.packages/core/src/security/secret-detection.ts:374- Intent forbids weakening secret handling. The exact-name exception is applied in the global core detector, so schema-valid arbitrary config such asplugins.<id>.config.range_token: shortsecretnow bypasses the plaintext-secret persistence guard because the short value also misses entropy heuristics. Keep core field-name detection conservative and apply the value-aware cursor exception only at the session tool-argument sanitizer that needs it.packages/skills/src/skills/bridge/mcp-tool-bridge.ts:80- Intent requires intended MCP schema behavior. Composition remains semantically lossy: a commonanyOfcontaining{type:"null"}converts that branch toType.Any, making the entire union accept every value, whileoneOfis converted toanyOfand loses its exactly-one constraint. Implement null/type-array handling and preserveoneOfasoneOfinstead of coalescing both keywords intoType.Union.packages/daemon/src/api/obs-handlers/obs-explain-fresh-tail-verdict.ts:53- Intent requires actionable, internally consistent observability. This verdict treatseffective < configuredas proof that the request slid out, but the new horizon clamp prevents that slide for ordinary short turns; it therefore reports a false likely root cause without checking actual steps or trimming. Conversely, the residual trim can still drop the request when no clamp occurred, which this verdict misses. Drive the verdict from actual origin coverage/trim evidence.packages/daemon/src/wiring/proactive-degrade.ts:37- A missingcapEndpointHandledoes not uniquely mean autonomy is disabled:constructCapabilityLayeralso returns it undefined when an autonomy-enabled deployment's capability socket fails to activate. This matcher then emits the contradictory advice to enable autonomy even though it already is enabled. Propagate a closed degradation reason from capability-layer construction instead of inferring it from the missing-dependency string.packages/skills/src/tools/builtin/exec-diagnostics.ts:212- The generic runtime now advises agents to bypass PEP 668 with--break-system-packages, which can mutate or break host-managed Python installations. That conflicts with the required security posture and is unnecessary because the same hint already provides a workspace virtualenv path. Remove the system-package bypass guidance unless host mutation is an explicitly approved product behavior.🔧 Fix: Preserve truthful incident recovery outcomes
2 errors still open:
packages/orchestrator/src/execution/execution-pipeline.ts:721- Intent requires delivery outcomes to remain truthful. This passes every successful receipt towithDeliveredEvidence, although the delivery stage explicitly returnsokwithdeliveredChunks: 0for empty/suppressed output. A terminal execution failure can therefore be reclassified as delivered success when no message was sent. Attach evidence only fordeliveredChunks > 0or an authoritative successful non-text receipt.packages/orchestrator/src/execution/activity-turn-coordinator.ts:588- Intent requires tool and delivery outcomes to remain truthful. Recovery is correlated only bykindandtoolName: after oneexeccall fails, any later unrelated successfulexecmarks that failure recovered; one success also covers every earlier same-name failure. With delivery evidence, this converts a terminal failure tosuccess_with_recovered_failures. Require explicit recovery/task-success evidence from the execution authority rather than same-name event ordering.✅ **Test** - passed
✅ No issues found.
pnpm exec vitest run packages/agent/src/context-engine/lcd-assembler.test.ts packages/agent/src/context-engine/lcd-coverage.test.ts packages/agent/src/executor/executor-post-execution.test.ts packages/agent/src/executor/executor-response-filter.test.ts packages/agent/src/safety/background-failure-attribution.test.ts packages/orchestrator/src/execution/activity-turn-coordinator.test.ts packages/orchestrator/src/execution/turn-outcome-mapper.test.ts packages/channels/src/shared/strategies/render.test.ts packages/agent/src/session/sanitize-session-secrets.test.ts packages/agent/src/session/scrub-redacted-tool-calls.test.ts packages/skills/src/platform-tools/tools/gateway-tool.test.ts packages/agent/src/executor/resolve-response-locale-policy.test.ts packages/agent/src/executor/prompt-runner/response-locale-enforcement.test.ts packages/agent/src/executor/degraded-reply-i18n.test.ts packages/skills/src/skills/bridge/mcp-tool-bridge.test.ts packages/skills/src/skills/integrations/mcp-client/mcp-client-call.test.ts packages/agent/src/executor/stream-wrappers/request-body/prefix-stability.test.ts packages/observability/src/cache-trace/stream-fn-wrapper.test.ts packages/agent/src/bootstrap/sections/tool-descriptions.test.ts packages/agent/src/executor/prompt-compiler.test.ts packages/daemon/src/api/obs-handlers/obs-explain.test.ts packages/daemon/src/api/obs-handlers/system-health.test.ts test/architecture/daemon-boot-degrades-without-autonomy.test.ts test/architecture/mcp-timeout-default-parity.test.ts test/architecture/token-basis-lens-reconciliation.test.tspnpm exec tsx /var/folders/w_/w1lk_fns7nqcjkp2n3512g8c0000gn/T/no-mistakes-evidence/01KYJR3PT4ZHHRFM65EZEAYQ3C/evidence-runner.mts— exercised the real SQLite LCD assembler, gateway approval replay, and wiredobs.explainhandler against a frozen breaker incident.docs/integrations/mcp-server.mdx:17- The MCP export-policy inventory is a stale manual copy: it claims both 51 and 52 tools, while the authoritative registry now classifies 74. Consolidate or generate this inventory from tool-metadata-registry.ts in a dedicated follow-up.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.