fix(advisor): stop discarding consults that outlive the MCP default timeout - #388
Open
astyfx wants to merge 1 commit into
Open
fix(advisor): stop discarding consults that outlive the MCP default timeout#388astyfx wants to merge 1 commit into
astyfx wants to merge 1 commit into
Conversation
…imeout The Claude Agent SDK caps an MCP tool call at a hard 60-second wall clock that progress notifications do not extend. Stave set neither the per-server `timeout` nor `MCP_TOOL_TIMEOUT`, so that default applied to every Stave Local MCP tool — including `stave_consult_advisor`, whose own deadline is 2-10 minutes by effort tier. Any consult past the minute mark therefore failed structurally: it ran to completion, billed its tokens, and emitted its `completed` trace to the turn, while the client had already aborted and the MCP SDK dropped the reply with `if (signal.aborted) return;` — no error, no log, no metric. The turn detail said the consult succeeded and the primary never received a word of it. A 56.9s consult in the same session succeeded and a 139.5s one did not, which puts the boundary exactly at the documented default. State the outermost rung explicitly and derive it from the host-service backstop, so the ordering is enforced in code rather than by comment: one advisor call 2-10 min by effort tier host-service backstop 15 min MCP tool call (client) 16 min <- was an implicit 60s `stave-mcp-stdio-proxy` shares the same cap instead of its own 120s. Also add an `advisor_activity` `progress` heartbeat, throttled to one tick per 5s, naming what the provider was last seen doing. Both providers resolve only after generation finishes, so a consult was previously indistinguishable from a wedged thread for minutes at a time. The heartbeat is deliberately not a lifecycle stage: the reducer folds it into `lastProgressAt`/`progressDetail` rather than appending to the bounded `stages` list, so a chatty provider cannot evict the steps that matter or make `settledConsults` depend on tick timing. A heartbeat arriving after a consult settles returns the snapshot by reference, which the archive's existing identity check reads as a non-event. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
astyfx
requested review from
heath-s and
paul-seo73
and removed request for
a team
August 24, 2026 01:41
|
| Commit | Scanned at | New | Resolved | Net |
|---|---|---|---|---|
ffa9254 < |
2026-08-24 01:42 UTC | 0 | 0 | 0 |
Last scanned: ffa9254 · 2026-08-24 01:42 UTC
|
| Commit | Scanned at | New | Resolved | Net |
|---|---|---|---|---|
ffa9254 < |
2026-08-24 01:42 UTC | 0 | 0 | 0 |
Last scanned: ffa9254 · 2026-08-24 01:42 UTC
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
2 participants
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.
Summary
Every Advisor consult longer than 60 seconds was silently thrown away.
The Claude Agent SDK caps an MCP tool call at a hard 60s wall clock that progress notifications do not extend. Stave set neither the per-server
timeoutnorMCP_TOOL_TIMEOUT, so that default governed every Stave Local MCP tool — includingstave_consult_advisor, whose own deadline is 2–10 minutes by effort tier. Consults past the minute mark were structurally impossible to complete.The failure was invisible in the worst way. The consult ran to completion, billed its tokens, and emitted its
completedtrace to the turn event stream — so the turn detail readAdvisor consult 1/5 completed with Codex · gpt-5.6-sol in 139.5s. Meanwhile the client had already aborted and the MCP SDK discarded the reply withif (signal.aborted) return;: no error, no log, no metric. The UI was the only thing that ever saw the advice; the primary never received a word of it.The boundary is confirmed empirically — in one session a 56.9s consult succeeded and a 139.5s one did not.
Key changes
1. State the outermost timeout rung explicitly. Derived from the host-service backstop so the ordering is enforced in code rather than by comment:
resolveAdvisorTimeoutMsHOST_SERVICE_ADVISOR_CONSULT_TIMEOUT_MSSTAVE_LOCAL_MCP_TOOL_TIMEOUT_MS← was an implicit 60sThe host-service comment already argued that its backstop must sit above the advisor ceiling. That reasoning was simply missing one layer up.
stave-mcp-stdio-proxynow shares the same cap instead of setting its own 120s.2. An
advisor_activityprogressheartbeat (throttled to one tick per 5s) naming what the provider was last seen doing —Codex item: reasoning,Claude event: assistant,Loading the Claude runtime. Both providers resolve only once generation has finished, so a consult was previously indistinguishable from a wedged thread for its entire duration. Reuses the existingitem/completedandonProgressobservation points; no new provider plumbing.The heartbeat is deliberately not a lifecycle stage. The reducer folds it into
lastProgressAt/progressDetailrather than appending to the boundedstageslist (cap 12), so a chatty provider cannot evict the steps that matter or makesettledConsultsdepend on tick timing.Interaction with #384
#384 (
archive consults for review) landed on two of the same files after this work was written. The heartbeat needed one adaptation:applyAdvisorActivityEventstreats a new snapshot reference as "something changed" and upserts the archive row. A straggler heartbeat from a runner racing its own timeout would therefore have bumpedupdatedAton a consult the log had already closed. The reducer now returns the snapshot by reference when the consult is already terminal, which the archive's existingif (next === snapshot) continue;reads as a non-event — zero changes to any #384 file.Two invariants worth noting, both holding by construction rather than by check:
isNewExchangeonly fires onstarted, so a heartbeat can never open a card; andstartSnapshotnormalizes its seeded stage toarmed/started, so"progress"cannot enterstageseven through the event-replay synthesis path.What this deliberately does not do
AbortSignalinto the run. It cannot work: the tool handler runs in the Electron main process while the grant registry lives in the host-service child, and anAbortSignaldoes not survive the JSON IPC hop. Fixing the timeout removes the need — 16 min > 10 min means a healthy consult is never abandoned, so no orphan is created to cancel.transport:, and whether the CLI reads a per-server timeout inside or beside that wrapper could not be confirmed from the binary. Writing it to the wrong place risks the CLI rejecting a user's settings file — worse than the original bug. The fix is limited to the verified SDK path.defaultCodexReasoningEffort. Every catalog model defaults toxhigh, which plausibly explains the 139.5s; but the catalog comment states it mirrors whatcodex-cli'smodel/listreports, so the provider's own default is respected. The prompt is already minimal (4 lines plus caller context, no repo state or conversation history) and the Codex process is warm, so there was little else to trim — hence surfacing the wait rather than hiding it.Known limitation, documented
The MCP
timeoutis per-server, not per-tool, so it now applies to all 74 Stave Local MCP tools. An audit found one that can legitimately exceed 16 min:stave_create_workspaceblocks on the project's configured init command (typically a dependency install) with no deadline of its own, andstave_delegate_taskawaits it when creating a new worktree. This is not a regression — the previous effective cap was 60s, so every tool improves — but the ladder invariant holds by construction only for the consult path. Recorded in docs/providers/provider-runtimes.md.(
stave_respond_approvalwas checked for human-wait behaviour and does not wait: it forwards a decision to a pending request and throws immediately if none exists. The only human-waiting tool is Lens CDP approval, which is explicitly capped at 60s.)Verification
bun run typecheckclean;bun run build:desktopsucceeds (checked specifically becausestave-mcp-stdio-proxyis a separate bundle entry —out/main/stave-mcp-stdio-proxy.mjs, 7.26 kB, no Electron-only imports pulled in)reliability-gates,doc-paths,max-lines-ratchet,switch-exhaustivenessall passadvisor-exchange,stave-local-mcp-manifest,advisor-consult-log-render,turn-activity-render,advisor-consultgit stashbaseline at the same HEAD is empty in both directions — the same 11 pre-existing environment-dependent failures (provider binary discovery, PR-drafting untracked-diff timeout) plus one full-suite-only module-loading flake intests/host-service-client.test.tsthat passes in isolation and reproduces identically on the clean base. Test count rises by exactly the 6 tests added here.stages; a heartbeat cannot settle or resurrect a finished consult; a late heartbeat does not re-touch the archived entry (asserts reference identity on both maps); theprogressphase survives the provider event schemaWorth flagging: the ladder-ordering test exists because
tsconfig'sincludeis"src"only, sotscnever seestests/. Type-level agreement between these constants is not something the typechecker can enforce here.Notes
Split out of the same working tree as #387 (button group chrome), which is UI-only and independent — either can merge first.
One thing not chased down:
electron/main/codex-mcp.tsdoes not settool_timeout_secand leaves Codex to its own default, which I could not determine. If Codex is the primary, the same symptom may still be reachable by that path.🤖 Generated with Claude Code