Skip to content

fix(cli): bound, trace, and explain the requests the CLI makes - #6798

Merged
waleedlatif1 merged 5 commits into
stagingfrom
fix/cli-transport-hygiene
Aug 18, 2026
Merged

fix(cli): bound, trace, and explain the requests the CLI makes#6798
waleedlatif1 merged 5 commits into
stagingfrom
fix/cli-transport-hygiene

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

Four transport gaps, all of which failed silently. clig.dev mandates the first two explicitly.

  • Requests had no timeout. A connection that is accepted and then never answers hung the terminal indefinitely. SIM_TIMEOUT_SECONDS now 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. 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 via AbortSignal.any, not replaced, so neither can mask the other.
  • A configured proxy was silently ignored. Node's fetch honours HTTP(S)_PROXY only when NODE_USE_ENV_PROXY opts 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.
  • An API key went to any http:// endpoint with no signal. Now a warning, not a refusal — http://localhost:3000 is the documented dev setup 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 — verified with both a warning and tracing active.

Type of Change

  • Bug fix

Testing

Tested manually against staging: trace line, proxy warning, and cleartext warning all render on stderr while --output json on 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-docs and all 29 audits pass — including check:utils, which correctly caught a raw setTimeout promise in a new test and sent me to the local sleep helper.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

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.
@vercel

vercel Bot commented Aug 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 18, 2026 12:54am

Request Review

@cursor

cursor Bot commented Aug 18, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes core HTTP transport for every CLI command (timeouts, signal composition, error paths) and adds cleartext-key warnings; behavior is well-tested but affects long-running sync work and download streaming timeouts.

Overview
Adds per-request timeouts, debug tracing, and stderr environment warnings so sim-cli failures are visible and actionable instead of hanging or mislabeled.

SIM_TIMEOUT_SECONDS (default 3600s, 0 = no limit) aborts stalled connections via AbortSignal.timeout, composed with caller abort signals (fallback when AbortSignal.any is missing). Invalid or extreme values are rejected; timeouts surface as explicit “did not answer” errors with a hint to raise the bound—not as unreachable endpoints or disk write failures (files get).

SIM_DEBUG=1 logs method, URL, status, and duration to stderr only (no headers/bodies).

New environment helpers warn once per process when HTTP(S)_PROXY is set but Node will ignore it (needs NODE_USE_ENV_PROXY=1 and supported Node versions), and when an API key is sent over http to non-loopback hosts.

Docs in the CLI README and configuration guide document the new variables.

Reviewed by Cursor Bugbot for commit 09ff9c0. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The 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.

  • Adds configurable per-request timeouts while preserving caller cancellation and compatibility with early Node 20 releases.
  • Reports timeout failures consistently during both initial requests and streamed body downloads.
  • Adds one-time proxy and cleartext credential warnings on stderr.
  • Documents and tests the new timeout and debug environment settings.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

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

Comment thread packages/sim-cli/src/http/client.ts Outdated
Comment thread packages/sim-cli/src/http/client.ts
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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread packages/sim-cli/src/http/environment.ts
Comment thread packages/sim-cli/src/http/client.ts
…-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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread packages/sim-cli/src/index.ts
`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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread packages/sim-cli/src/http/client.ts Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@waleedlatif1
waleedlatif1 merged commit 0e84e92 into staging Aug 18, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/cli-transport-hygiene branch August 18, 2026 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant