Skip hidden commands in breaking change detection - #8463
Closed
nickwesselman wants to merge 1 commit into
Closed
Conversation
`extractManifestSurface` treated every manifest command as public surface, so renaming a flag on a `hidden` command failed the breaking change check even though CONTRIBUTING.md scopes the stable command surface to what the CLI actually exposes. Skip hidden commands when building the surface, which drops them from the removed command, flag and env var scans alike. Hidden-ness is read from each manifest independently, so a command that was visible in the baseline and is hidden now still reports as removed: a PR can't hide a command and strip its flags in one move without being flagged. An env var shared with a visible command also stays tracked through that command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-By: devx/a27b4557-3ac8-448a-8860-7a5ed6b20687
amcaplan
approved these changes
Sep 3, 2026
Contributor
|
I really appreciate the dedication to fixing broken windows in the experience of developing the CLI! |
Contributor
Author
|
I'm going to abandon this as the check is non-blocking and there is some nuance. We might hide a command to deprecate it, in which case removing after would be the breaking change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
The breaking change check treats every command in
packages/cli/oclif.manifest.jsonaspublic surface.
extractManifestSurfacenever reads a command'shiddenfield, sorenaming a flag on a hidden command fails
Breaking change detectioneven though nothingusers can discover has changed.
CONTRIBUTING.mdscopes the stable Command surface to what the CLI exposes; a commandmarked
static hidden = trueisn't part of it. The check is currently stricter than thewritten policy.
This surfaced on #8459, which renames
--with-demo-datato--demo-dataonstore store create dev— ahiddencommand whose flag only reachedmainalongsideunreleased work. The check reported a removed flag and a removed env var and failed the PR.
WHAT is this pull request doing?
Skips hidden commands when building the manifest surface:
Because
removedCommands,removedFlagsandremovedEnvVarsall derive from thatsurface, one guard covers all three scans.
Two deliberate details:
hiddenis read from each manifest independently, not unioned across baseline andcurrent. A command that was visible in the baseline and is hidden now still reports as a
removed command. That keeps the obvious loophole closed — a PR can't hide a command and
strip its flags in the same change without being flagged — and it errs the way the rest
of this file already errs, "widening rather than silently skipping potential removals."
removing it there is still reported.
extractManifestSurfaceis now exported so it can be tested directly.How to test your changes?
node --test workspace/src/major-change-check.test.js— 20 tests pass, including four newones covering hidden-command omission, env vars used only by hidden commands, env vars
shared with a visible command, and the per-manifest
hiddenreading.I also ran the extractor against the real manifests to confirm both directions:
mainvs Ask about demo data when creating a dev store interactively #8459's branch (the hidden-command flag rename) →removedCommands: [],removedFlags: [],removedEnvVars: [], so the check passes.mainwith a flag deleted fromapp build(a visible command) → still reportsapp:build --auth-alias, so genuine breaking changes are unaffected.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add— not user-facing, CI tooling only