fix(install): discard lockfile with git conflict markers, fail closed under --frozen

[Lachlan Heywood (lachieh)]

Description

When apm.lock.yaml still contains git merge conflict markers, every command that reads it exits 1 with a raw PyYAML scanner error, and the --frozen failure tip points at apm outdated and apm update, which fail on the same error. The only recovery was deleting the file by hand.

This PR implements the proposal in the issue comment:

• LockFile.read (already the single lockfile load owner) scans for <<<<<<< , >>>>>>> , or ||||||| at line start before parsing and raises LockfileConflictError, a LockfileFormatError subclass. The message names the file and the next action. apm update, apm outdated, apm lock export, apm install --dry-run, and every other command that requires the lockfile now report the real cause (best-effort readers such as apm view keep continuing without lock metadata). A bare ======= line is not treated as a marker; a real conflict always carries <<<<<<<.
• A full apm install (no package arguments, no --only) and apm lock warn (apm.lock.yaml contains git merge conflict markers; discarding it and resolving from apm.yml.), delete the conflicted file before the pipeline reads it, and let the pipeline write a fresh one. The ~15 downstream LockFile.read sites see "no lockfile", which is what npm, pnpm, and Bun do ("treat as absent, regenerate"). apm.yml is the input a non-frozen install resolves from anyway, so no new trust path is added. Lockfiles that are invalid for any other reason keep failing closed.
• Partial installs (apm install PACKAGE, --only apm, --only mcp, --mcp NAME) never discard, because they would write a lockfile missing the entries they do not resolve (--only apm would drop every MCP lock record, --only mcp every package). They fail closed with the named error; the positional-add path restores apm.yml through the existing transaction rollback. apm install --dry-run reports the conflict as a warning and leaves the file alone; its closing line still says "would make no changes" because the dry-run plan does not model the lockfile rewrite, which is unchanged from today.
• apm install --frozen fails closed with --frozen cannot use apm.lock.yaml: it contains git merge conflict markers. Resolve the merge conflict, or run 'apm install' without --frozen to regenerate it from apm.yml. and leaves the file untouched.
• frozen_install_tip returns nothing when the error carries no drift reasons (missing or unreadable lockfile). Those messages already name their next action, and the old tip named commands that cannot run.
• --force is unchanged.

The discard is owned by InstallTransaction, which snapshots the conflicted bytes before unlinking; rollback() restores the file unless the attempt already wrote a new lockfile. apm lock runs under its own transaction (as a context manager, so an interrupt after the discard also restores) and migrates a legacy apm.lock before the discard probe. That decision is registered as an architecture owner (conflicted-lockfile-discard in .apm/architecture/owners/install-deployment.json) with the install-deployment-conflicted-lockfile-discard guard: the linter requires the transaction to define discard and restore and to call restore from rollback(), requires both command callers to route through it, and flags any other has_conflict_markers use or discard_conflicted_lockfile definition under src/. The mutation matrix proves the guard fires when rollback stops restoring.

Limitations, stated deliberately:

• Deployed-file records in the conflicted file are not carried over, the same as an absent lockfile. apm install re-records them on deploy; apm lock writes an empty deployment set, as it does today when no lockfile exists. A manifest with nothing to lock ends with no lockfile, which is why the warning says "discarding" rather than "regenerating".
• apm install --mcp NAME still writes the server to apm.yml before it reads the lockfile (pre-existing ordering); the failure now prints the lockfile error at default verbosity instead of hiding it behind --verbose, while other lockfile format errors keep their redacted handling.
• InstallService.enforce_frozen and the dry-run preview do not resolve a legacy apm.lock. That predates [BUG] apm install should support automatic merge conflict resolution through forced reinstallation. #2979 and is left for a separate issue.

Issue and approved scope

Issue: #2979

Human scope-approval comment: the issue carries no <!-- apm-scope:v1 --> comment. status/accepted was applied by Sergio Sisternes (@sergio-sisternes-epam) (core maintainer, project remit) on 2026-09-18T15:05Z (timeline). The triage note recommended needs-design and asked for the default to be settled; the default implemented here is the one proposed in my comment, which also records what npm, Yarn, pnpm, Bun, and aube do. If a maintainer wants the default to fail closed instead, points 1 and 5 above stand and only the regeneration call in apm install/apm lock changes.

