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 4 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.
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.
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".
- 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.
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. Also hardens drift detection to catch acknowledged-removal-only staleness and keepsupdate-surfaceidempotent, addressing #539 without churn.Bug Fixes
TestAgentHelpOmitsHiddenFlagsto assert success and verify bothflagsandinherited_flags.Refactors
check-surfacerunsTestSurfaceSnapshotand now fails when.surfacebyte-differs with no additions or unacknowledged removals (baselineNeedsRegen);make helpupdated.update-surfaceonly rewrites.surfacewhen the snapshot differs and no unacknowledged removals remain (shouldWriteBaseline), still writes on removal-only updates, and appends a trailing newline; addedTestShouldWriteBaselineandTestBaselineNeedsRegen.Written for commit d7e7d00. Summary will update on new commits.