Conversation
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…it trap, dedupe listen default Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ts dev proxy invocation
prk-Jr
left a comment
There was a problem hiding this comment.
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.
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
left a comment
There was a problem hiding this comment.
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.
Summary
ts dev proxynow prints Clap help and exits immediately, instead of touching Safari's system proxy state (prompting forsudo) and then failing with a debug-formattederror-stackreport that leaked internalfile:linepaths.ts dev proxy ca …and an explicit-but-incomplete invocation (--fromwithout--to) keep working exactly as before — verified this empirically against the pinnedclapversion, not just by reading the derive macro.{report:?}to{report:#}(not plain{report}): error-stack's non-alternateDisplaystops 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
crates/trusted-server-cli/src/commands/dev/proxy/mod.rs#[command(arg_required_else_help = true)]toProxyArgsso Clap shows help beforerunever executes on a bare invocationcrates/trusted-server-cli/src/commands/dev/mod.rs{report:#}(chains everyerror-stackcontext frame) instead of{report:?}(leaked source locations)crates/trusted-server-cli/src/commands/dev/proxy/config.rsbase_args()/parse_args(&["ts"])test helpers, which broke from the newarg_required_else_help(it applies to anyCommandProxyArgsis flattened into, including these); now restate the--listendefault explicitly so parsing doesn't hit the same help short-circuit and abort the test binarycrates/trusted-server-cli/src/run.rsca pathstill parses under it), and a--from-only partial rule still parses instead of showing helpCloses
Closes #1063
Test plan
cargo test-fastly && cargo test-axum— not applicable, no fastly/axum/core files touchedcargo clippy-fastly && cargo clippy-axum— not applicablecargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run— not applicablecd crates/trusted-server-js/lib && npm run format— not applicablecd docs && npm run format— not applicablecargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1— not applicablefastly compute serve— not applicable (CLI-only, not a Fastly adapter change)cargo clippy -p trusted-server-cli --target aarch64-apple-darwin --all-targets --all-features -- -D warnings— clean./scripts/test-cli.sh(nativex86_64-unknown-linux-gnu, macOScfggates 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)ts dev proxy,ts dev proxy ca path, andts dev proxy --from a.example.comunder the same native build to confirm real output matches the acceptance criteriaChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!) — no new logging added