fix(set): take Go's --sync=false spelling instead of dying at the parser - #399
Merged
GeiserX merged 1 commit intoSep 7, 2026
Merged
Conversation
…arser
`tnet set` carries Go's `--remote-config` and `--sync` so that a command
line ported from Go reaches a refusal that names what this build cannot
do, instead of clap's "unexpected argument". That only worked for the
flag NAME. Both were declared as valueless clap flags, so Go's own way of
turning them off went nowhere:
$ tnet set --sync=false
error: unexpected value 'false' for '--sync' found; no more were expected
exit 2
$ tnet set --remote-config=false
error: unexpected value 'false' for '--remote-config' found; no more were expected
exit 2
Upstream registers both with `flag.BoolVar` (`cmd/tailscale/cli/set.go`
@ 53a0d659afa51835dd7a9283873cca44261454f8, `newSetFlagSet`), so
`--sync=false` and `--remote-config=false` are exactly how a Go command
line spells "off" — and `--sync=false` is the one value of that flag this
build has anything to say about. A ported line hit the same wall the
flags were added to remove.
WHAT CHANGED
`--remote-config` and `--sync` now take Go's optional attached value:
`num_args = 0..=1` + `require_equals` + `default_missing_value = "true"`.
That is Go's bool-flag grammar precisely — bare `--sync` is the flag's
presence, and a value can only arrive as `--sync=<v>` because Go's
`IsBoolFlag` never lets a bool flag consume the following argument. The
value spellings are `strconv.ParseBool`'s, via a new `parse_go_bool`, so
`--sync=0` and `--sync=T` work as they do in Go and a value Go rejects is
a parse error naming the flag rather than a silent "off":
$ tnet set --sync=nope
error: invalid value 'nope' for '--sync[=<BOOL>]': strconv.ParseBool: parsing "nope": invalid syntax
`--no-sync` / `--no-remote-config` stay, unchanged: they are this fork's
existing spelling, the README names them, and the refusal messages quote
them. A new `resolve_go_bool_tristate` folds the two spellings of one
switch into the single `Option<bool>` the gate already reads, so both
land on the same sentence. clap still refuses the pair together, in
either order. After the change:
$ tnet set --sync=false
Error: --no-sync (Go `--sync=false`) is not supported by this build: ... engine ask #34 ...
exit 1
$ tnet set --remote-config=false # asks for the status quo — accepted, goes to the daemon
No behaviour moved for any command line that already parsed, and no
refusal text changed.
WHAT TO READ FIRST
`src/bin/tnet.rs`: the two `#[arg(...)]` blocks on `Command::Set`, then
`parse_go_bool` and `resolve_go_bool_tristate` next to
`check_unmodelled_set_flags`. The test is
`gos_own_sync_and_remote_config_spellings_reach_the_ported_refusal`; it
calls the real clap parser and the real gate, and pins that Go's off
spelling produces the byte-identical refusal `--no-sync` produces. It
cannot pass against the previous declarations — `--sync=false` did not
parse at all.
VERIFICATION
cargo fmt --all --check clean
cargo clippy --all-targets -- -D warnings clean
cargo test --all-targets all suites ok, 0 failed
cargo build --release --bins ok
The before/after transcripts above are from `./target/debug/tnet`, built
at the parent commit and then with this change.
STALE ELSEWHERE
`docs/PARITY_GAP_ANALYSIS.md` (the `#34` row) and the README's "Four of
Go's `set` flags are parsed but not modelled" paragraph both describe the
refusals only in this fork's `--no-` spelling. Both are still true, but
they now understate the port: Go's own `--sync=false` /
`--remote-config=false` reach those same refusals. Left unedited on
purpose, to keep the ledger free of collisions.
DECISIONS
- Kept the refusal wording as it is, including its `--no-sync (Go
`--sync=false`)` opening. It already names both spellings, and a
reworded message would churn text the README and the existing tests
are written against for no gain in what it tells the operator.
- Used `require_equals` rather than letting `--sync false` bind. Go's
flag package never passes the following argument to a bool flag, so
binding it would accept a line Go reads differently.
- `parse_go_bool` is a second, local copy: `tailnetd` has the same
function for `cmd/tailscaled`'s `boolFlag`, but the two binaries share
no flag plumbing and hoisting it into the library would touch a binary
this change has no business in.
NOTED, NOT FIXED
- The other `set` boolean pairs (`--accept-routes`/`--no-accept-routes`,
`--ssh`, `--webclient`, …) are `flag.BoolVar` upstream too, so
`--accept-routes=false` still dies at clap the same way. Same class of
divergence, wider blast radius, and none of those has a by-name
refusal riding on it — worth its own change.
- `tnet set` has no catch-all for leftover non-flag arguments, so
`tnet set foo` is clap's "unexpected argument" (exit 2) rather than
Go's own message from `runSet` (exit 1). Also separate.
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GeiserX
deleted the
factory/tsd-aud310-c583-r1-s1n0/set-go-bool-spellings
branch
September 7, 2026 13:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tnet setcarries Go's--remote-configand--syncso that a commandline ported from Go reaches a refusal that names what this build cannot
do, instead of clap's "unexpected argument". That only worked for the
flag NAME. Both were declared as valueless clap flags, so Go's own way of
turning them off went nowhere:
Upstream registers both with
flag.BoolVar(cmd/tailscale/cli/set.go@ 53a0d659afa51835dd7a9283873cca44261454f8,
newSetFlagSet), so--sync=falseand--remote-config=falseare exactly how a Go commandline spells "off" — and
--sync=falseis the one value of that flag thisbuild has anything to say about. A ported line hit the same wall the
flags were added to remove.
WHAT CHANGED
--remote-configand--syncnow take Go's optional attached value:num_args = 0..=1+require_equals+default_missing_value = "true".That is Go's bool-flag grammar precisely — bare
--syncis the flag'spresence, and a value can only arrive as
--sync=<v>because Go'sIsBoolFlagnever lets a bool flag consume the following argument. Thevalue spellings are
strconv.ParseBool's, via a newparse_go_bool, so--sync=0and--sync=Twork as they do in Go and a value Go rejects isa parse error naming the flag rather than a silent "off":
--no-sync/--no-remote-configstay, unchanged: they are this fork'sexisting spelling, the README names them, and the refusal messages quote
them. A new
resolve_go_bool_tristatefolds the two spellings of oneswitch into the single
Option<bool>the gate already reads, so bothland on the same sentence. clap still refuses the pair together, in
either order. After the change:
No behaviour moved for any command line that already parsed, and no
refusal text changed.
WHAT TO READ FIRST
src/bin/tnet.rs: the two#[arg(...)]blocks onCommand::Set, thenparse_go_boolandresolve_go_bool_tristatenext tocheck_unmodelled_set_flags. The test isgos_own_sync_and_remote_config_spellings_reach_the_ported_refusal; itcalls the real clap parser and the real gate, and pins that Go's off
spelling produces the byte-identical refusal
--no-syncproduces. Itcannot pass against the previous declarations —
--sync=falsedid notparse at all.
VERIFICATION
The before/after transcripts above are from
./target/debug/tnet, builtat the parent commit and then with this change.
STALE ELSEWHERE
docs/PARITY_GAP_ANALYSIS.md(the#34row) and the README's "Four ofGo's
setflags are parsed but not modelled" paragraph both describe therefusals only in this fork's
--no-spelling. Both are still true, butthey now understate the port: Go's own
--sync=false/--remote-config=falsereach those same refusals. Left unedited onpurpose, to keep the ledger free of collisions.
DECISIONS
--no-sync (Go--sync=false)opening. It already names both spellings, and areworded message would churn text the README and the existing tests
are written against for no gain in what it tells the operator.
require_equalsrather than letting--sync falsebind. Go'sflag package never passes the following argument to a bool flag, so
binding it would accept a line Go reads differently.
parse_go_boolis a second, local copy:tailnetdhas the samefunction for
cmd/tailscaled'sboolFlag, but the two binaries shareno flag plumbing and hoisting it into the library would touch a binary
this change has no business in.
NOTED, NOT FIXED
setboolean pairs (--accept-routes/--no-accept-routes,--ssh,--webclient, …) areflag.BoolVarupstream too, so--accept-routes=falsestill dies at clap the same way. Same class ofdivergence, wider blast radius, and none of those has a by-name
refusal riding on it — worth its own change.
tnet sethas no catch-all for leftover non-flag arguments, sotnet set foois clap's "unexpected argument" (exit 2) rather thanGo's own message from
runSet(exit 1). Also separate.bead
tsd-aud310· sessiontsd-aud310-c583-r1-s1n0· baserefs/heads/main