diff --git a/Makefile b/Makefile index f5c96b89..82bc7419 100644 --- a/Makefile +++ b/Makefile @@ -354,16 +354,23 @@ test-release: MACOS_SIGN_P12= MACOS_SIGN_PASSWORD= MACOS_NOTARY_KEY= MACOS_NOTARY_KEY_ID= MACOS_NOTARY_ISSUER_ID= \ goreleaser release --snapshot --skip=publish,sign --clean -# Generate CLI surface snapshot (validates binary produces valid output) +# Verify the committed CLI surface snapshot (.surface) matches the command tree. +# TestSurfaceSnapshot is the authority: it diffs .surface against surface.Snapshot +# and fails on any drift, pointing here for regeneration. .PHONY: check-surface -check-surface: build - @command -v jq >/dev/null 2>&1 || { \ - echo "ERROR: jq is required for check-surface but was not found."; \ - echo "Install with: brew install jq (macOS), apt-get install jq (Debian/Ubuntu)"; \ +check-surface: check-toolchain + @BASECAMP_NO_KEYRING=1 $(GOTEST) $(BUILD_TAGS) ./internal/commands/ -run TestSurfaceSnapshot -count=1 || { \ + echo; echo "TestSurfaceSnapshot failed — if the committed .surface is stale, run: make update-surface"; \ exit 1; \ } - scripts/check-cli-surface.sh $(BUILD_DIR)/$(BINARY) /tmp/cli-surface.txt - @echo "CLI surface snapshot generated ($$(wc -l < /tmp/cli-surface.txt) entries)" + @echo "CLI surface snapshot up to date ($$(wc -l < .surface) entries)" + +# Regenerate the committed CLI surface snapshot (.surface) from the command tree. +# Accepts additions; removals must be acknowledged in .surface-breaking. +.PHONY: update-surface +update-surface: check-toolchain + @BASECAMP_NO_KEYRING=1 $(GOTEST) $(BUILD_TAGS) ./internal/commands/ -run TestSurfaceSnapshot -update-surface -count=1 + @echo "CLI surface snapshot regenerated ($$(wc -l < .surface) entries)" # Compare CLI surface against baseline (fails on removals) .PHONY: check-surface-diff @@ -542,7 +549,8 @@ help: @echo " lint-actions Lint GitHub Actions workflows (actionlint + zizmor)" @echo " tidy-check Verify go.mod/go.sum are tidy" @echo " check Run all checks (local CI gate)" - @echo " check-surface Generate CLI surface snapshot (validates --help --agent output)" + @echo " check-surface Verify committed .surface matches the command tree (TestSurfaceSnapshot)" + @echo " update-surface Regenerate committed .surface from the command tree" @echo " check-surface-diff Compare CLI surface snapshots (fails on removals)" @echo " check-skill-drift Verify skill references match CLI surface" @echo "" diff --git a/internal/cli/help_test.go b/internal/cli/help_test.go index 8c707055..cf0bdf28 100644 --- a/internal/cli/help_test.go +++ b/internal/cli/help_test.go @@ -532,6 +532,44 @@ func TestAgentHelpOmitsArgsWhenEmpty(t *testing.T) { assert.NotContains(t, out, `"args"`) } +// Hidden flags (e.g. recordings --assignee, MarkHidden'd as "Not supported") +// must not leak into agent help — pflag's VisitAll visits hidden flags, so +// emitAgentHelp filters them to match text --help, which never lists them. +func TestAgentHelpOmitsHiddenFlags(t *testing.T) { + isolateHelpTest(t) + + var buf bytes.Buffer + cmd := NewRootCmd() + cmd.AddCommand(commands.NewRecordingsCmd()) + cmd.SetOut(&buf) + cmd.SetArgs([]string{"recordings", "list", "--help", "--agent"}) + require.NoError(t, cmd.Execute()) + + var info struct { + Flags []struct { + Name string `json:"name"` + } `json:"flags"` + InheritedFlags []struct { + Name string `json:"name"` + } `json:"inherited_flags"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &info)) + + // Hidden flags must not leak via any emission path — local/parent-scoped + // (flags) or inherited (inherited_flags). + everywhere := make([]string, 0, len(info.Flags)+len(info.InheritedFlags)) + localNames := make([]string, 0, len(info.Flags)) + for _, f := range info.Flags { + everywhere = append(everywhere, f.Name) + localNames = append(localNames, f.Name) + } + for _, f := range info.InheritedFlags { + everywhere = append(everywhere, f.Name) + } + assert.NotContains(t, everywhere, "assignee", "hidden flag leaked into agent help (flags or inherited_flags)") + assert.Contains(t, localNames, "limit", "visible flag should still be present") +} + func TestLeafCommandHelpShowsArguments(t *testing.T) { isolateHelpTest(t) diff --git a/internal/cli/root.go b/internal/cli/root.go index 47e20dfd..5bb4fe98 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -771,8 +771,12 @@ func emitAgentHelp(cmd *cobra.Command) { } } - // Local flags + // Local flags. Hidden flags are skipped to match text --help, which never + // lists them — pflag's VisitAll visits hidden flags, so filter explicitly. cmd.NonInheritedFlags().VisitAll(func(f *pflag.Flag) { + if f.Hidden { + return + } info.Flags = append(info.Flags, agentFlag{ Name: f.Name, Shorthand: f.Shorthand, @@ -785,6 +789,9 @@ func emitAgentHelp(cmd *cobra.Command) { // Parent-scoped flags (e.g. --room on chat subcommands) — promoted into // flags to match text help's parentScopedFlags promotion. parentScopedFlags(cmd).VisitAll(func(f *pflag.Flag) { + if f.Hidden { + return + } info.Flags = append(info.Flags, agentFlag{ Name: f.Name, Shorthand: f.Shorthand, @@ -796,6 +803,9 @@ func emitAgentHelp(cmd *cobra.Command) { // Inherited flags — shared logic with filterInheritedFlags (text help) curatedInheritedFlags(cmd).VisitAll(func(f *pflag.Flag) { + if f.Hidden { + return + } info.InheritedFlags = append(info.InheritedFlags, agentFlag{ Name: f.Name, Shorthand: f.Shorthand, diff --git a/internal/commands/surface_test.go b/internal/commands/surface_test.go index 5d08ca06..0e6a9d76 100644 --- a/internal/commands/surface_test.go +++ b/internal/commands/surface_test.go @@ -1,21 +1,47 @@ package commands_test import ( + "bytes" "errors" "flag" "os" "strings" "testing" + "github.com/stretchr/testify/assert" + "github.com/basecamp/cli/surface" ) var updateSurface = flag.Bool("update-surface", false, "Update .surface baseline file") +// shouldWriteBaseline reports whether -update-surface should rewrite .surface: +// only in update mode, only when no unacknowledged removals remain, and only +// when the baseline actually differs from the freshly generated surface. The +// last condition matters for removal-only changes — removals acknowledged in +// .surface-breaking leave zero additions, so gating on additions alone would +// skip the write and leave stale removed lines in .surface — while still +// leaving an unchanged surface untouched. +func shouldWriteBaseline(update bool, unacknowledgedRemovals int, baseline, current []byte) bool { + return update && unacknowledgedRemovals == 0 && !bytes.Equal(baseline, current) +} + +// baselineNeedsRegen reports whether a committed baseline is stale in a way the +// additions/removals checks do not already surface: the bytes differ from the +// generated surface, yet there are no unacknowledged removals and no additions. +// The prime case is a removal acknowledged in .surface-breaking but never +// regenerated — check-surface would otherwise pass while .surface still carries +// the removed lines that check-skill-drift and check-smoke-coverage read. +func baselineNeedsRegen(baseline, current []byte, unacknowledgedRemovals, additions int) bool { + return !bytes.Equal(baseline, current) && unacknowledgedRemovals == 0 && additions == 0 +} + func TestSurfaceSnapshot(t *testing.T) { root := buildRootWithAllCommands() - current := surface.SnapshotString(root) + // Trailing newline so -update-surface writes a POSIX-clean file; the + // comparisons below TrimSpace, so it does not affect drift detection. + current := surface.SnapshotString(root) + "\n" baselinePath := "../../.surface" @@ -93,10 +119,54 @@ func TestSurfaceSnapshot(t *testing.T) { } } - // Write updated baseline only when -update-surface is set and no removals were found - if *updateSurface && len(removals) == 0 && len(additions) > 0 { + // Catch acknowledged-removal staleness (and any other byte drift the checks + // above don't flag): the baseline no longer matches the generated surface, + // so it must be regenerated to stay canonical for downstream consumers. + if !*updateSurface && baselineNeedsRegen(baseline, []byte(current), len(removals), len(additions)) { + t.Errorf("committed .surface is stale (differs from the generated surface with no additions or unacknowledged removals — e.g. an acknowledged removal was never regenerated); run make update-surface") + } + + if shouldWriteBaseline(*updateSurface, len(removals), baseline, []byte(current)) { if err := os.WriteFile(baselinePath, []byte(current), 0o644); err != nil { t.Fatalf("writing .surface: %v", err) } } } + +func TestShouldWriteBaseline(t *testing.T) { + base := []byte("A\nB\nC\n") + + // Removal-only: current dropped B, acknowledged (0 unacknowledged removals), + // no additions — must still write so the removed line leaves .surface. + assert.True(t, shouldWriteBaseline(true, 0, base, []byte("A\nC\n"))) + + // Addition: current gained D — writes. + assert.True(t, shouldWriteBaseline(true, 0, base, []byte("A\nB\nC\nD\n"))) + + // No change — must not write (avoids needless churn). + assert.False(t, shouldWriteBaseline(true, 0, base, base)) + + // Unacknowledged removal present — must not write (drift is a failure). + assert.False(t, shouldWriteBaseline(true, 1, base, []byte("A\nC\n"))) + + // Not in update mode — never writes. + assert.False(t, shouldWriteBaseline(false, 0, base, []byte("A\nC\n"))) +} + +func TestBaselineNeedsRegen(t *testing.T) { + base := []byte("A\nB\nC\n") + + // Acknowledged removal never regenerated: baseline still has B, current + // dropped it, 0 unacknowledged removals, 0 additions — stale, must flag. + assert.True(t, baselineNeedsRegen(base, []byte("A\nC\n"), 0, 0)) + + // Identical — not stale. + assert.False(t, baselineNeedsRegen(base, base, 0, 0)) + + // Additions present — already reported by the additions check, don't + // double-flag here. + assert.False(t, baselineNeedsRegen(base, []byte("A\nB\nC\nD\n"), 0, 1)) + + // Unacknowledged removal — already reported by the removals check. + assert.False(t, baselineNeedsRegen(base, []byte("A\nC\n"), 1, 0)) +}