Fix agent-help hidden-flag leak; make check-surface a real drift gate

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 ""

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)
 

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,

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))
+}