Fix agent-help hidden-flag leak; make check-surface a real drift gate#542
Fix agent-help hidden-flag leak; make check-surface a real drift gate#542jeremy wants to merge 3 commits into
Conversation
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.
github-actions
Bot
added
commands
There was a problem hiding this comment.
Pull request overview
This PR addresses #539 by fixing an agent-help bug (hidden flags leaking into --help --agent JSON output) and by turning make check-surface into a real drift gate tied to the authoritative TestSurfaceSnapshot, while keeping the committed .surface snapshot owned by the Go generator (no churn).
Changes:
- Filter hidden flags out of agent-help output so
--help --agentmatches text help behavior. - Rework
make check-surfaceto runTestSurfaceSnapshotand add anupdate-surfacetarget for regeneration guidance. - Make
.surfaceregeneration idempotent by ensuring it is written with a trailing newline.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Makefile | Makes check-surface enforce .surface drift via TestSurfaceSnapshot and adds an update-surface target. |
| internal/commands/surface_test.go | Ensures regenerated .surface includes a trailing newline for idempotence. |
| internal/cli/root.go | Filters hidden flags out of agent-help JSON emission across local/parent-scoped/inherited flag sources. |
| internal/cli/help_test.go | Adds a regression test asserting hidden flags (e.g. recordings --assignee) do not appear in agent-help output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21a3d68ee9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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.
| // 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" |
| var buf bytes.Buffer | ||
| cmd := NewRootCmd() | ||
| cmd.AddCommand(commands.NewRecordingsCmd()) | ||
| cmd.SetOut(&buf) | ||
| cmd.SetArgs([]string{"recordings", "list", "--help", "--agent"}) | ||
| _ = cmd.Execute() | ||
|
|
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.
| @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; \ | ||
| } |
| 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") | ||
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c209032cd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 || { \ |
There was a problem hiding this comment.
When a command or flag is removed and the author only adds the old line to .surface-breaking, this target still passes because TestSurfaceSnapshot filters acknowledged removals out before reporting failures; it then prints .surface as up to date even though the removed entries remain in the committed snapshot. The fresh issue after the update-surface fix is the non-update check path here: consumers like check-skill-drift and check-smoke-coverage still read stale .surface, so check-surface should also require the baseline to match the generated snapshot once removals are acknowledged.
Useful? React with 👍 / 👎.
Closes #539.
TL;DR
#539 asked to reconcile the committed
.surfacesnapshot withscripts/check-cli-surface.sh, on the premise that the file had drifted from its generator and CI didn't catch it. Investigating, the premise doesn't hold:.surfaceis owned and enforced by a different generator, which it already matches exactly. So instead of regenerating the file, this PR fixes the two things that are actually wrong and turns the misleadingcheck-surfacetarget into a real gate.What's actually going on
There are two independent surface generators, and they disagree:
scripts/check-cli-surface.sh(shell)surface.SnapshotString(Go,github.com/basecamp/cli)--help --agentJSON--assignee)--versionleak (#368)--versionis local, not inherited)check-surfacejust echoed a/tmpline countTestSurfaceSnapshot(internal/commands/surface_test.go)The committed
.surfaceis byte-identical (modulo trailing newline) to the Go generator's output.TestSurfaceSnapshotpasses precisely because.surface == surface.SnapshotString(root), and that test is the CI catch #539 thought was missing. Cross-version compatibility is separately handled bycheck-surface-compat→check-cli-surface-diff.sh, entirely on the script side.Regenerating
.surfaceto the shell script's curated shape (the literal ask) would have:TestSurfaceSnapshot(the two generators can't both own the file),.surface-skill-driftfor globals (--jq,--agent,--profile) that do work on those subcommands — curation drops true information, andWhat this PR changes instead (4 files, zero
.surfacechurn)1. Fix the agent-help hidden-flag leak (
internal/cli/root.go)The "2 additions" #539 noticed (
FLAG basecamp recordings [list] --assignee) are a leak, not new surface.--assigneeisMarkHiddenwith usage"Not supported — use reports assigned instead".emitAgentHelpsurfaced it anyway because pflag'sVisitAllvisits hidden flags. Skip hidden flags at all three emission sites so agent-help matches text--helpand the tree walker. Legitimate visible--assignee/--assigneesflags oncards/todosare unaffected. Regression test inhelp_test.go.2. Make
check-surfacea real gate (Makefile)It regenerated to
/tmpand echoed a count — caught nothing but a totally broken binary. Repoint it atTestSurfaceSnapshot(the authority) so it fails on drift and points at a newmake update-surfaceregeneration target.check-surface-compat/ thecli-surfaceCI job are untouched (they diff generated-vs-generated across versions and never read the committed file).3. Idempotent regeneration (
internal/commands/surface_test.go)-update-surfacewrote.surfacewithout a trailing newline (stripping the POSIX one, creating churn). Add one somake update-surfaceis idempotent.Verification
make check-surfacepasses, and bites when.surfacedrifts (pointing atmake update-surface).make update-surfaceis idempotent (16,586 lines, no diff).basecamp recordings --help --agentno longer emits--assignee; the shell script's output drops the 2 leak lines while keeping the 8 legit visible ones.TestSurfaceSnapshot,check-skill-drift,check-smoke-coveragegreen;bin/ciexits 0.Follow-up note for #539
The shell script and
.surfacestill legitimately differ on the curated-vs-full-globals axis, but no consumer cross-checks them, so it's benign. If we later want a single source of truth, the options are: (a) keep the Go generator as authority (status quo after this PR), (b) migrate the snapshot + compat gate onto the shell script and retireTestSurfaceSnapshot/.surface-breaking, or (c) reconcile the two generators ingithub.com/basecamp/cli. This PR takes (a) as the smallest correct step.Summary by cubic
Fixes the hidden flag leak in agent help and turns
check-surfaceinto a real drift gate bound toTestSurfaceSnapshot. Refinesupdate-surfaceto only rewrite.surfacewhen the snapshot actually changes while still handling removal-only updates, addressing #539 without churn.Bug Fixes
recordings list --assignee), matching text--helpand the command tree.TestAgentHelpOmitsHiddenFlagsto prevent regressions.Refactors
check-surfacenow runsTestSurfaceSnapshotand fails on drift with guidance to runmake update-surface;make helpupdated.update-surfacewrites a trailing newline, updates.surfaceon acknowledged removal-only changes, and only rewrites when the snapshot differs viashouldWriteBaseline; addedTestShouldWriteBaseline.Written for commit c209032. Summary will update on new commits.