Skip to content

fix(mcp): clarify install help forwarded options#2018

Merged
danielmeppiel merged 2 commits into
microsoft:mainfrom
WilliamK112:codex/apm-mcp-install-help-2006
Jul 4, 2026
Merged

fix(mcp): clarify install help forwarded options#2018
danielmeppiel merged 2 commits into
microsoft:mainfrom
WilliamK112:codex/apm-mcp-install-help-2006

Conversation

@WilliamK112

Copy link
Copy Markdown
Contributor

Summary

Part of #2006.

  • move the forwarded apm install --mcp option summary into the apm mcp install command description instead of rendering it after the Options: section
  • rename the note to "Forwarded install options" so it is clear these flags are forwarded, not Click-registered on the alias command
  • add regression assertions in both MCP command test suites so forwarded options stay out of the Click Options: section

Validation

  • .venv\Scripts\python.exe -m pytest tests\integration\test_commands_mcp_phase3c.py::TestMcpGroupHelp::test_mcp_install_help tests\integration\test_commands_mcp_flow.py::TestMcpGroupHelp::test_mcp_install_help -q
  • .venv\Scripts\python.exe -m pytest tests\integration\test_commands_mcp_phase3c.py tests\integration\test_commands_mcp_flow.py -q
  • .venv\Scripts\python.exe -m pytest tests\unit\test_cli_consistency.py -q
  • .venv\Scripts\python.exe -m ruff check src\apm_cli\commands\mcp.py tests\integration\test_commands_mcp_phase3c.py tests\integration\test_commands_mcp_flow.py
  • .venv\Scripts\python.exe -m ruff format --check src\apm_cli\commands\mcp.py tests\integration\test_commands_mcp_phase3c.py tests\integration\test_commands_mcp_flow.py
  • .venv\Scripts\apm.exe mcp install --help
  • git diff --check origin/main...HEAD

Copilot AI review requested due to automatic review settings July 4, 2026 12:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@danielmeppiel danielmeppiel added status/accepted Direction approved, safe to start work. status/shepherding Actively being driven by an APM shepherd run labels Jul 4, 2026
@danielmeppiel danielmeppiel self-assigned this Jul 4, 2026
Align docs/reference/cli/mcp.md terminology with the CLI help text
changed in the parent commit. The CLI now says 'Forwarded install
options'; the reference page still said 'Common forwarded options'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: ship_now

Unanimous net-positive: help-text placement fix is correct, improves UX, and introduces no regressions. Ship now.

cc @WilliamK112 -- a fresh advisory pass is ready for your review.

All active panelists converge on a clean net-win verdict. The epilog-to-description migration uses Click's \b escape correctly (python-architect), eliminates the native-vs-forwarded confusion that tripped users (devx-ux-expert), introduces zero supply-chain or auth surface changes (supply-chain-security-expert), and ships with adequate regression coverage (test-coverage-expert). No panelist raised a blocking or recommended-severity finding.

Against the maintainer reservation -- net-win on every angle -- the scorecard reads: correctness=win (Click API usage validated), UX=win (scan-order improvement, better label), security=neutral (pure static text), maintainability=neutral-to-positive (no new abstraction debt), docs=win. The single terminology-drift nit (docs still said "Common forwarded options") was folded into this shepherd run (commit 11de488) -- the reference page now says "Forwarded install options" to match the CLI.

No specialist disagreements exist. The devx-ux-expert and oss-growth-hacker both flagged "Forwarded" as mildly jargon-y, but both concluded the parenthetical cross-reference compensates adequately. The CEO concurs: the term teaches the composition model rather than hiding it.

Aligned with: pragmatic_as_npm (help text that surfaces forwarded options inline matches the zero-surprise CLI promise); oss_community_driven (bounded community fix, clean scope, adequate tests -- exactly the kind of improvement that keeps contributors engaged).

