fix(install): restore a discarded conflicted lockfile when the run fails - #3043
Closed
Lachlan Heywood (lachieh) wants to merge 5 commits into
Closed
Lachlan Heywood (lachieh) wants to merge 5 commits into
Lachlan Heywood (lachieh) wants to merge 5 commits into
Conversation
Lachlan Heywood (lachieh)
requested review from
Daniel Meppiel (danielmeppiel) and
Sergio Sisternes (sergio-sisternes-epam)
as code owners
September 20, 2026 12:25
Lachlan Heywood (lachieh)
force-pushed
the
lockfile-conflict-transaction
branch
from
September 20, 2026 12:25
874dbfd to
2ede637
Compare
12 tasks
Copilot started reviewing on behalf of
Lachlan Heywood (lachieh)
September 20, 2026 12:26
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate findings remain in legacy-lockfile handling, interruption rollback, and MCP error handling.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR makes conflicted lockfile removal transactional, restoring the original file when full installation or apm lock fails.
Changes:
- Adds conflict detection and transactional discard/restore behavior.
- Preserves fail-closed behavior for frozen, partial, and dry-run modes.
- Adds tests, documentation, usage guidance, and changelog updates.
| File | Summary | Final review note |
|---|---|---|
tests/unit/install/test_install_transaction.py |
Tests snapshot, discard, rollback, and restoration behavior. | — |
tests/unit/install/test_frozen.py |
Tests frozen-mode conflict handling. | — |
tests/unit/deps/test_lockfile_conflict_markers.py |
Tests conflict-marker detection. | — |
tests/integration/test_install_conflicted_lockfile_e2e.py |
Covers install and lock recovery flows. | — |
src/apm_cli/install/transaction.py |
Owns discarded lockfile restoration. | Nit (2 votes): Add an owner record and registry-backed architecture guard covering the transaction and its callers. |
src/apm_cli/install/service.py |
Improves frozen-mode diagnostics. | Moderate (1 vote): Resolve legacy lockfile paths for frozen reads. |
src/apm_cli/install/presentation/dry_run.py |
Reports conflicts without mutation. | Moderate (1 vote): Resolve legacy lockfile paths during dry runs. |
src/apm_cli/install/mcp/command.py |
Handles lockfile format failures. | Moderate (1 vote): Catch LockfileConflictError specifically so other format errors retain redacted actionable handling. |
src/apm_cli/install/errors.py |
Refines frozen recovery guidance. | — |
src/apm_cli/deps/lockfile.py |
Detects conflict markers and raises the conflict error. | — |
src/apm_cli/commands/lock.py |
Adds transactional lock generation. | Moderate (1 vote): Handle supported legacy apm.lock files before discard checks. Moderate (1 vote): Ensure BaseException paths roll back after discard. |
src/apm_cli/commands/install.py |
Applies discard policy to full installs. | — |
packages/apm-guide/.apm/skills/apm-usage/troubleshooting.md |
Adds recovery guidance. | — |
packages/apm-guide/.apm/skills/apm-usage/commands.md |
Updates frozen-mode guidance. | — |
docs/src/content/docs/troubleshooting/install-failures.md |
Documents conflict recovery and rollback. | — |
docs/src/content/docs/reference/lockfile-spec.md |
Defines conflict semantics. | — |
docs/src/content/docs/reference/cli/lock.md |
Documents lock recovery. | Nit (1 vote): Qualify restoration as applying before a replacement lockfile is written. |
docs/src/content/docs/reference/cli/install.md |
Documents frozen and full-install behavior. | — |
CHANGELOG.md |
Records the fix. | — |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Lachlan Heywood (lachieh)
force-pushed
the
lockfile-conflict-transaction
branch
from
September 20, 2026 23:22
f62b85a to
c77a49e
Compare
… under --frozen LockFile.read raises LockfileConflictError, a LockfileFormatError, when apm.lock.yaml contains git merge conflict markers, so every command names the file and a next action instead of printing a YAML scanner error. A full apm install and apm lock warn, discard the conflicted file, and resolve from apm.yml. Partial installs (positional packages, --only, --mcp) and apm install --frozen fail closed with the named error, and the frozen tip no longer points at apm outdated when the lockfile cannot be read. Fixes microsoft#2979
…tions Keep the conflict-marker read inside LockFile.read's normalising try so a non-UTF-8 lockfile still fails through LockfileFormatError, and leave such a file in place instead of probing it for discard. The generic --frozen unreadable-lockfile error now names the repair step. apm lock export resolves a legacy apm.lock like the other read-only consumers. Narrow the docs claim to commands that require the lockfile, update the apm-guide skill resources, mark the new test modules as component tests, and cite the PR number in the changelog.
Other lockfile format errors keep the redacted, verbose-only handling that path had before microsoft#3028 widened the except clause.
InstallTransaction now owns the conflicted-lockfile discard: it snapshots the bytes before unlinking and rollback puts the file back unless the attempt already wrote a new lockfile. apm lock runs under its own transaction so the same rule applies there. The module-level discard_conflicted_lockfile helper is removed. Follow-up to microsoft#3028 from the APM Review Panel.
…en apm lock Record InstallTransaction.discard_conflicted_lockfile as the canonical owner in .apm/architecture/owners/install-deployment.json with the install-deployment-conflicted-lockfile-discard guard, its linter check, and a mutation-matrix case proving rollback must restore the file. apm lock now migrates a legacy apm.lock before the discard probe and runs the transaction as a context manager so an interrupt after the discard still restores the conflicted lockfile. The MCP add path names only the conflict error at default verbosity; other lockfile format errors keep their redacted handling.
Lachlan Heywood (lachieh)
force-pushed
the
lockfile-conflict-transaction
branch
from
September 20, 2026 23:27
c77a49e to
0f17318
Compare
Contributor
Author
|
Closing temporarily. GitHub cannot base a cross-fork PR on a fork branch and stacked PRs are not enabled on this repository, so this PR's diff includes #3028's commits and Copilot keeps re-reviewing already-approved changes. The branch is rebased onto the current |
Contributor
Author
Contributor
Author
|
Superseded by #3028. Both commits from this branch ( Splitting this out did not work: GitHub cannot base a cross-fork PR on a fork branch and stacked PRs are not enabled on this repository, so this PR's diff always carried #3028's commits and reviewers re-reviewed already-approved code. Nothing here is dropped. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #3028, stacked on its branch. The branch lives on my fork, so this PR targets
mainand shows #3028's two commits until it merges; the change here is the last two commits (2ede6371,f62b85a5). It addresses the APM Review Panel's blocking item and the top follow-ups from the panel comment.1. Failed install after discard (blocking, Test Coverage Expert) and transactional discard (Python Architect #2). These are the same gap, so one fix closes both.
InstallTransaction.discard_conflicted_lockfile(path)replaces the module-level helper: it snapshots the conflicted bytes before unlinking, androllback()restores the file unless the attempt already wrote a new lockfile.apm locknow runs under its ownInstallTransaction(previously it relied on the pipeline's compatibility transaction, which never saw the discard), so the same rule applies there.Coverage:
tests/unit/install/test_install_transaction.py(discard thenfail()restores byte-exact; rollback keeps a lockfile written after the discard; commit does not restore; valid, corrupt, undecodable, and missing files are never discarded) andtests/integration/test_install_conflicted_lockfile_e2e.py::test_failed_run_after_discard_restores_the_conflicted_lockfilefor bothapm installandapm lockagainst a manifest whose dependency cannot resolve. Verified manually as well: both commands printapm.lock.yaml restored to its previous state.and leave the conflicted bytes intact; a successful run still regenerates.2. One conflict policy (Python Architect #5). Detection stays in
LockFile.read. The only durable outcome that differs from "raise" is the discard, and it now has one owner (the transaction) with two callers (apm installfull mode,apm lock). Frozen, partial, dry-run, and MCP paths do not compute a policy; they read and letLockfileConflictErrorsurface. I did not add a policy object over that: it would wrap one boolean.3. Docs precision (Doc Writer #3).
install.mdnow says a fullapm install(no package arguments, no--only) discards;lockfile-spec.md,lock.md, andinstall-failures.mdstate that a failed run restores the file. Frozen, partial, and dry-run wording is unchanged from #3028, which already said they never rewrite.4.
commands.md"hermes/target churn" (Doc Writer #4). Not applicable.git diff main...install-on-invalid-lockfile --word-diff -- packages/apm-guide/.apm/skills/apm-usage/commands.mdshows the only change ismissing->missing, contains git merge conflict markers,in the--frozenphrase, applied to each of the five pre-existing duplicateapm installrows. The hermes/target differences between those rows predate both PRs.5. Copilot review on this PR. The discard/restore decision is now a registered architecture owner (
conflicted-lockfile-discardin.apm/architecture/owners/install-deployment.json) with theinstall-deployment-conflicted-lockfile-discardguard: the linter check requires the transaction to define discard and restore and call restore fromrollback(), requires both command callers to go through the transaction, and flags any otherhas_conflict_markersuse ordiscard_conflicted_lockfiledefinition undersrc/. The mutation matrix proves the guard fires when rollback stops restoring. Also from that review:apm lockmigrates a legacyapm.lockbefore the discard probe and runs its transaction as a context manager so an interrupt after the discard still restores the file (both covered by e2e tests), and the MCP add path names onlyLockfileConflictErrorat default verbosity so other format errors keep their redacted handling. Theservice.py/dry_run.pylegacy-path notes describe behaviour that predates #2979 (enforce_frozenand the dry-run preview never resolvedapm.lock), so they are left for a separate issue.Out of scope, per the panel's own dissent:
--force(not a regenerate switch), CI autodetection, dry-run exit codes.Issue and approved scope
Issue: #2979 (accepted; implementation PR #3028)
Human scope-approval comment: maintainer request on #3028 to fix the panel's blocker and, optionally, the top-5 follow-ups.
Does this PR complete the issue? #3028 completes #2979; this PR closes the panel follow-ups on it. It references the issue without closing it.
Type of change
Testing
tests/unit+tests/test_console.py: 22534 passed.tests/integration/test_install_*.py,test_lock*.py,test_mcp_*.py,test_architecture_owner_rule_mutations.py,test_architecture_linter_entrypoint.py,tests/unit/scripts/test_architecture_runner.py, andtests/quality: pass, excepttest_matrix_covers_every_registry_guard_exactly_once, which fails identically onmain(two unrelated guards missing from the matrix).bash scripts/lint-architecture-boundaries.shexits 0.ruff check,ruff format --checkclean;mypyon the touched modules adds no errors.Spec conformance (OpenAPM v0.1)