fix(error): make a missing local runtime a terminal variant, not a malformed response - #7
Conversation
…lformed response
A stdio server whose launcher is not installed was reported as
`Error::malformed(missing_command_error(..))` — wrong twice over, since
nothing was malformed and no response ever arrived. The condition existed
only inside a formatted English sentence, so the supervisor could not tell
it from a transient transport failure and scheduled reconnects forever on a
five-minute ceiling against a binary that was never going to appear.
Add `Error::MissingRuntime { command, runtime }` and `is_missing_runtime()`,
mirroring the `Unauthorized` carve-out the module already argues for. The
two are the same argument pointed at opposite conclusions: a 401 says try
again with credentials, a missing `uvx` says stop.
`runtime` is `CommandKind`, the vocabulary the install record and the store
already use, rather than a new one. It is classified by the new
`spawn_env::required_runtime`, which `missing_command_error` now switches on
too, so the sentence a user reads and the value a caller branches on cannot
disagree about which runtime is missing. `npx`/`npm`/`node` and `uvx`/`uv`
are the same code path and are both covered.
The supervisor parks such a server instead of penalising it: no backoff
entry, because a backoff is a promise that waiting helps. Disabling clears
the verdict, so installing the runtime and toggling the server is the way
back — the same gesture that already clears a penalty.
The guidance text is unchanged, and `missing_command_error` keeps its
signature and its tests; it delegates to the variant's own rendering.
Adding a variant is not a breaking change here: `Error` is
`#[non_exhaustive]`, so no downstream crate can match it without a wildcard.
Refs tinyhumansai/openhuman#5600
Closes tinyhumansai#6
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Warning Your free Security trial is over. An organization admin can activate billing to continue. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
How this change flows4 changed behaviours across 16 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 36 further behaviours left out to keep the diagram readable. flowchart LR
n0["a_failed_reconnect_earns_a_backoff_penalty<br/>changed"]:::changed
n1["Supervisor<br/>changed"]:::changed
n2["McpStdioClient<br/>changed"]:::changed
n3["a_missing_uv_runtime_says_so_by_name<br/>changed"]:::changed
n4["new"]:::impacted
n5["tick"]:::impacted
n6["connected_to"]:::impacted
n7["install"]:::impacted
n8["supervisor"]:::impacted
n9["client_for"]:::impacted
n0 -->|calls| n4
n0 -->|tests| n4
n0 -->|calls| n5
n0 -->|tests| n5
n0 -->|calls| n7
n0 -->|tests| n7
n0 -->|calls| n8
n0 -->|tests| n8
n3 -->|calls| n9
n3 -->|tests| n9
n6 -->|calls| n4
n6 -->|calls| n7
n7 -->|calls| n4
n8 -->|uses| n1
n8 -->|calls| n4
n9 -->|uses| n2
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinyhumansai#5 landed after this branch was cut and touched the same supervisor cycle, for a different reason: it made the liveness probe report what it observed (`ProbeOutcome`, a consecutive-timeout streak, `judge_probe`) instead of asserting a drop on the first slow answer. Both behaviours have to survive, so every one of the five conflicts is "keep both", not "pick a side": - the `timeouts` streak and the `terminal` set are separate fields, and the new doc note says why they are not the same thing — a live session going quiet is a reason to wait longer, a missing launcher is a reason to stop; - the constructor initialises both; - the disable branch forgets backoff, streak and terminal verdict alike; - the `Ok` arm clears all three; - both accessors are kept. The guarded `Err(error) if error.is_missing_runtime()` arm stays ahead of the generic one, so a missing runtime cannot fall through into the retry path tinyhumansai#5 left untouched. The terminal skip stays after the liveness block, so a still-connected server is probed and torn down normally and only the pointless reconnect is skipped. `test.rs` merged cleanly: 58 insertions, no deletions, so tinyhumansai#5's tests are unmodified.
Phase 1 of #6. A missing local runtime stops being a formatted sentence inside
MalformedResponseand becomes a variant a caller can branch on, and the supervisor stops retrying it.Phase 2 (the openhuman-side status / install hint) is deliberately not here — it depends on a packaging decision that has not been made.
What changed
Error::MissingRuntime { command, runtime }+is_missing_runtime()(error/mod.rs), mirroring theUnauthorizedcarve-out this module already argues for. The module note now covers both, because they are the same argument pointed at opposite conclusions: a 401 says try again with credentials, a missinguvxsays stop.runtimeisCommandKind— the vocabulary the install record and the store already persist — rather than a new string or a new enum. It is classified by a newspawn_env::required_runtime(command) -> CommandKind, whichmissing_command_errornow switches on as well, so the sentence a user reads and the value a caller branches on cannot drift apart about which runtime is missing.commandis kept verbatim rather than normalised: an absolute path that is wrong for this machine reads very differently from a bareuvx, and a user needs to see the string that was actually looked up.The transport returns it (
transport/stdio/mod.rs:135) instead ofError::malformed(...).The supervisor parks it (
registry/supervisor/types.rs). A newErr(error) if error.is_missing_runtime()arm inserts into aterminal: HashSet<String>and — importantly — removes any backoff entry rather than adding one, because a backoff is a promise that waiting helps. Parked servers are skipped before the backoff gate on later ticks. Disabling the server clears the verdict, so installing the runtime and toggling the server off/on is the way back — the same gesture that already clears a penalty.terminally_failed_count()is added alongsidebacked_off_count()so the two states are separately observable.npx/Node is covered, and it was already the same code pathThe issue asks to confirm rather than assume.
missing_command_erroralready classified three ways —npx|npm|node,uvx|uv, everything else — so Node and uv reach the identical branch in the identical function. Extractingrequired_runtimefrom that existingmatchcovers both by construction; nothing had to be forced. The new stdio test asserts all five recognised launchers plus an unrecognised one.Scope note
This makes the failure honest and stops the pointless retry loop. It does not make an MCP server work on a host without its runtime — installing or bundling
uv/Node is a separate decision for the consuming app. Refs tinyhumansai/openhuman#5600, which stays open for exactly that reason.Tests, and the revert-check
Two new tests, both proven to fail with their own fix reverted rather than assumed to.
transport/stdio/test.rs—a_missing_runtime_is_a_variant_a_caller_can_branch_onasserts the variant and theruntimefield, not the message, which is the whole point. With the transport change reverted:registry/supervisor/test.rs—a_missing_runtime_is_terminal_and_earns_no_backoff_penaltyinstalls aTransport::Stdioserver withPATHset to a directory that does not exist, then asserts no penalty, a parked verdict, no second attempt atBACKOFF_MAX * 2, and that disabling clears it. With only the supervisor'sErrarm reverted (transport fix in place, so the variant is produced correctly):Full suite after restoring both: 659 + 4 + 149 + 12 + 18 passed, 0 failed.
cargo fmt --allclean;cargo clippy --all-targets --all-featuresadds nothing — the oneunknown lint: clippy::unused_async_trait_implwarning is present on a clean tree too.The existing
missing_command_errortests still pass unchanged, which is what verifies the user-facing text did not move: the function keeps its signature and its public doctest and now delegates to the variant's own rendering, so there is one copy of the sentence instead of two.API compatibility
Adding the variant is not a breaking change, and openhuman needs no paired edit. This is the fact that makes the PR safe to merge on its own, so it is stated here rather than left to be rediscovered. Re-verified 2026-08-24 against
openhumanmain@0b8b02cbd.1. The guarantee is structural, and it predates this PR.
Erroris#[non_exhaustive]on the base branch —crates/tinymcp/src/error/mod.rs:30, abovepub enum Error, untouched by this change. A downstream crate therefore cannot match it without a wildcard arm today, so adding a variant cannot break one by construction. Nothing here relies on the diff being reviewed for that.Note the attribute is on the enum, not on individual variants, so existing variants stay constructible from outside — which is why
observability.rscan build anUnauthorizedin a test, and why phase 2 will be able to build aMissingRuntimein one.2. openhuman does not match on the type at all. Every reference on
main@0b8b02cbd:src/openhuman/mcp/mod.rs:144pub use tinymcp::Error as McpError;— a re-exportsrc/openhuman/mcp/mod.rs:133McpError::Unauthorizedsrc/core/observability.rs:347,:1087,:3775src/core/observability.rs:3784,:3788tinymcp::Error::Unauthorized { .. }constructed in two testsZero
matcharms and zeromatches!over the type; everywhere else it is carried as an opaqueDisplay/anyhowvalue.src/api/models/socket.rs:63,69'spub struct McpErroris a separate wire struct that happens to share the name, and the twoapp/src/lib/mcp/*.tshits are frontend.3. The earlier check has not gone stale. It was first made at
e1c332bf0;mainhas since moved to0b8b02cbd, andGET /compare/e1c332bf0...0b8b02cbdlists 28 changed paths, none of which touchessrc/openhuman/mcp/,src/core/observability.rs, orsrc/api/models/socket.rs(the run is memory/config/CI work plus avendor/tinymemorybump). The line numbers above were re-read from0b8b02cbddirectly, not carried forward.4. The message text is unchanged, and nothing matched it anyway. Grepping
src/andapp/src/for"was not found. This MCP server needs","needs Node.js"and"docs.astral.sh/uv"returns nothing — so even the substring contract this PR replaces had no consumer in openhuman.So the submodule bump can land on its own. The openhuman follow-up is phase 2 — new behaviour, not a repair.
Two notes for the reviewer
registry/supervisor/types.rs:93and:149as the twoErrarms. Only:149is a connect failure;:93is thestore.list_servers()arm, which cannot produce aMissingRuntime. One site changed, not two.missing_runtime_guidancematchesCommandKindwith a_fallback rather than namingBinaryexplicitly —CommandKindis itself#[non_exhaustive], and a future variant should degrade to the generic "install it, or its runtime" sentence rather than fail to compile. The comment says so.Closes #6