fix(cli): CLI consistency report findings(#903)#910
Conversation
shreejaykurhade
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
shreejaykurhade
changed the title
shreejaykurhade
changed the title
shreejaykurhade
changed the title
sergio-sisternes-epam
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
This PR addresses CLI consistency report findings for APM 0.9.2 by fixing broken --help surfaces, aligning CLI output/help formatting, and documenting the apm experimental command family in the Starlight CLI reference.
Changes:
- Fix
apm experimental <subcommand> --helprouting by removing problematic Click group context settings. - Expand
apm mcp installto exposeNAME+ relevant options and forward them toapm install --mcp ...; add regression tests. - Normalize CLI output/help text (status symbol for
apm run script, remove trailing punctuation, add-yalias for runtime remove) and update CLI reference docs.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Bumps apm-cli lock version to 0.9.2. |
| tests/unit/test_mcp_command.py | Extends help/forwarding assertions for apm mcp install. |
| tests/unit/test_cli_consistency.py | Adds regression tests for help/output consistency items. |
| src/apm_cli/output/script_formatters.py | Updates script header output to use [>] running status. |
| src/apm_cli/commands/runtime.py | Adds -y alias for runtime remove --yes. |
| src/apm_cli/commands/pack.py | Removes trailing periods from --dry-run help strings. |
| src/apm_cli/commands/outdated.py | Removes trailing period from outdated command description. |
| src/apm_cli/commands/mcp.py | Adds explicit NAME + options for mcp install and constructs forwarded argv. |
| src/apm_cli/commands/experimental.py | Removes group context_settings that broke subcommand help. |
| docs/src/content/docs/reference/cli-commands.md | Documents apm mcp install args/options and adds full apm experimental section. |
| _, post_dd = _split_argv_at_double_dash(_get_invocation_argv()) | ||
| if post_dd: | ||
| pre_args = ctx.args[: len(ctx.args) - len(post_dd)] | ||
| forwarded = ["install", "--mcp", *pre_args, "--", *post_dd] | ||
| forwarded.extend(["--", *post_dd]) | ||
| else: | ||
| forwarded = ["install", "--mcp", *ctx.args] | ||
| forwarded.extend(ctx.args) |
There was a problem hiding this comment.
When the invocation includes a -- separator, this branch appends only the post--- tokens and drops any extra pre--- args captured in ctx.args (e.g., install flags not explicitly modeled on apm mcp install). That changes the alias semantics and can silently lose forwarded options whenever post--- argv is present. Preserve forwarding of any pre--- extra args in addition to re-inserting the separator (and add a regression test for an unknown/pre--- flag + post--- argv).
| @runtime.command(help="Remove an installed runtime") | ||
| @click.argument("runtime_name", type=click.Choice(["copilot", "codex", "llm"])) | ||
| @click.confirmation_option(prompt="Are you sure you want to remove this runtime?", help="Confirm the action without prompting") | ||
| @click.confirmation_option("--yes", "-y", prompt="Are you sure you want to remove this runtime?", help="Confirm the action without prompting") | ||
| def remove(runtime_name): |
There was a problem hiding this comment.
This change adds the -y alias for apm runtime remove, but the apm-guide CLI reference used by the apm-usage skill still lists apm runtime remove with only --yes in its key flags table. Please update packages/apm-guide/.apm/skills/apm-usage/commands.md accordingly so the generated guidance stays consistent with the CLI.
APM Expert Review Panel — PR #910
Python ArchitectPASS The One subtle observation: Removing Unused CLI Logging ExpertPASS
No direct DevX UX ExpertPASS Three UX improvements that matter:
The expanded Supply Chain Security ExpertPASS — no new surface The No OSS Growth Hacker — side-channel to CEOThe APM CEO — Arbitration & Strategic CallAPPROVE — one blocking ask, two non-blocking notes No specialist disagreements to arbitrate. Strategic read: This PR is infrastructure polish that pays down UX debt before it compounds. The [BLOCKING] Missing CHANGELOG entry Per repo convention (Keep a Changelog + project rules), every PR touching code, tests, or docs must have an ### Changed
- `apm mcp install` now declares all options explicitly (`--transport`, `--url`, `--env`, `--header`, `--mcp-version`, `--registry`, `--dev`, `--dry-run`, `--force`, `--no-policy`, `-v`) — improves `--help` discoverability and type-validates inputs before forwarding to `apm install --mcp`. (#910)
- `apm runtime remove` now accepts `-y` as a short alias for `--yes`. (#910)
- `apm experimental` group no longer sets `ignore_unknown_options`/`allow_interspersed_args` — subcommand routing handles option scoping correctly without them. (#910)
- `[>] Running script:` header now uses the canonical STATUS_SYMBOLS bracket notation instead of a bare leading space. (#910)
- Help text trailing-period normalisation across `pack`, `unpack`, `outdated`. (#910)[NON-BLOCKING] else:
forwarded.extend(ctx.args)With all options now explicitly declared, [NON-BLOCKING] PR title "fixed bugs and improved perf and docs" understates the Quality Gate Summary
Bottom line: one CHANGELOG line away from merge.
|
There was a problem hiding this comment.
Review: fix(cli): CLI consistency report findings (#903)
Thanks for the thorough consistency sweep, @shreejaykurhade. Most of the changes here are clean, well-tested, and welcome. However, two changes need to be reverted, and the mcp install rewrite needs a different approach.
Review methodology: This review incorporates findings from the APM Review Panel -- independent assessments from the Python Architect (structural patterns, drift risk) and DevX UX Expert (command-surface ergonomics, alias semantics). Both reached the same conclusion on the
mcp installchange independently.
What you need to do
1. Revert the uv.lock version bump
The 0.9.1 -> 0.9.2 change does not belong in a bugfix PR. Version bumps are handled in the release process. Please revert uv.lock to its original state.
2. Revert the mcp install option duplication in mcp.py and use the epilog approach instead
The current approach replaces transparent argument forwarding with 11 explicit @click.option declarations and manual argv reconstruction. This has two problems:
- Already incomplete: 8
install --mcpoptions are missing from the duplicated surface (--target/-t,--global/-g,--runtime,--exclude,--update,--trust-transitive-mcp,--parallel-downloads,--only). Runningapm mcp install myserver --target cursorwill error instead of forwarding. - Creates permanent sync burden: every future
installoption must be mirrored in decorators, function signature, forwarding block, docs, and tests. The Python Architect rated this as O(options^2) maintenance vs O(1) for the passthrough. The DevX UX Expert called it "the CLI equivalent of copy-paste inheritance." - Introduces a bug: when
--separator is present, pre---extra args are silently dropped (also flagged by Copilot).
What to do instead: Restore the original ignore_unknown_options + allow_extra_args forwarding, but add a declared NAME argument and use Click's epilog= parameter to list common options. This gives --help discoverability without any sync burden. See the inline comment on mcp.py for the recommended code pattern.
3. Resolve merge conflicts
The PR currently has a dirty merge state.
Non-blocking improvements (nice to have in this PR)
pack.py: the trailing-period removal is incomplete ----forceand--outputstill end with periods. An all-or-nothing sweep per file avoids internal inconsistency.runtime.py: updatepackages/apm-guide/.apm/skills/apm-usage/commands.mdto include the-yalias (also flagged by Copilot).test_cli_consistency.py: theoutdatedcolumn-width assertion is fragile -- checking exact spacing will break if Click changes column padding. Assert the substring without padding instead.
What works well (no changes needed)
experimentalgroup fix -- restores subcommand--helprouting; root-caused correctlyruntime remove -y-- good parity withdeps cleanandmarketplace remove[>]status symbol -- matches STATUS_SYMBOLS conventionapm experimentaldocs -- thorough, matches existing reference structure- Regression tests -- well-targeted, lock the
--helpcontract
| _, post_dd = _split_argv_at_double_dash(_get_invocation_argv()) | ||
| if post_dd: | ||
| pre_args = ctx.args[: len(ctx.args) - len(post_dd)] | ||
| forwarded = ["install", "--mcp", *pre_args, "--", *post_dd] | ||
| forwarded.extend(["--", *post_dd]) | ||
| else: | ||
| forwarded = ["install", "--mcp", *ctx.args] | ||
| forwarded.extend(ctx.args) |
There was a problem hiding this comment.
Bug: pre--- extra args silently dropped when -- is present.
Old code preserved both pre- and post--- args:
pre_args = ctx.args[: len(ctx.args) - len(post_dd)]
forwarded = ["install", "--mcp", *pre_args, "--", *post_dd]New code only forwards post--- tokens:
forwarded.extend(["--", *post_dd]) # pre-DD ctx.args: goneThis silently drops flags before -- that are not in the explicit option list, breaking existing working invocations. Also flagged by Copilot.
| @click.confirmation_option("--yes", "-y", prompt="Are you sure you want to remove this runtime?", help="Confirm the action without prompting") | ||
| def remove(runtime_name): |
There was a problem hiding this comment.
Nit: update packages/apm-guide/.apm/skills/apm-usage/commands.md to include the -y alias so generated guidance stays consistent with the CLI. (Also flagged by Copilot.)
| [[package]] | ||
| name = "apm-cli" | ||
| version = "0.9.1" | ||
| version = "0.9.2" |
There was a problem hiding this comment.
Revert: version bump does not belong in a bugfix PR.
0.9.1 -> 0.9.2 should be handled in the release process, not bundled with CLI consistency fixes. It forces version coordination on every other in-flight PR.
| assert result.exit_code == 0 | ||
| assert "outdated Show outdated locked dependencies." not in result.output | ||
| assert "outdated Show outdated locked dependencies" in result.output | ||
|
|
There was a problem hiding this comment.
Nit: this assertion is fragile. Checking exact column spacing ("outdated Show...") will break if Click changes column widths. Consider asserting just the substring without padding.
| @@ -140,7 +140,6 @@ def _handle_unknown_flag(name: str, logger: CommandLogger) -> None: | |||
| @click.group( | |||
There was a problem hiding this comment.
Good fix. Removing context_settings restores subcommand --help routing -- exactly the right call.
| def test_script_run_header_uses_running_status_symbol(): | ||
| formatter = ScriptExecutionFormatter(use_color=False) | ||
|
|
||
| assert formatter.format_script_header("build", {})[0] == "[>] Running script: build" |
There was a problem hiding this comment.
Well-targeted regression tests that lock the --help contract. Good addition.
|
@sergio-sisternes-epam @danielmeppiel
|
danielmeppiel
added
panel-review
APM Review Panel VerdictDisposition: REQUEST_CHANGES (one required pre-merge fix, then approve) Per-persona findingsPython Architect: This is a routine CLI consistency fix. No class hierarchies change; the touched code is entirely procedural Click decorators and output strings. 1. OO / class diagramclassDiagram
direction TB
class ScriptExecutionFormatter {
<<IOBoundary>>
-use_color bool
-console Console
+format_script_header(name, params) List
+format_completion() List
+_styled(text, style) str
}
class ExperimentalGroup {
<<Pure>>
invoke_without_command=True
+experimental(ctx, verbose)
}
class McpInstallCommand {
<<Pure>>
ignore_unknown_options=True
allow_extra_args=True
+mcp_install(ctx, name)
}
class PackCommand {
<<Pure>>
+pack_cmd(ctx, ...)
+unpack_cmd(ctx, ...)
}
class RuntimeCommand {
<<Pure>>
+remove(runtime_name)
}
class CommandLogger {
+progress(msg)
+error(msg)
}
ExperimentalGroup ..> CommandLogger : creates
McpInstallCommand ..> CommandLogger : creates
PackCommand ..> CommandLogger : creates
RuntimeCommand ..> CommandLogger : creates
ScriptExecutionFormatter ..> Console : "[I/O]"
class ScriptExecutionFormatter:::touched
class ExperimentalGroup:::touched
class McpInstallCommand:::touched
class PackCommand:::touched
class RuntimeCommand:::touched
classDef touched fill:#fff3b0,stroke:#d47600
2. Execution flow diagramKey behavioral change: flowchart TD
A["apm mcp install NAME [..opts] [-- cmd argv]"]
A --> B["Click: name=NAME\nctx.args=[..opts]"]
B --> C{"post '--' args\nin raw argv?"}
C -- yes --> D["_split_argv_at_double_dash()\n[FS] reads sys.argv"]
C -- no --> E["forwarded =\n['install','--mcp', name, *ctx.args]"]
D --> F["forwarded =\n['install','--mcp', name, *pre_args,\n'--', *post_dd]"]
F --> G["cli.main(forwarded, standalone_mode=False)"]
E --> G
G --> H["[install] apm install --mcp NAME [..opts]"]
style D fill:#ffe8b0,stroke:#c07000
Previously
Design patterns
Bug: CLI Logging Expert: All output changes are correct. DevX UX Expert: All ergonomic improvements align with package-manager conventions.
Minor nit (not a blocker): Supply Chain Security Expert: No security findings. The Auth Expert: Not activated -- the diff touches only CLI command surface files ( OSS Growth Hacker: Positive conversion signal. This PR directly improves discoverability on two growing surfaces: Side-channel to CEO: the missing CEO arbitrationAll five specialists are in agreement: this is a low-risk, high-value consistency patch that improves discoverability and ergonomics with no security or architectural regressions. The only concrete blocker is the Required actions before merge
Optional follow-ups
|
|
@danielmeppiel @sergio-sisternes-epam Fixed the requested pre-merge issue by adding Verified with: The help output now includes: |
* chore(release): cut 0.9.4 CHANGELOG entry for 0.9.4 covers all 7 PRs merged since v0.9.3: - #974 SKILL_BUNDLE day-0 install parity (Added) - #954 automate apm-triage-panel workflow (Added) - #970 python-architect mermaid classDiagram trap (Changed) - #911 REQUESTS_CA_BUNDLE TLS validation (Fixed) - #971 triage-panel project-sync dispatch (Fixed) - #910 CLI consistency cleanup (Fixed) - #958 issue templates label taxonomy (Fixed) - #953 docs auto-deploy after bot-cut releases (Fixed) Open milestone 0.9.4 issues (41) reassigned to 0.9.5. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore(changelog): tighten 0.9.4 entries (so-what for developers) Refactor per Keep-a-Changelog spirit: lead with developer impact, trim agent-internals prose, group maintainer-only changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore(changelog): add #660 install.sh air-gapped entry to 0.9.4 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fixed bugs and improved perf and docs * fix(cli): restore mcp install passthrough forwarding * fix(cli): resolve runtime remove merge conflict * fix(cli): document --mcp-version in mcp install help --------- Co-authored-by: Daniel Meppiel <51440732+danielmeppiel@users.noreply.github.com>
* chore(release): cut 0.9.4 CHANGELOG entry for 0.9.4 covers all 7 PRs merged since v0.9.3: - #974 SKILL_BUNDLE day-0 install parity (Added) - #954 automate apm-triage-panel workflow (Added) - #970 python-architect mermaid classDiagram trap (Changed) - #911 REQUESTS_CA_BUNDLE TLS validation (Fixed) - #971 triage-panel project-sync dispatch (Fixed) - #910 CLI consistency cleanup (Fixed) - #958 issue templates label taxonomy (Fixed) - #953 docs auto-deploy after bot-cut releases (Fixed) Open milestone 0.9.4 issues (41) reassigned to 0.9.5. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore(changelog): tighten 0.9.4 entries (so-what for developers) Refactor per Keep-a-Changelog spirit: lead with developer impact, trim agent-internals prose, group maintainer-only changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore(changelog): add #660 install.sh air-gapped entry to 0.9.4 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Fixes the CLI consistency issues reported on 2026-04-24 for APM 0.9.2. #903
Implemented
apm experimental <subcommand> --helpsolist,enable,disable, andresetshow their own subcommand-specific help.apm mcp install --helpto includeNAMEand supported MCP install options.apm run scriptoutput from a leading space to[>] Running script: ....apm pack --dry-runandapm unpack --dry-runhelp text.-yshort alias forapm runtime remove --yes.apm outdateddescription.apm experimentalsection todocs/src/content/docs/reference/cli-commands.md.uv.lockaligned withpyproject.tomlpackage version0.9.2.Fixes #
Type of change
Testing
Test commands
Docs validation
npm run build
Result: docs build completed successfully and internal links are valid.
Manual CLI verification