Growth signal. Help text that explains command composition ('forwarded') teaches the CLI mental model in situ -- compounds positively with quickstart docs and reinforces APM's composable-commands narrative.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 1 Clean epilog-to-help migration uses Click \b escape correctly; no architectural concerns, net UX improvement.
CLI Logging Expert 0 0 0 Clean net-positive: correct Click \b usage, better placement (help vs epilog), and more accurate label. No regressions.
DevX UX Expert 0 0 1 Moving forwarded-options block from epilog to description is a clear UX win; eliminates native-vs-forwarded confusion.
Supply Chain Security Expert 0 0 0 Pure static help-text rewording; no runtime, dependency, auth, or path changes. No supply-chain surface affected.
OSS Growth Hacker 0 0 1 Small net win: 'Forwarded install options' teaches the command-composition mental model and the cross-ref drives discoverability.
Doc Writer 0 0 3 Label change accurate and clearer; terminology drift in docs folded (commit 11de488 syncs reference page).
Test Coverage Expert 0 0 0 Help-text-only change ships with adequate regression tests; mutation-break analysis passes; no critical-promise surface touched.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top follow-ups

  1. [DevX UX Expert] Consider whether 'Forwarded' label needs further clarification for day-one users -- minor jargon concern; parenthetical compensates today but worth revisiting if onboarding friction surfaces in issues.

Shepherd actions taken

  • Folded: docs terminology sync -- docs/src/content/docs/reference/cli/mcp.md updated from "Common forwarded options" to "Forwarded install options" (commit 11de488, pushed to author fork).
  • Deferred: test-assertion deduplication across identical test files (pre-existing condition; would require broader refactor of two 2040-line files; scope-crossing).

Recommendation

All angles satisfy the maintainer's net-win reservation. Correctness, UX, security, maintainability, and docs are each neutral-to-positive with no regressions. The single terminology-drift nit is now resolved. Ship now.


Full per-persona findings

Python Architect

  • [nit] Identical test assertions duplicated across two integration test files at tests/integration/test_commands_mcp_flow.py
    These two files are pre-existing identical files. If the help text changes again, both must be updated. A shared helper would reduce maintenance surface. Pre-existing condition; not introduced by this PR.
    Suggested: Extract _assert_forwarded_options_in_description(output) into a shared test utilities module.

CLI Logging Expert

No findings.

DevX UX Expert

  • [nit] 'Forwarded' is slightly jargon-y for first-encounter users at src/apm_cli/commands/mcp.py:95
    A brand-new user may not immediately parse 'Forwarded install options' as meaning 'these flags get passed through to apm install'. The parenthetical cross-reference compensates well, so this is cosmetic at most.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

  • [nit] Consider whether 'Forwarded' is the friendliest word for a day-one user at src/apm_cli/commands/mcp.py:95
    Parenthetical compensates today. No action required; flagged for awareness.

Auth Expert -- inactive

PR touches only src/apm_cli/commands/mcp.py (help text) and test files; no auth, token, or credential resolution surfaces affected.

Doc Writer

  • [nit] Terminology drift between CLI help and reference docs -- FOLDED in this shepherd run
    docs/reference/cli/mcp.md still said 'Common forwarded options' while CLI now says 'Forwarded install options'. Fixed in commit 11de488.
  • [nit] Cross-reference is accurate and improved -- docs/reference/cli/install.md confirms all options listed are documented.
  • [nit] Epilog-to-help structural move improves discoverability; no additional doc change needed beyond the label sync.

Test Coverage Expert

No findings.

Performance Expert -- inactive

PR touches only src/apm_cli/commands/mcp.py (help text strings) and test files; no cache, deps, install pipeline, or transport paths affected.

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

@danielmeppiel danielmeppiel removed the status/shepherding Actively being driven by an APM shepherd run label Jul 4, 2026

@danielmeppiel danielmeppiel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as maintainer: triaged via apm-issue-autopilot, driven to green CI, advisory review panels clean. Batch-merge authorized.

@danielmeppiel danielmeppiel merged commit bc2662f into microsoft:main Jul 4, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/accepted Direction approved, safe to start work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants