fix(cli): skip update check for unknown subcommands to fail fast#1541
fix(cli): skip update check for unknown subcommands to fail fast#1541maro114510 wants to merge 3 commits into
Conversation
|
@microsoft-github-policy-service agree |
maro114510
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the CLI’s update-notification behavior so it only runs for recognized subcommands, and adds integration tests to verify update checks are skipped for invalid/unknown commands.
Changes:
- Gate
_check_and_notify_updates()behind a “known subcommand” check in the CLI entrypoint. - Add integration tests covering invalid command, valid command, and no-subcommand scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/integration/test_version_notification.py | Adds integration tests ensuring the update check is skipped for invalid/unknown commands and when no subcommand is provided. |
| src/apm_cli/cli.py | Changes update-check gating logic to run only when a recognized subcommand is invoked. |
| # Check for updates only for known commands; skip on invalid input to fail fast. | ||
| if not ctx.resilient_parsing and ctx.invoked_subcommand in cli.commands: | ||
| _check_and_notify_updates() |
…S discipline Dispatched v0.3+ panel against microsoft/apm#1541 (+41/-2, 2 files, small CLI fix). Results: - Executor cost: ~$0.21 (vs $2.85 on PR #1424) - Per-kLoC: $5.12 (vs $1.15 baseline) — fixed Sonnet-executor overhead dominates at small scale (~93% of total cost) - Panel cost shape holds in dollar terms; per-kLoC ratio inverted - Arbiter trigger correctly did NOT fire (0 BLOCKERs) KEY EMPIRICAL VALIDATION of v0.3.4 PER-LENS DIFFERENTIATION: The executor reflected per-lens against the CAPABILITY PROFILE template and concluded 4/5 lenses genuinely TRIVIAL, but security lens was INADEQUATE on TRIVIAL/Haiku — it surfaced a real MEDIUM bypass concern but could not validate it without out-of-diff function body access. This empirically generates the per-element justification the v0.3.4 corpus requires architects to record at design time. Recommended carve-out: 'Security lens uses Haiku when all referenced functions are in-diff; escalates to REVIEWER with tool access when it must reason about out-of-diff internals.' Implication for PR #1424: security lens was likely mis-bound to TRIVIAL. The blocker false-positive (_substitute_plugin_root alleged undefined, refuted only via out-of-diff gh api lookup) is consistent with TRIVIAL- class inadequacy on cross-file reasoning. A v0.3.4 re-architect would correctly bind security to REVIEWER, expected cost delta +$0.50-1.00 per run with measurable security finding fidelity improvement. REPORT updated with multi-scenario section (small-PR + different-skill). Deferral list narrowed: full S1-S5 × {v0.2,v0.3+} matrix remains follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
added
the
panel-review
|
@maro114510 thank you for your contribution. Could you please check Copilot's feedback for this PR? I also requested an APM Panel Review, which might provide extra feedback that needs fixing (focus only on blocking). Finally, make sur ewe get a green on all CI tasks. Once everythins is fixed will approve. |
Addresses Copilot's feedback: routes the known-command check through Click's public resolution API rather than inspecting the internal commands dict directly, making the guard robust to future dynamic command loading.
Head branch was pushed to by a user without write access
|
Thanks for the review!
|
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Correct minimal guard clause preventing a spurious network call on unknown/bare commands; one recommended idiom improvement and one comment nit. |
| CLI Logging Expert | 0 | 0 | 3 | Output hygiene improvement is correct -- silencing the update banner before an error message is the right design; no findings block merge. |
| DevX UX Expert | 0 | 1 | 1 | Correct fail-fast behavior on invalid commands; the bare apm no-subcommand case silently also drops the update check -- defensible but undocumented. |
| Supply Chain Security Expert | 0 | 0 | 2 | Change is security-neutral to security-positive: narrows the update-check attack surface to recognized commands; no new vulnerabilities introduced. |
| OSS Growth Hacker | 0 | 0 | 1 | Net-positive UX fix: removes confusing update-nudge/error collisions on invalid input; real-work commands retain full nudge coverage, so upgrade conversion is unaffected. |
| Doc Writer | 0 | 1 | 1 | Fallback self-check fired: the Unreleased CHANGELOG block has no entry for this user-visible behavior fix, and the self-update docs may need precision. |
| Test Coverage Expert | 0 | 1 | 2 | Two of three new tests correctly defend real user promises at integration tier; the unknown-command test may be tautological if Click raises UsageError before the group callback fires. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add a unit test at
tests/unit/test_cli_update_check_gate.pythat constructs a mock Click context withctx.invoked_subcommand='nonexistent'and directly exercises the guard, bypassing Click dispatch -- The integration test for the unknown-command case may pass vacuously if Click 8 raisesUsageErrorbefore the group callback fires; a unit test makes the guard's correctness unambiguous and independent of Click routing internals. - [Doc Writer] Add a
### Fixedentry to the[Unreleased]CHANGELOG block: "Update notifications now appear only during recognized command invocations -- not on help display or mistyped commands. (fix(cli): skip update check for unknown subcommands to fail fast #1541)" -- The[Unreleased]block is empty; this is a user-observable behavior change and P6 requires a changelog line. - [Python Architect] Replace
cli.commandswithctx.command.commandson line 146 ofsrc/apm_cli/cli.py-- Zero-cost idiomatic Click swap;ctx.command.commandsalways refers to the live group object dispatching the current invocation, eliminating a stale-closure risk if the cli group is reconstructed in a future test harness. - [DevX UX Expert] Update the inline comment on lines 145-146 to explicitly document that bare
apm(help display) now skips the update check and that this is intentional -- Without an explicit comment, a future maintainer may treat the bare-invocation skip as a bug and revert it. - [CLI Logging Expert] Tighten test assertions: replace
assertIn('invalid', result.output)withassertIn("No such command 'invalid'", result.output), and addassertIn('Usage:', result.output)to the bare-invocation test -- The broad assertion passes on any output containing 'invalid'; the help-text assertion ensures a broken help path would not pass silently.
Architecture
classDiagram
direction LR
class cli_module {
<<Module>>
}
class helpers_module {
<<Module>>
}
class cli_group {
<<ClickGroup>>
+commands dict
+callback(ctx, verbose) None
}
class ClickContext {
<<ValueObject>>
+resilient_parsing bool
+invoked_subcommand str
+command Group
}
class _check_and_notify_updates {
<<Pure>>
+call() None
}
class check_for_updates {
<<IOBoundary>>
+call(current_version str) str
}
cli_module *-- cli_group : defines
helpers_module *-- _check_and_notify_updates : defines
helpers_module *-- check_for_updates : defines
cli_group ..> ClickContext : receives
cli_group ..> _check_and_notify_updates : calls when guard passes
_check_and_notify_updates ..> check_for_updates : delegates
class cli_group:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
User(["apm CMD"]) --> GrpCB["cli group callback\ncli.py:128"]
GrpCB --> G1{"ctx.resilient_parsing?"}
G1 -- Yes --> SkipA["skip update check"]
G1 -- No --> G2{"ctx.invoked_subcommand\nin cli.commands?\ncli.py:146"}
G2 -- "No: None or unknown string" --> SkipB["skip update check"]
G2 -- "Yes: known command" --> CANU["_check_and_notify_updates\n_helpers.py"]
CANU --> G3{"is_self_update_enabled?\nAPM_E2E_TESTS set?"}
G3 -- "disabled or E2E" --> SkipC["silent return"]
G3 -- enabled --> NET["check_for_updates\nversion_checker.py"]
NET -- "latest_version" --> Warn["print update notice\nclick.echo to stdout"]
NET -- "None or raises" --> SkipC
Warn --> Dispatch["Click: dispatch subcommand"]
SkipA --> Dispatch
SkipB --> Dispatch
SkipC --> Dispatch
Dispatch --> Known{"known command?"}
Known -- Yes --> RunCmd["execute subcommand"]
Known -- No --> ErrOut["UsageError: No such command X\nexit code 2"]
sequenceDiagram
participant U as User
participant CB as cli callback
participant UPD as _check_and_notify_updates
participant NET as GitHub API
participant CK as Click dispatcher
U->>CB: apm invalid
Note over CB: invoked_subcommand = invalid
Note over CB: GUARD: invalid not in cli.commands -- skip
CB->>CK: update check bypassed
CK-->>U: Error No such command invalid [exit 2]
U->>CB: apm init test-project
Note over CB: invoked_subcommand = init
Note over CB: GUARD: init in cli.commands -- proceed
CB->>UPD: _check_and_notify_updates()
UPD->>NET: check_for_updates(current_version)
NET-->>UPD: latest_version or None
UPD-->>CB: returns
CB->>CK: invoke init subcommand
CK-->>U: init output [exit 0]
Recommendation
No panelist raised a blocking concern; zero findings at blocking severity. The core change is correct, aligned with pragmatic-as-npm conventions, and supported by all active specialists. The two highest-value pre-merge actions are the CHANGELOG entry (P6, trivial) and a unit test for the guard clause (test-coverage-expert structural concern). The ctx.command.commands idiom swap, comment rewrite incorporating output-hygiene framing, and test assertion tightening can land in this PR or an immediate follow-up -- they do not gate merge. This is an external-contributor PR and P7 applies: move it.
Full per-persona findings
Python Architect
-
[recommended] Use
ctx.command.commandsinstead ofcli.commandsinside the group callback atsrc/apm_cli/cli.py:146
cli.commandsworks correctly today becausecli.add_command()calls at module load time populate the dict before any invocation. However, referencing the module-level namecliinside its own callback creates an implicit self-referential closure: ifcliis reconstructed or aliased in a test harness, the guard silently reads stale membership from the original object.ctx.command.commandsaccesses the identical dict through the Click context, which always refers to the live group object dispatching the current invocation. This is the idiomatic Click pattern and is a zero-cost one-word swap.
Suggested:if not ctx.resilient_parsing and ctx.invoked_subcommand in ctx.command.commands: -
[nit] Comment conflates two semantically distinct skip reasons at
src/apm_cli/cli.py:145
The comment reads "skip on invalid input to fail fast." A bareapminvocation (ctx.invoked_subcommand is None) is not invalid input -- it is the canonical way to show help. Conflating it with unknown commands makes the intent harder to reason about in a future audit of this guard.
Suggested: Split into three lines explaining unknown-command vs. bare-invocation separately.
CLI Logging Expert
-
[nit] Comment says "fail fast" but the motivation is output hygiene, not speed at
src/apm_cli/cli.py:145
The update check skip is not about making the error path faster -- the check is non-blocking and adds negligible latency. The real motivation is: printing an unrelated update banner before Click's error message is confusing noise.
Suggested:# Check for updates only for known commands; skip on error/help paths to keep output clean. -
[nit]
test_no_update_check_on_unknown_commandassertion is too broad attests/integration/test_version_notification.py:80
assertIn('invalid', result.output)passes as long as 'invalid' appears anywhere. A tighter assertion documents the Click error contract explicitly.
Suggested:self.assertIn("No such command 'invalid'", result.output) -
[nit]
test_no_update_check_without_subcommandmakes no assertion that help still renders attests/integration/test_version_notification.py:97
A broken help path (exception swallowed by CliRunner) would still pass.
Suggested: Add:self.assertIn('Usage:', result.output)
DevX UX Expert
-
[recommended] Bare
apm(no subcommand) now permanently skips the update check -- intent should be explicit atsrc/apm_cli/cli.py:146
ctx.invoked_subcommand in cli.commandsreturnsFalsefor both an invalid command AND forNone(bareapm). The bare-invocation case is a silent behavior change: a user who runsapmto browse help will never see an update banner. This is arguably the right call (npm, pip, cargo do not make network calls on bare help), but it should be explicitly acknowledged in the comment or PR description.
Suggested: Update comment to explicitly acknowledge that bareapmalso skips the update check and that this is intentional. -
[nit]
test_update_check_runs_on_valid_commandmissing exit-code assertion attests/integration/test_version_notification.py:83
Ifinitfails silently in the isolated filesystem, the test still passes.
Suggested: Addself.assertEqual(result.exit_code, 0, result.output)before the mock assertion.
Supply Chain Security Expert
-
[nit] Pre-existing: broad exception catch silently swallows TLS errors in the update check at
src/apm_cli/commands/_helpers.py
The bareexcept Exception: passin_check_and_notify_updates()swallows TLS errors. This PR does not introduce or worsen this -- it reduces call frequency. A future issue to narrow exception scope would be appropriate. -
[nit] Pre-existing:
APM_REPOenv var can redirect update-check URL to an arbitrary GitHub repo atsrc/apm_cli/utils/version_checker.py
_get_air_gap_repo()readsAPM_REPOfrom env. Impact is constrained by semver validation on the returnedtag_name. This PR does not introduce or worsen this.
OSS Growth Hacker
- [nit] CHANGELOG framing opportunity
Frame this as a UX improvement: "Update notifications now appear only during real command invocations -- not on help display or mistyped commands." That framing signals APM's UX maturity and is shareable on its own merit.
Auth Expert -- inactive
PR #1541 modifies only the update-check gate condition in cli.py and its integration test -- no auth-related files are touched, and the change does not affect authentication behavior, token management, credential resolution, host classification, or remote-host fallback semantics.
Doc Writer
-
[recommended] Missing CHANGELOG entry for user-visible behavior change at
CHANGELOG.md
The[Unreleased]section is empty. This PR makes a user-observable behavioral change: the startup update notification is no longer emitted when an unknown or invalid command is invoked. Per the repo's Keep-a-Changelog discipline, all user-observable fixes belong in### Fixed.
Suggested: "- The startup update notification is no longer shown when an unknown command is invoked. APM now skips the version check on unrecognized input, so error output is not preceded by an unrelated notification. (fix(cli): skip update check for unknown subcommands to fail fast #1541)" -
[nit]
docs/src/content/docs/reference/cli/self-update.mdwording is loose for the new gating condition
The phrase "during normal command execution" is loose enough not to be wrong, but a reader consulting this section would benefit from precision about when the check is skipped.
Suggested: "APM checks for new releases at most once per day when a recognized command runs. The check is skipped on unknown commands and bare invocations."
Test Coverage Expert
-
[recommended]
test_no_update_check_on_unknown_commandmay be tautological: Click 8 can raiseUsageErrorbefore group callback fires attests/integration/test_version_notification.py:72
For the guard to defend the unknown-command promise, Click must call the group callback WITHctx.invoked_subcommandset to the unrecognised name BEFORE raisingUsageError. Click 8'sMultiCommand.invoke()may raiseUsageErrorbefore the group callback executes. If so, both old and new code produce identical output andmock_check.assert_not_called()passes trivially in EITHER version. Test outcome: unknown (pytest not available in review env).
Suggested: Addtests/unit/test_cli_update_check_gate.pythat constructs a mock Click context withctx.invoked_subcommand='nonexistent'and directly exercises the guard.
Proof (unknown at integration-with-fixtures):tests/integration/test_version_notification.py::TestUpdateCheckSkippedOnInvalidCommand.test_no_update_check_on_unknown_command-- proves: An unknown CLI command does not trigger an update-check network call [devx] -
[nit]
test_no_update_check_without_subcommanddoes not document that bare-apm skipping update check is intentional attests/integration/test_version_notification.py:92
Before this PR, bareapmDID trigger the update check. A comment prevents a future maintainer from reverting it.
Suggested: Add:# Intentional: bare 'apm' (help display) no longer pays the update-check network cost. -
[nit] Patch target
apm_cli.cli._check_and_notify_updatesis correct -- patched at the import-site lookup incli.pyattests/integration/test_version_notification.py:71
Pertests.instructions.md, patch where the name is looked up. The new tests do this correctly. Positive observation; no change needed.
Performance Expert -- inactive
This PR touches only the CLI startup guard for invalid commands -- it does not change any dependency resolution, fetch, materialize, verify, cache, transport, or parallelism path.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1541 · sonnet46 11.8M · ◷
github-actions
Bot
removed
the
panel-review
Thank you for maintaining this project. I opened this PR to fix a small issue I noticed.
Description
The update check ran in the CLI group handler, which Click invokes before resolving the subcommand name.
As a result, even a typo like
apm updsatewould block on a GitHub API call (synchronous, up to 2 s timeout) before theNo such commanderror was shown.The docstring said "non-blockingly" but the call was fully synchronous.
The guard
ctx.command.get_command(ctx, ctx.invoked_subcommand) is not Noneroutes through Click's public command-resolution API (Group.get_command), which is the same mechanism Click uses internally when dispatching subcommands. This makes the guard robust to future plugin or lazy-loaded commands, while keeping the logic consistent with Click's own resolution path.One side effect: bare
apmwith no subcommand also skips the check now (invoked_subcommandisNone), which is intentional since there's nothing to execute.Type of change
Testing