fix(install): preview positional packages in dry runs - #2664
fix(install): preview positional packages in dry runs#2664Daniel Meppiel (danielmeppiel) wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes apm install PACKAGE --dry-run so that validated positional package additions are reflected in the printed dry-run plan even when the existing apm.yml is not modified. It introduces an immutable ProspectiveInstallPlan as the single source of truth for dry-run rendering, orphan intent preview, and selected dependency counts, and adds tests + an architecture-boundary lint guard to prevent re-parsing in the renderer.
Changes:
- Add
ProspectiveInstallPlan(frozen dataclass) to represent dry-run preview state derived from manifest deps + validated positional additions. - Update the install command to carry validated additions into dry-run and route rendering/preflight/counts through the frozen plan.
- Add regression tests, an architecture boundary lint check + mutation test, and update CLI docs/usage guide to document non-persistence of positional additions.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_install_command.py | Adds regression test covering positional local package dry-run preview without mutating the project. |
| tests/unit/install/test_dry_run_render.py | Adapts legacy render fixtures to the new plan and adds plan count semantics test. |
| tests/unit/commands/test_install_context.py | Extends InstallContext field coverage to include validated_additions. |
| tests/integration/test_architecture_authorities.py | Adds authority/guard assertions and a mutation test to reject renderer-side reparsing. |
| src/apm_cli/install/presentation/dry_run.py | Updates dry-run renderer to accept a ProspectiveInstallPlan and consume plan-derived state. |
| src/apm_cli/install/dry_run_plan.py | Introduces the frozen ProspectiveInstallPlan and plan-derived counts/intended keys. |
| src/apm_cli/commands/install.py | Threads validated additions into dry-run plan construction and returns plan-selected counts. |
| scripts/lint-architecture-boundaries.sh | Adds a boundary check enforcing ProspectiveInstallPlan as the sole owner for dry-run preview state. |
| packages/apm-guide/.apm/skills/apm-usage/commands.md | Documents dry-run preview behavior for positional packages without persisting to existing manifests. |
| docs/src/content/docs/reference/cli/install.md | Updates --dry-run reference to clarify positional packages appear in preview but aren’t persisted to existing apm.yml. |
| .github/instructions/architecture.instructions.md | Adds canonical-owner table entry for the prospective dry-run plan. |
| .apm/instructions/architecture.instructions.md | Adds canonical-owner table entry for the prospective dry-run plan (APM-internal copy). |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
| if plan.should_install_apm and plan.apm_dependencies: | ||
| logger.progress(f"APM dependencies ({plan.apm_dependency_count}):") | ||
| for dep in plan.apm_dependencies: | ||
| action = "update" if update else "install" | ||
| logger.progress(f" - {dep.repo_url}#{dep.reference or 'main'} -> {action}") |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 2 | 1 | 0 | Keep #2664 on #2612, enforce one typed preview authority, and avoid claiming read-only behavior while cache writes remain. |
| CLI Logging Expert | 2 | 2 | 1 | Dry-run selection is not truthful: positional previews overcount, and --only lsp can diagnose unselected APM dependencies. |
| DevX UX Expert | 2 | 3 | 0 | Fix remaining dry-run mutations and validation drift, then narrow attribution to positional preview work. |
| Supply Chain Security Expert | 3 | 1 | 0 | Narrow to #2612, but fix dry-run writes and preserve exact dependency identity and selection. |
| OSS Growth Hacker | 0 | 2 | 0 | Preserve community credit by making #2664 a clearly sequenced #2612 follow-up to #2580 and #2592. |
| Doc Writer | 0 | 3 | 0 | Narrow the documentation and PR body to #2612; reserve #2550 and #2549 for community PRs #2580 and #2592. |
| Test Coverage Expert | 0 | 2 | 0 | The focused #2612 test passes; add the final-summary assertion and remove evidence overlapping #2580/#2592. |
| Performance Expert | 0 | 2 | 0 | Core #2612 adds no network work and only linear parsing; narrow the overlaps and avoid whole-suite guard runs. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Python Architect] (blocking-severity) Remove [BUG]
apm install --dry-runreports "No dependencies found in apm.yml" when onlydependencies.lspis declared #2550/[BUG]apm install --dry-runcreates~/.apm/apm.yml,config.json, andapm_modules/while reporting "no changes made" #2549 implementation and claims; rebase as a [BUG]apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612-only follow-up to fix: include LSP dependencies in install dry-run #2580 and fix: keep global install dry-run out of user state #2592. -- This avoids duplicate work, preserves contributor ownership, and keeps the change reviewable. - [Supply Chain Security Expert] (blocking-severity) Prevent changed-reference dry runs from persisting
apm.ymland add an automated regression case. -- The mutation was reproduced and directly contradicts the scoped [BUG]apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 promise. - [Python Architect] (blocking-severity) Make one structured preview plan authoritative, preserving resolved dependency identity, prospective replacements, and effective registry context. -- Canonical-string reparsing and alternate construction can change source semantics and let install paths drift.
- [CLI Logging Expert] (blocking-severity) Apply
only_packagesconsistently to preview rendering, counts, and preflight diagnostics. -- A positional preview currently reports or diagnoses manifest dependencies the user did not select. - [Test Coverage Expert] Assert the final
Dry run completed: would install 1 APM dependencysummary in the focused [BUG]apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 scenario. -- The current passing test protects listing and manifest immutability but leaves the completion count without a durable regression trap.
Architecture
classDiagram
direction LR
class InstallCommand {
<<Module>>
+validate_packages()
+build_preview_plan()
}
class InstallContext {
<<ParameterObject>>
+dry_run bool
+validated_additions tuple
}
class ProspectiveInstallPlan {
<<ValueObject>>
+selected_apm_dependencies tuple
+all_apm_dependencies tuple
+intended_dependency_keys frozenset
}
class DependencyReference {
<<ValueObject>>
+get_unique_key() str
}
class DryRunRenderer {
<<Renderer>>
+render_and_exit(plan)
}
InstallCommand *-- InstallContext : builds
InstallCommand *-- ProspectiveInstallPlan : builds once
ProspectiveInstallPlan o-- DependencyReference : preserves
DryRunRenderer --> ProspectiveInstallPlan : consumes
class InstallCommand:::touched
class ProspectiveInstallPlan:::touched
class DryRunRenderer:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
U["apm install PACKAGE --dry-run"]
V["Validate into structured dependency references"]
P["Build one prospective plan"]
S["Select requested APM dependency view"]
C["Run policy and security checks on selected view"]
R["Render plan and final count"]
N["Leave existing apm.yml unchanged"]
U --> V --> P --> S --> C --> R --> N
Recommendation
Rebase and fold #2664 into a #2612-only change, preferably after #2580 and #2592 land. Preserve Aryan Singh K. (@aryansk)'s ownership, then address manifest mutation, identity-preserving resolution, selected-package truthfulness, and the missing summary assertion before requesting another advisory pass.
Full per-persona findings
Python Architect
- [blocking] The documented full no-write contract is not enforced end-to-end at
src/apm_cli/commands/install.py.
Update checks and policy caches can still write. Remove this [BUG]apm install --dry-runcreates~/.apm/apm.yml,config.json, andapm_modules/while reporting "no changes made" #2549 expansion from the focused [BUG]apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 change and leave it to fix: keep global install dry-run out of user state #2592. - [blocking]
ProspectiveInstallPlanis not yet the sole preview authority atsrc/apm_cli/install/dry_run_plan.py.
Re-parsing canonical strings can lose object-form source semantics, while the renderer retains an alternate raw construction path. Carry structured validated references and require the plan. - [recommended] Rebase fix(install): preview positional packages in dry runs #2664 to retain only the [BUG]
apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 positional-preview change.
fix: include LSP dependencies in install dry-run #2580 already owns [BUG]apm install --dry-runreports "No dependencies found in apm.yml" when onlydependencies.lspis declared #2550's LSP preview and fix: keep global install dry-run out of user state #2592 owns [BUG]apm install --dry-runcreates~/.apm/apm.yml,config.json, andapm_modules/while reporting "no changes made" #2549's global dry-run work. Keep those community PRs and credit visible.
CLI Logging Expert
- [blocking] Positional dry runs report all manifest APM dependencies instead of the requested selection at
src/apm_cli/install/dry_run_plan.py.
only_packagesis present, but counts and rendering useall_apm_dependencies. - [blocking]
--only lspstill runs diagnostics against unselected APM dependencies atsrc/apm_cli/commands/install.py.
Remove this overlapping scope and leave the LSP behavior to fix: include LSP dependencies in install dry-run #2580. - [recommended] Read-only bootstrap previews omit projected creation and user scope.
This belongs to fix: keep global install dry-run out of user state #2592's global dry-run scope rather than [BUG]apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612. - [recommended] The bundled guide contradicts its own no-write contract.
Restore a single [BUG]apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612-only statement. - [nit] Emit one dry-run completion summary.
Let the command lifecycle own the final count-bearing outcome.
DevX UX Expert
- [blocking] Existing dependency updates can still write
apm.ymlduring dry-run atsrc/apm_cli/commands/install.py.
Skip persistence in dry-run and carry prospective replacements into the plan. - [blocking] Dry-run no longer follows configured default-registry routing at
src/apm_cli/commands/install.py.
Use the same effective registry through a read-only path. - [recommended] Limit positional previews to packages the command would install.
Rendering, counts, and preflight currently include unrelated manifest dependencies. - [recommended] Rebase onto or sequence after the community LSP and global dry-run fixes.
Preserve Aryan Singh K. (@aryansk)'s authorship and keep fix(install): preview positional packages in dry runs #2664 scoped to [BUG]apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612. - [recommended] Remove contradictory dry-run documentation.
Align docs and the packaged guide with the narrowed contract.
Supply Chain Security Expert
- [blocking] Dry-run can still rewrite
apm.ymlatsrc/apm_cli/commands/install.py.
A changed existing dependency ref reproduces the mutation. - [blocking] Dry-run reconstructs additions with incorrect source identity at
src/apm_cli/install/dry_run_plan.py.
Keep the resolved structured reference and effective default-registry semantics. - [blocking] The prospective plan ignores positional package selection at
src/apm_cli/install/dry_run_plan.py.
Model the selected dependency view separately from complete manifest intent. - [recommended] Narrow fix(install): preview positional packages in dry runs #2664 to [BUG]
apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 and reserve community overlap.
Let fix: include LSP dependencies in install dry-run #2580 and fix: keep global install dry-run out of user state #2592 retain ownership; if any material is retained, link and credit those PRs.
OSS Growth Hacker
- [recommended] Resolve the exact contributor overlap before merging.
The body claims issues already addressed by Aryan Singh K. (@aryansk)'s open PRs. Prefer landing those first and clearly sequence fix(install): preview positional packages in dry runs #2664 as a [BUG]apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 follow-up. - [recommended] Remove contradictory dry-run promises from the packaged guide.
State the narrowed positional-preview promise once.
Auth Expert -- inactive
The changes touch install dry-run planning/presentation and related docs, tests, and architecture guards, not auth paths.
Doc Writer
- [recommended] Remove PR-body claims for [BUG]
apm install --dry-runreports "No dependencies found in apm.yml" when onlydependencies.lspis declared #2550 and [BUG]apm install --dry-runcreates~/.apm/apm.yml,config.json, andapm_modules/while reporting "no changes made" #2549.
Keep onlyFixes #2612; remove LSP-only and global no-write claims and evidence. - [recommended] Restore the [BUG]
apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612-only install documentation contract atdocs/src/content/docs/reference/cli/install.md.
Positional packages appear in preview but are not added to an existing manifest. - [recommended] Keep the architecture contract scoped to positional preview planning.
Retain the prospective-plan owner row but narrow its guard away from overlapping behavior.
Test Coverage Expert
- [recommended] The [BUG]
apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 regression does not protect the final dry-run count attests/unit/test_install_command.py.
Proof (missing at integration-with-fixtures):tests/unit/test_install_command.py::TestInstallCommandAutoBootstrap::test_positional_local_package_dry_run_previews_validated_addition
Addassert "Dry run completed: would install 1 APM dependency" in result.output. - [recommended] Narrow Scenario Evidence to [BUG]
apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 and add principle mapping.
Proof (passed):tests/unit/test_install_command.py::TestInstallCommandAutoBootstrap::test_positional_local_package_dry_run_previews_validated_addition-- 1 passed in 3.00s.
Performance Expert
- [recommended] Replace three full architecture-lint mutation runs with a focused prospective-plan checker.
The whole-suite mutation test copies and scans thousands of unrelated files. - [recommended] Narrow fix(install): preview positional packages in dry runs #2664 to [BUG]
apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 and leave overlapping behavior to fix: include LSP dependencies in install dry-run #2580 and fix: keep global install dry-run out of user state #2592.
The positional path is linear and adds no network work; the overlap adds unrelated hot-path and guard coupling.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve validated dependency identity, filter previews to the requested package, and keep changed refs in memory so dry-run output matches the corresponding install without mutating an existing manifest. This addresses the review-panel follow-ups while narrowing the PR to #2612. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7645f3b to
aeaa358
Compare
Give direct dry-run validation fixtures the required manifest version and add the new architecture rule to the frozen inventory. This addresses the first CI recovery findings after the rebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 1 | 0 | 0 | Route the remaining MCP policy consumer through the prospective plan. |
| CLI Logging Expert | 0 | 2 | 0 | Local additions and ref updates still use misleading plan text. |
| DevX UX Expert | 0 | 0 | 0 | Positional dry-run behavior is truthful, non-mutating, documented, and covered. |
| Supply Chain Security Expert | 0 | 0 | 0 | Manifest mutation, registry identity drift, and unselected preflights are fixed. |
| OSS Growth Hacker | 0 | 0 | 0 | Scope and contributor ownership are now explicit. |
| Doc Writer | 0 | 0 | 0 | Docs and PR body match the narrowed behavior and credit. |
| Test Coverage Expert | 0 | 1 | 0 | Add fixture-level policy coverage for unrequested dependencies. |
| Performance Expert | 0 | 0 | 0 | The plan remains linear, local, and inexpensive. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Python Architect] (blocking-severity) Fold now: make
prospective_plan.mcp_dependenciesthe MCP policy input and extend the guard. -- One prospective-plan authority prevents policy behavior from diverging. - [Test Coverage Expert] Fold now: add a positional dry-run integration fixture for an unrequested denied manifest dependency. -- The governed-policy promise needs a fixture-level regression trap.
- [CLI Logging Expert] Fold now: preserve
./namein local-path output and describe ref changes as updates rather than new installs. -- Accurate paths and terminology keep feedback aligned with the request.
Architecture
flowchart TD
A["apm install PACKAGE --dry-run"]
V["Validate request into prospective package"]
P["ProspectiveInstallPlan"]
S["Selected APM security and plugin checks"]
M["Plan-owned MCP policy input"]
R["Render plan and counts"]
A --> V --> P
P --> S
P --> M
P --> R
Recommendation
Fold these three bounded items into the current PR, led by the prospective-plan authority correction, then return it for maintainer consideration with the narrowed scope and community credit unchanged.
Full per-persona findings
Python Architect
- [blocking] Route the MCP policy input through the canonical preview plan at
src/apm_cli/commands/install.py:1813.
_dr_preflightrecombines rawmcp_depsandshould_install_mcpeven though the plan owns both. Pass the plan's MCP dependencies and extend the registered guard.
CLI Logging Expert
- [recommended] Render the requested local path in the dependency plan at
src/apm_cli/install/presentation/dry_run.py:40.
Use the dependency's source-aware display reference rather than constructing output fromrepo_url. - [recommended] Do not describe a ref update as a new package.
Carry preview action into the plan so validation, resolution, rendering, and the final summary use consistent update terminology.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
The final diff changes install dry-run planning, output, tests, documentation, and architecture files, with no authentication changes.
Doc Writer
No findings.
Test Coverage Expert
- [recommended] Selected-package policy checks lack fixture-level regression coverage at
src/apm_cli/commands/install.py:1812.
Proof (missing at integration-with-fixtures):tests/integration/test_policy_install_e2e.py::TestI10DryRunDenied::test_positional_dry_run_excludes_unrequested_denied_manifest_dependency.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Route MCP policy checks through the prospective plan, preserve local display paths, and distinguish ref updates throughout the dry-run lifecycle. Fixture-level policy coverage proves unrequested denied dependencies stay outside the preview. This addresses the second panel pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | The prospective plan has one owner with behavioral and static guardrails. |
| CLI Logging Expert | 0 | 0 | 0 | Local paths and ref-update output are now consistent. |
| DevX UX Expert | 0 | 0 | 0 | UX, docs, and tests align with #2612. |
| Supply Chain Security Expert | 0 | 0 | 0 | Identity, registry routing, immutability, and selected checks hold. |
| OSS Growth Hacker | 0 | 0 | 0 | Scope and community credit are explicit. |
| Doc Writer | 0 | 1 | 1 | Consolidate one duplicate guide note and sharpen one PR-body phrase. |
| Test Coverage Expert | 0 | 0 | 0 | All scenario claims and owner mutations pass. |
| Performance Expert | 0 | 0 | 0 | Planning remains O(n+k), local, and negligible. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 2 follow-ups
- [Doc Writer] Fold the positional dry-run note into the install-flags text and remove the duplicate. -- One authoritative location prevents wording drift.
- [Doc Writer] Replace generic
security checksin the PR body withinsecure-source validation. -- The specific term accurately describes this PR.
Architecture
flowchart TD
V["Validated positional request"] --> P["ProspectiveInstallPlan"]
P --> A["Selected APM checks"]
P --> M["Selected MCP policy input"]
P --> R["Dry-run renderer"]
P --> C["Final summary counts"]
Recommendation
The maintainer can ship after folding these two bounded documentation edits; the implementation and tests are otherwise converged.
Full per-persona findings
Python Architect
- [nit] Pattern assessment only; no action requested.
The immutable value-object plan and CommandLogger subclassing fit the current scope.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
The final diff contains no auth, token, or host-resolution changes.
Doc Writer
- [recommended] Fold the duplicate positional dry-run note into the existing install command entry in
packages/apm-guide/.apm/skills/apm-usage/commands.md. - [nit] Use
insecure-source validationinstead of genericsecurity checksin the PR body.
Test Coverage Expert
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Keep the dry-run contract with the canonical install command entry instead of repeating it after unrelated policy guidance. This addresses the terminal panel documentation follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | One frozen plan owner feeds selected checks, rendering, and counts with dual guardrails. |
| CLI Logging Expert | 0 | 0 | 0 | Guide consolidation preserves the dry-run output contract. |
| DevX UX Expert | 0 | 0 | 0 | Dry-run UX, documentation, scope, and contributor credit are consistent. |
| Supply Chain Security Expert | 0 | 0 | 0 | Identity, registry routing, policy checks, and manifest immutability hold. |
| OSS Growth Hacker | 0 | 0 | 0 | #2612-only scope and independent community ownership are confirmed. |
| Doc Writer | 0 | 0 | 0 | Duplicate guidance, security terminology, scope, and credit are resolved. |
| Test Coverage Expert | 0 | 0 | 0 | All five scenario claims and the owner mutation matrix are covered. |
| Performance Expert | 0 | 0 | 0 | Planning remains O(n+k), adds no network round trips, and is negligible. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
flowchart TD
V["Validated positional request"] --> P["ProspectiveInstallPlan"]
P --> A["Selected APM checks"]
P --> M["Selected MCP policy input"]
P --> R["Dry-run renderer"]
P --> C["Final summary counts"]
G["Registered static rule and mutation case"] --> P
Recommendation
The maintainer may ship this exact head now; the panel identified no follow-up work requiring tracking.
Folded in this run
- (panel) Narrowed the PR to [BUG]
apm install PACKAGE --dry-runomits the new package from its plan on 0.28.0 #2612 and preserved Aryan Singh K. (@aryansk)'s independent fix: include LSP dependencies in install dry-run #2580 and fix: keep global install dry-run out of user state #2592 ownership without cherry-picking -- resolved inaacc9ea037f360bfe48ca996de9194231f290cad. - (panel) Kept ref updates prospective and preserved structured dependency identity and registry routing -- resolved in
aeaa358f85c6d97339b1a9f206c08fade315f1aa. - (panel) Routed rendering, checks, and counts through the requested dependency view -- resolved in
aeaa358f85c6d97339b1a9f206c08fade315f1aa. - (panel) Added final count coverage and a registered prospective-plan owner guard -- resolved in
aeaa358f85c6d97339b1a9f206c08fade315f1aa. - (panel) Routed MCP policy input through the plan and extended mutation coverage -- resolved in
dd8e5e04acff467fbd10c024bb3600617e2a816f. - (panel) Preserved local display paths and consistent ref-update terminology -- resolved in
dd8e5e04acff467fbd10c024bb3600617e2a816f. - (panel) Added fixture-level policy-selection proof -- resolved in
dd8e5e04acff467fbd10c024bb3600617e2a816f. - (panel) Consolidated positional dry-run guidance at the canonical install command entry -- resolved in
45c68576c3c445efb7f82f05bd75da84aeb370eb.
Regression-trap evidence (mutation-break gate)
test_positional_dry_run_previews_ref_update_without_persisting-- removed the dry-run persistence guard and updated-package tracking; the test failed as expected; guards restored.test_positional_dry_run_excludes_unrequested_manifest_dependencies-- removed selected APM filtering; the test failed as expected; guard restored.test_positional_local_package_dry_run_previews_validated_addition-- removed source-aware display; the test failed as expected; guard restored.test_positional_dry_run_excludes_unrequested_denied_manifest_dependency-- removed plan-selected policy input; the test failed as expected; guard restored.test_owner_rule_catches_its_guard_mutation-- replaced plan-owned MCP policy input; the architecture mutation test caught it; guard restored.
Lint contract
The full CI-mirror ruff, format, pylint R0801, auth-signal, architecture-boundary, and file-length checks exited 0. CI Lint passed.
CI
All required checks passed on https://github.com/microsoft/apm/actions/runs/33495665073 after 1 CI fix iteration.
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2664 | 45c6857 |
ship_now | 4 | 8 | 0 | 2 | green | MERGEABLE | BLOCKED | awaiting required review |
Convergence
4 outer iterations; 2 Copilot rounds with no inline findings. Final panel stance: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect
- [nit] Pattern assessment only; no action requested.
The immutable value-object plan and CommandLogger subclassing are the simplest correct design at this scope.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
The final diff contains no authentication, token, credential-resolution, or host changes.
Doc Writer
No findings.
Test Coverage Expert
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
fix(install): preview positional packages in dry runs
TL;DR
apm install PACKAGE --dry-runnow validates and previews the requested positional package, including an in-memory ref change, without changing an existingapm.yml. Rendering, policy checks, insecure-source validation, and final counts consume one prospective plan.Note
Fixes #2612.
Community overlap
This PR no longer claims or implements #2550 or #2549. Those issues remain with Aryan Singh K. (@aryansk)'s community PRs #2580 and #2592. Their work was not cherry-picked here, so their authorship and review ownership remain intact.
Problem
Validation kept positional additions in memory during dry-run, but the renderer reparsed the unchanged manifest. The package could therefore be absent from the preview and final count. Existing dependency ref changes could also be written during a preview.
Approach
ProspectiveInstallPlan.Implementation
flowchart LR V[Validated positional request] --> M[Prospective manifest in memory] M --> P[ProspectiveInstallPlan] P --> S[Selected dependency checks] P --> R[Dry-run renderer] P --> C[Final count]The architecture owner registry now records the prospective dry-run plan. A registered static rule and mutation case prevent the command or renderer from rebuilding preview state outside that owner.
Trade-offs
apm install --dry-runreports "No dependencies found in apm.yml" when onlydependencies.lspis declared #2550 and [BUG]apm install --dry-runcreates~/.apm/apm.yml,config.json, andapm_modules/while reporting "no changes made" #2549 remain intentionally outside this PR to avoid duplicate work and preserve contributor credit.Validation
Scenario evidence
tests/unit/test_install_command.py::TestInstallCommandAutoBootstrap::test_positional_local_package_dry_run_previews_validated_additiontests/unit/test_install_command.py::TestInstallCommandAutoBootstrap::test_positional_dry_run_excludes_unrequested_manifest_dependenciestests/integration/test_policy_install_e2e.py::TestI10DryRunDenied::test_positional_dry_run_excludes_unrequested_denied_manifest_dependencyapm.ymltests/unit/test_install_command.py::TestInstallCommandAutoBootstrap::test_positional_dry_run_previews_ref_update_without_persistingtests/integration/test_architecture_owner_rule_mutations.pyHow to test
apm install ./local-package --dry-run.apm.yml, lockfile, modules, and deployment files are unchanged.apm.ymlremains byte-identical.apm-spec-waiver: This fixes internal install preview planning without extending the OpenAPM manifest contract.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com