fix(cli): bound, trace, and explain the requests the CLI makes - #6798
Conversation
Four transport gaps, all of which failed silently. A request had no timeout, so a connection that was accepted and then never answered hung the terminal indefinitely. `SIM_TIMEOUT_SECONDS` now bounds one, defaulting to 3600s — deliberately above every timeout the server itself applies, since a synchronous workflow run is allowed 3000s on a paid plan and a tighter default would abort real work and report it as a transport failure. `0` removes the bound, for a self-hosted deployment that runs executions without one of its own. The caller's abort signal is composed with the timeout rather than replaced, so neither masks the other. Node ignores HTTP(S)_PROXY unless NODE_USE_ENV_PROXY opts in, and only from v22.21 and v24.5, so on a network that reaches the API only through a proxy every command failed to connect while the variable that would have fixed it was already set. The CLI cannot enable that from inside the process — Node reads it at startup — so it says what to do rather than bundling an HTTP stack for a setting the platform now owns. An API key was sent to any http:// endpoint with no signal. Now a warning, not a refusal: http is the documented way to reach a local dev server, and a deployment terminating TLS at a gateway is real. Loopback stays silent. `SIM_DEBUG=1` traces method, URL, status and duration. Bodies and headers are deliberately absent — the request carries the API key, and `secrets set` carries the secret itself. All four write to stderr, so a piped stdout stays parseable.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
New Docs in the CLI README and configuration guide document the new variables. Reviewed by Cursor Bugbot for commit 09ff9c0. Configure here. |
Greptile SummaryThe PR bounds CLI requests, improves timeout diagnostics, warns about ignored proxies and cleartext API-key transport, and adds opt-in request tracing without exposing headers or bodies.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/sim-cli/src/http/client.ts | Adds validated request bounds, backward-compatible abort-signal composition, timeout-specific errors, and safe request tracing; the previously reported timeout and Node 20 issues are fixed. |
| packages/sim-cli/src/http/environment.ts | Adds one-time stderr warnings for ignored proxy configuration and API keys sent over remote cleartext HTTP. |
| packages/sim-cli/src/commands/protocol/files-get.ts | Distinguishes timeout-aborted response streams from genuine filesystem write failures. |
| packages/sim-cli/src/index.ts | Converts timeout errors raised while consuming response bodies into actionable CLI output. |
| packages/sim-cli/src/http/client.test.ts | Covers timeout validation, sub-millisecond bounds, Node 20 signal composition, timeout diagnostics, and redacted tracing behavior. |
| packages/sim-cli/src/http/environment.test.ts | Covers proxy support boundaries, one-time warning behavior, and cleartext endpoint classification. |
Reviews (5): Last reviewed commit: "fix(cli): keep a sub-millisecond timeout..." | Re-trigger Greptile
Two ways the new timeout could fail before the request was made. `AbortSignal.any` arrived in Node 20.3 and this package supports Node 20, so composing a caller's abort signal with the timeout threw a bare TypeError on the earliest 20.x releases. It is now used when present and composed through an AbortController when not. `AbortSignal.timeout` rejects a fractional millisecond outright, and past 2^31-1 ms it does not fail at all — it clamps to 1ms, so the longest timeout anyone asked for became the shortest. The value is now rounded and refused above what Node can actually wait, pointing at 0 for an unbounded wait. Also unstubs env vars between tests: `stubEnv` is not undone by `unstubAllGlobals`, so a SIM_TIMEOUT_SECONDS set for one test configured every test after it.
|
@cursor review |
…-body `runtimeCanProxy` treated any release between 22 and 24 as capable, so on Node 23 — which reached end of life before the backport — a configured proxy was ignored and the CLI stayed silent about it, which is the exact failure the warning exists to report. The table is now the two lines that shipped the support, and anything after them. `AbortSignal.timeout` keeps firing after `fetch` resolves, so a bound that elapsed while the body was still being read — a large `files get` — escaped the client's own handling and printed a raw TimeoutError stack. The top-level handler now names it, which covers the streaming path as well as the JSON one. A user's own Ctrl-C raises AbortError and is deliberately left alone.
|
@cursor review |
`files get --output-file` streams the body to disk, and `streamToFile` converted anything the stream threw into a write failure. So a request bound elapsing mid-download read as `Could not write <path>: ...`, sending the reader to check permissions and free space for a timeout they can raise, and hiding the one instruction that resolves it. The predicate and that instruction now live beside the timeout that raises them, so the client, the top-level handler and the download path all say the same thing. The wrapping stays where it is: the staged-download cleanup runs off that failure, and rethrowing past it would leak the temporary directory.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6f2fdc4. Configure here.
Zero is how this function says "no bound", so rounding a positive SIM_TIMEOUT_SECONDS down to zero inverted the request: anything under 0.0005s asked for the shortest possible timeout and got none at all, leaving a stalled request to hang. Introduced by the rounding that fixed the fractional-millisecond rejection. Floored at 1ms for every positive value; only a literal 0 still disables.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 09ff9c0. Configure here.
Summary
Four transport gaps, all of which failed silently. clig.dev mandates the first two explicitly.
SIM_TIMEOUT_SECONDSnow bounds one. The default is 3600s, deliberately above every timeout the server itself applies — a synchronous workflow run is allowed 3000s on a paid plan (DEFAULT_SYNC_TIMEOUTS_SECONDS), so a tighter default would abort real work and report it as a transport failure.0removes the bound, for a self-hosted deployment that runs executions without one of its own. The caller's abort signal is composed with the timeout viaAbortSignal.any, not replaced, so neither can mask the other.fetchhonoursHTTP(S)_PROXYonly whenNODE_USE_ENV_PROXYopts in, and only from v22.21/v24.5 — so on a network that reaches the API only through a proxy, every command failed to connect while the variable that would have fixed it was already set. The CLI cannot enable it from inside the process (Node reads it at startup), so it names the fix rather than bundling an HTTP stack for a setting the platform now owns. Measured: bundling undici to proxy ourselves costs +900 KB on a 450 KB bundle.http://endpoint with no signal. Now a warning, not a refusal —http://localhost:3000is the documented dev setup and a deployment terminating TLS at a gateway is real. Loopback stays silent.SIM_DEBUG=1traces method, URL, status and duration. Bodies and headers are deliberately absent: the request carries the API key, andsecrets setcarries the secret itself.All four write to stderr, so a piped stdout stays parseable — verified with both a warning and tracing active.
Type of Change
Testing
Tested manually against staging: trace line, proxy warning, and cleartext warning all render on stderr while
--output jsonon stdout still parses; loopback and https stay silent.Suite is 361 passed / 1 skipped (was 348). All eight new assertions were proven red by reverting their source change and green on restore.
lint,type-check,build,check:cli-docsand all 29 audits pass — includingcheck:utils, which correctly caught a rawsetTimeoutpromise in a new test and sent me to the localsleephelper.Checklist