fix(install): positional URL total-failure exits non-zero (closes #2126)

[danielmeppiel]

Positional installs returned exit code 0 when every requested package failed validation because the all-failed branch returned normally. This change uses the install command existing exit-1 mechanism and adds a CliRunner regression test for an unreachable positional URL.

How to test

• uv run --extra dev python -m pytest tests/unit/test_install_command.py -q
• uv run --extra dev python -m pytest tests/ -k install -q
• Run apm install https://127.0.0.1:1/org/repo.git and confirm exit code 1.

Closes #2126

[Copilot]

Pull request overview

Fixes apm install <positional-url> incorrectly exiting 0 when all positional packages fail validation, aligning behavior with the manifest-declared install path and the documented CLI contract.

Changes:

• Exit with status code 1 when positional package validation results in outcome.all_failed.
• Add a unit regression test covering the “positional URL totally fails validation” path.

Show a summary per file

File	Description
src/apm_cli/commands/install.py	Converts the “all positional packages failed validation” short-circuit from a normal return (exit 0) to sys.exit(1).
tests/unit/test_install_command.py	Adds a CliRunner regression test asserting exit code 1 and expected validation-failure messaging for a positional URL.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Low

[danielmeppiel]

APM Review Panel: ship_now

Fixes silent exit-0 on positional install total-validation failure, ensuring CI correctly surfaces broken installs as non-zero exits.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All active panel lanes converge on a clean signal: this is a bounded, well-proven bug fix with no architectural surface expansion, no security implications, no breaking change to user-facing contracts, and no documentation gap. The python-architect noted existing collect/render and value-object patterns are consistent with this change and explicitly recommends no modification. No other panelist raised actionable concerns.

Mutation-break evidence is decisive: the output assertion fails without summary_rendered = True, and the integration assertion fails without sys.exit(1). Both traps are durable and test user-observable behavior, not implementation internals. The Copilot inline brittleness comment on assert_called_once was folded to assert_called, which is the right trade-off between precision and refactor resilience. The 189 affected-file tests pass, the lint contract is green, and CI is green after an unrelated flaky red-team test was rerun.

Strategically this is a CI-reliability fix that protects every downstream consumer relying on APM's exit codes for pipeline gating. It reinforces the pragmatic-as-npm principle: package managers must never lie about failure to their orchestrators.

Aligned with: Pragmatic as npm: exit codes are the package-manager contract with CI; a silent exit 0 on failure violates the reliability expectation users bring from npm, pip, and cargo. OSS community-driven: the CHANGELOG entry clearly communicates the fix so adopters can identify and upgrade past the bug.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Minimal correct fix; summary_rendered plus sys.exit(1) correctly interacts with the existing finally guard.
CLI Logging Expert	0	0	0	Total validation failure now exits cleanly without a misleading interruption line.
DevX UX Expert	0	0	0	Exit behavior now matches package-manager conventions with no remaining UX issue.
Supply Chain Security Expert	0	0	0	The positional validation path now fails closed with no dependency-safety regression.
OSS Growth Hacker	0	0	0	The CI-reliability fix has clear CHANGELOG wording and harms no conversion surface.
Test Coverage Expert	0	0	0	Unit and integration regression traps defend the exit code and output contract.

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

Architecture

classDiagram
    direction LR
    class install {
      <<ClickCommand>>
      +install(packages, target, ...)
    }
    class _ValidationOutcome {
      <<ValueObject>>
      +valid list
      +invalid list
      +all_failed bool
      +has_failures bool
    }
    class CommandLogger {
      <<Base>>
      +validation_summary(outcome)
      +install_interrupted(elapsed_seconds)
    }
    install ..> _ValidationOutcome : reads all_failed
    install ..> CommandLogger : delegates output
    class install:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm install positional URL"] --> B["Validate packages"]
    B --> C{"All failed?"}
    C -- No --> D["Continue install pipeline"]
    C -- Yes --> E["Mark summary rendered"]
    E --> F["Exit 1"]
    F --> G["Skip interruption fallback"]

Loading

Recommendation

The bounded fix is ready for maintainer review. CI is green, mutation-break confirms both traps hold, and no panelist raised a follow-up worth tracking.

Folded in this run

• (panel) Tightened the legacy integration test to require exit code 1 -- resolved in fb904c0cb.
• (panel) Suppressed the misleading Install interrupted fallback on total validation failure -- resolved in 518bad9dc.
• (panel) Added an Unreleased reliability entry for CI scripting users -- resolved in da9b48d90.
• (copilot) Removed exact validator call-count coupling while preserving invocation coverage -- resolved in 969ccc909.

Copilot signals reviewed

• tests/unit/test_install_command.py:349 -- LEGIT: exact call-count coupling was unnecessary for the user-visible exit and output contract (resolved in 969ccc909).

Regression-trap evidence (mutation-break gate)

• tests/unit/test_install_command.py::TestInstallCommandAutoBootstrap::test_install_positional_url_total_validation_failure_exits_one -- removed summary_rendered = True; test FAILED as expected; guard restored.
• tests/integration/test_wave7_marketplace_install_coverage.py::TestInstallErrorPaths::test_install_package_validation_failure -- replaced sys.exit(1) with return; test FAILED as expected; guard restored.

Lint contract

The full CI-mirror lint contract completed with exit 0: ruff check, ruff format check, pylint R0801, and scripts/lint-auth-signals.sh.

CI

All required checks passed on ac78b8f05bff7fce4a676d9179ee3b2f5500b301; CI run: https://github.com/microsoft/apm/actions/runs/29117533513. One unrelated red-team timing failure passed locally and on the single failed-job rerun; the aggregate gate then passed.

Mergeability status

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#2131	ac78b8f	ship_now	2	4	0	2	green	MERGEABLE	BLOCKED	pending required review

Convergence

2 outer iterations; 2 Copilot rounds. Final panel stance: ship_now.

Ready for maintainer review.

---

Full per-persona findings

Python Architect

• [nit] Design-pattern contract note: the existing collect-then-render and validation value-object patterns fit this two-line fix. No modification suggested.

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 changed install exit, tests, and CHANGELOG do not touch authentication or credential resolution.

Doc Writer -- inactive

The corrected behavior matches the existing documented exit contract, and the CHANGELOG records the fix.

Test Coverage Expert

No findings.

Performance Expert -- inactive

The terminal failure branch does not alter install hot-path performance.

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

[danielmeppiel]

Superseded by #2155. The reliability campaign consolidated all 13 point fixes into a single architectural cure PR that re-homes each fix under its canonical owner module and adds owner-invariant guards. The fix for #2126 is folded into #2155 and re-verified there (full CI green). Closing this point PR in favor of the consolidated cure; the branch is preserved and this can be reopened if #2155 is not merged.