amadan
fleets: tag-box — omitted -tags is a usage error, not a silent clear
Review finding: -tags defaults to "", and "" is the legal, meaningful
"clear every tag" value, so a forgotten -tags on `carlos fleets tag-box
<fleet> <label>` was silently wiping the box's tags and printing "cleared"
as if that were asked for. Same tri-state hazard runAppsPlace's -target
already documents and guards (apps.go) — applied the identical fs.Visit
shape: an omitted -tags is now a usage error (nonzero exit), not read as
"clear".
Added the pinned regression case ("fleets tag-box omitted -tags", fleet
and label present, only -tags missing) to the same subprocess usage-error
table apps place's "omitted target" case lives in
(TestFleetsAndPlaceCLIUsageErrors) — confirmed it fails without the fix
(falls through to mode resolution instead of refusing) and passes with it.
go build ./... && go vet ./... clean; go test ./... -count=1 green across
all 21 packages; gofmt -l clean on touched files.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+30
−6
fleets.go+23 −6fleets_cli_test.go+7 −0
diff --git a/fleets.go b/fleets.go| index add35edaa..8dffa5b06 100644 |
| --- a/fleets.go |
| +++ b/fleets.go |
| @@ -261,19 +261,36 @@ func printFleetToken(fleet, label, token string, rotated bool) { |
| // box's tags, checked client-side with registry.ValidBoxTags BEFORE the API |
| // call — the same "a shape the server is certain to refuse should cost |
| // nothing" principle validFleetName/validBoxLabel already state above. |
| -// -tags has no distinct "absent" state from -tags "" (both are the flag's |
| -// zero value); that ambiguity is fine here because "" is itself a valid, |
| -// meaningful value (clear every tag), not an error condition. |
| +// |
| +// -tags is TRI-state, the exact hazard runAppsPlace's -target comment |
| +// documents (apps.go): "" is a LEGAL, meaningful value here — it clears |
| +// every tag — so a flag left unmentioned and one explicitly set to "" are |
| +// not the same request, and the flag's own zero value cannot tell them |
| +// apart. Without a positive check, a forgotten `-tags` would silently wipe |
| +// a box's tags and print "cleared" as if that were asked for. fs.Visit |
| +// reports only flags actually present on the command line, so — exactly |
| +// like runAppsPlace's targetSet — that is what decides "not named" from |
| +// "named, empty", not the value in *tags. |
| func runFleetsTagBox(args []string) { |
| fs := flag.NewFlagSet("fleets tag-box", flag.ExitOnError) |
| acctRaw := fs.String("account", "ops", "account (sqid or name) the fleet belongs to") |
| acctIDRaw := fs.String("account-id", "", "account sqid, matched as a sqid ONLY — supersedes -account") |
| consoleFlag := fs.String("console", "", "console to tag through (default: the only logged-in one)") |
| - tags := fs.String("tags", "", `comma-separated tags, full replace ("" clears every tag)`) |
| + tags := fs.String("tags", "", `comma-separated tags, full replace ("" clears every tag) — required`) |
| fs.Parse(args) |
| + const tagBoxUsage = "usage: carlos fleets tag-box -tags <tags> [-account <sqid|name>] [-account-id <sqid>] " + |
| + "[-console <origin>] <fleet> <label>" |
| if fs.NArg() != 2 { |
| - log.Fatal("usage: carlos fleets tag-box -tags <tags> [-account <sqid|name>] [-account-id <sqid>] " + |
| - "[-console <origin>] <fleet> <label>") |
| + log.Fatal(tagBoxUsage) |
| + } |
| + tagsSet := false |
| + fs.Visit(func(f *flag.Flag) { |
| + if f.Name == "tags" { |
| + tagsSet = true |
| + } |
| + }) |
| + if !tagsSet { |
| + log.Fatal(tagBoxUsage) |
| } |
| fleet, label := strings.TrimSpace(fs.Arg(0)), strings.TrimSpace(fs.Arg(1)) |
| if err := validFleetName(fleet); err != nil { |
diff --git a/fleets_cli_test.go b/fleets_cli_test.go| index 970fd412f..167ef85c2 100644 |
| --- a/fleets_cli_test.go |
| +++ b/fleets_cli_test.go |
| @@ -682,6 +682,13 @@ func TestFleetsAndPlaceCLIUsageErrors(t *testing.T) { |
| {"fleets tag-box no args", []string{"fleets", "tag-box"}, "usage: carlos fleets tag-box"}, |
| {"fleets tag-box bad tags grammar", []string{"fleets", "tag-box", "-tags", "Office", "acme", "pi-1"}, "invalid tags"}, |
| {"fleets tag-box bad fleet name", []string{"fleets", "tag-box", "-tags", "office", "Bad Name", "pi-1"}, "not a usable fleet name"}, |
| + // Review finding: -tags is TRI-state, the exact -target hazard the |
| + // "apps place omitted target" case below pins. -tags defaults to "" |
| + // and "" is the legal, meaningful "clear every tag" value, so an |
| + // omitted -tags must NEVER be read as "clear" — that would silently |
| + // destroy a box's tags on a forgotten flag. fleet/label ARE present |
| + // here (only -tags is missing) so this fails on the flag, not the args. |
| + {"fleets tag-box omitted -tags", []string{"fleets", "tag-box", "acme", "pi-1"}, "usage: carlos fleets tag-box"}, |
| {"fleets detach-box no args", []string{"fleets", "detach-box"}, "usage: carlos fleets detach-box"}, |
| {"apps place missing app", []string{"apps", "place", "-target", "fleet/pi"}, "usage: carlos apps place"}, |
| {"apps place bad target shape", []string{"apps", "place", "-app", "titogo", "-target", "eu"}, "not a usable placement target"}, |