This PR completes the issue.

Fixes #2979

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

Unit: tests/unit/install/test_install_transaction.py (discard then fail() restores byte-exact; rollback keeps a lockfile written after the discard; commit does not restore; valid, corrupt, undecodable, and missing files are never discarded), tests/unit/deps/test_lockfile_conflict_markers.py (read raises the named error for two-way and diff3 markers, corrupt-without-markers stays a plain LockfileFormatError, marker text mid-line is accepted, =======-only is not a conflict, discard removes only a conflicted file) and two cases in tests/unit/install/test_frozen.py (frozen fails closed and leaves the file; no apm outdated tip for missing or conflicted lockfiles).

End to end via CliRunner: tests/integration/test_install_conflicted_lockfile_e2e.py covers apm install and apm lock discarding with the warning and recording local content; apm install --frozen, apm install ./pkg, --only apm, --only mcp, and --mcp NAME exiting 1 with the named error and the file untouched; --dry-run naming the conflict without touching the file; a corrupt lockfile without markers still failing closed on a full install; and apm update / apm outdated / apm lock export raising the named error without a YAML trace.

uv run pytest tests/unit tests/test_console.py: 22632 passed. bash scripts/lint-architecture-boundaries.sh exits 0, and the owner-guard mutation matrix, linter entrypoint, and tests/quality suites pass. tests/integration + tests/spec_conformance: 9489 passed, 13 failed. All 13 fail identically on main in this environment (network access to gitlab.com, a missing pwsh, real zsh startup enrolment, the local git factory determinism check, and test_architecture_owner_rule_mutations.py::test_matrix_covers_every_registry_guard_exactly_once). ruff check, ruff format --check clean; mypy on the touched modules adds no new errors (the lock export path was switched from LockFile.from_yaml to LockFile.read so it goes through the load owner, and its None check replaces the is_file() check).

Manual, in a scratch project with local .apm/instructions and a conflicted lockfile: apm install warns and writes a lockfile byte-identical to a clean install; apm lock warns and regenerates; apm install --frozen, apm install ./pkgA, --only mcp, --mcp foo, apm update, apm outdated, and apm lock export exit 1 with the named error and leave the file untouched; --dry-run warns and exits 0.

apm lock export now resolves a legacy apm.lock through resolve_lockfile_path_for_read like the other read-only consumers, the generic --frozen could not read error names its repair step, and a non-UTF-8 lockfile still fails through LockfileFormatError (it is never probed for discard). The apm-guide skill resources carry the new frozen and recovery guidance.

The APM Review Panel's blocking item (no coverage of a failed install after the discard) and its transactional-discard and single-owner follow-ups were originally split into a stacked PR; GitHub cannot base a cross-fork PR on a fork branch and stacked PRs are not enabled here, so that work is folded in as the last two commits and #3043 is closed.

An independent review pass against the first cut found the partial-install, --only mcp, --mcp, and --dry-run gaps above; all four are fixed and covered by the e2e tests.

Spec conformance (OpenAPM v0.1)

• N/A -- this PR does not change OpenAPM-observable behaviour.

No normative statement covers invalid-lockfile handling. req-pl-013 (require_hashes fails closed when the lockfile is absent or unreadable) still holds: enforce_installed_hash_policy runs against the freshly written lockfile after regeneration, and --frozen never regenerates.

[Copilot]

🟡 Changes recommended

Resolve the remaining lockfile path, decoding, frozen-guidance, and documentation issues.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds conflict-marker detection for lockfiles, regenerating them during full installs while failing closed for frozen and partial operations.

Changes:

• Updates install, lock, MCP, dry-run, and frozen-mode handling.
• Adds unit and integration coverage.
• Updates CLI documentation and changelog.

File summaries

File	Description
tests/unit/install/test_frozen.py	Tests frozen conflict behavior.
tests/unit/deps/test_lockfile_conflict_markers.py	Tests marker detection and discard behavior.
tests/integration/test_install_conflicted_lockfile_e2e.py	Covers CLI recovery and failure modes.
src/apm_cli/install/service.py	Adds frozen-mode failure handling.
src/apm_cli/install/presentation/dry_run.py	Reports conflicts without modifying files.
src/apm_cli/install/mcp/command.py	Surfaces MCP lockfile errors.
src/apm_cli/install/errors.py	Refines frozen recovery guidance.
src/apm_cli/deps/lockfile.py	Detects and discards conflicted lockfiles.
src/apm_cli/commands/lock.py	Applies recovery and export handling.
src/apm_cli/commands/install.py	Regenerates conflicted files during full installs.
docs/src/content/docs/troubleshooting/install-failures.md	Documents recovery steps.
docs/src/content/docs/reference/lockfile-spec.md	Documents conflict semantics.
docs/src/content/docs/reference/cli/lock.md	Documents apm lock behavior.
docs/src/content/docs/reference/cli/install.md	Documents install and frozen behavior.
CHANGELOG.md	Records the fix.

Review details

Suppressed comments (7)

CHANGELOG.md:29

• The #2979 suffix is the linked issue number, not this pull request's number. The changelog contract requires each entry to end with the actual PR number; replace this suffix with the PR number when it is known.

