refactor(http): remove runtime curl subprocesses#881
Conversation
Summary: replace curl-backed Zig HTTP helpers with native std.http wrappers; migrate providers, channels, gateway, tools, memory API, update, voice, and SSE paths; rename runtime HTTP helpers and errors from Curl* to Http*. Boundary: keep curl as auxiliary/operator tooling for Docker builds, compose healthchecks, docs/examples, and shell-safety fixtures; the removal is limited to NullClaw runtime subprocess execution. Validation: zig build test --summary all (6781/6790 passed, 9 skipped); git diff --check. Risk: broad network migration touches gateway, tools, and security-adjacent paths; SOCKS proxy URLs now fail closed because Zig 0.16 std.http supports HTTP(S) proxies only.
|
Is Zig ready for non-curl HTTP? I looked into it and it only supports TLS 1.3? This will break lots of |
addadi
left a comment
There was a problem hiding this comment.
Review: REQUEST CHANGES
High-risk refactor requiring additional work before merge.
Critical Issues (Must Fix)
-
SOCKS5 proxy support removed
- Breaking change:
proxy: "socks5://..."configs will fail - README updated but no migration guide for existing users
- Action: Document migration or add feature flag for SOCKS5
- Breaking change:
-
Test coverage gap
- 6781/6790 tests passed - which 9 failed?
- Need to verify failures are pre-existing, not introduced by this PR
- Action: Provide list of failing tests with justification
-
Timeout handling semantics changed
- curl
--max-time= total operation timeout - std.http timeout = per-request stage timeout
- Action: Verify behavior matches or document difference
- curl
-
Lost mid-flight cancellation
CancelWatcherCtxremoved (SIGTERM killing)checkInterrupted()only checks flag before request starts- Long-running requests cannot be cancelled mid-flight
- Action: Document limitation or add cancellation support
Code Quality
✅ Clean error renaming (Curl* → Http*)
✅ Test injection hooks for mocking
Security
✅ Removes subprocess attack surface
✅ Uses native TLS
Recommendations
- Add feature flag to opt-in to native HTTP
- Consider gradual rollout (one module at a time)
- Provide performance benchmarks vs curl
- Add rollback strategy
This refactor is well-intentioned but needs the above resolved before merge.
Summary: replace curl-backed Zig HTTP helpers with native std.http wrappers; migrate providers, channels, gateway, tools, memory API, update, voice, and SSE paths; rename runtime HTTP helpers and errors from Curl* to Http*.
Boundary: keep curl as auxiliary/operator tooling for Docker builds, compose healthchecks, docs/examples, and shell-safety fixtures; the removal is limited to NullClaw runtime subprocess execution.
Validation: zig build test --summary all (6781/6790 passed, 9 skipped); git diff --check.
Risk: broad network migration touches gateway, tools, and security-adjacent paths; SOCKS proxy URLs now fail closed because Zig 0.16 std.http supports HTTP(S) proxies only.
Addresses the curl part from #192
Thank you for the awesome project, and I'm very new to Zig. I've really tested most flows related to Telegram, web_search across different providers and OpenAI connections and tried to add as much test coverage as possible. The real downside I see from the patch is the removal of SOCKS5 support (and I've discovered it late, but since I already started the work I thought I'd open the PR anyway).
I've also thought about adding the native provider in parallel, not sure what's the opinion on that.