Show help for a bare ts dev proxy invocation

[dhruv8sh]

Summary

• A bare ts dev proxy now prints Clap help and exits immediately, instead of touching Safari's system proxy state (attempting 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.

Out of scope, tracked separately: switching proxy error rendering from {report:?} to {report:#} was tried and reverted. error-stack 0.6's non-alternate Display and its {:#} alternate both filter .attach(...) payloads before rendering — only {:?} (Debug) shows them. The CA error paths in this file carry all of their actionable guidance via .attach(...), so switching to {report:#} would have silently dropped that guidance behind a bare "certificate authority error". Fixing the file:line leak without regressing CA error messages needs those errors to carry guidance in a Display context instead, which is a bigger change than this PR — issue #1063 explicitly allows deferring it.

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; narrow DEFAULT_LISTEN to pub(crate)
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; add clap_applies_the_real_listen_default, which pins the real clap default_value independently of the test-helper constant
crates/trusted-server-cli/tests/support/mod.rs	Switch parse_from to try_parse_from in resolve() — parse_from calls process::exit on a parse failure, which under the new arg_required_else_help would terminate the test binary instead of failing an assertion
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
docs/guide/ts-dev-proxy.md	Correct the invocation-behavior description: a bare invocation now prints help, not the no rewrite rule error

Closes

Closes #1063 (problem 1, the file:line leak, is deferred per the issue's own allowance — see Summary)

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
• 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

[prk-Jr]

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.

[ChristianPavilonis]

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.

[prk-Jr]

Summary

Reviewed commit 40b65c62c56207325d3285538c7657a0dfa8ccb3.

The bare invocation is stopped by Clap before proxy startup, and CA subcommands and explicit partial rules still parse. The prior visibility and test-helper suggestions have been applied. No new runtime correctness defect found.

Non-blocking

🤔 [P3] Align the description and guide with the actual help-only change

Body-level: PR description and docs/guide/ts-dev-proxy.md:80–81 (outside diff).

The PR body still claims a {report:?} → {report:#} error-rendering change, but dev/mod.rs is unchanged and still formats Debug. The guide says all invocations without rewrite flags print no rewrite rule, although the newly guarded bare invocation prints Clap help. Remove the inaccurate rendering claim and distinguish bare help from explicit incomplete-rule errors. The wider error-report rendering task may remain separate, as issue #1063 allows.

Proposed guide wording (apply manually because the guide is outside the diff):

A bare `ts dev proxy` prints help and exits before proxy startup. An
invocation with explicit options but no complete rewrite rule reports
`no rewrite rule: pass --map FROM=TO (or -f/--from with -t/--to)`.

The prior DEFAULT_LISTEN visibility and unrelated --insecure test-workaround comments are resolved in commit 40b65c62. The only changes since the previous reviewed commit are those two small source cleanups. CA install/uninstall/regenerate were inspected and parser-tested, not executed against the system keychain.

Validation

All 32 CLI argument-parser tests and 17 proxy configuration tests passed locally on macOS arm64, including bare help, all four CA actions, and incomplete-rule parsing. System keychain/proxy operations were not executed. General build/lint gates rely on remote CI.

CI Status

• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• integration tests: PASS
• CodeQL: PASS
• Analyze (javascript-typescript): PASS
• cargo test (ts CLI, native): PASS
• vitest: PASS
• format-typescript: PASS (required)
• Analyze (javascript-typescript): PASS
• cargo test (axum native): PASS
• cargo fmt: PASS (required)
• Analyze (rust): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo test: PASS (required)
• format-docs: PASS (required)
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• CLAUDE.md symlink guard: PASS
• prepare integration artifacts: PASS
• Analyze (actions): PASS
• cargo test (cross-adapter parity): PASS

[aram356]

Summary

The core fix works as intended: ts dev proxy with no arguments now prints Clap help and exits without touching system proxy state or attempting sudo, ts dev proxy ca <action> still parses under arg_required_else_help, and a partial --from-only rule still reaches run. Verified on real macOS (aarch64-apple-darwin): 178 CLI tests pass, cargo fmt --all -- --check is clean, and native clippy with -D warnings is clean.

Two blocking issues remain. The {report:#} error-rendering fix that the PR description presents as a headline change was reverted mid-review and is not in the head, and the sudo/side-effect half of #1063 still reproduces whenever any single flag is passed. Separately, the reworked base_args() helper silently dropped all test coverage of the real --listen default.

1 of the inline comments below carries a one-click GitHub suggestion. The remaining comments describe the fix in prose because the change touches files outside the diff, or adds a new test, and can't be auto-applied.

Blocking

🔧 wrench

• {report:#} rendering was reverted but is still claimed in the PR body — see "Cross-cutting" below
• #1063's sudo / side-effect bug still reproduces with any single flag — see "Cross-cutting" below
• New base_args() removes all coverage of the real --listen default — see inline at crates/trusted-server-cli/src/commands/dev/proxy/config.rs:363

Non-blocking

⛏ nitpick

• DEFAULT_LISTEN doc comment overstates its guarantee — see inline at crates/trusted-server-cli/src/commands/dev/proxy/mod.rs:79
• Inconsistent use clap::Parser as _ placement — see inline at crates/trusted-server-cli/tests/support/mod.rs:212

Cross-cutting / body-level findings

• 🔧 The {report:?} → {report:#} fix was reverted, but the PR body still claims it. The PR description lists this as one of three headline changes, and issue #1063 makes it acceptance criterion #1 ("without ... printing error-stack debug output"). Commit 1476137e7 contained it; commit ae1d7c6a3 reverted it. The head at crates/trusted-server-cli/src/commands/dev/mod.rs:34 still reads format!("{report:?}").

  Live run against this PR's 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
  

  Internal source locations still leak. I scratch-applied the one-character change and it renders exactly what the PR body describes, with both context frames chained and no file:line:

  [ts] invalid rule configuration: no rewrite rule: pass --map FROM=TO (or -f/--from with -t/--to)
  

  Please either restore the change in dev/mod.rs:

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

  or drop the claim from the PR description so the recorded intent matches the code. Apply manually — dev/mod.rs is not part of this diff, so there is no RIGHT-side hunk to anchor a suggestion to.

• 🔧 Issue #1063's sudo / side-effect bug still reproduces with any single flag. arg_required_else_help fires only on a zero-argument invocation. One unrelated flag satisfies it, and parsing then falls straight into run, which calls browser::restore_system_proxy_if_pending at proxy/mod.rs:259 before config::resolve at proxy/mod.rs:261:

  $ ts dev proxy --insecure
  sudo: a password is required
  sudo: a password is required
  warning: Safari: could not auto-restore the system proxy for 'Wi-Fi' (needs admin). ...
  [ts] invalid rule configuration
  ├╴at crates/trusted-server-cli/src/commands/dev/proxy/mod.rs:261:41
  

  Reproduced identically with --listen 127.0.0.1:18080 and with --from a.example.com.

  This is not a contrived input. restore_system_proxy_if_pending early-returns unless the safari-proxy-restore file exists, and that file was present on my machine from an earlier hard-killed run — exactly the state described in #1063. The flag-bearing invocation therefore hits the same sudo prompt and the same debug-formatted report that the issue was filed about.

  The issue's suggested approach was arg_required_else_help, but its acceptance criteria are behavioural: no touching of system proxy state and no sudo attempt before discovering there is no rule. The root cause is the ordering in run, so resolving rules first would close the whole class:

  // Resolve rules first: a bare-or-flagged invocation with no rule must not
  // touch system proxy state or attempt sudo before failing.
  let mut cfg = config::resolve(args).change_context(ProxyError::Config)?;
  
  browser::restore_system_proxy_if_pending(&config::ca_dir(args), false);

  Worth flagging honestly: the existing comment at proxy/mod.rs:255-258 argues for the current ordering ("a missing/bad rule must not strand the system proxy"), so this is a genuine design tension rather than a simple oversight — swapping the order trades "a bad rule strands the proxy" against "a bad rule prompts for sudo". Either resolution is defensible; what should not stand is closing #1063 while its second stated problem is still reachable. If you would rather scope the ordering change to a follow-up issue, that is reasonable — please say so explicitly on this PR and in the issue so the remaining gap is recorded. Apply manually — the target lines are outside this PR's hunks.

CI Status

All 20 reported checks pass. The cargo test (ts CLI, native) job runs on macos-latest, so the three new #[cfg(target_os = "macos")] tests do execute in CI rather than being silently skipped.

• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• integration tests: PASS
• CodeQL: PASS
• Analyze (javascript-typescript): PASS
• cargo test (ts CLI, native): PASS
• vitest: PASS
• format-typescript: PASS (required)
• cargo test (axum native): PASS
• cargo fmt: PASS (required)
• Analyze (rust): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo test: PASS (required)
• format-docs: PASS (required)
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• CLAUDE.md symlink guard: PASS
• prepare integration artifacts: PASS
• Analyze (actions): PASS
• cargo test (cross-adapter parity): PASS