a73x

98d2e097

refactor: mux parses the attach line with cliflags

a73x   2026-08-27 06:21

Commit message
refactor: mux parses the attach line with cliflags

The hand-rolled loop becomes an Opts struct: the field's type is the
flag's arity, its name is the spelling, and what a flag MEANS stays in the
post-checks. `-A` is an alias of `agent`; HOST and quic:// arrive through
the positional hook, which is where the two-of-either conflict is now
seen. `--help` becomes a result of its own, written to stdout and exiting
0 like muxd's.

One deliberate behaviour change: `--sock /a --sock /b` and `--via a --via
b` were conflicts and are now last-wins, as everywhere else a flag is
repeated. Two bare hosts and two quic:// targets stay conflicts — those
carry no flag to correct.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
src/cli/mux_main.zig
Old New
@@ -20,6 +20,7 @@ const handoff = @import("handoff");
20 const sockpath = @import("sockpath"); 20 const sockpath = @import("sockpath");
21 const wallview = @import("wallview"); 21 const wallview = @import("wallview");
22 const wall = @import("wall"); 22 const wall = @import("wall");
23 const cliflags = @import("cliflags");
23 const TmpDir = @import("testtmp").TmpDir; 24 const TmpDir = @import("testtmp").TmpDir;
24 25
25 const usage = 26 const usage =
@@ -35,7 +36,7 @@ const usage =
35 \\ the default (`0`); NAME is printable ASCII, no space, no '#' or '/' 36 \\ the default (`0`); NAME is printable ASCII, no space, no '#' or '/'
36 \\ -A forwards this client's ssh-agent into the session, like ssh -A: 37 \\ -A forwards this client's ssh-agent into the session, like ssh -A:
37 \\ whoever typed last is whose agent signs, and only while attached 38 \\ whoever typed last is whose agent signs, and only while attached
38 \\ --version prints the version 39 \\ --version prints the version, --help prints this
39 \\ 40 \\
40 \\ mux wall [SPELLING...] shows several sessions at once, one stripe 41 \\ mux wall [SPELLING...] shows several sessions at once, one stripe
41 \\ each; `Ctrl-\ 1-9` focuses a tile and types into it, `Ctrl-\ h/j/k/l` 42 \\ each; `Ctrl-\ 1-9` focuses a tile and types into it, `Ctrl-\ h/j/k/l`
@@ -55,8 +56,9 @@ const usage =
55 ; 56 ;
56 57
57 /// What the command line asked for. A tagged union rather than a struct of 58 /// What the command line asked for. A tagged union rather than a struct of
58 /// optionals so the two failure modes are results in their own right, and so 59 /// optionals so every answer that is not a transport — the refusals, and the
59 /// the parse can be tested without a process to exit from. 60 /// two questions about the binary itself — is a result in its own right, and
61 /// so the parse can be tested without a process to exit from.
60 const ParseResult = union(enum) { 62 const ParseResult = union(enum) {
61 /// At most one of these is set; both null means the default local socket. 63 /// At most one of these is set; both null means the default local socket.
62 /// `session` defaults to "" (empty), the wire-compatible name that puts 64 /// `session` defaults to "" (empty), the wire-compatible name that puts
@@ -75,6 +77,9 @@ const ParseResult = union(enum) {
75 /// `--version`: not a transport at all, so it short-circuits the rest of 77 /// `--version`: not a transport at all, so it short-circuits the rest of
76 /// the parse rather than being reconciled with it. 78 /// the parse rather than being reconciled with it.
77 version, 79 version,
80 /// `--help`: the usage someone ASKED for, so main writes it to stdout
81 /// and exits 0, unlike every refusal below.
82 help,
78 /// More than one transport named — a request that cannot be honoured 83 /// More than one transport named — a request that cannot be honoured
79 /// rather than one to reconcile. 84 /// rather than one to reconcile.
80 conflict, 85 conflict,
@@ -171,93 +176,92 @@ fn insideThisSession(
171 std.mem.eql(u8, en, proto.resolveName(session)); 176 std.mem.eql(u8, en, proto.resolveName(session));
172 } 177 }
173 178
174 fn parseArgs(args: []const [:0]const u8, env_key: ?[]const u8) ParseResult { 179 /// The attach line, read off the struct: the field's type is the flag's
175 var sock: ?[]const u8 = null; 180 /// arity and its name is the flag's spelling. What a flag MEANS stays here,
176 var via: ?[]const u8 = null; 181 /// in the post-checks below.
177 var host: ?[]const u8 = null; 182 const Opts = struct {
178 var quic: ?[]const u8 = null; 183 sock: ?[]const u8 = null,
179 var key: ?[]const u8 = null; 184 via: ?[]const u8 = null,
180 var idle_ms: u32 = client.quic_idle_ms_default; 185 key: ?[]const u8 = null,
181 // Rides every transport below, unlike --key: a session name is not 186 /// Optional so that only a name that was TYPED is validated: `""` is the
182 // authenticating anything, so there is no "no quic:// means ignore it" 187 /// wire's own default spelling and would fail a check written for a name.
183 // escape hatch — it applies whichever spelling wins. 188 session: ?[]const u8 = null,
184 var session: []const u8 = ""; 189 quic_idle_ms: u32 = client.quic_idle_ms_default,
185 // Rides every transport too, and for the same reason: an offer to 190 agent: bool = false,
186 // answer for this client's agent is about the client, not the wire it 191 /// The three below are not flags, and the leading underscore is what
187 // reached the daemon over. 192 /// says so: they are what `positional` saw.
188 var agent = false; 193 _host: ?[]const u8 = null,
189 194 _quic: ?[]const u8 = null,
190 var i: usize = 1; 195 _conflict: bool = false,
191 while (i < args.len) : (i += 1) { 196
192 const a = args[i]; 197 pub const aliases = .{.{ "-A", "agent" }};
193 // First branch, and it returns rather than recording: asking a binary 198
194 // its version must answer whatever else is on the line, including a 199 /// A bare word is a host to hop to, `quic://...` a transport spelling.
195 // transport that would otherwise conflict or fail to parse. 200 /// A second of either is as ambiguous as naming two transports, so it
196 if (std.mem.eql(u8, a, "--version")) { 201 /// lands in the same refusal; `quic://` with nothing after it names no
197 return .version; 202 /// host at all and is refused as the usage mistake it is.
198 } else if (std.mem.eql(u8, a, "--sock") and i + 1 < args.len) { 203 pub fn positional(self: *Opts, word: []const u8) bool {
199 i += 1; 204 if (std.mem.startsWith(u8, word, "quic://")) {
200 if (sock != null) return .conflict; 205 const host_port = word["quic://".len..];
201 sock = args[i]; 206 if (host_port.len == 0) return false;
202 } else if (std.mem.eql(u8, a, "--via") and i + 1 < args.len) { 207 if (self._quic != null) self._conflict = true;
203 i += 1; 208 self._quic = host_port;
204 if (via != null) return .conflict; 209 return true;
205 via = args[i];
206 } else if (std.mem.eql(u8, a, "--key") and i + 1 < args.len) {
207 i += 1;
208 key = args[i];
209 } else if (std.mem.eql(u8, a, "--session") and i + 1 < args.len) {
210 i += 1;
211 // A name that cannot be spelled must not become wire bytes: catch
212 // it here, at usage-error altitude, rather than downstream where
213 // it would look like a rejected attach.
214 if (!proto.validSessionName(args[i])) return .usage_error;
215 session = args[i];
216 } else if (std.mem.eql(u8, a, "--quic-idle-ms") and i + 1 < args.len) {
217 i += 1;
218 const n = std.fmt.parseInt(u32, args[i], 10) catch return .usage_error;
219 // Zero means "no idle timeout" to ngtcp2, the inverse of what
220 // anyone typing a timeout of zero is asking for.
221 if (n == 0) return .usage_error;
222 idle_ms = n;
223 } else if (std.mem.eql(u8, a, "-A")) {
224 // Named explicitly: the bare-word arm below only takes words
225 // that do not start with '-', so an unnamed flag is a usage
226 // error rather than a hostname.
227 agent = true;
228 } else if (std.mem.startsWith(u8, a, "quic://")) {
229 // Counted with the others, so `mux quic://a:1 --sock /x` is the
230 // same conflict as naming any other two transports.
231 if (quic != null) return .conflict;
232 quic = a["quic://".len..];
233 if (quic.?.len == 0) return .usage_error;
234 } else if (a.len > 0 and a[0] != '-') {
235 // A bare word is a host to hop to. Two of them is as ambiguous
236 // as naming two transports, so it lands in the same place.
237 if (host != null) return .conflict;
238 host = a;
239 } else {
240 // Includes `--sock`/`--via` with no value left to take: a flag
241 // whose argument is missing is a usage mistake, not a transport.
242 return .usage_error;
243 } 210 }
211 if (self._host != null) self._conflict = true;
212 self._host = word;
213 return true;
244 } 214 }
215 };
216
217 comptime {
218 cliflags.assertDocumented(Opts, usage, &.{});
219 }
220
221 fn parseArgs(args: []const [:0]const u8, env_key: ?[]const u8) ParseResult {
222 var o: Opts = .{};
223 switch (cliflags.parse(Opts, &o, args[1..])) {
224 .ok => {},
225 .help => return .help,
226 .version => return .version,
227 .unknown_arg, .missing_value, .bad_number => return .usage_error,
228 }
229
230 if (o._conflict) return .conflict;
231
232 // A name that cannot be spelled must not become wire bytes: caught here,
233 // at usage-error altitude, rather than downstream where it would look
234 // like a rejected attach.
235 if (o.session) |name| {
236 if (!proto.validSessionName(name)) return .usage_error;
237 }
238
239 // Zero means "no idle timeout" to ngtcp2, the inverse of what anyone
240 // typing a timeout of zero is asking for.
241 if (o.quic_idle_ms == 0) return .usage_error;
245 242
246 // Every pairing of the four is two transports for one session. 243 // Every pairing of the four is two transports for one session.
247 const named: u8 = @as(u8, @intFromBool(sock != null)) + 244 const named: u8 = @as(u8, @intFromBool(o.sock != null)) +
248 @intFromBool(via != null) + @intFromBool(host != null) + 245 @intFromBool(o.via != null) + @intFromBool(o._host != null) +
249 @intFromBool(quic != null); 246 @intFromBool(o._quic != null);
250 if (named > 1) return .conflict; 247 if (named > 1) return .conflict;
251 248
252 if (quic) |hp| { 249 // Rides every transport below, unlike --key: a session name is not
253 // Neither spelling being set is not a refusal: main has a default 250 // authenticating anything, so there is no "no quic:// means ignore it"
254 // path to try, and parse is not allowed to look at the filesystem. 251 // escape hatch — it applies whichever spelling wins. So does `agent`,
252 // and for the same reason: an offer to answer for this client's agent is
253 // about the client, not the wire it reached the daemon over.
254 const session = o.session orelse "";
255
256 if (o._quic) |host_port| {
257 // Neither spelling of the key being set is not a refusal: main has a
258 // default path to try, and parse cannot look at the filesystem.
255 return .{ .quic = .{ 259 return .{ .quic = .{
256 .host_port = hp, 260 .host_port = host_port,
257 .key = xdg.pickKey(key, env_key), 261 .key = xdg.pickKey(o.key, env_key),
258 .idle_ms = idle_ms, 262 .idle_ms = o.quic_idle_ms,
259 .session = session, 263 .session = session,
260 .agent = agent, 264 .agent = o.agent,
261 } }; 265 } };
262 } 266 }
263 // A key with no quic:// has nothing to authenticate and is ignored 267 // A key with no quic:// has nothing to authenticate and is ignored
@@ -265,8 +269,8 @@ fn parseArgs(args: []const [:0]const u8, env_key: ?[]const u8) ParseResult {
265 // listener was meant, here it is one env var away from being set for 269 // listener was meant, here it is one env var away from being set for
266 // every invocation in a shell, and refusing `mux --sock ...` because 270 // every invocation in a shell, and refusing `mux --sock ...` because
267 // MUX_KEY_FILE happens to be exported would be absurd. 271 // MUX_KEY_FILE happens to be exported would be absurd.
268 if (host) |h| return .{ .host = .{ .name = h, .idle_ms = idle_ms, .session = session, .agent = agent } }; 272 if (o._host) |h| return .{ .host = .{ .name = h, .idle_ms = o.quic_idle_ms, .session = session, .agent = o.agent } };
269 return .{ .attach = .{ .sock = sock, .via = via, .session = session, .agent = agent } }; 273 return .{ .attach = .{ .sock = o.sock, .via = o.via, .session = session, .agent = o.agent } };
270 } 274 }
271 275
272 pub fn main() !u8 { 276 pub fn main() !u8 {
@@ -323,6 +327,12 @@ pub fn main() !u8 {
323 _ = std.posix.write(std.posix.STDOUT_FILENO, s) catch {}; 327 _ = std.posix.write(std.posix.STDOUT_FILENO, s) catch {};
324 return 0; 328 return 0;
325 }, 329 },
330 .help => {
331 // stdout, unlike every refusal below: a usage someone asked for
332 // is output, and they may well have piped it into a pager.
333 _ = std.posix.write(std.posix.STDOUT_FILENO, usage) catch {};
334 return 0;
335 },
326 .usage_error => { 336 .usage_error => {
327 std.debug.print("{s}", .{usage}); 337 std.debug.print("{s}", .{usage});
328 return 2; 338 return 2;
@@ -732,28 +742,47 @@ test "parseArgs: naming two transports is a conflict, however it is spelled" {
732 try std.testing.expect(parse(&.{ "mux", "--sock", "/tmp/x.sock", "vm1" }) == .conflict); 742 try std.testing.expect(parse(&.{ "mux", "--sock", "/tmp/x.sock", "vm1" }) == .conflict);
733 try std.testing.expect(parse(&.{ "mux", "vm1", "--via", "ssh box muxd proxy" }) == .conflict); 743 try std.testing.expect(parse(&.{ "mux", "vm1", "--via", "ssh box muxd proxy" }) == .conflict);
734 try std.testing.expect(parse(&.{ "mux", "--sock", "/a", "--via", "c" }) == .conflict); 744 try std.testing.expect(parse(&.{ "mux", "--sock", "/a", "--via", "c" }) == .conflict);
735 // Two of the same kind is the same ambiguity as two different kinds. 745 // Two of the same kind is the same ambiguity as two different kinds —
746 // for the two spellings that carry no flag. A flag repeated is not
747 // ambiguous, it is corrected: the last value wins, as everywhere else.
736 try std.testing.expect(parse(&.{ "mux", "vm1", "vm2" }) == .conflict); 748 try std.testing.expect(parse(&.{ "mux", "vm1", "vm2" }) == .conflict);
737 try std.testing.expect(parse(&.{ "mux", "--sock", "/a", "--sock", "/b" }) == .conflict); 749 try std.testing.expect(parse(&.{ "mux", "quic://a:1", "quic://b:2" }) == .conflict);
750 const s2 = parse(&.{ "mux", "--sock", "/a", "--sock", "/b" });
751 try std.testing.expectEqualStrings("/b", s2.attach.sock.?);
752 const v2 = parse(&.{ "mux", "--via", "ssh a", "--via", "ssh b" });
753 try std.testing.expectEqualStrings("ssh b", v2.attach.via.?);
738 } 754 }
739 755
740 test "parseArgs: unknown flags and valueless flags are usage errors" { 756 test "parseArgs: unknown flags and valueless flags are usage errors" {
741 try std.testing.expect(parse(&.{ "mux", "--wat" }) == .usage_error); 757 try std.testing.expect(parse(&.{ "mux", "--wat" }) == .usage_error);
742 try std.testing.expect(parse(&.{ "mux", "-x" }) == .usage_error); 758 try std.testing.expect(parse(&.{ "mux", "-x" }) == .usage_error);
743 // A flag whose value is missing must not be mistaken for a bare host. 759 // A flag whose value is missing must not be mistaken for a bare host.
744 // Every value-taking flag has to have a row here: the fall-through that 760 // Every value-taking flag has to have a row here: one outcome answers
745 // catches a missing value is one `else` arm shared by all of them, so a 761 // for all of them, so a flag added without a row is a flag nobody
746 // flag added without a row here is a flag nobody actually checked. 762 // actually checked.
747 inline for (.{ "--sock", "--via", "--key", "--quic-idle-ms", "--session" }) |flag| { 763 inline for (.{ "--sock", "--via", "--key", "--quic-idle-ms", "--session" }) |flag| {
748 try std.testing.expect(parse(&.{ "mux", flag }) == .usage_error); 764 try std.testing.expect(parse(&.{ "mux", flag }) == .usage_error);
749 } 765 }
750 } 766 }
751 767
768 test "parseArgs: --help is the usage someone asked for, wherever it sits" {
769 try std.testing.expect(parse(&.{ "mux", "--help" }) == .help);
770 try std.testing.expect(parse(&.{ "mux", "-h" }) == .help);
771 try std.testing.expect(parse(&.{ "mux", "vm1", "--help" }) == .help);
772 // Even where a value belongs, and beside a line that would otherwise be
773 // refused: asking for the usage is not a way to mistype a flag.
774 try std.testing.expect(parse(&.{ "mux", "--sock", "--help" }) == .help);
775 try std.testing.expect(parse(&.{ "mux", "--wat", "--help" }) == .help);
776 }
777
752 test "-A rides every transport spelling" { 778 test "-A rides every transport spelling" {
753 try std.testing.expect(parse(&.{ "mux", "-A", "somehost" }).host.agent); 779 try std.testing.expect(parse(&.{ "mux", "-A", "somehost" }).host.agent);
754 try std.testing.expect(parse(&.{ "mux", "-A", "--sock", "/tmp/x.sock" }).attach.agent); 780 try std.testing.expect(parse(&.{ "mux", "-A", "--sock", "/tmp/x.sock" }).attach.agent);
755 try std.testing.expect(parse(&.{ "mux", "quic://h:1", "-A" }).quic.agent); 781 try std.testing.expect(parse(&.{ "mux", "quic://h:1", "-A" }).quic.agent);
756 try std.testing.expect(!parse(&.{ "mux", "somehost" }).host.agent); 782 try std.testing.expect(!parse(&.{ "mux", "somehost" }).host.agent);
783 // `-A` is an alias, not a flag of its own, so the field's own spelling
784 // has to work too.
785 try std.testing.expect(parse(&.{ "mux", "--agent", "--sock", "/tmp/x.sock" }).attach.agent);
757 } 786 }
758 787
759 test "parseArgs: quic:// is a transport like any other" { 788 test "parseArgs: quic:// is a transport like any other" {
@@ -830,6 +859,11 @@ test "parseArgs: --quic-idle-ms parses, and refuses what ngtcp2 would invert" {
830 test "parseArgs: --version wins wherever it appears" { 859 test "parseArgs: --version wins wherever it appears" {
831 try std.testing.expect(parse(&.{ "mux", "--version" }) == .version); 860 try std.testing.expect(parse(&.{ "mux", "--version" }) == .version);
832 try std.testing.expect(parse(&.{ "mux", "--sock", "/x", "--version" }) == .version); 861 try std.testing.expect(parse(&.{ "mux", "--sock", "/x", "--version" }) == .version);
862 // Including beside a line that would otherwise be a conflict or a
863 // mistake: asking a binary its version must answer whatever else is on
864 // the line.
865 try std.testing.expect(parse(&.{ "mux", "vm1", "--sock", "/x", "--version" }) == .version);
866 try std.testing.expect(parse(&.{ "mux", "--wat", "--version" }) == .version);
833 } 867 }
834 868
835 test "parseArgs: --session rides every transport spelling" { 869 test "parseArgs: --session rides every transport spelling" {