Conversation
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
a67eda2 to
1476137
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Reviewed at 1476137e in a clean worktree. The arg_required_else_help change is correct, well-targeted, and closes every acceptance criterion in #1063 on its own — and confirming the optional-subcommand interaction empirically rather than trusting the derive macro is exactly what the issue asked for.
One blocking ask, on the {report:#} hunk that rides along. Three non-blocking nits follow as suggestions.
Blocking: {report:#} drops every .attach(...), which silently guts the two ca regenerate abort messages. Details inline on dev/mod.rs. My recommendation is to drop that hunk from this PR and file it separately, exactly as #1063 invited ("a slightly wider change — it affects every ts dev proxy error, not just this one — so it is acceptable to leave out and file separately if it grows"). It needs its own tests (there are none for rendered error output) and a design call on attachments-vs-contexts. The arg_required_else_help half is ready to land without it.
Test coverage against the issue's acceptance criteria:
| AC | Covered |
|---|---|
Bare ts dev proxy prints help, no sudo / no side effects |
yes — dev_proxy_bare_invocation_shows_help_before_running |
ca path, install, uninstall, regenerate still work |
partial — only path (suggestion on run.rs) |
| Incomplete-but-explicit invocation still gives the concise no-rule error | yes — new parse test plus the pre-existing no_rule_passed_is_a_no_rule_error |
| Regression test covers the bare invocation | yes |
cargo clippy and the CLI suite pass |
yes — verified below |
Verified locally, against 1476137e on aarch64-apple-darwin:
cargo fmt --all -- --check— cleancargo clippy -p trusted-server-cli --target aarch64-apple-darwin --all-targets --all-features -- -D warnings— cleancargo test --package trusted-server-cli --target aarch64-apple-darwin --lib— 177 passed, 0 failed (both new tests execute on macOS)- Each suggestion below applied in isolation and re-verified (fmt clean, tests pass)
- Fastly/axum/cloudflare/spin gates not run — no files outside
trusted-server-cliare touched
Note that gh pr checks 1167 currently reports no checks on this branch. Both new tests are #[cfg(target_os = "macos")], so only the test-cli job (runs-on: macos-latest) actually executes them; the Linux test-axum job's ./scripts/test-cli.sh compiles them out.
| match command { | ||
| #[cfg(target_os = "macos")] | ||
| DevCommand::Proxy(args) => proxy::run(&args).map_err(|report| format!("{report:?}")), | ||
| DevCommand::Proxy(args) => proxy::run(&args).map_err(|report| format!("{report:#}")), |
There was a problem hiding this comment.
Blocking. {report:#} chains context frames, but error-stack 0.6's Display impl explicitly discards attachments:
// error-stack-0.6.0/src/fmt/mod.rs:1164
FrameKind::Context(context) => Some(context.to_string()),
FrameKind::Attachment(_) => None,Two error paths in proxy/mod.rs carry their entire user-facing message in an attachment rather than a context:
proxy/mod.rs:218— "could not revoke the previously-installed CA from the keychain; aborting regenerate... Remove the old CA manually (Keychain Access), then retry."proxy/mod.rs:235— "could not remove old CA file {path} during regenerate ({err}); aborting so the stale key is not silently reused"
Before this change {report:?} printed those. After it, a user who hits either abort on ts dev proxy ca regenerate sees exactly:
certificate authority error
Confirmed empirically rather than by reading the impl:
ALT = [certificate authority error] # Report::new(CertAuthority).attach("could not revoke ...")
PLAIN = [certificate authority error]
CFG_ALT = [invalid rule configuration: no rewrite rule: pass --map FROM=TO (or -f/--from with -t/--to)]
CFG_PLAIN = [invalid rule configuration]
So the reasoning in the PR description holds for ConfigError and fails for the CA paths. attach_printable is not the fix — Display drops that too. I checked the other .attach sites (upstream/mod.rs, upstream/connect.rs); those are per-request and terminate in a 502 plus a log, so they never reach this formatter. The blast radius is precisely the two ca regenerate aborts.
Preferred fix: drop this hunk from the PR and file it separately, per #1063's own guidance. The bare-invocation AC is already satisfied without it, since run never executes.
If you would rather keep it here, promote both messages to contexts so {report:#} renders them, and add a test asserting the rendered string (nothing currently covers error rendering):
// proxy/mod.rs — ProxyError
/// The previously-installed CA could not be revoked from the OS trust store.
#[display(
"could not revoke the previously-installed CA from the keychain; aborting regenerate so \
on-disk key material still matches OS trust. Remove the old CA manually (Keychain \
Access), then retry."
)]
CaRevokeFailed,
/// An old CA file could not be removed during regenerate.
#[display("could not remove old CA file {path} during regenerate ({source}); aborting so the stale key is not silently reused")]
CaFileRemoval { path: String, source: String },then Report::new(ProxyError::CaRevokeFailed) / Report::new(ProxyError::CaFileRemoval { .. }) at proxy/mod.rs:218 and :235.
Third option, if the goal is only to stop the at file:line leak that #1063 actually reported: keep {report:?} and install Report::install_debug_hook::<core::panic::Location>(|_, _| {}) at CLI startup.
There was a problem hiding this comment.
Good catch, and confirmed against the fmt source — I hadn't accounted for error-stack 0.6 dropping attachments in Display. Went with your third option's spirit but simpler: reverted dev/mod.rs to {report:?} in ae1d7c6, since the bare-invocation AC this PR targets never reaches run() at all (clap short-circuits to help first), so the formatter change wasn't load-bearing for this PR's actual scope. Filing the CA-attachment→context promotion separately as you suggested rather than scope-creeping it in here.
| parse_args(&["ts", "--listen", "127.0.0.1:18080"]) | ||
| } | ||
|
|
||
| fn parse_args(argv: &[&str]) -> crate::commands::dev::proxy::ProxyArgs { |
There was a problem hiding this comment.
Non-blocking, but worth fixing while you are here. parse_args uses Parser::parse_from, and arg_required_else_help now applies to every Command that ProxyArgs is flattened into — including this test wrapper. On the help short-circuit, parse_from calls Error::exit(), i.e. std::process::exit(2). That does not fail one test; it aborts the whole test binary with no test name and no attribution:
error: test failed ... process didn't exit successfully: ... (exit status: 2)
note: test exited abnormally
Reproduced by adding a defaults-only parse_args(&["ts"]) call. This PR works around the two existing call sites by passing --listen, but the trap stays armed for whoever writes the next defaults-only test. Switching to the fallible parser turns it back into an ordinary test failure:
W::try_parse_from(argv).expect("should parse proxy args").aVerified: compiles, cargo fmt --check clean, all 17 config tests pass.
crates/trusted-server-cli/tests/support/mod.rs:207 has the identical Wrapper::parse_from shape. All its current call sites pass arguments, so it is not live today, but it is the same latent trap.
There was a problem hiding this comment.
Fixed in ae1d7c6 — switched parse_args to try_parse_from(...).expect("should parse proxy args"). Verified the exit trap is real: a defaults-only parse_args(&["ts"]) call aborts the whole test binary with parse_from, and becomes an ordinary panic/failure with try_parse_from. Left tests/support/mod.rs:207 alone since it's genuinely out of scope here (no live call site), but noted it for whoever touches that next.
| a: crate::commands::dev::proxy::ProxyArgs, | ||
| } | ||
| W::parse_from(["ts"]).a | ||
| parse_args(&["ts", "--listen", "127.0.0.1:18080"]) |
There was a problem hiding this comment.
Minor. The deleted comment ("Construct via clap so defaults match the real surface") was carrying a real guarantee, and it is gone now: 127.0.0.1:18080 is duplicated between proxy/mod.rs:98's default_value and two spots here, with no test asserting they agree. A future change to the real default leaves base_args() silently pinned to the stale value.
Cheapest fix is a shared const referenced from both sides — clap 4 accepts a &'static str expression in #[arg(default_value = ...)]. For the clap_parses_rewrite_host_as_a_bool call site below, any neutral flag sidesteps the duplication entirely, e.g. parse_args(&["ts", "--insecure"]).rewrite_host.
There was a problem hiding this comment.
Done in ae1d7c6: added pub const DEFAULT_LISTEN in proxy/mod.rs, used it both in the arg's default_value and in base_args(), and switched the clap_parses_rewrite_host_as_a_bool false-case to your suggested neutral flag (--insecure) so it no longer needs the listen literal at all.
| parse(&["ts", "dev", "proxy", "ca", "path"]); | ||
| } |
There was a problem hiding this comment.
Minor. #1063's acceptance criteria list all four ca actions ("ts dev proxy ca path, ca install, ca uninstall, and ca regenerate still work"), but only path is exercised. This also splits the bundled second assertion out of a test whose name is solely about the bare invocation.
Verified: cargo fmt --check clean, 3/3 tests pass.
| parse(&["ts", "dev", "proxy", "ca", "path"]); | |
| } | |
| } | |
| #[test] | |
| #[cfg(target_os = "macos")] | |
| fn dev_proxy_ca_subcommands_still_parse_under_arg_required_else_help() { | |
| for action in ["path", "install", "uninstall", "regenerate"] { | |
| parse(&["ts", "dev", "proxy", "ca", action]); | |
| } | |
| } |
There was a problem hiding this comment.
Applied your suggested diff verbatim in ae1d7c6 — split out dev_proxy_ca_subcommands_still_parse_under_arg_required_else_help covering all four actions (path/install/uninstall/regenerate), and the bare-invocation test now only asserts the help short-circuit.
…it trap, dedupe listen default Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
|
Follow-up self-review after ae1d7c6 caught two more small issues in the same code: a local |
2 participants
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