Skip to content

Show help for a bare ts dev proxy invocation - #1176

Open
dhruv8sh wants to merge 6 commits into
mainfrom
fix/dev-proxy-bare-invocation-help
Open

dhruv8sh wants to merge 6 commits into
mainfrom
fix/dev-proxy-bare-invocation-help

Conversation

@dhruv8sh

@dhruv8sh dhruv8sh commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • A bare ts dev proxy now prints Clap help and exits immediately, instead of touching Safari's system proxy state (prompting for sudo) and then failing with a debug-formatted error-stack report that leaked internal file:line paths.
  • ts dev proxy ca … and an explicit-but-incomplete invocation (--from without --to) keep working exactly as before — verified this empirically against the pinned clap version, not just by reading the derive macro.
  • Also switched proxy error rendering from {report:?} to {report:#} (not plain {report}): error-stack's non-alternate Display stops after the first context frame, which would have silently dropped the actual cause (e.g. "no rewrite rule: pass --map FROM=TO …") behind the generic wrapper message.

Changes

File Change
crates/trusted-server-cli/src/commands/dev/proxy/mod.rs Add #[command(arg_required_else_help = true)] to ProxyArgs so Clap shows help before run ever executes on a bare invocation
crates/trusted-server-cli/src/commands/dev/mod.rs Render proxy errors with {report:#} (chains every error-stack context frame) instead of {report:?} (leaked source locations)
crates/trusted-server-cli/src/commands/dev/proxy/config.rs Fix base_args()/parse_args(&["ts"]) test helpers, which broke from the new arg_required_else_help (it applies to any Command ProxyArgs is flattened into, including these); now restate the --listen default explicitly so parsing doesn't hit the same help short-circuit and abort the test binary
crates/trusted-server-cli/src/run.rs Add regression tests: bare invocation shows help (and ca path still parses under it), and a --from-only partial rule still parses instead of showing help

Closes

Closes #1063

Test plan

  • cargo test-fastly && cargo test-axum — not applicable, no fastly/axum/core files touched
  • cargo clippy-fastly && cargo clippy-axum — not applicable
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run — not applicable
  • JS format: cd crates/trusted-server-js/lib && npm run format — not applicable
  • Docs format: cd docs && npm run format — not applicable
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1 — not applicable
  • Manual testing via fastly compute serve — not applicable (CLI-only, not a Fastly adapter change)
  • Other:
    • cargo clippy -p trusted-server-cli --target aarch64-apple-darwin --all-targets --all-features -- -D warnings — clean
    • ./scripts/test-cli.sh (native x86_64-unknown-linux-gnu, macOS cfg gates temporarily removed locally to actually execute the macOS-only code on this Linux box) — 175 passed, 1 unrelated pre-existing failure (restore_system_proxy_if_pending_removes_file_with_empty_service, a genuinely macOS-only test hitting its own no-op branch off-macOS; untouched by this change)
    • Live-ran ts dev proxy, ts dev proxy ca path, and ts dev proxy --from a.example.com under the same native build to confirm real output matches the acceptance criteria

Checklist

  • Changes follow AGENTS.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!) — no new logging added
  • New code has tests
  • No secrets or credentials committed

@dhruv8sh dhruv8sh changed the title Fix/dev proxy bare invocation help Show help for a bare ts dev proxy invocation Sep 16, 2026
@dhruv8sh dhruv8sh changed the title Show help for a bare ts dev proxy invocation Show help for a bare ts dev proxy invocation Sep 16, 2026

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at afba58bc3 against origin/main (35560c81d), verified in a scratch worktree on macOS arm64.

The fix does what #1063 asked: arg_required_else_help = true short-circuits a bare ts dev proxy into clap's help before browser::restore_system_proxy_if_pending can run, so there is no sudo prompt and no system-proxy side effect on the failure path. I confirmed that behaviourally against a build of this head rather than inferring it from the derive macro:

$ ts dev proxy                  -> full clap help, exit 2, no sudo prompt
$ ts dev proxy ca path          -> /.../dev-proxy/ca-cert.pem, exit 0
$ ts dev proxy --from a.example.com -> concise "no rewrite rule" error, not help

I also checked the blast radius of the new flag. grep -rn ProxyArgs crates/ --include=*.rs turns up only dev/mod.rs, proxy/mod.rs, the proxy/config.rs tests and tests/support/mod.rs:207; every resolve(&[...]) call site in tests/support/mod.rs already passes at least one arg, so none of them falls into the help short-circuit. Rules can't arrive from the environment either -- config.rs touches std::env only for XDG_DATA_HOME -- so a bare invocation is always an error and the flag cannot reject an otherwise-valid run.

Local results at head: cargo fmt --all -- --check clean, cargo clippy --manifest-path crates/trusted-server-cli/Cargo.toml --target aarch64-apple-darwin --all-targets --all-features -- -D warnings clean, cargo test --manifest-path crates/trusted-server-cli/Cargo.toml --target aarch64-apple-darwin 178/7/29/1 passed, 0 failed. All 20 remote checks are green. One note on the PR body: the "1 unrelated pre-existing failure" (restore_system_proxy_if_pending_removes_file_with_empty_service) passes here -- it looks like an artefact of running on Linux with the cfg gates stripped, and the macOS CI job agrees.

Nothing blocking. Three findings below: one stale-description/follow-up note, two readability nits.


🤔 thinking -- the PR body describes an error-rendering change that isn't in the final diff, and #1063's first problem is still live

The Summary and the Changes table both list the dev/mod.rs {report:?} -> {report:#} switch. Commit 1476137e made that change and commit ae1d7c6a reverted it, so dev/mod.rs doesn't appear in origin/main...HEAD at all. Head is still:

DevCommand::Proxy(args) => proxy::run(&args).map_err(|report| format!("{report:?}")),

#1063 listed two problems and this PR says Closes #1063. Problem 2 (side effects before the error) is genuinely fixed. Problem 1 (internal file:line leaking into user-facing output) is not -- reproduced against a build of this head:

[ts] invalid rule configuration
├╴at crates/trusted-server-cli/src/commands/dev/proxy/mod.rs:261:41
│
╰─▶ no rewrite rule: pass --map FROM=TO (or -f/--from with -t/--to)
    ╰╴at crates/trusted-server-cli/src/commands/dev/proxy/config.rs:269:20

The revert itself is right, and worth recording so nobody undoes it: error-stack 0.6's impl Display for Report<C> filters FrameKind::Attachment(_) => None before the alternate() branch (fmt/mod.rs:1164), so {:#} drops every .attach(...) payload exactly like {} does -- alternate() only chains additional contexts. The ca regenerate abort path and the stale-CA-file removal path in proxy/mod.rs carry all of their actionable guidance in .attach(...) on top of a context that displays as just "certificate authority error", so {report:#} would have shown the user that bare string and nothing else. Getting this right needs the CA errors to carry their guidance in a Display context (or attach_printable plus a custom renderer), which is a bigger change than this PR.

Could you drop those two lines from the body and file the rendering work as a follow-up issue? As written, Closes #1063 will close an issue whose problem 1 is still open. (The issue text does sanction deferring it -- "acceptable to leave out and file separately" -- so this is a bookkeeping ask, not a code one.)

👍 praise

Switching parse_from -> try_parse_from(...).expect("should parse proxy args") in both config.rs::parse_args and tests/support/mod.rs::resolve is the right hardening, and it's a subtle one: with arg_required_else_help in play, parse_from would have had clap call process::exit(2) from inside a test, killing the test binary mid-run instead of failing a single test. Making DEFAULT_LISTEN the single source for both the clap default_value and the test helper is the right call too, and I appreciate that the ca-subcommand interaction #1063 flagged as risky was verified empirically rather than assumed -- I re-ran it independently and it holds.


Not verified on my side, for the record: ca install | uninstall | regenerate were parse-tested but never executed (they mutate the macOS login keychain and the on-disk CA), so the {report:#} claim above rests on the error-stack source plus the code paths rather than an observed failure. --launch browser orchestration and the Safari system-proxy restore path are unchanged by this PR and weren't exercised. Non-macOS hosts compile none of this code.

Comment thread crates/trusted-server-cli/src/commands/dev/proxy/mod.rs Outdated
Comment thread crates/trusted-server-cli/src/commands/dev/proxy/config.rs Outdated
DEFAULT_LISTEN only has in-crate consumers, so pub(crate) is enough.
Use the existing base_args() helper instead of an unrelated --insecure
flag to satisfy arg_required_else_help in the rewrite_host test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Reviewed 40b65c62c56207325d3285538c7657a0dfa8ccb3 against 6cae7f5da8911c746cf873581885f90c3820dd96. The Clap parsing guard prevents a bare ts dev proxy from reaching proxy startup, while CA subcommands and explicit partial rules still parse. macOS CI and the focused parser tests pass.

P3 finding

🔧 The guide still promises the old result for a bare invocation (docs/guide/ts-dev-proxy.md:80)

The guide says every invocation without --map, -f, or -t produces the no rewrite rule error. A completely bare invocation now prints Clap help instead. The old error remains only when another explicit option reaches configuration resolution, so the guide contradicts the central behavior of this PR.

Please state that a bare invocation prints help, while an invocation containing options but no complete rewrite rule reports no rewrite rule.

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.

Show help for a bare ts dev proxy invocation

3 participants