From 21a3d68ee97b33075b54dfea7d3aa5b97e3c9794 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 17 Jul 2026 23:35:16 -0700 Subject: [PATCH 1/4] Fix agent-help hidden-flag leak; make check-surface a real drift gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The committed .surface snapshot is generated and enforced by the Go surface package (surface.SnapshotString) via TestSurfaceSnapshot, not by scripts/check-cli-surface.sh. The two generators diverge: the Go one walks the command tree (full inherited globals, excludes hidden flags, no --version leak), while the shell script walks --help --agent (curated salient globals post-#499, and — the bug here — emits hidden flags). Rather than churn .surface to the script's shape (which would break TestSurfaceSnapshot and enshrine the leak), fix the underlying issues: - emitAgentHelp leaked hidden flags because pflag's VisitAll visits them, so `basecamp recordings --help --agent` surfaced the MarkHidden'd, "Not supported" --assignee flag. Skip hidden flags at all three emission sites to match text --help and the tree walker. Add a regression test. - check-surface previously regenerated to /tmp and echoed a line count, catching nothing. Repoint it at TestSurfaceSnapshot (the authority) so it actually fails on drift, and add an update-surface target for regeneration. check-surface-compat / the cli-surface CI job are unchanged (they diff generated-vs-generated across versions). - TestSurfaceSnapshot's -update-surface wrote .surface without a trailing newline; add one so update-surface is idempotent and POSIX-clean. Closes #539. --- Makefile | 21 ++++++++++++++------- internal/cli/help_test.go | 28 ++++++++++++++++++++++++++++ internal/cli/root.go | 12 +++++++++++- internal/commands/surface_test.go | 4 +++- 4 files changed, 56 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index f5c96b89..249f8755 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 "CLI surface drift: 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 diff --git a/internal/cli/help_test.go b/internal/cli/help_test.go index 8c707055..673d1878 100644 --- a/internal/cli/help_test.go +++ b/internal/cli/help_test.go @@ -532,6 +532,34 @@ 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"}) + _ = cmd.Execute() + + var info struct { + Flags []struct { + Name string `json:"name"` + } `json:"flags"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &info)) + + names := make([]string, len(info.Flags)) + for i, f := range info.Flags { + names[i] = f.Name + } + assert.NotContains(t, names, "assignee", "hidden flag leaked into agent help") + assert.Contains(t, names, "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..d6c64649 100644 --- a/internal/commands/surface_test.go +++ b/internal/commands/surface_test.go @@ -15,7 +15,9 @@ var updateSurface = flag.Bool("update-surface", false, "Update .surface baseline 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" From 745544ef2229052226f670d40215f1354f3e8d79 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 17 Jul 2026 23:50:23 -0700 Subject: [PATCH 2/4] Write .surface on acknowledged removal-only updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestSurfaceSnapshot's -update-surface only wrote .surface when there were additions, so a removal-only change (removed lines acknowledged in .surface-breaking) left the removed CMD/FLAG lines in .surface — and consumers that read it as the current surface (check-skill-drift, check-smoke-coverage) would keep validating removed commands. Write whenever there are no unacknowledged removals instead. --- internal/commands/surface_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/commands/surface_test.go b/internal/commands/surface_test.go index d6c64649..09f8e433 100644 --- a/internal/commands/surface_test.go +++ b/internal/commands/surface_test.go @@ -95,8 +95,11 @@ 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 { + // Write updated baseline whenever -update-surface is set and there are no + // unacknowledged removals. Gating only on additions would skip removal-only + // updates (removals acknowledged via .surface-breaking), leaving stale + // removed lines in .surface for consumers like check-skill-drift. + if *updateSurface && len(removals) == 0 { if err := os.WriteFile(baselinePath, []byte(current), 0o644); err != nil { t.Fatalf("writing .surface: %v", err) } From c209032cd5e40cbe914e581f84f66c6f3a2d6777 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 18 Jul 2026 00:00:57 -0700 Subject: [PATCH 3/4] Only rewrite .surface when the snapshot actually changed Refine the -update-surface write condition into a testable predicate, shouldWriteBaseline: write when update mode is on, there are no unacknowledged removals, and the baseline bytes differ from the freshly generated surface. The byte-difference guard keeps removal-only updates (acknowledged in .surface-breaking) writing while making an unchanged surface a true no-op. Add a TestShouldWriteBaseline regression test covering removal-only, addition, no-change, unacknowledged-removal, and non-update cases. Also refresh 'make help': describe check-surface as the drift gate and list the new update-surface target. --- Makefile | 3 ++- internal/commands/surface_test.go | 40 +++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 249f8755..f5f101bf 100644 --- a/Makefile +++ b/Makefile @@ -549,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/commands/surface_test.go b/internal/commands/surface_test.go index 09f8e433..c06ca423 100644 --- a/internal/commands/surface_test.go +++ b/internal/commands/surface_test.go @@ -1,17 +1,31 @@ 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) +} + func TestSurfaceSnapshot(t *testing.T) { root := buildRootWithAllCommands() @@ -95,13 +109,29 @@ func TestSurfaceSnapshot(t *testing.T) { } } - // Write updated baseline whenever -update-surface is set and there are no - // unacknowledged removals. Gating only on additions would skip removal-only - // updates (removals acknowledged via .surface-breaking), leaving stale - // removed lines in .surface for consumers like check-skill-drift. - if *updateSurface && len(removals) == 0 { + 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"))) +} From d7e7d0087d77d7df8bab9626a0aaa0d6e73d2293 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 18 Jul 2026 13:35:42 -0700 Subject: [PATCH 4/4] Harden surface drift gate and hidden-flag test from review - check-surface now fails when the committed .surface differs from the generated surface even if the only delta is an acknowledged removal that was never regenerated (baselineNeedsRegen). Previously such a baseline passed the gate while still feeding stale removed lines to check-skill-drift and check-smoke-coverage. - TestAgentHelpOmitsHiddenFlags now asserts Execute() succeeds and checks both flags and inherited_flags, matching the guarantee that no hidden flag appears anywhere in agent help. - check-surface's failure hint is now conditional (TestSurfaceSnapshot can fail for reasons other than drift). Add TestBaselineNeedsRegen covering the acknowledged-removal, no-change, addition, and unacknowledged-removal cases. --- Makefile | 2 +- internal/cli/help_test.go | 22 +++++++++++++------ internal/commands/surface_test.go | 35 +++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/Makefile b/Makefile index f5f101bf..82bc7419 100644 --- a/Makefile +++ b/Makefile @@ -360,7 +360,7 @@ test-release: .PHONY: check-surface check-surface: check-toolchain @BASECAMP_NO_KEYRING=1 $(GOTEST) $(BUILD_TAGS) ./internal/commands/ -run TestSurfaceSnapshot -count=1 || { \ - echo; echo "CLI surface drift: committed .surface is stale. Run: make update-surface"; \ + echo; echo "TestSurfaceSnapshot failed — if the committed .surface is stale, run: make update-surface"; \ exit 1; \ } @echo "CLI surface snapshot up to date ($$(wc -l < .surface) entries)" diff --git a/internal/cli/help_test.go b/internal/cli/help_test.go index 673d1878..cf0bdf28 100644 --- a/internal/cli/help_test.go +++ b/internal/cli/help_test.go @@ -543,21 +543,31 @@ func TestAgentHelpOmitsHiddenFlags(t *testing.T) { cmd.AddCommand(commands.NewRecordingsCmd()) cmd.SetOut(&buf) cmd.SetArgs([]string{"recordings", "list", "--help", "--agent"}) - _ = cmd.Execute() + 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)) - names := make([]string, len(info.Flags)) - for i, f := range info.Flags { - names[i] = f.Name + // 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, names, "assignee", "hidden flag leaked into agent help") - assert.Contains(t, names, "limit", "visible flag should still be present") + 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) { diff --git a/internal/commands/surface_test.go b/internal/commands/surface_test.go index c06ca423..0e6a9d76 100644 --- a/internal/commands/surface_test.go +++ b/internal/commands/surface_test.go @@ -26,6 +26,16 @@ func shouldWriteBaseline(update bool, unacknowledgedRemovals int, baseline, curr 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() @@ -109,6 +119,13 @@ func TestSurfaceSnapshot(t *testing.T) { } } + // 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) @@ -135,3 +152,21 @@ func TestShouldWriteBaseline(t *testing.T) { // 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)) +}