amadan
services test: C46 — the deliver-line guard trusted unverified -h output
TestServicesDeliverLineIsARealCommand (added for C44) closed only one axis
of the class it was meant to close. It ignored the subprocess exit code
and matched flag names by strings.Contains, so:
- a typo'd verb (e.g. `carlos secret set` instead of `secrets set`)
exits nonzero and falls through to a DIFFERENT usage page — main.go's
top-level usage() for a bad first word, a family's own bare usage
constant for a bad sub-verb — which the test never rejected: it read
whatever came back regardless of exit code.
- the top-level usage page happens to contain "-app" as a substring
inside an unrelated verb's description ("carlos version target -app
<a> 0.5.0"), so a deliver line naming a nonexistent verb passed the
flag search anyway.
- a prefix-preserving flag rename (-app -> -application) would also
silently pass: strings.Contains(helpOut, "-app") matches "-application"
as a substring.
Both are the identical operator experience the original C43 bug produced:
a refusal while holding a secret that can never be shown again — so both
have to fail the guard, not just the flag-name axis.
Fix: two exact-match helpers, helpOutputIsForVerb (checks for the literal
"Usage of <verb>:" header flag.FlagSet's default Usage() prints — a typo'd
verb's fall-through page never contains it) and helpOutputHasFlag (matches
a flag as a whole line-leading token, never a substring).
TestServicesDeliverLineIsARealCommand now asserts the subprocess exited 0
AND that its output is the verb's own usage page BEFORE trusting anything
in it, then matches each flag by name. Two new permanent fixture-based
tests (TestHelpOutputIsForVerbRejectsFallThrough,
TestHelpOutputHasFlagRejectsPrefixPreservingRename) pin both axes without
needing a subprocess.
Both failure modes were also reproduced live against the real binary and
reverted (not part of this commit): a temporary typo in services.go's
deliverLine ("carlos secret set...") made the fixed test FAIL on the
nonzero-exit/fall-through-page check; a temporary rename of secrets set's
-app flag to -application (appconfig_cli.go) made it FAIL on the
exact-flag-name check, where the prior substring check would have passed.
Full gate clean: go build/vet ./..., gofmt -l (touched files), go test
./... (all 21 packages), GOOS=darwin go build ./.... No smart quotes
(U+201C/201D) introduced. Test-only change — no production code touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UimU7fAfeAoBMm6HKCPwQ
1 file changed,
+108
−3
services_cli_test.go+108 −3
diff --git a/services_cli_test.go b/services_cli_test.go| index c27bf4d17..3a51719c4 100644 |
| --- a/services_cli_test.go |
| +++ b/services_cli_test.go |
| @@ -540,6 +540,86 @@ func parseDeliverCommand(t *testing.T, line string) (verb, flags []string) { |
| return verb, flags |
| } |
| +// helpOutputIsForVerb reports whether helpOut is the `-h` output of EXACTLY |
| +// the verb it was asked about, not a fall-through to a different usage page |
| +// (C46, review). Every sub-verb's flag.NewFlagSet in this binary is named |
| +// after the verb itself (e.g. flag.NewFlagSet("secrets set", ...)), and |
| +// Go's flag package always prints "Usage of <name>:" as that FlagSet's own |
| +// -h header — so this line is a reliable anchor for "this really is that |
| +// verb's own usage", which a typo'd verb cannot produce: a typo'd TOP-LEVEL |
| +// word falls through to main.go's usage() (real English text that |
| +// coincidentally contains "-app" in "carlos version target -app <a> 0.5.0" |
| +// — the review's own example), and a typo'd SUB-verb falls through to that |
| +// family's own bare usage constant (secretsUsage etc.), neither of which |
| +// ever prints "Usage of <name>:" for the verb that was actually typed. |
| +func helpOutputIsForVerb(helpOut string, verb []string) bool { |
| + want := "Usage of " + strings.Join(verb, " ") + ":" |
| + for _, line := range strings.Split(helpOut, "\n") { |
| + if strings.TrimSpace(line) == want { |
| + return true |
| + } |
| + } |
| + return false |
| +} |
| + |
| +// helpOutputHasFlag reports whether helpOut's flag.PrintDefaults output |
| +// lists EXACTLY flagName as a registered flag — a NAME match, never a |
| +// substring match (C46, review). Each flag prints as " -name" or " -name |
| +// type" at the START of its own line, so this checks that shape rather than |
| +// searching for flagName anywhere in the text: a strings.Contains check |
| +// would let a search for -app pass against a renamed -application flag, |
| +// since "-application" begins with the plain substring "-app" — the exact |
| +// false positive the review demonstrated. |
| +func helpOutputHasFlag(helpOut, flagName string) bool { |
| + for _, line := range strings.Split(helpOut, "\n") { |
| + trimmed := strings.TrimSpace(line) |
| + if trimmed == flagName || strings.HasPrefix(trimmed, flagName+" ") { |
| + return true |
| + } |
| + } |
| + return false |
| +} |
| + |
| +// TestHelpOutputIsForVerbRejectsFallThrough — C46 (review), a permanent |
| +// fixture-based proof (no subprocess needed) that helpOutputIsForVerb |
| +// rejects the exact fall-through the review demonstrated against the real |
| +// binary: `carlos secret set -h` (typo'd verb) exits nonzero and prints the |
| +// TOP-LEVEL usage page, which itself contains "-app" as a substring inside |
| +// an unrelated verb's description — a page a naive Contains-based flag |
| +// search would have wrongly accepted as if it were secrets set's own -h. |
| +func TestHelpOutputIsForVerbRejectsFallThrough(t *testing.T) { |
| + topLevelFallThrough := "usage: carlos <command> [flags]\n\n" + |
| + " version target the semver ship counts iterations under: print it, or set\n" + |
| + " it -- \"carlos version target -app <a> 0.5.0\"\n" + |
| + " secrets sealed per-app secrets: set | unset | list | genkey\n" |
| + if helpOutputIsForVerb(topLevelFallThrough, []string{"secrets", "set"}) { |
| + t.Fatal("helpOutputIsForVerb matched a fall-through top-level usage page, " + |
| + "which happens to contain \"-app\" as a substring in an unrelated verb's description") |
| + } |
| + real := "Usage of secrets set:\n -app string\n \tapp name\n" |
| + if !helpOutputIsForVerb(real, []string{"secrets", "set"}) { |
| + t.Fatal("helpOutputIsForVerb did not match the real usage page for the verb it names") |
| + } |
| +} |
| + |
| +// TestHelpOutputHasFlagRejectsPrefixPreservingRename — C46 (review), a |
| +// permanent fixture-based proof that helpOutputHasFlag matches flag NAMES, |
| +// not substrings: the prior strings.Contains-based check would have let a |
| +// search for -app pass against a hypothetically renamed -application flag, |
| +// since "-application" begins with the plain substring "-app". |
| +func TestHelpOutputHasFlagRejectsPrefixPreservingRename(t *testing.T) { |
| + renamed := "Usage of secrets set:\n -application string\n \tapp name\n" |
| + if helpOutputHasFlag(renamed, "-app") { |
| + t.Fatal("helpOutputHasFlag(-app) matched -application -- a substring match, not an exact one") |
| + } |
| + if !helpOutputHasFlag(renamed, "-application") { |
| + t.Fatal("helpOutputHasFlag(-application) should match its own exact name") |
| + } |
| + if !helpOutputHasFlag("Usage of secrets set:\n -app string\n \tapp name\n", "-app") { |
| + t.Fatal("helpOutputHasFlag(-app) should match its own exact name") |
| + } |
| +} |
| + |
| // TestServicesDeliverLineIsARealCommand — C44 (review), the systemic fix |
| // for C43: a printed "deliver it" command whose flag did not exist |
| // (`carlos env set ... -secret`, env set has no such flag) sent an operator |
| @@ -553,6 +633,17 @@ func parseDeliverCommand(t *testing.T, line string) (verb, flags []string) { |
| // COMMAND, so the check that runs it is itself a subprocess, built through |
| // carlosTestEnv and run from a scratch cmd.Dir — the constraint this whole |
| // suite is written around. |
| +// |
| +// C46 (review): the first cut of this test trusted -h output it never |
| +// verified belonged to the verb it asked about, and matched flags by |
| +// substring -- both closed only ONE axis of the class C44 exists to close |
| +// (wrong flag), not the other (wrong verb: a typo'd verb falls through to a |
| +// DIFFERENT usage page, which is real English text that can coincidentally |
| +// contain a flag name -- see helpOutputIsForVerb's doc comment for the |
| +// review's own example). This now asserts the subprocess exited 0, asserts |
| +// the output is that verb's OWN usage page (helpOutputIsForVerb) BEFORE |
| +// trusting anything in it, and matches each flag by name |
| +// (helpOutputHasFlag), not by substring. |
| func TestServicesDeliverLineIsARealCommand(t *testing.T) { |
| bin := buildCarlos(t) |
| scratch := t.TempDir() |
| @@ -565,10 +656,24 @@ func TestServicesDeliverLineIsARealCommand(t *testing.T) { |
| cmd := exec.Command(bin, append(append([]string{}, verb...), "-h")...) |
| cmd.Env = env |
| cmd.Dir = scratch |
| - helpOut, _ := cmd.CombinedOutput() // -h exits 0 for a real verb; output is what matters either way |
| + raw, err := cmd.CombinedOutput() |
| + helpOut := string(raw) |
| + // A real verb's own -h exits 0 (flag.ExitOnError treats -h as help |
| + // requested, not an error). A typo'd verb -- top-level or sub-verb -- |
| + // exits nonzero and prints a DIFFERENT page; trusting that page's |
| + // text at all, even before searching it, is the bug this guards. |
| + if err != nil { |
| + t.Fatalf("deliver line %q names `carlos %s -h`, which exited nonzero (%v) -- not a real verb:\n%s", |
| + line, strings.Join(verb, " "), err, helpOut) |
| + } |
| + if !helpOutputIsForVerb(helpOut, verb) { |
| + t.Fatalf("deliver line %q names `carlos %s`, but its -h output is not that verb's own "+ |
| + "usage page (no \"Usage of %s:\" line) -- a fall-through to a different page must fail here, "+ |
| + "not be searched:\n%s", line, strings.Join(verb, " "), strings.Join(verb, " "), helpOut) |
| + } |
| for _, f := range flags { |
| - if !strings.Contains(string(helpOut), f) { |
| - t.Fatalf("deliver line %q names flag %q, which `carlos %s -h` does not list:\n%s", |
| + if !helpOutputHasFlag(helpOut, f) { |
| + t.Fatalf("deliver line %q names flag %q, which `carlos %s -h` does not list as a real flag:\n%s", |
| line, f, strings.Join(verb, " "), helpOut) |
| } |
| } |