Repository navigation
fix(ov_cli): show the underlying cause of connection errors - #6312
Merged
Merged
Conversation
reqwest's Display drops the source() chain. from_reqwest now appends it so JSON and table output name causes like invalid peer certificate: UnknownIssuer. The Connection Error card shows the cause by default instead of only guessing offline or wrong URL. Fixes volcengine#6288
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
10 of 11 tasks
qin-ctx
approved these changes
Oct 20, 2026
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
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
ovdoesn't say why a request failed. reqwest'sDisplayonly prints "error sending request for url (...)". The actual cause (TLS, DNS, connect) is in thesource()chain, andError::from_reqwestdropped it. On top of that, the Connection Error card only showed the detail in verbose mode and always guessed "offline or wrong URL", so a TLS problem looked like a dead server.This appends the source chain in
from_reqwestand shows it in the Connection Error card by default.Human Involvement
Related Issue
Fixes #6288
Type of Change
Changes Made
error.rs:with_sources()walkssource()and appends each cause, skipping one that is already at the end of the message. All fourfrom_reqwestbranches use it, so timeout/decode/builder messages carry the cause too. The wording of those categories is unchanged.error_ui.rs: the Network card puts the detail in the visible body, and the headline no longer presents offline/wrong URL as the explanation. It still points there after the detail. zh-CN copy updated to match. Other cards are untouched.Testing
I reproduced the issue before changing anything: a self-signed HTTPS server on localhost,
ov health, not verbose.Before:
After:
ov -o json healthbefore:After:
With nothing listening, the card now ends in
tcp connect error: Connection refused (os error 111).Commands run locally:
RUST_MIN_STACK=67108864 cargo test -p ov_cli: 509 unit + 4 integration tests pass. The larger stack is needed because somehelp_uitests overflow the default stack onmainas well. I ran it with a throwawayHOMEthat only has the CLI language set, since two config integration tests wait for a language prompt otherwise.cargo clippy -p ov_cli --all-targets: no warnings in the touched filesrustfmt --check --edition 2024 crates/ov_cli/src/error.rs crates/ov_cli/src/error_ui.rs: cleangit diff --check: cleanAs far as I can see, CI doesn't run
cargo testforov_cli, so the above is the test signal for this change.Checklist
Additional Notes
tests/cli/conftest.pystill match.wrap_textcollapses newlines. I didn't want to change card rendering for every error type in this PR.