fix: resolve abbreviations of native commands - #176
Conversation
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 2 warnings · 🔵 2 minor points · ⚪ 1 nitpick
🔍 Full review · 10 files reviewed
⚪ Nitpick
commands/root.go:41— The index-load failure is reported viadebugLogf, which readsviper.GetBool("debug"). At this pointExecuteContexthas not run, so the bound--debugflag is not parsed (Changedis false) and viper falls back to the env variable. A user passing--debugon the command line therefore never sees this message, so a corrupt or placeholder commands.json (e.g. the empty file CI touches) disables abbreviation resolution with no diagnosable output.
Verification
list --all --format=jsonemitscommandsas an object keyed by command name (CustomJsonDescriptor + DescriptorUtils), somap[string]Commandunmarshals — I generated the real index and parsed it with the PR's struct.- Legacy entries whose canonical name matches a native name or alias are skipped, so
list,helpandcompletiondo not create false ambiguity against their native overrides. - Replaying the resolver over the real 191-command index for every prefix-abbreviation of the native commands expands
p:init,a:config-v,p:convetc. and shadows no visible legacy command. - The Makefile recipe leaves
$@untouched and exits non-zero when the php run fails, and every goreleaser target that builds the phar now also lists commands.json, with clean-phar removing both. - Commands disabled by the upsun runtime config (project:variable:*, self:update, …) are present in the index but block no native abbreviation, so the config mismatch causes no missed expansion today.
New unit table test commands/abbreviation_test.go covers the resolver with a stubbed loader (run by the test CI job, which touches an empty commands.json, so real index parsing in internal/legacy/commands.go has no unit coverage), and integration-tests/abbreviation_test.go exercises the end-to-end path in the integration-test job, which builds the real index through make single. No test covers the failure path where the embedded index is missing or unparsable.
Review details
- Commit: d37617d
- Model: claude-opus-5
Review 1 of 10 for this pull request · View the full run
There was a problem hiding this comment.
Note
Reviewed — No new blocking findings · 🔵 1 minor point · 3 still open (1 nitpick)
🔁 Incremental · 3 files reviewed
🔵 Minor point
commands/abbreviation.go:134— The new comment and check make hidden commands count toward ambiguity, but hidden aliases never reach this code: the index is produced by CustomJsonDescriptor::getCommandData, which fills thealiasesfield from$command->getVisibleAliases()(legacy/src/Console/CustomJsonDescriptor.php:139), so aliases registered viasetHiddenAliases()(e.g.logs,snapshots,user:role,i:act,local:install) are absent from commands.json. Symfony's Application::find() matches those aliases. An abbreviation that matches only a native command in the index but also matches a hidden alias is therefore expanded to the native command, whereas the legacy CLI would have reported ambiguity or run the legacy command. No current hidden alias collides with the small native command set, so this is latent today; adding a native command (or a vendor config with different aliases) makes it bite silently.
Outstanding from earlier reviews:
- 🔵 #4098987925 —
commands/abbreviation.go:32: Common legacy flags leave the reported bug unfixed for those invocations. — Only--help/-hwere special-cased;-n(legacy's --no-interaction shorthand),-hinside a combined shorthand like-vh, and--help=truestill fail isRootBoolFlag, soupsun -n p:initremains unexpanded. - 🔵 #4098987928 —
internal/legacy/commands.go:12: Ships ~370KB of unused help text in every released binary. — The Makefile recipe still stores the fulllist --all --format=jsonoutput; commands.go still embeds it whole and reads only name/aliases/hidden. - ⚪
commands/root.go:41: Silent feature loss with no way to diagnose it from the command line. — commands/root.go still reports the index-load failure via debugLogf before ExecuteContext parses --debug. (first raised)
Verification
- The
--no-interactionplus< /dev/nulladded to the commands.json recipe suppresses SelfInstallChecker (it returns early when !isInteractive, legacy/src/Service/SelfInstallChecker.php:40) and the update check (Application.php:516). PLATFORMSH_CLI_APPLICATION_VERSIONmaps to the existingapplication.versiondefault key, so applyEnvironmentOverrides picks it up for the phar built from legacy/.- Removing the hidden-filter makes resolveAbbreviation strictly more conservative — a hidden competitor now yields nil and the args pass through to the legacy CLI unchanged.
slicesis still used (IndexFunc/Contains/Clone) after deleting the DeleteFunc call, so abbreviation.go still compiles.- Treating
--help/-has a root boolean flag only affects the scan position;upsun --help p:initbecomes--help init, which Cobra resolves to the native command's help.
Unit coverage is in commands/abbreviation_test.go (the ver case flipped to expect no expansion, plus a new --help p:init case), run by the test job, which embeds an empty touched commands.json; integration-tests/abbreviation_test.go exercises the real generated index in the integration-test job, which builds the phar and runs make single (thus the changed commands.json recipe) before make integration-test.
Review 2 of 10 for this pull request · View the full run
|
Re: hidden aliases (review 5310891011): no change for now. Aliases registered with 🤖 Addressed by Claude Code |
Abbreviations like "p:init" or "a:config-v" failed because the Go layer only matched exact names, and the legacy CLI does not know about the native commands. The build now embeds the legacy CLI's "list --all --format=json" output, generated from the phar with all experiments enabled, so it includes every command regardless of config. The Go layer resolves abbreviations with Symfony Console's rules across native and legacy commands, and expands only a unique match to a native command. Everything else is passed to the legacy CLI as before. The index is only parsed when a name abbreviates a native command. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A value-taking flag before the command, e.g. "--context p:init init", could have its value mistaken for the command name. Abbreviations are now only expanded after known boolean root flags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Set the version when generating the command index, as the Go wrapper does, so the phar does not run "git describe", which crashed in CI. - Run the index generation non-interactively, so that local builds do not stop at the self-install prompt. - Count hidden commands toward ambiguity: the legacy CLI can resolve to them, e.g. "ver" to the hidden "version:list". - Allow --help and -h before an abbreviated command. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y CLI - Read the new hidden_aliases field of the command index. Hidden aliases only work in full, so they prevent expansion when typed exactly, but are not matched by abbreviations. - Leave out legacy commands disabled by the config (disabled_commands and wrapped_disabled_commands), as the legacy CLI does not register them. The index itself stays config-independent, as one index is shared by all builds and the config can change at runtime. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
55654d3 to
7811cf0
Compare
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 1 still open (1 nitpick)
🔍 Full review · 11 files reviewed
Outstanding from earlier reviews:
- ⚪
commands/root.go:41: Silent feature loss with no way to diagnose it from the command line. — root.go still callsdebugLogfbeforeExecuteContexthas parsed flags, so passing--debugon the command line doesn't show the index-load failure. (first raised)
Verification
- The legacy CustomJsonDescriptor emits
commandskeyed by name and ahidden_aliasesfield, which matches the Gomap[string]CommandandHiddenAliasesJSON tags. - The Makefile recipe passes
--no-interactionand< /dev/nulland keeps the phar's exit status throughstatus=$$?, so a failedlistfails the build. resolveAbbreviationrequires one full match among matched names and counts prefix-only matches toward ambiguity, as Symfony'sfind()does with its{^expr$}icheck.- An exact hidden legacy alias stops expansion, and a hidden alias never counts as an abbreviation candidate, matching HiddenAliasesCommandLoader.
- release.yml builds through
make release, which now depends oncommands.json, so thego:embedinput exists in release builds.
New table-driven unit tests in commands/abbreviation_test.go cover expandAbbreviation and enabledLegacyCommands (CI test job). integration-tests/abbreviation_test.go runs the real binary in the integration-test job. No test runs the --help <abbrev> / -h <abbrev> path end to end, and CI's test job embeds an empty commands.json.
Review details
- Commit: 7811cf0
- Model: claude-opus-5-5
Review 3 of 10 for this pull request · View the full run
Cobra adds the help flag during execution, after looking up the command, so "--help init" treated "init" as the flag's value and passed the arguments to the legacy CLI, which has no "init" command. This also made expansion after "--help" ineffective. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open (1 nitpick)
🔁 Incremental · 4 files reviewed
Outstanding from earlier reviews:
- ⚪
commands/root.go:41: Silent feature loss with no way to diagnose it from the command line. — Execute still calls debugLogf, which reads viperdebug, before ExecuteContext parses flags, so the--debugcommand-line flag is ignored at that point. (first raised)
Verification
cmd.InitDefaultHelpFlag()is now called on root beforeExecute.--helpis a bool with NoOptDefVal, so Cobra'sstripFlagsno longer takes the followinginitas its value.isRootBoolFlagnow checksroot.Flags()as well as the persistent flags, so the localhelp/hflag counts as boolean without the old hard-coded--help/-hspecial case.- The unit test calls
root.InitDefaultHelpFlag()in its setup, so the "after help flag" case runs through the same flag-lookup path as production. - No persistent root flag uses the
hshorthand, soInitDefaultHelpFlagstill registers-hon root.
This push adds unit cases in commands/abbreviation_test.go (help flag defined in the test setup) and two integration cases (--help p:init, --help init) in integration-tests/abbreviation_test.go. Both run under the repository's Go test jobs in .github/workflows/ci.yml.
Review 4 of 10 for this pull request · View the full run
|
Re: the 🤖 Addressed by Claude Code |
Abbreviations of Go-native commands, such as
p:initora:config-v, failed with errors likeThe namespace "p" is ambiguous: the Go layer only matched exact names, and the legacy CLI doesn't know about the native commands.internal/legacy/archives/commands.jsonfrom the phar (list --all --format=json, with all experiments enabled and an isolatedHOME), so it lists every legacy command regardless of config. It is embedded, not committed.help <abbreviation>is resolved the same way.disabled_commands,wrapped_disabled_commands) are left out in Go. Hidden aliases only prevent expansion when typed in full, matching the legacy CLI.🤖 Generated with Claude Code