- A full `apm install` and `apm lock` now warn, discard `apm.lock.yaml`, and resolve from `apm.yml` when the lockfile still contains git merge conflict markers, instead of exiting with a YAML parse error. `apm install --frozen`, partial installs, and read-only commands such as `apm update` and `apm outdated` fail closed with an error that names the conflict and the next action, and the `--frozen` failure tip no longer points at commands that cannot read the lockfile. (#2979)

docs/src/content/docs/reference/cli/install.md:153

• The CLI behavior changed here, but the maintained packages/apm-guide/.apm/skills/apm-usage/ resources were not updated: commands.md:15 still describes frozen mode only as missing/out-of-sync, and troubleshooting.md:85-89 has no merge-conflict recovery. Add the concise install/lock conflict behavior there so the package guidance does not give stale recovery instructions.

- **Frozen mode.** With `--frozen`, install resolves only what is in `apm.lock.yaml`. A missing lockfile, a direct dependency missing from it, or MCP config state that differs from `apm.yml` exits `1` before lockfile, target config, deployment, or cache mutation. Cold-cache installs (empty `apm_modules/`) with git `apm_package` deps are tolerated: MCP checks are skipped for absent package directories (the packages will be hydrated by the pipeline), and their MCP server configs are restored from the lockfile so no false drift is reported. Remote `claude_skill` dependencies declared at a repository root or subdirectory are also accepted from their locked type before materialization; once present, the lock type and detected skill shape must agree. Missing local paths still fail. A lockfile that contains git merge conflict markers also exits `1` and is never rewritten under `--frozen`. See [`config-consistency`](../../baseline-checks/#config-consistency) for the full manifest rule. Run normal `apm install` to create or repair MCP-only lock state, or to discard a conflicted lockfile and resolve from `apm.yml`, then retry frozen mode. Add-style invocations (`apm install PACKAGE` and `apm install --mcp NAME`) are rejected because they mutate `apm.yml`. Orphan package lock entries are tolerated; local-path deps are skipped. This is a structural check, not a content check -- run `apm audit --ci` for hash verification.

docs/src/content/docs/reference/lockfile-spec.md:398

• This "Every command" claim is broader than the current behavior: commands/view.py::_lookup_lockfile_ref and commands/deps/cli.py catch Exception around LockFile.read and continue without lockfile metadata, so those readers still do not name this conflict. Narrow the sentence to commands that require the lockfile, or update those best-effort readers to surface the error.

conflict rather than a YAML error. Every command that reads the lockfile names
the file and the next action. A full `apm install` (no package arguments, no

src/apm_cli/commands/lock.py:313

• apm lock export is a read-only lockfile consumer, but this new LockFile.read call still uses only get_lockfile_path. A project that has only the supported legacy apm.lock is therefore reported as having no lockfile, and a conflict in that file is never classified; route the path through resolve_lockfile_path_for_read(project_root, read_only=True) as the other read-only consumers do (for example, commands/outdated.py:449).

    lockfile = LockFile.read(lockfile_path)

src/apm_cli/deps/lockfile.py:1276

• This discard probe performs a second unguarded UTF-8 decode. A non-UTF-8 lockfile reaches it before the pipeline's LockFile.read, so a full install reports a raw UnicodeDecodeError instead of the normalized fail-closed lockfile error. Catch and normalize the decode here, leaving the file in place so it cannot be discarded as conflicted.

    if not path.exists() or not has_conflict_markers(path.read_text(encoding="utf-8")):
        return False

src/apm_cli/install/errors.py:103

• This new early return also applies to FrozenInstallError from the generic unreadable-lockfile path in InstallService.enforce_frozen: that message only says --frozen could not read ... and does not tell the user how to recover. Preserve actionable guidance for unreadable (non-missing) lockfiles, or make that exception message include the normal-install repair action while keeping the missing-file case free of the obsolete outdated tip.

    if not error.reasons:
        return ""

tests/integration/test_install_conflicted_lockfile_e2e.py:16

• This new integration module invokes the Click CLI in-process via CliRunner and touches a temporary filesystem, so it needs the module-level component behavioral marker. Without pytestmark = pytest.mark.component, the new tests are left outside the repository's marker-only taxonomy and are not selected by component-scoped runs (see tests/quality/test_test_taxonomy.py:152-163).

import pytest
from click.testing import CliRunner

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Sergio Sisternes (sergio-sisternes-epam)]

Thank you for contributing this pull request.

This PR is linked to #2979, which already carries maintainer status/accepted. Advisory triage recommendation is ready-for-review. That is not merge approval, not assignment, and not a request to run a review panel.

CODEOWNERS already requested danielmeppiel and sergio-sisternes-epam. This note does not add or change review requests.

A responsible human maintainer still needs to review the implementation against the accepted issue, including the chosen default: detect git conflict markers in the lockfile load owner, regenerate on a full non-frozen apm install / apm lock, and fail closed under --frozen and partial installs.

---

Generated by autopilot-pr-triage-worker. This comment is AI-generated and may contain errors.

[Sergio Sisternes (sergio-sisternes-epam)]

APM Review Panel: ship_with_followups

Conflicted lockfile recovery matches accepted #2979: detect in load, regenerate on full non-frozen install/lock, fail closed under --frozen and partial installs.

panel-mode=full; personas=python-architect,test-coverage-expert,doc-writer,performance-expert,cli-logging-expert,devx-ux-expert,supply-chain-security-expert

cc Lachlan Heywood (@lachieh) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

This PR implements the accepted #2979 default without expanding product surface: conflict detection lives in LockFile.read, a full non-frozen install/lock regenerates via the existing missing-lock path, and --frozen plus partial installs stay fail-closed. Supply-chain, CLI logging, and performance agree the change is load-bearing and cheap: no integrity bypass, named next-action copy, and a single precompiled regex on small YAML.

Specialists converge on one real durability gap, not a security hole. discard_conflicted_lockfile unlinks apm.lock.yaml outside InstallTransaction, so a later install failure cannot restore the conflicted file through the transaction; the author documented that git checkout recovers it, and there is no test of unlink-then-fail. Architect's ask for one conflict-outcome owner is the same story as cleanup, not a competing design. Test-coverage's --force gap is a false product claim: --force is not a lock-regenerate switch and should stay unchanged. DevX CI autodetection and a dry-run machine-detectable exit contract would rewrite accepted scope; keep dry-run as warn + exit 0.

Docs should stay as precise as the code: full install/lock recover, frozen/partial fail closed, dry-run does not rewrite. Drop unrelated hermes/target churn in commands.md. CODEOWNERS should confirm the implementation against the accepted default; remaining work is follow-up, not a new policy.

Dissent. test-coverage-expert marked unlink-then-fail missing coverage as blocking and asked for a --force override test; python-architect rated the same unlink as recommended, and product intent is that --force is not a regenerate switch. I keep unlink-then-fail as the top follow-up (author-stated limitation, git checkout recovers) and drop the --force test. I also side against DevX CI-detect and dry-run exit-code expansion because they change accepted #2979.

Aligned with: Portable by manifest: a conflicted lock is treated as missing on full install/lock and rewritten from the manifest; frozen and partial paths refuse to proceed with a bad pin file. Secure by default: regeneration reuses the existing missing-lockfile path; --frozen and non-conflict corruption stay fail-closed. Supply-chain found no integrity bypass. Governed by policy: detect-in-load plus mode-based outcome (regenerate vs fail-closed) matches the accepted default; do not add a silent CI autodetection policy. OSS community-driven: external author on an accepted issue; keep review on the ratified default rather than extra flags that would move the contract. Pragmatic as npm: full install recovers a conflicted lock the way a missing lock already does; frozen/partial stay closed; --force is left alone.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	Conflicted-lockfile detect is centralized, but discard-vs-fail-closed and the unlink still fork outside InstallTransaction.
CLI Logging Expert	0	0	0	Conflict named with next action; empty frozen tip is intentional so outdated/update are not suggested.
DevX UX Expert	0	2	1	Tighten regenerate vs fail-closed defaults, dry-run exit codes for CI, and non-transactional lockfile updates.
Supply Chain Security Expert	0	0	0	Conflicted-lock regen is the existing missing-lockfile path; --frozen and non-conflict corruption stay fail-closed. No integrity bypass.
Doc Writer	0	2	1	Conflict matrix is in the right pages; keep recover vs fail-closed explicit, canonicalize in lockfile-spec, and drop unrelated commands.md hermes churn.
Test Coverage Expert	1	2	0	PR adds focused unit + integration tests but misses failed-install-after-discard, --force, and lifecycle unlink-then-fail snapshot.
Performance Expert	0	0	0	Single multiline regex in LockFile.read on small YAML; no measurable perf impact; regex precompiled at module scope.

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

Top 5 follow-ups

1. [Test Coverage Expert] (blocking-severity) Add an integration fixture for failed install after conflicted-lock unlink/discard. -- Author-stated limitation: discard is outside the transaction, so durable state after unlink-then-fail is unguarded. git checkout recovers; the gap is a regression trap, not a new product switch.
2. [Python Architect] Move conflicted-lock discard into InstallTransaction so a failed install can restore the prior file. -- Unlink today happens before resolve; later failure cannot roll the conflicted lock back through the existing transaction owner.
3. [Doc Writer] Keep recover vs fail-closed explicit; do not claim every install rewrites a conflicted lock. -- Full install/lock recover; frozen/partial fail closed; dry-run warns and does not rewrite. Over-claim would fight the accepted default.
4. [Doc Writer] Revert unrelated hermes/target wording churn in packages/apm-guide/.apm/skills/apm-usage/commands.md. -- A lockfile-conflict patch should not rewrite the hermes/target table; duplicate install rows already disagree.
5. [Python Architect] Collapse discard vs fail-closed vs warn into one lockfile conflict policy used by install, lock, frozen, dry-run, and MCP. -- Detection is already centralized in LockFile.read; durable outcome is still recomputed at each call site.

Architecture

classDiagram
    direction LR
    class LockFile {
      <<ValueObject>>
      +read(path) LockFile
    }
    class has_conflict_markers {
      <<Pure>>
    }
    class LockfileFormatError {
      <<DomainError>>
    }
    class LockfileConflictError {
      <<DomainError>>
      +path Path
    }
    class discard_conflicted_lockfile {
      <<IOBoundary>>
    }
    class InstallTransaction {
      <<UnitOfWork>>
      +commit(result) InstallResult
      +fail(error) InstallResult
    }
    class InstallService {
      +LockFile.read for frozen
    }
    class FrozenInstallError {
      <<DomainError>>
    }
    LockfileConflictError --|> LockfileFormatError
    LockFile ..> has_conflict_markers : detect
    discard_conflicted_lockfile ..> has_conflict_markers : detect
    LockFile ..> LockfileConflictError : raises
    InstallService ..> LockFile : reads
    InstallService ..> FrozenInstallError : raises
    note for LockFile "Canonical detect: LockFile.read raises LockfileConflictError"
    note for discard_conflicted_lockfile "Outcome fork: unlink on full install and apm lock"
    note for InstallTransaction "Durable lockfile mutation belongs in this unit of work"
    class LockFile:::touched
    class LockfileConflictError:::touched
    class discard_conflicted_lockfile:::touched
    class InstallService:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    installEntry["commands/install.py:install"] --> packages["_install_apm_packages"]
    packages --> migrate["[FS] migrate_lockfile_if_needed"]
    migrate --> fullGate{"full_install: not frozen and not packages and InstallMode.ALL"}
    fullGate -->|yes| discard["[FS] lockfile.py:discard_conflicted_lockfile path.unlink"]
    fullGate -->|no| laterRead["[I/O] LockFile.read"]
    discard --> resolve["_install_apm_dependencies"]
    laterRead --> conflict{"LockfileConflictError?"}
    conflict -->|frozen InstallService| frozenErr["raise FrozenInstallError"]
    conflict -->|partial add or --only| failClosed["fail closed; lockfile left in place"]
    frozenErr --> txnFail["InstallTransaction.fail"]
    lockEntry["commands/lock.py:_run_lock"] --> lockDiscard["[FS] discard_conflicted_lockfile"]
    lockDiscard --> lockResolve["_install_apm_dependencies"]
    mcpEntry["install/mcp/command.py:run_mcp_install"] --> mcpWrite["[FS] add_mcp_to_apm_yml"]
    mcpWrite --> mcpRead["[I/O] LockFile.read during integration"]
    mcpRead -->|LockfileFormatError| mcpClick["raise click.ClickException; apm.yml already written"]
    dryEntry["presentation/dry_run.py:render_and_exit"] --> dryRead["[I/O] LockFile.read"]
    dryRead -->|LockfileConflictError| dryWarn["logger.warning; treat lock as missing"]

Loading

Recommendation

CODEOWNERS should confirm detect-in-load, regenerate on full non-frozen install/lock, and fail-closed under --frozen and partial installs. Fold the docs precision pass (no every-install recover claim; revert commands.md hermes churn) if it is still cheap in this PR. Track unlink-then-fail coverage and transactional discard as follow-ups; leave --force, CI autodetection, and dry-run exit codes out of scope.

---

Full per-persona findings

Python Architect

• [recommended] Conflicted-lockfile outcome is split across call sites instead of one owner. at src/apm_cli/commands/install.py:1837
  Detection is centralized in LockFile.read; durable outcome (unlink vs fail closed vs warn) is recomputed at install, lock, frozen, dry-run, and MCP.
  Suggested: One lockfile conflict policy invoked from those call sites.
• [recommended] discard_conflicted_lockfile unlinks apm.lock.yaml outside InstallTransaction. at src/apm_cli/deps/lockfile.py:1283
  unlink before resolve; later install failure cannot restore the conflicted file via InstallTransaction.
• [nit] MCP install catches LockfileFormatError after writing apm.yml. at src/apm_cli/install/mcp/command.py:308
  Pre-existing order; conflicted lock can leave manifest write without regenerated lock.

CLI Logging Expert

No findings.

DevX UX Expert

• [recommended] Make regenerate vs fail-closed explicit for interactive vs CI
  The PR implements both regenerate-on-full-install and fail-closed for frozen/partial installs. Users and CI need a clear rule about which behavior is the default. CEO: accepted scope already covers this; do not add CI autodetection.
• [recommended] Clarify dry-run output and ensure machine-detectable exit codes
  Dry-run currently reports human-readable outcomes like "would make no changes" vs warnings. Author documented this limitation. CEO: keep warn + exit 0.
• [nit] Address non-transactional lockfile/regeneration risks in UX and docs
  Regenerating without atomic replace risks partial state if interrupted.

Supply Chain Security Expert

No findings.

Doc Writer

• [recommended] Do not over-claim that every install recovers a conflicted lockfile at docs/src/content/docs/reference/cli/install.md
  Full install/lock recover; frozen/partial fail closed; dry-run does not rewrite.
• [recommended] Revert unrelated hermes/target wording churn in commands.md at packages/apm-guide/.apm/skills/apm-usage/commands.md:15
  A lockfile-conflict patch should not rewrite the hermes/target table; duplicate install rows already disagree.
• [nit] State conflict semantics once; point other pages at lockfile-spec at docs/src/content/docs/reference/lockfile-spec.md
  Canonical definition belongs in lockfile-spec; recovery steps in install-failures.md.

Test Coverage Expert

• [blocking] No test that a failed install after discard/unlink leaves durable state safe
  Author-stated limitation: discard outside transaction.
  Proof (missing at): tests/integration/test_install_failed_after_discard_unlink.py
• [recommended] No explicit --force override test
  Product intent: --force is unchanged and is NOT a lock regenerate switch. CEO dropped this follow-up.
  Proof (missing at): tests/integration/test_install_force_override.py
• [recommended] No ApmLifecycle snapshot for unlink-then-fail
  Overlaps the unlink-then-fail coverage gap.
  Proof (missing at): tests/integration/test_lifecycle_unlink_then_fail_snapshot.py

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.}

---

Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.

[Sergio Sisternes (sergio-sisternes-epam)]

Thank you Lachlan Heywood (@lachieh)

I have enabled the merge queue for this PR. Please review the blocker actions from the APM Review Panel. Once the test coverage is fixed.

Optionally, if you can take out the top 5 recommendations, that could help us reduce the technical debt.

Thank you for your contribution.

Sergio

[Lachlan Heywood (lachieh)]

Thanks Sergio Sisternes (@sergio-sisternes-epam). The follow-ups are addressed in #3043. I left this PR as is so the merge queue can take it.

I tried to stack on this branch, but cross-fork PRs can't target fork branches as the base so that branch will show 2 commits until this one merges.

[Lachlan Heywood (lachieh)]

Rebased onto main (CHANGELOG moved under the post-0.31.0 Unreleased section) and added 4456bb6b: the MCP-add path now names only LockfileConflictError at default verbosity, so other lockfile format errors keep their redacted, verbose-only handling. The panel's blocker (unlink-then-fail coverage, with transactional restore) is on the follow-up branch and #3043 will be reopened right after this merges, so its review shows only that diff.

[Lachlan Heywood (lachieh)]

Sergio Sisternes (@sergio-sisternes-epam) the APM Review Panel's blocking item is now fixed in this PR rather than a stacked follow-up, so there is nothing left to merge separately.

41a919a9 moves the conflicted-lockfile discard into InstallTransaction: it snapshots the bytes before unlinking and rollback() restores the file unless the attempt already wrote a new one. That closes follow-ups 1 and 2 together (the missing unlink-then-fail coverage and the non-transactional discard), with unit coverage in test_install_transaction.py and an e2e case asserting both apm install and apm lock restore the conflicted file after a failed run.

7943869b registers the decision as an architecture owner (conflicted-lockfile-discard, guard install-deployment-conflicted-lockfile-discard) so no future caller can fork the policy undetected, which is follow-up 5 plus Copilot's request on the closed PR. It also has apm lock migrate a legacy apm.lock before the discard probe and run its transaction as a context manager, so an interrupt after the discard still restores.

Follow-up 3 (docs precision) is in the docs commits. On follow-up 4, the commands.md diff here is only the --frozen phrase in the five pre-existing duplicate apm install rows; the hermes/target differences between those rows predate this PR. Per the panel's own dissent I left --force, CI autodetection, and dry-run exit codes out of scope.

I originally split this into #3043, but GitHub cannot base a cross-fork PR on a fork branch and stacked PRs are not enabled here, so that PR's diff always included this one's commits and reviewers kept re-reviewing approved code. #3043 is closed and its work is here.