Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #726 +/- ##
=======================================
Coverage 55.66% 55.67%
=======================================
Files 126 126
Lines 13759 13763 +4
=======================================
+ Hits 7659 7662 +3
- Misses 6100 6101 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
geoffjay
left a comment
There was a problem hiding this comment.
Review: fix(cli): switch reqwest to rustls-tls to eliminate system-configuration panic
Stack position: issue-541 is directly on main with no parent PR
dependency, but git-spice reports (needs restack) — the branch is behind
main and must be rebased before the conductor can merge it.
Code quality: approved
The fix is correct and the approach is well-chosen. Specific notes:
Two-pronged strategy — The PR correctly addresses the panic with both a
dependency fix and a defence-in-depth call-site guard:
-
crates/cli/Cargo.tomlandcrates/wrap/Cargo.tomlswitch to
reqwest = { workspace = true, features = ["json"] }, inheriting
default-features = false, features = ["rustls-tls"]from the workspace
definition. This removes the direct native-tls declaration from both
crates. -
Every
Client::builder()site gains.no_proxy(), which tells reqwest
to skip macOS system-proxy detection entirely. This is the bulletproof
guard: even if native-tls is present in the binary (see below), the
SCDynamicStoreCreate()call that causes the NULL-pointer panic is never
reached.
no_proxy() comment quality — The comments on the new Client::builder()
calls are clear and accurate: they name the exact call chain
(hyper-util → system-configuration) and justify why proxy detection is
irrelevant for localhost-only clients.
.expect("Failed to build HTTP client") — Appropriate here. new()
returns Self, not Result<Self>, so ? is not available. The message is
clear, and Client::builder().no_proxy().build() failing at runtime is as
unlikely as the original reqwest::Client::new() panicking for any reason
other than the TLS init bug.
Test cleanup — Removing try_new_client and catch_unwind is the right
outcome. The tests now assert unconditionally, which is what the fix enables.
Observation (no action required)
Feature unification via ask, communicate, notify —
crates/cli/Cargo.toml depends directly on all three of these crates:
notify = { path = "../notify" }
ask = { path = "../ask" }
communicate = { path = "../communicate" }All three still declare:
reqwest = { version = "0.12", features = ["json"] } # no default-features = falseWith resolver = "2", Cargo unifies features within a binary: the absence
of default-features = false in these crates re-enables default-tls
(native-tls) for the unified reqwest build. This means the
system-configuration framework is still compiled into the cli binary — it
just isn't called, thanks to no_proxy().
Practically: the panic is fixed. But the binary still carries the
native-tls/system-configuration dependency unnecessarily. A follow-up
switching ask, communicate, and notify to reqwest = { workspace = true, features = ["json"] } would fully remove that dead weight and close the gap
the enricher agent flagged in memory.
Action required before merge
Rebase onto main:
git-spice branch restack
git-spice branch submitOnce restacked, add merge-queue to trigger the conductor.
fix(cli): switch reqwest to rustls-tls to eliminate system-configuration panic
fix(cli): eliminate system-configuration panic via no_proxy and workspace reqwest (closes #541)