Skip to content

Show help for a bare ts dev proxy invocation #1176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dhruv8sh wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Show help for a bare ts dev proxy invocation #1176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1476137
Show help for a bare ts dev proxy invocation
dhruv8sh Sep 15, 2026
ae1d7c6
Address PR review: preserve CA error attachments, fix test process-ex…
dhruv8sh Sep 15, 2026
85ac1ff
Move test-only clap import to module scope, harden second parse trap
dhruv8sh Sep 15, 2026
afba58b
Merge branch 'main' into fix/dev-proxy-bare-invocation-help
dhruv8sh Sep 16, 2026
adfad9a
Merge branch 'main' into fix/dev-proxy-bare-invocation-help
prk-Jr Sep 16, 2026
40b65c6
Narrow DEFAULT_LISTEN visibility and simplify rewrite_host test
dhruv8sh Sep 16, 2026
ffd5bf8
Rework proxy CLI arg-parsing tests to cover the real clap defaults
dhruv8sh Sep 22, 2026
fe35b68
Merge branch 'main' into fix/dev-proxy-bare-invocation-help
aram356 Sep 24, 2026
057a647
Merge branch 'main' into fix/dev-proxy-bare-invocation-help
aram356 Sep 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 119 additions & 63 deletions crates/trusted-server-cli/src/commands/dev/proxy/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ fn resolve_basic_auth(args: &ProxyArgs) -> Result<Option<BasicAuth>, ConfigError

