fix: add --target flag to apm update command#1358
fix: add --target flag to apm update command#1358sergio-sisternes-epam wants to merge 2 commits into
Conversation
The update command lacked a --target option, forcing users without copilot-instructions.md to have no way to set the deployment target. Add --target/-t with TargetParamType (matching install.py pattern) and thread it through _run_dep_update to _install_apm_dependencies, which already accepted the parameter. The back-compat shim path intentionally does not forward target because self-update has no target concept. Closes microsoft#1297 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
Adds an explicit --target/-t option to apm update so users can override target auto-detection / apm.yml targets when refreshing dependencies, aligning update with the existing install target-resolution pattern.
Changes:
- Add
--target/-t(usingTargetParamType()) to theapm updateClick command. - Thread the parsed
targetvalue through_run_dep_update()into_install_apm_dependencies(). - Add unit tests asserting
--targetforwarding behavior (long + short forms, defaultNone, and composition with--yes).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/apm_cli/commands/update.py |
Adds --target/-t option and forwards it into the install pipeline during dependency updates. |
tests/unit/commands/test_update_command.py |
Adds tests covering apm update --target parsing and propagation to _install_apm_dependencies. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean pattern-match to install.py; keyword-only signature preserves back-compat; no architectural concerns. Ship. |
| CLI Logging Expert | 0 | 1 | 2 | Help text omits valid target values; module-level Flags doc not updated; no verbose trace for resolved target. No logging regressions introduced. |
| DevX UX Expert | 0 | 2 | 1 | Good flag parity; help text strips the valid-values list users need to self-serve, and the no-apm.yml path silently swallows --target. |
| Supply Chain Security Expert | 0 | 0 | 0 | No supply-chain risk: TargetParamType enforces an allowlist via parse_target_field before the value reaches the install pipeline; no path, shell, or credential surface opened. |
| OSS Growth Hacker | 0 | 1 | 1 | Flag parity closes a real bare-project friction gap; no CHANGELOG entry means the fix ships silently -- add one to convert existing users. |
| Doc Writer | 0 | 3 | 0 | New --target/-t flag on apm update is undocumented: reference page and commands skill both need updating; CHANGELOG entry missing. |
| Test Coverage Expert | 0 | 1 | 0 | 4 new unit tests verify --target forwarding; integration-with-fixtures tier gap exists for the new flag; one recommended finding. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [DevX UX Expert] Emit a _rich_warning when --target is passed but the shim path activates (no apm.yml found); currently the flag is silently dropped. -- Silent flag drops are a UX lie: users get no feedback that --target was ignored. One warning line fixes the contract.
- [CLI Logging Expert + DevX UX Expert] Append the accepted-values clause from install.py's --target help text to update.py's help string. -- Two independent specialists flagged this; a user running
apm update --helpcannot self-serve valid target values. Copy the Values: ... sentence verbatim. - [Doc Writer + OSS Growth Hacker] Add CHANGELOG [Unreleased] entry and update docs/reference/cli/update.md Options table and packages/apm-guide commands.md to document --target/-t. -- Three surfaces (CHANGELOG, reference page, skill doc) are all missing the new flag; existing users who hit issue [BUG] No way to set target for apm update #1297 need a changelog entry to know to upgrade.
- [Test Coverage Expert] Add one integration-with-fixtures test to tests/integration/test_update_e2e.py:
apm update --dry-run --target claudeasserts exit_code==0. -- CLI command surface changes require integration-with-fixtures tier per the test-floor matrix; the 4 unit tests mock the boundary and cannot certify the end-to-end parse-validate-route chain. Evidence outcome: missing. - [DevX UX Expert] Update module docstring line ~25 which claims 'Other apm install flags are NOT mirrored here on purpose' -- --target was just mirrored. -- Stale inline docs mislead future contributors about intentionality; a one-line fix.
Architecture
classDiagram
direction LR
class TargetParamType {
<<click.ParamType>>
+name str
+convert(value, param, ctx) str
}
class update {
<<ClickCommand>>
+assume_yes bool
+dry_run bool
+verbose bool
+check_only bool
+target str | None
}
class _run_dep_update {
<<Pure>>
+assume_yes bool
+dry_run bool
+verbose bool
+project_root Path | None
+target str | None
}
class _install_apm_dependencies {
<<IOBoundary>>
+apm_package APMPackage
+update_refs bool
+target str | None
+plan_callback callable
}
class InstallRequest {
<<ValueObject>>
+target str | None
}
class InstallService {
<<ApplicationService>>
+run(request) InstallResult
}
class InstallLogger {
<<CommandLogger>>
+verbose bool
+dry_run bool
}
class UpdatePlan {
<<ValueObject>>
+entries tuple
+has_changes bool
}
update ..> TargetParamType : validated by
update ..> _run_dep_update : delegates to
_run_dep_update ..> _install_apm_dependencies : calls
_run_dep_update *-- InstallLogger : creates
_install_apm_dependencies ..> InstallRequest : builds
_install_apm_dependencies ..> InstallService : delegates to
_install_apm_dependencies ..> UpdatePlan : returns via plan_callback
class update:::touched
class _run_dep_update:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm update --target claude"]) --> B["update() click handler"]
B --> C{"TargetParamType.convert()"}
C -->|valid target str| D["_find_apm_yml()"]
C -->|invalid value| E["click.UsageError -- bad target"]
D -->|no apm.yml| F["back-compat shim -- target NOT forwarded"]
D -->|found| G["_run_dep_update target=target"]
G --> H["APMPackage.from_apm_yml()"]
H --> I["_install_apm_dependencies update_refs=True target=target"]
I --> J["InstallService.run InstallRequest target=target"]
J --> K["_plan_callback UpdatePlan"]
K -->|no changes| L["already up to date"]
K -->|dry_run| M["dry run exit"]
K -->|assume_yes or TTY confirm| N["apply install pipeline"]
N --> O(["Updated N deps"])
Recommendation
Merge this PR. The implementation is clean, the security surface is closed by the existing TargetParamType allowlist, and the fix resolves a real bare-project onboarding gap with four unit tests from an external contributor. Track the followups above -- particularly the silent --target drop on the shim path (devx-ux-expert) and the CHANGELOG/docs gap (doc-writer + oss-growth-hacker) -- and land them before the next changelog-bearing release. The integration-with-fixtures test gap (test-coverage-expert, evidence: missing) is the highest-signal item to close post-merge; it should be a fast follow in the same release window.
Full per-persona findings
Python Architect
- [nit] Redundant 'target' dest name in
@click.option decorator atsrc/apm_cli/commands/update.py:124
The third positional arg 'target' to@click.option is the Click dest override. When it equals the long-option name stripped of dashes, it is redundant and adds visual noise. install.py uses the same style, so it is a codebase-wide convention; fix only if the team standardises on dropping the explicit dest.
CLI Logging Expert
-
[recommended] --target help text omits valid values enumeration present in install.py at
src/apm_cli/commands/update.py
The install.py --target option help text enumerates all accepted values (copilot, claude, cursor, opencode, codex, gemini, windsurf, agent-skills, all) and explains special-case semantics. The update.py help text stops after the resolution chain note and provides no hint of what values are accepted. A user running 'apm update --help' gets no signal that '--target florp' is invalid until runtime.
Suggested: Append the accepted-values sentence from install.py to the help string: "Values: copilot, claude, cursor, opencode, codex, gemini, windsurf, agent-skills, all." -
[nit] Module-level Flags section does not document --target at
src/apm_cli/commands/update.py
The module docstring lists --yes, --dry-run, and --verbose under a Flags section but --target is absent after this PR. -
[nit] No verbose-mode trace message when --target is explicitly provided at
src/apm_cli/commands/update.py
install.py emits a verbose diagnostic showing the resolved target before the pipeline runs. update.py passes target silently with no log line under --verbose.
Suggested: In _run_dep_update:if verbose and target: _rich_info(f"Targeting harness: {target}", symbol="info")
DevX UX Expert
-
[recommended] update --help omits the valid-values enumeration that install --help surfaces at
src/apm_cli/commands/update.py:129
install.py's --target help text enumerates every accepted value plus semantics for 'agent-skills' and 'all'. update.py's text stops after the resolution-chain hint. A user who has never run apm install first cannot self-serve from 'apm update --help' alone -- they must go to docs or try-and-error a TargetParamType validation error.
Suggested: Copy the Values: ... clause from install.py's --target help string verbatim into update.py's help. The two help strings should be identical from 'Values:' onward. -
[recommended] --target is silently ignored when no apm.yml is found and the shim path activates at
src/apm_cli/commands/update.py:157
When _find_apm_yml() returns None the command invokes the back-compat self-update shim. The 'target' variable is in scope but never forwarded, and no warning is emitted. A user running 'apm update --target claude' outside a project gets the shim output with no indication that their flag was dropped.
Suggested: Before invoking _self_update_cmd:if target is not None: _rich_warning("--target is only valid inside an apm.yml project; ignoring for the self-update shim.") -
[nit] Module docstring 'Other apm install flags are NOT mirrored here on purpose' is now stale at
src/apm_cli/commands/update.py:25
--target was just mirrored from install. The docstring should note --target was added or drop the claim.
Suggested: Change to 'Only --target is mirrored from apm install; other install flags remain intentionally absent.'
Supply Chain Security Expert
No findings.
OSS Growth Hacker
-
[recommended] No CHANGELOG entry -- fix ships invisibly to existing users who hit this exact gap at
CHANGELOG.md
Users running bare .github copilot projects who stumbled on this missing flag will not know to upgrade unless there is a CHANGELOG entry. The PR body describes a maintainer-confirmed need, which is exactly the kind of user-pain story that converts 'I should upgrade' to 'I am upgrading now'.
Suggested: Add under ### Added: '-apm updatenow accepts--target / -tto set the deployment harness, matchingapm install(fixes bare-.github copilot projects with no copilot-instructions.md).' -
[nit] Help text could name the bare-project scenario explicitly to aid discoverability at
src/apm_cli/commands/update.py:128
Suggested: Append to help string: 'Required when no copilot-instructions.md is present and auto-detect cannot infer a target.'
Auth Expert -- inactive
PR only adds a --target flag to the update command and threads it through to _install_apm_dependencies; no auth, token management, credential resolution, host classification, or remote-host fallback code is touched.
Doc Writer
-
[recommended] docs/reference/cli/update.md Options table is missing --target/-t at
docs/src/content/docs/reference/cli/update.md
The reference page is the single source of truth for apm update options. Its Options table only lists --yes/-y, --dry-run, and --verbose/-v. A user reading the docs will not know --target exists.
Suggested: Add row:| --target, -t | all | Comma-separated list of deployment targets. Follows the same resolution chain as apm install --target. | -
[recommended] packages/apm-guide/.apm/skills/apm-usage/commands.md apm update row omits --target at
packages/apm-guide/.apm/skills/apm-usage/commands.md
The commands.md skill document describes apm update as accepting only --yes, --dry-run, --verbose. Any agent or tooling consuming this skill file will give users an incomplete picture.
Suggested: Extend the apm update flags cell to include--target(comma-separated, e.g.--target claude,cursor). -
[recommended] CHANGELOG.md [Unreleased] section has no entry for the new --target flag at
CHANGELOG.md
Adding a flag to a CLI command is a user-facing Added item.
Suggested: Add under ### Added: '-apm updatenow accepts--target/-t(comma-separated target list) to scope dependency updates to specific deployment targets, matching theapm install --targetresolution chain. (fix: add --target flag to apm update command #1358)'
Test Coverage Expert
- [recommended] apm update --target has no integration-with-fixtures test; unit tests alone cannot certify the CLI command surface promise at the required tier floor at
tests/integration/test_update_e2e.py
The tier-floor matrix requires integration-with-fixtures for any CLI command surface change. The 4 new unit tests mock _install_apm_dependencies at the boundary. The existing test_update_e2e.py has 7 test functions but NONE invoke 'apm update --target'.
Suggested: Add a test to TestUpdateE2E:apm update --dry-run --target claudeagainst the temp_project fixture, asserts exit_code == 0. Gate with existing pytestmark skipif on GITHUB_APM_PAT/GITHUB_TOKEN.
Proof (missing at integration-with-fixtures):tests/integration/test_update_e2e.py::test_update_dry_run_with_target_flag-- proves: apm update --target claude parses, validates, and routes the target value through the full CLI pipeline without error [DevX]
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pypi.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "pypi.org"See Network Configuration for more information.
Generated by PR Review Panel for issue #1358 · ● 2.3M · ◷
github-actions
Bot
removed
the
panel-review
- Widen target type annotation to str | list[str] | None - Add multi-target test case - Improve help text with valid target examples - Add shim-path warning when --target is silently dropped - Update module docstring - Add CHANGELOG entry - Remove redundant Click dest parameter Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Panel feedback addressed in
|
| Finding | Status | Fix |
|---|---|---|
| [R] Help text omits accepted-values enumeration | Fixed | Appended Values: copilot, claude, cursor, opencode, codex, gemini, windsurf, agent-skills, all to --target help |
| [R] Silent --target drop on shim path | Fixed | Added _rich_warning when --target is passed but no apm.yml found |
| [R] Missing CHANGELOG entry | Fixed | Added under [Unreleased] ### Added |
| [R] Docs/apm-guide commands.md missing --target | Fixed | Updated reference page and commands.md skill |
| [R] Integration test gap | Noted | Valid follow-up; unit tests cover the regression |
[N] TargetParamType type annotation too narrow |
Fixed | Widened to `str |
| [N] Module docstring stale claim | Fixed | Updated to note --target was added |
All Copilot review threads resolved. CI green, lint clean.
Description
The
apm updatecommand lacks a--targetoption. Users who start with a bare.githubcopilot project (nocopilot-instructions.md) have no way to set the deployment target when updating packages.Fix: Add
--target/-twithTargetParamType()to theupdatecommand, matching the pattern already used ininstall.py. Thread the parameter through_run_dep_update()to_install_apm_dependencies(), which already acceptstarget: str | None = None.Design decisions:
install.pypattern: same decorator shape, sameTargetParamType, same help text styletargetbecause self-update has no target concept_run_dep_update(target: str | None = None)so all existing callers remain unaffectedFixes #1297
Type of change
Testing
4 new tests in
tests/unit/commands/test_update_command.py: target forwarding, short flag, default None, and composition with--yes.