fix(uninstall): scan devDependencies.apm so --dev installs can be removed (closes #1549)

[danielmeppiel]

Closes #1549.

Problem

apm install --dev <pkg> writes the package under devDependencies.apm in apm.yml, but apm uninstall <pkg> only reads/writes dependencies.apm. The result is that every dev-installed package reports

[!] <pkg> - not found in apm.yml
[!] No packages found in apm.yml to remove

and the entry leaks forever. There is no way to remove a --dev install short of editing apm.yml by hand.

Fix shape

apm uninstall now scans both dependencies.apm and devDependencies.apm when looking for matches, and writes the package out of whichever section it lives in. Symmetric to install: --dev installs put the entry in devDependencies.apm; uninstall pulls it back out of the same section. Empty devDependencies wrappers that we synthesised purely to satisfy the read are dropped before writing, so manifests for projects that never used --dev stay byte-identical to today.

Tests

New regression trap at tests/unit/test_uninstall_dev_dependencies.py:

• test_uninstall_removes_package_from_dev_dependencies -- the headline regression: a package recorded under devDependencies.apm is removed end-to-end.
• test_uninstall_dry_run_finds_dev_dependency -- --dry-run locates the dev entry and leaves apm.yml untouched.
• test_uninstall_preserves_unrelated_dev_dependency -- removing one dev dep does not touch other dev or prod entries.

Mutation-break gate: deleting the new devDependencies handling makes all three tests fail with the original not found in apm.yml symptom; restoring the fix makes them pass. The full tests/unit/ -k 'uninstall or dev_depend' suite (227 tests) and tests/integration/ uninstall coverage (23 tests) stay green.

How to test

# Reproduce the bug on main
mkdir /tmp/apm-1549 && cd /tmp/apm-1549
apm init
apm install --dev microsoft/apm-sample-package
apm uninstall microsoft/apm-sample-package
# main: '[!] microsoft/apm-sample-package - not found in apm.yml'
# this PR: package is removed from devDependencies.apm cleanly

Validation evidence

• uv run --extra dev ruff check src/ tests/ -- silent
• uv run --extra dev ruff format --check src/ tests/ -- silent
• uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ -- 10.00/10
• bash scripts/lint-auth-signals.sh -- clean
• pytest tests/unit/ -k 'uninstall or dev_depend' -- 227 passed
• pytest tests/integration/test_uninstall_*.py tests/integration/test_wave5_e2e_coverage.py -k uninstall -- 23 passed, 2 skipped

[Copilot]

Pull request overview

This PR fixes apm uninstall so packages installed with apm install --dev can be discovered and removed from devDependencies.apm, addressing issue #1549.

Changes:

• Extends uninstall dependency scanning to include both production and development APM dependency sections.
• Updates manifest write-back logic to remove matched packages from the appropriate section.
• Adds regression tests for uninstalling, dry-running, and preserving unrelated dev dependencies.

Show a summary per file

File	Description
src/apm_cli/commands/uninstall/cli.py	Adds devDependencies.apm handling during uninstall.
tests/unit/test_uninstall_dev_dependencies.py	Adds regression coverage for uninstalling dev dependencies.
CHANGELOG.md	Documents the uninstall fix under Unreleased.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 2

[danielmeppiel]

APM Review Panel: ship_with_followups

Restores install/uninstall symmetry for --dev packages, closing a user-reported bug where devDependencies.apm entries were invisible to apm uninstall.

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

All eight panelists converge on ship. Zero blocking-severity findings. The fix is minimal, scoped, and architecturally sound: uninstall now scans both manifest sections and removes from the originating section. The python-architect confirms the dual-scan plus write-back-to-origin pattern is correct; supply-chain-security and auth confirm no sensitive surface is touched.

The strongest cross-panel signal is a UX polish gap: both cli-logging-expert and devx-ux-expert independently flag that the progress message does not distinguish which section a package was removed from. This is a real signal - now that both sections are scanned, the user loses observability of where the removal happened. However, this is pre-existing behavior extended to a new code path, not a regression introduced by this PR. The doc-writer's finding that docs/reference/cli/uninstall.md is now stale is the highest-priority follow-up because it is a user-facing contract that will mislead readers post-merge.

The test-coverage-expert's missing-test finding on the synthesized-section cleanup path is nit-tier and correctly scoped: the invariant (projects without devDependencies stay clean) is manifest aesthetics, not correctness. No principle surface (secure-by-default, governed-by-policy) is at stake, so this does not escalate above nit.

Aligned with: Portable by manifest: Manifest semantics are now consistent: what install --dev writes, uninstall can remove. Byte-identical output for projects that never used --dev. Pragmatic as npm: Matches user expectation from npm/pip where uninstall finds the package regardless of which dependency group it lives in. OSS community driven: Community-reported bug (#1549) fixed promptly with regression tests; healthy contributor loop.

Growth signal. This is a textbook community bug-fix loop: user reports confusing behavior (#1549), maintainer ships a fix with tests in the same milestone. Worth a callout in the next release note as evidence of responsiveness. The oss-growth-hacker's suggestion to lead the CHANGELOG with the user-positive outcome ("apm uninstall now correctly removes --dev packages") rather than maintainer-speak is good release-note hygiene.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Clean scoped fix; no architectural concern. The dual-section scan plus write-back-to-origin pattern is correct and minimal.
CLI Logging Expert	0	1	1	Progress message does not distinguish prod vs dev section; otherwise logging surface is clean and unchanged.
DevX UX Expert	0	1	1	Clean bugfix restoring symmetry between install --dev and uninstall; one minor UX signal gap in the progress message.
Supply Chain Security Expert	0	0	0	No supply-chain security surface touched; manifest-only YAML read/write with no download, auth, path traversal, or lockfile integrity changes. Ship.
OSS Growth Hacker	0	1	2	Solid bug-fix with good CHANGELOG entry; minor polish opportunities in release wording and story fit.
Doc Writer	0	2	1	CHANGELOG entry is accurate; reference/cli/uninstall.md Behavior step 1 is now stale and should mention devDependencies.apm.
Test Coverage Expert	0	0	1	Bug fix ships with well-targeted regression traps at integration-with-fixtures tier; one nit on the manifest-cleanup path.

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

Top 4 follow-ups

1. [Doc Writer] Update docs/reference/cli/uninstall.md Behavior step 1 to cover devDependencies.apm -- The canonical user-facing description of removal behavior is now stale and will mislead readers post-merge.
2. [CLI Logging Expert] Include section name (dependencies vs devDependencies) in verbose/progress removal message -- Both cli-logging-expert and devx-ux-expert flagged this independently; users debugging manifest state lose observability of where the removal happened.
3. [OSS Growth Hacker] Reword CHANGELOG entry to lead with user-positive outcome -- Release notes are a conversion surface; leading with what the user gains is clearer than internal terminology.
4. [Test Coverage Expert] Add assertion that uninstalling a prod dep from a manifest without devDependencies leaves no synthesized devDependencies key -- Covers the had_dev_section=False cleanup branch; low priority since it is manifest aesthetics, not correctness.

Architecture

classDiagram
    direction LR
    class UninstallCommand {
      <<ClickCommand>>
      +uninstall(ctx, packages, dry_run, verbose, global_)
    }
    class ManifestData {
      <<YAML dict>>
      +dependencies.apm
      +devDependencies.apm
    }
    class UninstallValidation {
      <<Function>>
      +_validate_uninstall_packages(packages, current_deps, logger, lockfile, auth_resolver, dry_run)
    }
    class YamlIO {
      <<Module>>
      +load_yaml(path) dict
      +dump_yaml(data, path)
    }
    UninstallCommand ..> YamlIO : reads/writes apm.yml
    UninstallCommand ..> ManifestData : scans prod + dev sections
    UninstallCommand ..> UninstallValidation : validates combined deps

Loading

flowchart TD
    A[apm uninstall pkg] --> B[load apm.yml]
    B --> C[ensure dependencies.apm]
    C --> D[ensure or synthesize devDependencies.apm]
    D --> E[current_deps = prod_deps + dev_deps]
    E --> F[validate requested packages]
    F --> G{dry run?}
    G -->|yes| H[report removals only]
    G -->|no| I{package in dev_deps?}
    I -->|yes| J[remove from devDependencies.apm]
    I -->|no| K[remove from dependencies.apm]
    J --> L[drop synthesized empty dev wrapper]
    K --> L
    L --> M[dump apm.yml]
    M --> N[continue uninstall cleanup]

Loading

Recommendation

Merge now. The fix is correct, minimal, and ships with regression tests. Track two post-merge items: (1) update the uninstall reference doc to mention devDependencies.apm scanning, and (2) consider adding section-name context to the removal progress message in a follow-up polish PR.

---

Full per-persona findings

Python Architect

• [nit] or-[] idiom creates detached list when source is falsy but non-None at src/apm_cli/commands/uninstall/cli.py:103
  prod_deps = data['dependencies']['apm'] or [] is safe here because the code reassigns prod_deps and dev_deps back into the data dict before dumping, but future editors may not notice that coupling.
  Suggested: Optionally add a one-line comment that the local lists are reassigned back into data before dump_yaml.

CLI Logging Expert

• [recommended] logger.progress message does not tell the user which section the package was removed from at src/apm_cli/commands/uninstall/cli.py:143
  When a package lives in devDependencies.apm the message still says 'Removed from apm.yml', identical to prod removal. The section can matter for humans debugging manifest state.
  Suggested: Capture whether the package came from devDependencies or dependencies before removal and include that section in verbose/progress output.
• [nit] Success message says removed N package(s) without distinguishing dev vs prod counts at src/apm_cli/commands/uninstall/cli.py:148
  If a user removes a mix of prod and dev packages, the final message has no breakdown. Low priority because this pattern existed before the PR.

DevX UX Expert

• [recommended] Progress message does not indicate which section the package was removed from (prod vs dev) at src/apm_cli/commands/uninstall/cli.py:141
  Users debugging manifest state benefit from knowing whether uninstall removed from dependencies.apm or devDependencies.apm, especially now that both are scanned.
  Suggested: Capture membership before removal and log the section name.
• [nit] CHANGELOG placeholder was present in the first commit but fixed in the second
  Cosmetic only; final diff has fix(uninstall): scan devDependencies.apm so --dev installs can be removed (closes #1549) #1552 and is fine.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

• [recommended] CHANGELOG wording could lead with the user-positive outcome at CHANGELOG.md:12
  Release notes are a conversion surface; leading with 'apm uninstall now correctly removes packages installed with --dev' is clearer than maintainer-speak about leaked entries.
  Suggested: Reword to lead with the positive outcome for users.
• [nit] PR body How to test uses /tmp, which is less Windows-friendly
  PR body only, but platform-neutral examples help contributor funnel polish.
• [nit] This community bug-fix loop is worth mentioning in release communications
  Community report to quick fix is a useful trust signal.

Auth Expert -- inactive

PR #1552 does not touch auth.py, token_manager.py, AuthResolver, or any credential resolution surface.

Doc Writer

• [recommended] docs/reference/cli/uninstall.md Behavior step 1 still says only dependencies.apm; update to cover devDependencies.apm too at docs/src/content/docs/reference/cli/uninstall.md:78
  After this PR, apm uninstall removes the entry from whichever of dependencies.apm or devDependencies.apm it lives in. The uninstall reference page is the canonical user-facing description of removal order.
  Suggested: Mention dependencies.apm or devDependencies.apm, whichever section the package was installed to.
• [recommended] CHANGELOG could note that synthesized empty devDependencies wrappers are dropped at CHANGELOG.md:10
  The PR body calls out that projects that never used --dev stay byte-identical. This user-observable invariant is not in the changelog.
  Suggested: Append a short clause that no synthesized devDependencies block is left behind.
• [nit] CHANGELOG credits @aetos382 while PR author is @danielmeppiel; confirm attribution is intended at CHANGELOG.md:10
  If this is shepherd convention, no change is needed; otherwise correct before release.

Test Coverage Expert

• [nit] No assertion on empty-devDependencies cleanup when section was synthesised (had_dev_section=False path) at tests/unit/test_uninstall_dev_dependencies.py
  The new branch that drops the devDependencies key when it was synthesized is not directly asserted. This is manifest aesthetics, not correctness, so nit.
  Suggested: Add a test uninstalling a prod dependency from a manifest with no devDependencies key and assert devDependencies is still absent.
  Proof (missing at): tests/unit/test_uninstall_dev_dependencies.py::TestUninstallDevDependencies::test_uninstall_drops_synthesised_dev_section -- proves: Projects that never used --dev keep a clean manifest after uninstalling a prod dependency [devx]
  assert 'devDependencies' not in _read_apm_yml(tmp_path)

Performance Expert -- inactive

PR #1552 touches only uninstall manifest YAML read/write logic, CHANGELOG.md, and a new unit test file; no dependency download, materialization, cache, transport, ref-resolution, or lockfile hot-path code is modified.

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

[danielmeppiel]

APM Review Panel: ship_now

apm uninstall now matches apm install --dev by removing dev-only packages from devDependencies.apm, with docs, changelog, logging, and regression coverage folded.

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

The prior advisory follow-ups have converged. The implementation now scans both manifest sections, writes back only the section that owned the package, keeps prod-only manifests from growing a synthetic devDependencies key, and names the manifest section in progress output. Local validation and GitHub checks are green on the latest head.

Aligned with: pragmatic as npm: install and uninstall now preserve package-manager symmetry for dev dependencies; portable by manifest: the manifest cleanup keeps dependency state truthful.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	0	The fix is narrow and keeps manifest mutation localized to uninstall.
CLI Logging Expert	0	0	0	Progress output now identifies dependencies.apm vs devDependencies.apm.
DevX UX Expert	0	0	0	The uninstall lifecycle now matches the --dev install mental model.
Supply Chain Security Expert	0	0	0	No new supply-chain or token surface is introduced.
OSS Growth Hacker	0	0	0	The changelog now leads with the user-visible outcome.
Doc Writer	0	0	0	The uninstall reference now documents both manifest sections.
Test Coverage Expert	0	0	0	Unit regression traps cover dev removal, dry run, preservation, and no synthesized dev section.

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

Architecture

flowchart TD
    CLI[apm uninstall] --> Manifest[apm.yml]
    Manifest --> Prod[dependencies.apm]
    Manifest --> Dev[devDependencies.apm]
    Prod --> Validate[_validate_uninstall_packages]
    Dev --> Validate
    Validate --> Remove[remove from owning section]
    Remove --> Clean[drop synthetic empty devDependencies]

Loading

Folded in this run

• (panel) Updated docs/src/content/docs/reference/cli/uninstall.md Behavior step 1 to cover devDependencies.apm -- resolved in 6987f67ccefb303c84e7a8c392933fa97da140d1.
• (panel) Reworded the CHANGELOG entry to lead with the user-positive outcome -- resolved in 6987f67ccefb303c84e7a8c392933fa97da140d1.
• (panel) Included the manifest section name in uninstall progress output -- resolved in 6987f67ccefb303c84e7a8c392933fa97da140d1.
• (panel) Added a regression assertion that prod-only manifests do not synthesize devDependencies -- resolved in 6987f67ccefb303c84e7a8c392933fa97da140d1.

Copilot signals reviewed

No inline Copilot review comments were present in the pull request comments API for this pass.

Regression-trap evidence (mutation-break gate)

• tests/unit/test_uninstall_dev_dependencies.py::TestUninstallDevDependencies::test_uninstall_prod_dependency_does_not_synthesize_dev_section -- deleted del data["devDependencies"]; test FAILED as expected; guard restored.

Lint contract

uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both silent.

CI

All 15 PR checks are green on 6987f67ccefb303c84e7a8c392933fa97da140d1 after 0 CI fix iteration(s).

Mergeability status

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

Recommendation

Ready for maintainer review. Merge when the required human review policy is satisfied.

---

Full per-persona findings

No active panelist raised a remaining finding. Auth Expert was inactive because the touched files do not change token management, host classification, credential resolution, git/HTTP authorization headers, or AuthResolver behavior.

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