#[cfg(test)]
mod tests {
use clap::Parser as _;
use hyper::header::HeaderValue;
use rustls::pki_types::ServerName;

Expand All @@ -358,31 +359,19 @@ mod tests {
AddressPolicy, OriginKey, ReferenceIdentity, Transport, VerifyMode,
};

fn base_args() -> crate::commands::dev::proxy::ProxyArgs {
// Construct via clap so defaults match the real surface.
use clap::Parser;
#[derive(clap::Parser)]
struct W {
#[command(flatten)]
a: crate::commands::dev::proxy::ProxyArgs,
}
W::parse_from(["ts"]).a
}

fn parse_args(argv: &[&str]) -> crate::commands::dev::proxy::ProxyArgs {
use clap::Parser;
#[derive(clap::Parser)]
struct W {
#[command(flatten)]
a: crate::commands::dev::proxy::ProxyArgs,
}
W::parse_from(argv).a
W::try_parse_from(argv).expect("should parse proxy args").a
}

#[test]
fn clap_parses_rewrite_host_as_a_bool() {
assert!(
!parse_args(&["ts"]).rewrite_host,
!parse_args(&["ts", "--from", "a.example.com", "--to", "b.example.com"]).rewrite_host,
"absent --rewrite-host is false"
);
assert!(
Expand All @@ -391,11 +380,25 @@ mod tests {
);
}

#[test]
fn clap_applies_the_real_listen_default() {
let args = parse_args(&["ts", "--rewrite-host"]);
assert_eq!(
args.listen,
crate::commands::dev::proxy::DEFAULT_LISTEN,
Comment on lines 380 to +388

@aram356 aram356 Sep 24, 2026

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.

📝 note — ffd5bf8ff's commit message says it "fixes two invalid --rewrite-host true/false assertions (that flag takes no value)", but I can't find anything in the diff that corresponds to that.

I checked every commit on the branch plus the whole tree at head for --rewrite-host followed by a value, and there are no matches anywhere — not in the base, not in any intermediate commit, not now. What actually changed in this test is the absent case: !base_args().rewrite_host became !parse_args(&["ts", "--from", ..., "--to", ...]).rewrite_host. Both forms pass --rewrite-host as the bare flag it is.

No code problem here, and the test itself is fine. Flagging it only because it is the same class of drift as the {report:#} finding: a message claiming a change the diff does not contain. Since this message is what lands in main's history, it is worth amending so a future bisector isn't hunting for a fix that was never made.

"should apply the real clap --listen default"
);
}

#[test]
fn single_rule_from_to_keeps_from_host_by_default() {
let mut args = base_args();
args.from = Some("www.example-publisher.com".into());
args.to = Some("to.edgecompute.app".into());
let args = parse_args(&[
"ts",
"--from",
"www.example-publisher.com",
"--to",
"to.edgecompute.app",
]);
let cfg = resolve(&args).expect("should resolve");
let rule = cfg
.rules
Expand All @@ -411,9 +414,12 @@ mod tests {

#[test]
fn rewrite_host_uses_to() {
let mut args = base_args();
args.map = vec!["www.example-publisher.com=to.edgecompute.app".into()];
args.rewrite_host = true;
let args = parse_args(&[
"ts",
"--map",
"www.example-publisher.com=to.edgecompute.app",
"--rewrite-host",
]);
let cfg = resolve(&args).expect("should resolve");
assert_eq!(
rewrite_for(
Expand All @@ -429,10 +435,13 @@ mod tests {

#[test]
fn resolve_pins_host_to_ip() {
let mut args = base_args();
args.map = vec!["www.example-publisher.com=ts.edgecompute.app".into()];
// Mixed case to confirm the host key is lowercased.
args.resolve = vec!["TS.EdgeCompute.app:192.0.2.10".into()];
let args = parse_args(&[
"ts",
"--map",
"www.example-publisher.com=ts.edgecompute.app",
"--resolve",
"TS.EdgeCompute.app:192.0.2.10", // Mixed case to confirm the host key is lowercased.
]);
let cfg = resolve(&args).expect("should resolve");
assert_eq!(
cfg.resolve.get("ts.edgecompute.app"),
Expand All @@ -443,10 +452,13 @@ mod tests {

#[test]
fn resolve_accepts_ipv6_target() {
let mut args = base_args();
args.map = vec!["a.example.com=b.edgecompute.app".into()];
// Split-on-first-colon must keep the colon-bearing IPv6 address intact.
args.resolve = vec!["b.edgecompute.app:::1".into()];
let args = parse_args(&[
"ts",
"--map",
"a.example.com=b.edgecompute.app",
"--resolve",
"b.edgecompute.app:::1", // Split-on-first-colon must keep the colon-bearing IPv6 address intact.
]);
let cfg = resolve(&args).expect("should resolve");
assert_eq!(
cfg.resolve.get("b.edgecompute.app"),
Expand All @@ -457,11 +469,15 @@ mod tests {

#[test]
fn resolve_host_not_matching_any_rule_warns_but_succeeds() {
let mut args = base_args();
args.map = vec!["a.example.com=b.edgecompute.app".into()];
// A pin for a host that is no rule's TO is a likely typo: it should warn
// (not error) and still be recorded.
args.resolve = vec!["typo.edgecompute.app:192.0.2.10".into()];
let args = parse_args(&[
"ts",
"--map",
"a.example.com=b.edgecompute.app",
// A pin for a host that is no rule's TO is a likely typo: it should warn
// (not error) and still be recorded.
"--resolve",
"typo.edgecompute.app:192.0.2.10",
]);
let cfg = resolve(&args).expect("an unmatched --resolve host should warn, not error");
assert!(
cfg.resolve.contains_key("typo.edgecompute.app"),
Expand All @@ -471,9 +487,13 @@ mod tests {

#[test]
fn resolve_rejects_malformed_value() {
let mut args = base_args();
args.map = vec!["a.example.com=b.edgecompute.app".into()];
args.resolve = vec!["b.edgecompute.app:not-an-ip".into()];
let args = parse_args(&[
"ts",
"--map",
"a.example.com=b.edgecompute.app",
"--resolve",
"b.edgecompute.app:not-an-ip",
]);
let err = resolve(&args).expect_err("a non-IP --resolve target should error");
assert!(
matches!(err.current_context(), ConfigError::Resolve { .. }),
Expand All @@ -483,20 +503,24 @@ mod tests {

#[test]
fn map_value_must_be_from_equals_to() {
let mut args = base_args();
args.map = vec!["not-a-map".into()];
let args = parse_args(&["ts", "--map", "not-a-map"]);
assert!(resolve(&args).is_err(), "malformed --map errors");
}

#[test]
fn basic_auth_on_non_loopback_listen_is_rejected() {
// Injected Basic auth on a non-loopback bind would expose the upstream
// credentials to any reachable network client.
let mut args = base_args();
args.map = vec!["a.example.com=b.edgecompute.app".into()];
args.listen = "0.0.0.0:18080".into();
args.allow_non_loopback = true;
args.basic_auth = Some("dev:secret".into());
let args = parse_args(&[
"ts",
"--map",
"a.example.com=b.edgecompute.app",
"--listen",
"0.0.0.0:18080",
"--allow-non-loopback",
"--basic-auth",
"dev:secret",
]);
let err =
resolve(&args).expect_err("non-loopback listen with --basic-auth should be rejected");
assert!(
Expand All @@ -508,7 +532,14 @@ mod tests {
);

// The same non-loopback bind without credentials is allowed.
args.basic_auth = None;
let args = parse_args(&[
"ts",
"--map",
"a.example.com=b.edgecompute.app",
"--listen",
"0.0.0.0:18080",
"--allow-non-loopback",
]);
assert!(
resolve(&args).is_ok(),
"non-loopback without --basic-auth is allowed"
Expand All @@ -518,8 +549,7 @@ mod tests {
#[test]
fn invalid_from_host_is_rejected() {
// A FROM with characters that would break the PAC JS / Host header.
let mut args = base_args();
args.map = vec!["bad\"host=to.edgecompute.app".into()];
let args = parse_args(&["ts", "--map", "bad\"host=to.edgecompute.app"]);
let err = resolve(&args).expect_err("a malformed FROM host should error");
assert!(
matches!(err.current_context(), ConfigError::InvalidFrom { .. }),
Expand All @@ -529,14 +559,25 @@ mod tests {

#[test]
fn non_loopback_listen_requires_flag() {
let mut args = base_args();
args.map = vec!["a.example.com=b.edgecompute.app".into()];
args.listen = "0.0.0.0:18080".into();
let args = parse_args(&[
"ts",
"--map",
"a.example.com=b.edgecompute.app",
"--listen",
"0.0.0.0:18080",
]);
assert!(
resolve(&args).is_err(),
"non-loopback without flag is rejected"
);
args.allow_non_loopback = true;
let args = parse_args(&[
"ts",
"--map",
"a.example.com=b.edgecompute.app",
"--listen",
"0.0.0.0:18080",
"--allow-non-loopback",
]);
assert!(resolve(&args).is_ok(), "non-loopback allowed with flag");
}

Expand All @@ -559,11 +600,15 @@ mod tests {

#[test]
fn resolve_precomputes_typed_rule_identity_and_headers() {
let mut args = base_args();
args.map = vec!["www.example.com=TO.Example.com:8443".into()];
args.rewrite_host = true;
args.insecure = true;
args.resolve = vec!["to.example.com:192.0.2.10".into()];
let args = parse_args(&[
"ts",
"--map",
"www.example.com=TO.Example.com:8443",
"--rewrite-host",
"--insecure",
"--resolve",
"to.example.com:192.0.2.10",
]);

let cfg = resolve(&args).expect("should resolve");
let rule = cfg
Expand Down Expand Up @@ -602,10 +647,7 @@ mod tests {

#[test]
fn resolve_keeps_ip_reference_identities_http1_only() {
let mut args = base_args();
args.map = vec!["www.example.com=127.0.0.1".into()];
args.rewrite_host = true;

let args = parse_args(&["ts", "--map", "www.example.com=127.0.0.1", "--rewrite-host"]);
let cfg = resolve(&args).expect("should resolve");
let rule = cfg
.rules
Expand Down Expand Up @@ -655,9 +697,13 @@ mod tests {
let dir = tempfile::tempdir().expect("should create temp dir");
let missing = dir.path().join("no-such-file.txt");

let mut args = base_args();
args.map = vec!["a.example.com=b.edgecompute.app".into()];
args.basic_auth_file = Some(missing.to_string_lossy().into_owned());
let args = parse_args(&[
"ts",
"--map",
"a.example.com=b.edgecompute.app",
"--basic-auth-file",
&missing.to_string_lossy(),
]);

let err = resolve(&args).expect_err("should fail when file is missing");
assert!(
Expand All @@ -666,9 +712,19 @@ mod tests {
);
}

#[test]
#[should_panic(expected = "DisplayHelpOnMissingArgumentOrSubcommand")]
fn bare_invocation_is_rejected_at_parse_time() {
// `arg_required_else_help` makes a fully-bare `ts` fail to parse at all,
// before `resolve` (and its `NoRule` check) ever runs.
parse_args(&["ts"]);
}
Comment on lines +715 to +721

@aram356 aram356 Sep 24, 2026

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.

🔧 wrench — This assertion is pinned to clap's private internal representation, not its public API.

parse_args surfaces the failure through expect, so the panic payload is the Debug rendering of clap::Error. I dumped the actual text to be sure rather than reasoning about it, and the string this test matches lives inside clap's non-public ErrorInner struct:

panic message: "should parse proxy args: ErrorInner { kind: DisplayHelpOnMissingArgumentOrSubcommand, context: FlatMap { keys: [], values: [] }, message: Some(Formatted(StyledStr(...

Two consequences:

  1. ErrorInner and its Debug shape are not part of clap's public API. A clap patch release that renames the field, reorders it, or changes the derive would make this test pass or fail for reasons unrelated to ts. The pinned version is 4.6.1 today, but nothing here fails loudly if that representation drifts.
  2. #[should_panic] cannot tell where the panic came from. Any future panic in this test body that happens to contain the expected substring satisfies it.

Credit where due: the substring is specific enough to discriminate the error kind — I verified that mutating the call to parse_args(&["ts", "--no-such-flag"]) fails the test with kind: UnknownArgument, so it is not vacuous. The problem is the coupling, not the strength.

This PR already does the same assertion the right way, with clap's public typed API, in run.rs:682-690:

let error = Args::try_parse_from(["ts", "dev", "proxy"])
    .expect_err("a bare `ts dev proxy` should short-circuit to help, not run");
assert_eq!(
    error.kind(),
    clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand,
    "should print help instead of touching system proxy state or attempting sudo"
);

Applying that pattern here needs a fallible sibling to parse_args, since the current helper swallows the typed error:

fn parse_args(argv: &[&str]) -> crate::commands::dev::proxy::ProxyArgs {
    try_parse_args(argv).expect("should parse proxy args")
}

fn try_parse_args(
    argv: &[&str],
) -> Result<crate::commands::dev::proxy::ProxyArgs, clap::Error> {
    #[derive(clap::Parser)]
    struct W {
        #[command(flatten)]
        a: crate::commands::dev::proxy::ProxyArgs,
    }
    W::try_parse_from(argv).map(|w| w.a)
}

#[test]
fn bare_invocation_is_rejected_at_parse_time() {
    // `arg_required_else_help` makes a fully-bare `ts` fail to parse at all,
    // before `resolve` (and its `NoRule` check) ever runs.
    let error = try_parse_args(&["ts"])
        .expect_err("a fully-bare invocation should short-circuit to help");
    assert_eq!(
        error.kind(),
        clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand,
        "should short-circuit to help rather than reaching resolve"
    );
}

Note try_parse_args returns the inner ProxyArgs rather than the wrapper — returning W makes expect_err require W: Debug, which the local wrapper does not derive.

I verified this in a scratch worktree: cargo fmt --all -- --check clean, cargo clippy-cli clean, full CLI suite passing, and no post-verification drift. It also retains equivalent mutation coverage — deleting #[command(arg_required_else_help = true)] from ProxyArgs fails both this test and the run.rs one, same as the current should_panic version.

Apply manually — the change spans two hunks in this file (the parse_args helper and this test), so it cannot be expressed as a single suggestion.


#[test]
fn no_rule_passed_is_a_no_rule_error() {
let args = base_args();
// An invocation with some other flag but no rule still reaches
// `resolve`: `arg_required_else_help` only rejects a fully-bare `ts`.
let args = parse_args(&["ts", "--insecure"]);
let err = resolve(&args).expect_err("should error when no rule is passed");
assert!(
matches!(err.current_context(), ConfigError::NoRule),
Expand Down
7 changes: 6 additions & 1 deletion crates/trusted-server-cli/src/commands/dev/proxy/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,13 @@ async fn finish_interrupted_run<Restore, Stop, Drain>(
let _ = tokio::time::timeout(std::time::Duration::from_secs(2), drain_manager).await;
}

/// Default `--listen` address, shared with the `config` tests so they cannot
/// silently drift from the real default.
Comment thread
dhruv8sh marked this conversation as resolved.
pub(crate) const DEFAULT_LISTEN: &str = "127.0.0.1:18080";

/// `ts dev proxy [OPTIONS]` — see the design spec §4.
#[derive(Debug, clap::Args)]
#[command(arg_required_else_help = true)]
pub struct ProxyArgs {
/// Rewrite rule `FROM=TO` (repeatable).
#[arg(long = "map", value_name = "FROM=TO")]
Expand All @@ -94,7 +99,7 @@ pub struct ProxyArgs {
pub to: Option<String>,

/// Proxy listen address. Non-loopback requires `--allow-non-loopback`.
#[arg(long, value_name = "ADDR", default_value = "127.0.0.1:18080")]
#[arg(long, value_name = "ADDR", default_value = DEFAULT_LISTEN)]
pub listen: String,

/// Permit binding a non-loopback `--listen` (disables blind tunnel/forward).
Expand Down
28 changes: 28 additions & 0 deletions crates/trusted-server-cli/src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,4 +1073,32 @@ mod tests {
"error should explain unsupported option"
);
}

#[test]
#[cfg(target_os = "macos")]
fn dev_proxy_bare_invocation_shows_help_before_running() {
let error = Args::try_parse_from(["ts", "dev", "proxy"])
.expect_err("a bare `ts dev proxy` should short-circuit to help, not run");
assert_eq!(
error.kind(),
clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand,
"should print help instead of touching system proxy state or attempting sudo"
);
}

#[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]);
}
}

#[test]
#[cfg(target_os = "macos")]
fn dev_proxy_partial_rule_parses_instead_of_showing_help() {
// An explicit but incomplete rule (`--from` with no `--to`) must reach
// `run` and surface the concise no-rule error there, not clap help.
parse(&["ts", "dev", "proxy", "--from", "a.example.com"]);
}
}
2 changes: 1 addition & 1 deletion crates/trusted-server-cli/tests/support/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ fn resolve(argv: &[&str]) -> config::ResolvedConfig {
#[command(flatten)]
args: trusted_server_cli::commands::dev::proxy::ProxyArgs,
}
let parsed = Wrapper::parse_from(argv);
let parsed = Wrapper::try_parse_from(argv).expect("should parse proxy args");
config::resolve(&parsed.args).expect("should resolve test config")
}

Comment thread
dhruv8sh marked this conversation as resolved.
Expand Down
3 changes: 2 additions & 1 deletion docs/guide/ts-dev-proxy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ shorthand, or one or more `--map FROM=TO` rules:
ts dev proxy -f www.example-publisher.com -t trusted-server-example.edgecompute.app
```

With no `--map`/`-f`/`-t`, the proxy exits with
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)`.
Comment on lines +80 to 82

@aram356 aram356 Sep 24, 2026

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.

🤔 thinking — Correcting this paragraph was the right call, and the bare-invocation half is now accurate.

The second half is where I'd push slightly. As shipped, an invocation with explicit options but no complete rule does more than "report no rewrite rule: ..." — it prompts for sudo (twice, on a machine with a leftover safari-proxy-restore file), emits a Safari restore warning, and prints the message wrapped in an error-stack debug report with two file:line attachments. The concise one-line message the guide quotes is what users get only once the {report:#} finding is addressed.

So this text describes the intended end state rather than current behaviour. If the two blocking findings land in this PR, it becomes accurate exactly as written and needs no edit. If either is deferred, I'd soften it here so the guide doesn't promise output the binary doesn't produce yet.

No change requested on its own — it just resolves differently depending on how you decide the blocking findings.


Connection options — `--rewrite-host`, `--basic-auth`/`--basic-auth-file`,
Expand Down
Loading