From ef1922c45f3d4dca6f852b7c70322ea2cb962382 Mon Sep 17 00:00:00 2001 From: Sebastian Alpers Date: Tue, 28 Jul 2026 15:41:27 +0200 Subject: [PATCH 1/3] fix(install): stop --frozen rewriting the lockfile it freezes req-lk-006 requires a frozen-install mode "in which the lockfile is never written or rewritten and the install fails on any direct dependency for which the lockfile has no pin". Only the second clause was implemented, by lockfile_satisfies_manifest. A frozen install deployed files and then rewrote apm.lock.yaml to claim them, printing "Lockfile presence verified". That silently repairs the state issue #2379 is about. A committed lockfile that under-records the deployed set is the everyday mistake (commit a deployed target, forget the regenerated lockfile), and the files it omits carry no recorded hash, which puts them outside content-integrity's hash comparison AND its hidden-Unicode scan. CI running `apm install --frozen` was the one place positioned to catch it and instead laundered it, which is why consumers hand-roll `git diff --exit-code apm.lock.yaml`. Install reaches LockFile.write from eight call sites, so suppression lives at the one chokepoint they funnel through rather than as a flag threaded to each -- a new write site inherits the guarantee. Writes are recorded rather than dropped, so the service can compare what was withheld against the committed lockfile and fail with the unrecorded paths named. The comparison is one-directional: only paths the install would ADD to the ledger fail. Claims it would drop are tolerated, matching the tolerance FrozenInstallError already documents for removed deps, and because a legitimately narrower install (--target filter, --only) produces exactly that. This repo's own committed lockfile is such a case -- a local install drops 180 deployment-ledger rows -- and it passes. Equivalence uses is_semantically_equivalent, which ignores generated_at and apm_version per Section 5.5 ("their absence MUST NOT affect content-equivalence comparison"), so a newer CLI reading an older lockfile is not a rewrite. Deployment still happens, as with `npm ci`. The remedy differs from the pre-existing frozen failure, so FrozenInstallError now carries its own tip instead of both Click handlers hardcoding one. --- CHANGELOG.md | 12 ++ CONFORMANCE.json | 3 +- CONFORMANCE.md | 2 +- docs/src/content/docs/integrations/ci-cd.md | 1 + .../src/content/docs/reference/cli/install.md | 7 +- src/apm_cli/deps/lockfile.py | 46 +++++ src/apm_cli/install/errors.py | 31 +++- src/apm_cli/install/service.py | 142 ++++++++++++--- tests/spec_conformance/test_lockfile_reqs.py | 60 ++++++ tests/unit/install/test_frozen.py | 172 +++++++++++++++++- 10 files changed, 435 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44f9f98b05..ef8d8fe812 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `apm install --frozen` no longer writes `apm.lock.yaml`, which req-lk-006 + requires it to leave untouched. It previously deployed files and rewrote the + lockfile to claim them, so a committed lockfile that under-recorded the + deployed set was silently repaired in CI instead of being reported -- and the + files it omitted stayed outside `content-integrity`'s hash and hidden-Unicode + scanners. Frozen installs now fail and name the unrecorded paths. The check is + one-directional -- claims the install would drop (a `--target` filter, + `--only`, a removed dependency) are tolerated, as removed deps already were -- + and `generated_at` / `apm_version` are excluded, so a newer CLI reading an + older lockfile is not treated as a rewrite. (#2379) + If CI starts failing on `--frozen` after upgrading, run `apm install` + locally and commit the updated `apm.lock.yaml`. - Partial dependency updates preserve concrete deployment targets for refreshed and untouched packages, including skills under `.agents/skills/`, instead of demoting them to `legacy`. (#2924) - Transient resolution-staging paths are shorter, so `apm install` no longer fails with `[WinError 206] The filename or extension is too long.` from a deep Windows checkout. The staging root drops from a full `uuid4().hex` to 12 hex characters and each per-destination slot from a full SHA-256 digest to 16, freeing 68 characters on every staged path. This is not a guarantee of arbitrary long-path support. Orphaned staging roots left by earlier versions are still cleaned up. (#2896) diff --git a/CONFORMANCE.json b/CONFORMANCE.json index 07fdca397f..1f0305be1f 100644 --- a/CONFORMANCE.json +++ b/CONFORMANCE.json @@ -121,8 +121,9 @@ "keyword": "MUST", "section": "5.5", "status": "active", - "test_count": 1, + "test_count": 2, "tests": [ + "tests/spec_conformance/test_lockfile_reqs.py::TestFrozenInstallNeverWritesLockfile::test_frozen_install_withholds_the_write_and_fails_instead", "tests/spec_conformance/test_lockfile_reqs.py::test_frozen_mcp_validation_fails_before_durable_mutation" ] }, diff --git a/CONFORMANCE.md b/CONFORMANCE.md index f4bc15cc77..be456b746b 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -46,7 +46,7 @@ Repository-coordinate segments are case-insensitive for `github.com`, GitHub Ent | [req-lk-003](docs/src/content/docs/specs/openapm-v0.1.md#req-lk-003) | MUST | 5.2 | consumer | active | 2 | - | | [req-lk-004](docs/src/content/docs/specs/openapm-v0.1.md#req-lk-004) | MUST | 5.4 | consumer | active | 1 | - | | [req-lk-005](docs/src/content/docs/specs/openapm-v0.1.md#req-lk-005) | MUST | 5.5 | consumer | active | 2 | - | -| [req-lk-006](docs/src/content/docs/specs/openapm-v0.1.md#req-lk-006) | MUST | 5.5 | consumer | active | 1 | - | +| [req-lk-006](docs/src/content/docs/specs/openapm-v0.1.md#req-lk-006) | MUST | 5.5 | consumer | active | 2 | - | | [req-lk-007](docs/src/content/docs/specs/openapm-v0.1.md#req-lk-007) | SHOULD | 5.5 | consumer | active | 1 | - | | [req-lk-008](docs/src/content/docs/specs/openapm-v0.1.md#req-lk-008) | MUST | 5.6 | consumer | active | 1 | - | | [req-lk-009](docs/src/content/docs/specs/openapm-v0.1.md#req-lk-009) | MUST | 5.6 | consumer | active | 1 | - | diff --git a/docs/src/content/docs/integrations/ci-cd.md b/docs/src/content/docs/integrations/ci-cd.md index 730aed13ee..0ab8eebe4f 100644 --- a/docs/src/content/docs/integrations/ci-cd.md +++ b/docs/src/content/docs/integrations/ci-cd.md @@ -242,6 +242,7 @@ See the [Pack a bundle guide](../../producer/pack-a-bundle/) for the full workfl - **Pin APM version** in CI to avoid unexpected changes: `pip install apm-cli==0.22.0` - **Commit `apm.lock.yaml`** so CI resolves the same dependency versions as local development +- **Install with `apm install --frozen` in CI** to verify the committed lockfile is actually current. It never writes `apm.lock.yaml` and exits `1`, naming the paths, if the install deploys files the lockfile does not record. A file the lockfile omits carries no recorded hash, which puts it outside the `content-integrity` check below -- so this is the gate that keeps that check's scope honest, and it replaces a hand-rolled `git diff --exit-code apm.lock.yaml` step. - **Commit `.github/`, `.claude/`, `.cursor/`, `.opencode/`, and `.gemini/` deployed files** so contributors and cloud-based Copilot get agent context without running `apm install` - **If using `apm compile`** (for Codex, Gemini instructions), run it in CI and fail the build if the output differs from what's committed - **Use `GITHUB_APM_PAT`** for private dependencies; never use the default `GITHUB_TOKEN` for cross-repo access diff --git a/docs/src/content/docs/reference/cli/install.md b/docs/src/content/docs/reference/cli/install.md index 7b092b560d..5758742f24 100644 --- a/docs/src/content/docs/reference/cli/install.md +++ b/docs/src/content/docs/reference/cli/install.md @@ -30,7 +30,7 @@ With no arguments it installs everything from `apm.yml`. With one or more `PACKA | Flag | Default | Description | |---|---|---| | `--update` | off | Re-resolve dependencies to the latest version or Git ref allowed by `apm.yml` and rewrite `apm.lock.yaml`. Mutable Git refs must resolve against upstream; APM does not fall back to stale refs from the local bare Git cache. Mutually exclusive with `--frozen`. For interactive use with a confirmation prompt, use [`apm update`](../update/) instead. | -| `--frozen` | off | Lockfile-only install: refuse to resolve anything new and fail before any project, config, deployment, or cache write if `apm.lock.yaml` is missing or out of sync with `apm.yml`, including MCP state. Mirrors `npm ci`. Mutually exclusive with `--update`, positional package additions, and `--mcp`. | +| `--frozen` | off | Lockfile-only install: never write `apm.lock.yaml`, refuse to resolve anything new, and fail before any project, config, deployment, or cache write if `apm.lock.yaml` is missing or out of sync with `apm.yml`, including MCP state. Also fails -- after deployment -- if the install deploys files the lockfile does not record. Mirrors `npm ci`. Mutually exclusive with `--update`, positional package additions, and `--mcp`. | | `--dry-run` | off | Print the install plan without deployment writes. Positional packages and ref changes appear in the preview after validation but do not change an existing `apm.yml`. Project auto-bootstrap still keeps its new manifest and any explicit `--target` selection for the next run; global dry-run bootstrap uses temporary preview state and does not create `~/.apm`. The `-g --mcp` path creates no user manifest, lockfile, or runtime configuration. | | `--force` | off | Overwrite locally-authored files on collision **and** bypass the security scan's critical-finding block. Does **not** suppress general install errors (any reported error still exits `1`, matching npm / pip / cargo) or select ref freshness. Add `--update` or `--refresh` to resolve mutable refs upstream; [`apm update`](../update/) does so with or without `--force`. Use only after independent verification. | | `--verbose`, `-v` | off | Show per-file paths and full error context in the diagnostic summary. | @@ -150,7 +150,8 @@ in `apm.yml`, then run `apm install` again. - **Lockfile replay and Git ref freshness.** Plain and `--frozen` installs may trust `apm.lock.yaml` and the local Git cache, reusing the locked commit for unchanged Git dependencies across the full resolved graph. In contrast, `apm install --update`, `apm install --refresh`, [`apm update`](../update/) with or without `--force`, [`apm lock --update`](../lock/), and [`apm outdated`](../outdated/) establish mutable Git refs from upstream instead of accepting stale refs from a local bare Git cache. APM picks up upstream changes to a transitive package's `apm.yml` only when you regenerate the graph -- run `apm update` or `apm lock --update`. See the [lockfile specification](../../lockfile-spec/) for the replay contract. - **Semver ranges on git deps.** `ref:` accepts semver ranges (`^1.2.0`, `~1.4`, `>=2.0 <3`, `1.5.x`) for git-source deps, including positional virtual-subdirectory references. APM runs `git ls-remote` against the dep, picks the highest tag matching the range, and pins the resolved tag plus commit SHA, version, and original constraint in `apm.lock.yaml`. Subsequent installs replay the lockfile without network; use `--update` (or change the manifest constraint) to re-resolve. See [manage dependencies](../../../consumer/manage-dependencies/#pin-a-semver-range) for the supported syntax. - **No-op nudge.** When the lockfile is already satisfied and nothing needs deploying, install prints `[i] Run 'apm update' to check for newer versions.` so you know the silent success was not a missed refresh. -- **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. See [`config-consistency`](../../baseline-checks/#config-consistency) for the full manifest rule. Run normal `apm install` to create or repair MCP-only lock state, 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. +- **Frozen mode.** With `--frozen`, install resolves only what is in `apm.lock.yaml` and **never writes it**. 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. See [`config-consistency`](../../baseline-checks/#config-consistency) for the full manifest rule. Run normal `apm install` to create or repair MCP-only lock state, 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. Deployment still happens, as with `npm ci`. This is a structural check, not a content check -- run `apm audit --ci` for hash verification. +- **Frozen mode fails rather than rewriting.** If the install deploys files the committed `apm.lock.yaml` does not record, `--frozen` exits `1` and names those paths instead of silently updating the lockfile. A file no lockfile row claims carries no recorded hash, which leaves it outside `content-integrity`'s hash comparison *and* its hidden-Unicode scan, so this is the install-time half of that guarantee; `apm audit --ci` remains the on-disk content check. Unlike the structural checks above this one runs after the pipeline, so the deployed files are already on disk -- only the lockfile write was withheld. Recover by running `apm install` without `--frozen` and committing the updated `apm.lock.yaml`. The check is one-directional and matches the tolerance above: claims the install would *drop* (a `--target` filter, `--only`, a removed dependency) do not fail. `generated_at` and `apm_version` are excluded, so a newer CLI reading an older lockfile is not a rewrite. - **Local `.apm/` deployment.** After dependencies are integrated, primitives in the project's own `.apm/` directory are deployed to the same targets. Local files win on collision. Skipped at `--global` and with `--only mcp`. - **User-scope root context hint.** Compilation stays explicit. After `apm install -g`, targets with native user-scope instruction files pick up global instructions during install. Targets whose user-scope instruction surface is a root context file require [`apm compile --global`](../compile/#global-compilation); install prints a one-line `[i]` hint and writes no root context file. - **OpenCode user scope.** `apm install -g --target opencode` deploys skills to @@ -267,7 +268,7 @@ apm install owner/skill-bundle --skill '*' # reset to all skills | Code | Meaning | |---|---| | `0` | Successful install or `--dry-run` preview. A preview does not certify real install success. For Agent Plugins v1 packages, a mixed install still succeeds when target exclusion skips one package but at least one other package deploys. | -| `1` | Install failure: security scan blocked a critical finding, auth error, manifest or required MCP/LSP config write error, dependency resolution error, Agent Plugins v1 target exclusion left no package deployed on a non-dry-run install, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, any reported install error (the diagnostic summary closes with `Installation failed with N error(s)`), or unhandled exception. `--force` does **not** suppress general install errors. The diagnostic summary names the cause. | +| `1` | Install failure: security scan blocked a critical finding, auth error, manifest or required MCP/LSP config write error, dependency resolution error, Agent Plugins v1 target exclusion left no package deployed on a non-dry-run install, `--frozen` with a missing lockfile, a direct dependency absent from `apm.lock.yaml`, or an install that deploys files `apm.lock.yaml` does not record, any reported install error (the diagnostic summary closes with `Installation failed with N error(s)`), or unhandled exception. `--force` does **not** suppress general install errors. The diagnostic summary names the cause. | | `2` | Usage error: no deployment target detectable (no `--target`, no `target(s):` in `apm.yml`, no default target configured via `apm config set target `, and no harness signal in the project), `--ssh` and `--https` both passed, `--frozen` and `--update` both passed, `--root` combined with `--global`, or a Click flag conflict. | ## Notes diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index 7fb23cfdd5..740a297481 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -7,6 +7,9 @@ import logging import os +from collections.abc import Iterator +from contextlib import contextmanager +from contextvars import ContextVar from dataclasses import dataclass, field from datetime import datetime, timezone from pathlib import Path @@ -39,6 +42,41 @@ class _ExistingLockfileUnset: _EXISTING_LOCKFILE_UNSET = _ExistingLockfileUnset() +# req-lk-006 requires a frozen-install mode "in which the lockfile is never +# written or rewritten". An install reaches ``LockFile.write`` from eight +# call sites (dependency lockfile build, local bundles, target-contraction +# reconciliation, local-content persist, MCP and LSP integration, ...), so +# the suppression lives at the one chokepoint they all funnel through +# instead of as a flag threaded to each -- a new write site then inherits +# the guarantee rather than needing to remember it. +# +# Suppressed writes are *recorded*, not merely dropped: the caller compares +# them against the committed lockfile to decide whether the frozen install +# can honestly report success (see ``InstallService``). +# +# A ContextVar rather than a module global so the mode cannot leak across +# concurrent installs in one process (tests, programmatic callers). +_suppressed_writes: ContextVar[list[str] | None] = ContextVar( + "apm_suppressed_lockfile_writes", default=None +) + + +@contextmanager +def suppress_lockfile_writes() -> Iterator[list[str]]: + """Record and discard every :meth:`LockFile.write` in this context. + + Yields the list of serialised lockfiles that *would* have been written, + in call order. Serialised rather than held by reference so that later + mutation of the same in-memory :class:`LockFile` cannot rewrite + history: each entry is exactly the content that ``write`` withheld. + """ + attempts: list[str] = [] + token = _suppressed_writes.set(attempts) + try: + yield attempts + finally: + _suppressed_writes.reset(token) + def installed_apm_version() -> str: """Return the running APM distribution version for lockfile metadata.""" @@ -983,6 +1021,10 @@ def write( ) -> None: """Write lock file to disk, preserving legacy timestamp behavior. + Inside :func:`suppress_lockfile_writes` (``apm install --frozen``, + req-lk-006) the serialised content is recorded for the caller and + the file on disk is left byte-for-byte untouched. + New lockfiles omit ``generated_at``. When the on-disk lockfile already carries the field, keep it stable for semantic no-ops and refresh it for substantive writes. This behavior should be changed to remove the legacy @@ -992,6 +1034,10 @@ def write( Callers that already loaded the destination can pass ``existing_lockfile`` to avoid parsing the same bytes again. """ + suppressed = _suppressed_writes.get() + if suppressed is not None: + suppressed.append(self.to_yaml()) + return from ..utils.atomic_io import atomic_write_text from ..utils.staging_guard import assert_no_staging_paths from ..utils.yaml_io import load_yaml_str diff --git a/src/apm_cli/install/errors.py b/src/apm_cli/install/errors.py index cd635ad59a..f3510c87a2 100644 --- a/src/apm_cli/install/errors.py +++ b/src/apm_cli/install/errors.py @@ -73,24 +73,41 @@ def __init__(self, message: str, *, diagnostic_context: str = ""): class FrozenInstallError(RuntimeError): """Raised when ``apm install --frozen`` cannot proceed. - Two trigger conditions: + Three trigger conditions: * Lockfile (``apm.lock.yaml``) is missing entirely. * Lockfile is structurally out of sync with ``apm.yml`` -- a direct dependency declared in the manifest has no entry in the lockfile. In that case ``reasons`` carries one human-readable line per missing dep so the renderer can list them. + * The install deploys files the committed lockfile does not record, so + honouring req-lk-006's "never written or rewritten" would have left + the project claiming less than it deploys -- and unclaimed files are + outside the audit's content checks. ``reasons`` names those paths. - The check is intentionally narrow: it flags the cases where running - install without ``--frozen`` would mutate the lockfile. Drift in + The first two are structural and run before the pipeline. Drift in transitive deps or removed deps is allowed, mirroring how ``uv`` treats ``--frozen`` and how ``npm ci`` only enforces direct-deps - presence. + presence; the third follows the same rule and ignores claims the + install would *drop*. + + ``tip`` is the remediation line the CLI prints, carried on the error + because the two conditions have different remedies and both Click + handlers render this exception the same way. """ - def __init__(self, message: str, *, reasons: list[str] | None = None): + DEFAULT_TIP = "Tip: run 'apm outdated' to see what changed, then 'apm update'." + + def __init__( + self, + message: str, + *, + reasons: list[str] | None = None, + tip: str = DEFAULT_TIP, + ): super().__init__(message) self.reasons = list(reasons or []) + self.tip = tip def frozen_install_tip(error: FrozenInstallError) -> str: @@ -104,7 +121,9 @@ def frozen_install_tip(error: FrozenInstallError) -> str: ) if has_mcp_drift: return "Tip: run 'apm install' without --frozen to create or repair MCP lock state." - return "Tip: run 'apm outdated' to see what changed, then 'apm update'." + # Falls back to the error's own tip: the no-rewrite check has a different + # remedy than drift and carries it on the exception (see FrozenInstallError). + return error.tip class PolicyViolationError(RuntimeError): diff --git a/src/apm_cli/install/service.py b/src/apm_cli/install/service.py index 9dd5699dfd..0580ebd8d5 100644 --- a/src/apm_cli/install/service.py +++ b/src/apm_cli/install/service.py @@ -115,8 +115,9 @@ def run(self, request: InstallRequest) -> InstallResult: to import (e.g. missing optional extras). Adapters are responsible for presenting this to the user. FrozenInstallError: when ``request.frozen`` is True and the - lockfile is missing or structurally out of sync with - ``request.apm_package``. Raised before the pipeline + lockfile is missing, structurally out of sync with + ``request.apm_package``, or not what the install + produces. The first two are raised before the pipeline runs so no resolve / download work is wasted. """ # Enforce --frozen BEFORE invoking the pipeline. The check is @@ -126,6 +127,35 @@ def run(self, request: InstallRequest) -> InstallResult: if request.frozen: self.enforce_frozen(request) + runner = self._build_script_runner(request) + event = self._build_event("pre-install", request) + runner.fire("pre-install", event) + + if request.frozen: + # req-lk-006: the lockfile is never written in frozen mode. + # Suppress at the chokepoint, then decide from what was + # withheld whether this install can report success. The + # pipeline still deploys, as `npm ci` does. + from apm_cli.deps.lockfile import suppress_lockfile_writes + + with suppress_lockfile_writes() as withheld: + result = self._run_pipeline(request) + self._enforce_frozen_no_rewrite(request, withheld) + else: + result = self._run_pipeline(request) + + if result.disposition in { + InstallDisposition.SUCCESS, + InstallDisposition.PARTIAL_SUCCESS, + }: + post_event = self._build_event("post-install", request) + runner.fire("post-install", post_event) + + return result + + @staticmethod + def _run_pipeline(request: InstallRequest) -> InstallResult: + """Invoke the install pipeline for *request*.""" # Local import keeps service module import-cheap and matches the # existing pipeline's lazy-import discipline. try: @@ -133,11 +163,7 @@ def run(self, request: InstallRequest) -> InstallResult: except ImportError as e: # pragma: no cover -- defensive raise InstallNotAvailableError(f"APM dependency system not available: {e}") from e - runner = self._build_script_runner(request) - event = self._build_event("pre-install", request) - runner.fire("pre-install", event) - - result = run_install_pipeline( + return run_install_pipeline( request.apm_package, update_refs=request.update_refs, verbose=request.verbose, @@ -166,15 +192,6 @@ def run(self, request: InstallRequest) -> InstallResult: transaction=request.transaction, ) - if result.disposition in { - InstallDisposition.SUCCESS, - InstallDisposition.PARTIAL_SUCCESS, - }: - post_event = self._build_event("post-install", request) - runner.fire("post-install", post_event) - - return result - # -- Lifecycle script helpers ------------------------------------------ @staticmethod @@ -261,20 +278,11 @@ def enforce_frozen(request: InstallRequest) -> None: loads it, and checks both package dependencies and the canonical current MCP config view. Any miss raises before install mutation. """ - from pathlib import Path - from apm_cli.deps.lockfile import LockFile from apm_cli.install.errors import FrozenInstallError from apm_cli.install.plan import lockfile_satisfies_manifest - manifest_path = getattr(request.apm_package, "package_path", None) - if manifest_path is None: - project_dir = Path(".") - elif Path(manifest_path).is_file(): - project_dir = Path(manifest_path).parent - else: - project_dir = Path(manifest_path) - lockfile_path = project_dir / "apm.lock.yaml" + lockfile_path = _frozen_lockfile_path(request) if not lockfile_path.exists(): raise FrozenInstallError( @@ -349,3 +357,85 @@ def enforce_frozen(request: InstallRequest) -> None: "--frozen: apm.lock.yaml is out of sync with apm.yml.", reasons=reasons, ) + + @staticmethod + def _enforce_frozen_no_rewrite(request: InstallRequest, withheld: list[str]) -> None: + """Raise when a withheld write claims deployed files disk does not. + + Withholding the write satisfies req-lk-006 on its own, but silently: + a committed lockfile that under-records the deployed set would stay + green while install deployed files it does not claim, and a file no + lockfile row claims carries no recorded hash, which puts it outside + ``content-integrity``'s hash comparison *and* its hidden-Unicode + scan for as long as it stays omitted (#2379). CI is the one place + positioned to catch that, so frozen mode reports it. + + Deliberately one-directional, matching the tolerance + :class:`FrozenInstallError` already documents. Only paths the + install would *add* to the ledger fail; claims it would drop do + not, because a legitimately narrower install (``--target`` filter, + ``--only``, a removed dependency) produces exactly that and + ``no-orphaned-packages`` / ``deployed-files-present`` already own + the opposite direction. + + Equivalence is judged by ``is_semantically_equivalent``, which + ignores ``generated_at`` and ``apm_version``: per Section 5.5 those + two fields "MUST NOT affect content-equivalence comparison", so a + newer CLI reading an older lockfile is not by itself a rewrite. + """ + if not withheld: + return + + from apm_cli.deps.lockfile import LockFile + from apm_cli.install.drift import _collect_tracked_files + from apm_cli.install.errors import FrozenInstallError + + committed = LockFile.read(_frozen_lockfile_path(request)) + if committed is None: # pragma: no cover -- enforce_frozen ran first + return + committed_paths = set(_collect_tracked_files(committed)) + + unclaimed: set[str] = set() + for payload in withheld: + candidate = LockFile.from_yaml(payload) + if committed.is_semantically_equivalent(candidate): + continue + unclaimed |= set(_collect_tracked_files(candidate)) - committed_paths + if not unclaimed: + return + + raise FrozenInstallError( + "--frozen: apm.lock.yaml does not record everything this install deploys.", + reasons=[ + f" - {path} is deployed by this install but not recorded in apm.lock.yaml" + for path in _outermost(unclaimed) + ], + tip="Tip: run 'apm install' without --frozen, then commit apm.lock.yaml.", + ) + + +def _outermost(paths: set[str]) -> list[str]: + """Sort *paths*, dropping any already covered by a listed ancestor. + + ``deployed_files`` records a primitive's directory as well as each file + beneath it, so an unrecorded skill otherwise reports twice. Sorted + order puts an ancestor before its descendants, so a single pass over + the accumulated output is enough. + """ + outermost: list[str] = [] + for path in sorted(paths): + if not any(path.startswith(f"{parent}/") for parent in outermost): + outermost.append(path) + return outermost + + +def _frozen_lockfile_path(request: InstallRequest) -> Path: + """Locate ``apm.lock.yaml`` beside the request's manifest.""" + manifest_path = getattr(request.apm_package, "package_path", None) + if manifest_path is None: + project_dir = Path(".") + elif Path(manifest_path).is_file(): + project_dir = Path(manifest_path).parent + else: + project_dir = Path(manifest_path) + return project_dir / "apm.lock.yaml" diff --git a/tests/spec_conformance/test_lockfile_reqs.py b/tests/spec_conformance/test_lockfile_reqs.py index 628fbae9ab..e7ed19b23d 100644 --- a/tests/spec_conformance/test_lockfile_reqs.py +++ b/tests/spec_conformance/test_lockfile_reqs.py @@ -8,6 +8,7 @@ from __future__ import annotations from pathlib import Path, PurePosixPath +from unittest.mock import MagicMock import jsonschema import pytest @@ -758,3 +759,62 @@ def test_dropped_target_merge_hook_state_reconciled_fail_safe(tmp_path): "target while its ownership record remains", "MUST leave that document or record unmodified and\nemit an actionable diagnostic", ) + + +class TestFrozenInstallNeverWritesLockfile: + """req-lk-006's first clause, at the write chokepoint. + + req-lk-006 is one sentence carrying two obligations: a frozen mode "in + which the lockfile is never written or rewritten" AND a failure "on any + direct dependency for which the lockfile has no pin". Only the second + had behavioural coverage -- ``lockfile_satisfies_manifest`` -- so a + frozen install deployed files and rewrote the lockfile to claim them, + which is how a committed lockfile that under-records survives CI + (issue #2379). The existing req-lk-006 test asserts a fixture's + ``resolved_hash`` field, not frozen behaviour. + """ + + @pytest.mark.req("req-lk-006") + def test_frozen_install_withholds_the_write_and_fails_instead(self, tmp_path): + """A frozen install MUST leave apm.lock.yaml byte-identical, and MUST + NOT report success when the lockfile it withheld claims deployed + files the committed one does not.""" + from apm_cli.deps.lockfile import LockedDependency, LockFile, suppress_lockfile_writes + from apm_cli.install.errors import FrozenInstallError + from apm_cli.install.service import InstallService + + def _lock(deployed): + lock = LockFile(lockfile_version="1", apm_version="0.0.0-test") + lock.add_dependency( + LockedDependency( + repo_url="https://github.com/o/r", + resolved_ref="main", + resolved_commit="a" * 40, + depth=1, + deployed_files=list(deployed), + ) + ) + return lock + + lockfile_path = tmp_path / "apm.lock.yaml" + (tmp_path / "apm.yml").write_text("name: t\nversion: 1.0.0\n") + _lock([".claude/skills/recorded/SKILL.md"]).write(lockfile_path) + committed_bytes = lockfile_path.read_bytes() + + request = MagicMock() + request.apm_package.package_path = tmp_path / "apm.yml" + + # What the install would deploy: the recorded skill plus one the + # committed lockfile never claims. + produced = _lock([".claude/skills/recorded/SKILL.md", ".claude/skills/unclaimed/SKILL.md"]) + with suppress_lockfile_writes() as withheld: + produced.write(lockfile_path) + + assert lockfile_path.read_bytes() == committed_bytes, "MUST never be rewritten" + + with pytest.raises(FrozenInstallError) as exc: + InstallService._enforce_frozen_no_rewrite(request, withheld) + assert ".claude/skills/unclaimed/SKILL.md" in " ".join(exc.value.reasons) + assert lockfile_path.read_bytes() == committed_bytes + + assert_spec_contains("the lockfile is never written\nor rewritten") diff --git a/tests/unit/install/test_frozen.py b/tests/unit/install/test_frozen.py index 259f1dae21..28e55e51f5 100644 --- a/tests/unit/install/test_frozen.py +++ b/tests/unit/install/test_frozen.py @@ -1,6 +1,7 @@ """Unit tests for ``InstallService.enforce_frozen``. Issue: https://github.com/microsoft/apm/issues/1203 (P0). +The no-rewrite half (req-lk-006) is issue #2379. """ from __future__ import annotations @@ -10,14 +11,14 @@ import pytest -from apm_cli.deps.lockfile import LockedDependency, LockFile +from apm_cli.deps.lockfile import LockedDependency, LockFile, suppress_lockfile_writes from apm_cli.install.errors import FrozenInstallError from apm_cli.install.request import InstallRequest -from apm_cli.install.service import InstallService +from apm_cli.install.service import InstallService, _outermost from apm_cli.models.dependency.reference import DependencyReference -def _write_lockfile(project_dir: Path, deps: list[LockedDependency]) -> None: +def _build_lockfile(deps: list[LockedDependency]) -> LockFile: lock = LockFile( lockfile_version="1", generated_at="2025-01-01T00:00:00+00:00", @@ -25,7 +26,11 @@ def _write_lockfile(project_dir: Path, deps: list[LockedDependency]) -> None: ) for dep in deps: lock.add_dependency(dep) - (project_dir / "apm.lock.yaml").write_text(lock.to_yaml()) + return lock + + +def _write_lockfile(project_dir: Path, deps: list[LockedDependency]) -> None: + (project_dir / "apm.lock.yaml").write_text(_build_lockfile(deps).to_yaml()) def _write_apm_yml(project_dir: Path) -> None: @@ -392,3 +397,162 @@ def verbose_detail(self, message: str) -> None: f"Expected at least one cold-cache verbose log from enforce_frozen; got: {recording_logger.verbose_messages}" ) assert any("locked APM package" in message for message in recording_logger.verbose_messages) + + +def _locked(deployed: list[str]) -> LockedDependency: + return LockedDependency( + repo_url="https://github.com/o/r", + resolved_ref="main", + resolved_commit="a" * 40, + depth=1, + deployed_files=list(deployed), + deployed_file_hashes={p: f"sha256:{'0' * 64}" for p in deployed if "." in Path(p).name}, + ) + + +class TestSuppressLockfileWrites: + """req-lk-006's "never written or rewritten" half, at the chokepoint.""" + + def test_write_is_withheld_and_recorded(self, tmp_path: Path): + path = tmp_path / "apm.lock.yaml" + lock = _build_lockfile([_locked([".claude/skills/a/SKILL.md"])]) + + with suppress_lockfile_writes() as withheld: + lock.write(path) + + assert not path.exists(), "frozen mode must not create the lockfile" + assert len(withheld) == 1 + assert ".claude/skills/a/SKILL.md" in withheld[0] + + def test_recorded_payload_is_a_snapshot_not_a_reference(self, tmp_path: Path): + """Later mutation of the same object must not rewrite history.""" + lock = _build_lockfile([_locked([".claude/skills/a/SKILL.md"])]) + + with suppress_lockfile_writes() as withheld: + lock.write(tmp_path / "apm.lock.yaml") + lock.add_dependency( + LockedDependency(repo_url="https://github.com/late/r", resolved_ref="main") + ) + + assert "late/r" not in withheld[0] + + def test_writes_resume_after_the_context_exits(self, tmp_path: Path): + path = tmp_path / "apm.lock.yaml" + lock = _build_lockfile([]) + + with suppress_lockfile_writes(): + pass + lock.write(path) + + assert path.exists(), "suppression must not leak past its context" + + +class TestEnforceFrozenNoRewrite: + """A frozen install that withheld a *different* lockfile must fail. + + Suppressing the write alone would satisfy req-lk-006's letter while + leaving the project claiming less than it deploys -- the state that + puts deployed files outside the audit's scanners (#2379). + """ + + def _request(self, tmp_path: Path, committed: list[str]) -> InstallRequest: + _write_apm_yml(tmp_path) + _write_lockfile(tmp_path, [_locked(committed)]) + return _make_request(project_dir=tmp_path, manifest_deps=[]) + + def test_no_withheld_writes_passes(self, tmp_path: Path): + req = self._request(tmp_path, [".claude/skills/a", ".claude/skills/a/SKILL.md"]) + + InstallService._enforce_frozen_no_rewrite(req, []) + + def test_equivalent_withheld_write_passes(self, tmp_path: Path): + """A site that saves an unchanged lockfile must not fail the install.""" + deployed = [".claude/skills/a", ".claude/skills/a/SKILL.md"] + req = self._request(tmp_path, deployed) + same = _build_lockfile([_locked(deployed)]).to_yaml() + + InstallService._enforce_frozen_no_rewrite(req, [same]) + + def test_provenance_only_difference_passes(self, tmp_path: Path): + """Section 5.5: generated_at / apm_version MUST NOT affect equivalence. + + Otherwise every CI run on a newer CLI than the one that wrote the + committed lockfile would fail --frozen. + """ + deployed = [".claude/skills/a", ".claude/skills/a/SKILL.md"] + req = self._request(tmp_path, deployed) + newer = _build_lockfile([_locked(deployed)]) + newer.generated_at = "2099-12-31T23:59:59+00:00" + newer.apm_version = "99.0.0" + + InstallService._enforce_frozen_no_rewrite(req, [newer.to_yaml()]) + + def test_unrecorded_deployed_file_fails_and_is_named(self, tmp_path: Path): + req = self._request(tmp_path, [".claude/skills/a", ".claude/skills/a/SKILL.md"]) + grown = _build_lockfile( + [ + _locked( + [ + ".claude/skills/a", + ".claude/skills/a/SKILL.md", + ".claude/skills/b", + ".claude/skills/b/SKILL.md", + ] + ) + ] + ).to_yaml() + + with pytest.raises(FrozenInstallError, match="does not record everything") as exc: + InstallService._enforce_frozen_no_rewrite(req, [grown]) + + # Only the outermost unclaimed path: the directory covers the file. + assert exc.value.reasons == [ + " - .claude/skills/b is deployed by this install but not recorded in apm.lock.yaml" + ] + assert "without --frozen" in exc.value.tip + + def test_dropped_claims_alone_do_not_fail(self, tmp_path: Path): + """A narrower install (target filter, --only, removed dep) is tolerated. + + ``FrozenInstallError`` already documents that removed deps are + allowed; only the under-recording direction is a frozen failure. + Verified against microsoft/apm's own committed lockfile, which + differs from a local install by 180 dropped ledger rows. + """ + req = self._request( + tmp_path, [".claude/skills/a", ".claude/skills/a/SKILL.md", ".claude/skills/gone"] + ) + narrower = _build_lockfile( + [_locked([".claude/skills/a", ".claude/skills/a/SKILL.md"])] + ).to_yaml() + + InstallService._enforce_frozen_no_rewrite(req, [narrower]) + + def test_non_ledger_difference_alone_does_not_fail(self, tmp_path: Path): + """Dependency metadata churn is not an unrecorded deployed file.""" + deployed = [".claude/skills/a", ".claude/skills/a/SKILL.md"] + req = self._request(tmp_path, deployed) + extra_dep = _build_lockfile( + [ + _locked(deployed), + LockedDependency( + repo_url="https://github.com/new/dep", resolved_ref="main", depth=1 + ), + ] + ).to_yaml() + + InstallService._enforce_frozen_no_rewrite(req, [extra_dep]) + + +class TestOutermost: + def test_drops_paths_covered_by_a_listed_ancestor(self): + assert _outermost( + {".claude/skills/b/SKILL.md", ".claude/skills/b", ".agents/skills/c"} + ) == [".agents/skills/c", ".claude/skills/b"] + + def test_keeps_siblings_and_near_prefix_matches(self): + """``a/b`` must not swallow ``a/bc`` -- only a real path segment counts.""" + assert _outermost({".claude/skills/b", ".claude/skills/bc"}) == [ + ".claude/skills/b", + ".claude/skills/bc", + ] From f574d7ddcbef298fa9f8bb37ad5d339afb4eab3c Mon Sep 17 00:00:00 2001 From: Sebastian Alpers Date: Tue, 28 Jul 2026 16:01:31 +0200 Subject: [PATCH 2/3] fix(install): skip the frozen lockfile verdict when the install failed A failed pipeline returns a FAILED InstallResult rather than raising, so _enforce_frozen_no_rewrite still ran and could replace the install's own diagnostics with a lockfile complaint. Gate it on the same disposition set that already decides whether post-install scripts fire: a failed install has the real cause in its diagnostics and an unreliable ledger. Also corrects the _enforce_frozen_no_rewrite docstring, which described the check as inspecting disk when it compares the withheld lockfiles against the committed apm.lock.yaml. Both from review feedback on #2384. --- src/apm_cli/install/service.py | 23 +++++++-- tests/unit/install/test_frozen.py | 80 +++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 4 deletions(-) diff --git a/src/apm_cli/install/service.py b/src/apm_cli/install/service.py index 0580ebd8d5..489851d44f 100644 --- a/src/apm_cli/install/service.py +++ b/src/apm_cli/install/service.py @@ -140,14 +140,23 @@ def run(self, request: InstallRequest) -> InstallResult: with suppress_lockfile_writes() as withheld: result = self._run_pipeline(request) - self._enforce_frozen_no_rewrite(request, withheld) else: + withheld = None result = self._run_pipeline(request) - if result.disposition in { + install_worked = result.disposition in { InstallDisposition.SUCCESS, InstallDisposition.PARTIAL_SUCCESS, - }: + } + + # Only judge the withheld lockfile against an install that got far + # enough to be meaningful. A failed pipeline has its own + # diagnostics and an unreliable ledger, so raising here would + # replace the real cause with a confusing lockfile complaint. + if withheld is not None and install_worked: + self._enforce_frozen_no_rewrite(request, withheld) + + if install_worked: post_event = self._build_event("post-install", request) runner.fire("post-install", post_event) @@ -360,7 +369,13 @@ def enforce_frozen(request: InstallRequest) -> None: @staticmethod def _enforce_frozen_no_rewrite(request: InstallRequest, withheld: list[str]) -> None: - """Raise when a withheld write claims deployed files disk does not. + """Raise when a withheld write claims paths the committed lockfile omits. + + Compares each lockfile the frozen install *would* have written + against the committed ``apm.lock.yaml`` on disk. It reads that one + file and does not otherwise inspect the filesystem: the deployed set + is taken from the withheld lockfiles, which is what install recorded + for the files it just deployed. Withholding the write satisfies req-lk-006 on its own, but silently: a committed lockfile that under-records the deployed set would stay diff --git a/tests/unit/install/test_frozen.py b/tests/unit/install/test_frozen.py index 28e55e51f5..027e8e5ca8 100644 --- a/tests/unit/install/test_frozen.py +++ b/tests/unit/install/test_frozen.py @@ -544,6 +544,86 @@ def test_non_ledger_difference_alone_does_not_fail(self, tmp_path: Path): InstallService._enforce_frozen_no_rewrite(req, [extra_dep]) +class TestFrozenRunEndToEnd: + """``run()`` wiring: withhold the write, then judge it -- but only when + the pipeline got far enough for the verdict to mean anything.""" + + def _project(self, tmp_path: Path) -> InstallRequest: + _write_apm_yml(tmp_path) + _write_lockfile(tmp_path, [_locked([".claude/skills/a", ".claude/skills/a/SKILL.md"])]) + return _make_request(project_dir=tmp_path, manifest_deps=[]) + + def _under_recording_pipeline(self, tmp_path: Path, disposition): + """A pipeline that deploys an unclaimed file, then reports *disposition*.""" + from apm_cli.models.results import InstallResult + + def _fake_pipeline(_pkg, **_kwargs): + _build_lockfile( + [_locked([".claude/skills/a", ".claude/skills/a/SKILL.md", ".claude/skills/b"])] + ).write(tmp_path / "apm.lock.yaml") + return InstallResult(disposition=disposition, exit_code=0) + + return _fake_pipeline + + def test_failed_pipeline_keeps_its_own_diagnostics(self, tmp_path: Path): + """A failed install must not be re-reported as a lockfile complaint. + + Its diagnostics are the real cause and its ledger is unreliable, so + the frozen verdict is skipped entirely. + """ + from apm_cli.models.results import InstallDisposition + + request = self._project(tmp_path) + committed = (tmp_path / "apm.lock.yaml").read_bytes() + + with patch( + "apm_cli.install.pipeline.run_install_pipeline", + new=self._under_recording_pipeline(tmp_path, InstallDisposition.FAILED), + ): + result = InstallService().run(request) + + assert result.disposition is InstallDisposition.FAILED + assert (tmp_path / "apm.lock.yaml").read_bytes() == committed + + def test_successful_pipeline_is_judged_and_lockfile_untouched(self, tmp_path: Path): + from apm_cli.models.results import InstallDisposition + + request = self._project(tmp_path) + committed = (tmp_path / "apm.lock.yaml").read_bytes() + + with ( + patch( + "apm_cli.install.pipeline.run_install_pipeline", + new=self._under_recording_pipeline(tmp_path, InstallDisposition.SUCCESS), + ), + pytest.raises(FrozenInstallError, match="does not record everything"), + ): + InstallService().run(request) + + assert (tmp_path / "apm.lock.yaml").read_bytes() == committed + + def test_non_frozen_install_writes_normally(self, tmp_path: Path): + """Suppression must be scoped to --frozen, not global.""" + from apm_cli.models.results import InstallDisposition + + _write_apm_yml(tmp_path) + _write_lockfile(tmp_path, [_locked([".claude/skills/a"])]) + pkg = MagicMock() + pkg.package_path = tmp_path / "apm.yml" + pkg.get_apm_dependencies.return_value = [] + pkg.get_dev_apm_dependencies.return_value = [] + request = InstallRequest(apm_package=pkg, frozen=False) + committed = (tmp_path / "apm.lock.yaml").read_bytes() + + with patch( + "apm_cli.install.pipeline.run_install_pipeline", + new=self._under_recording_pipeline(tmp_path, InstallDisposition.SUCCESS), + ): + InstallService().run(request) + + assert (tmp_path / "apm.lock.yaml").read_bytes() != committed + + class TestOutermost: def test_drops_paths_covered_by_a_listed_ancestor(self): assert _outermost( From 3e1a1284b16e30f29e56ab97947d472979658074 Mon Sep 17 00:00:00 2001 From: Sebastian Alpers Date: Tue, 18 Aug 2026 22:10:00 +0200 Subject: [PATCH 3/3] fix(install): take the deployed-files view from its canonical owner Review follow-ups on the frozen no-rewrite check. `_enforce_frozen_no_rewrite` borrowed `drift._collect_tracked_files`, a module-private wrapper, to read the deployed-files ledger. Both it and `security/file_scanner.py` are thin calls to `DeploymentLedgerCodec.legacy_deployed_file_claims`, which `scripts/lint-architecture-boundaries.sh` names as the single owner of that vocabulary, so ask the owner directly instead of reaching across a module boundary for an underscored name. Also from review: * Note the single-writer invariant behind re-reading the committed lockfile after the pipeline, and the O(n^2) prefix scan in `_outermost` (error path only, process about to exit). * Say in the tip that the files were already deployed and only the lockfile write was withheld -- unlike the structural checks, this one fails after deployment, so the workspace is not pristine. * Give the failure a recovery line in `CHANGELOG.md`, `ci-cd.md`, and the install reference. * Assert byte-identity of `apm.lock.yaml` in the frozen e2e happy path. It asserted exit 0 only, so a rewrite anywhere past the chokepoint the unit suite mocks would still have passed. --- docs/src/content/docs/integrations/ci-cd.md | 2 +- src/apm_cli/install/errors.py | 4 +--- src/apm_cli/install/service.py | 24 +++++++++++++++++---- tests/integration/test_update_e2e.py | 15 ++++++++++++- 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/docs/src/content/docs/integrations/ci-cd.md b/docs/src/content/docs/integrations/ci-cd.md index 0ab8eebe4f..8ee7e1cef5 100644 --- a/docs/src/content/docs/integrations/ci-cd.md +++ b/docs/src/content/docs/integrations/ci-cd.md @@ -242,7 +242,7 @@ See the [Pack a bundle guide](../../producer/pack-a-bundle/) for the full workfl - **Pin APM version** in CI to avoid unexpected changes: `pip install apm-cli==0.22.0` - **Commit `apm.lock.yaml`** so CI resolves the same dependency versions as local development -- **Install with `apm install --frozen` in CI** to verify the committed lockfile is actually current. It never writes `apm.lock.yaml` and exits `1`, naming the paths, if the install deploys files the lockfile does not record. A file the lockfile omits carries no recorded hash, which puts it outside the `content-integrity` check below -- so this is the gate that keeps that check's scope honest, and it replaces a hand-rolled `git diff --exit-code apm.lock.yaml` step. +- **Install with `apm install --frozen` in CI** to verify the committed lockfile is actually current. It never writes `apm.lock.yaml` and exits `1`, naming the paths, if the install deploys files the lockfile does not record. A file the lockfile omits carries no recorded hash, which puts it outside the `content-integrity` check below -- so this is the gate that keeps that check's scope honest, and it replaces a hand-rolled `git diff --exit-code apm.lock.yaml` step. If CI fails on it, run `apm install` locally and commit the updated `apm.lock.yaml`. - **Commit `.github/`, `.claude/`, `.cursor/`, `.opencode/`, and `.gemini/` deployed files** so contributors and cloud-based Copilot get agent context without running `apm install` - **If using `apm compile`** (for Codex, Gemini instructions), run it in CI and fail the build if the output differs from what's committed - **Use `GITHUB_APM_PAT`** for private dependencies; never use the default `GITHUB_TOKEN` for cross-repo access diff --git a/src/apm_cli/install/errors.py b/src/apm_cli/install/errors.py index f3510c87a2..5197c0e93f 100644 --- a/src/apm_cli/install/errors.py +++ b/src/apm_cli/install/errors.py @@ -121,9 +121,7 @@ def frozen_install_tip(error: FrozenInstallError) -> str: ) if has_mcp_drift: return "Tip: run 'apm install' without --frozen to create or repair MCP lock state." - # Falls back to the error's own tip: the no-rewrite check has a different - # remedy than drift and carries it on the exception (see FrozenInstallError). - return error.tip + return error.tip # the no-rewrite check carries its own remedy class PolicyViolationError(RuntimeError): diff --git a/src/apm_cli/install/service.py b/src/apm_cli/install/service.py index 489851d44f..b7aef29dfa 100644 --- a/src/apm_cli/install/service.py +++ b/src/apm_cli/install/service.py @@ -401,21 +401,30 @@ def _enforce_frozen_no_rewrite(request: InstallRequest, withheld: list[str]) -> if not withheld: return + from apm_cli.core.deployment_ledger import DeploymentLedgerCodec from apm_cli.deps.lockfile import LockFile - from apm_cli.install.drift import _collect_tracked_files from apm_cli.install.errors import FrozenInstallError + # Re-read rather than reuse the copy `enforce_frozen` loaded: this + # judges what is on disk now. `--frozen` is a CI primitive with a + # single writer, so nothing else is expected to touch the file + # between the two reads; drift.py's own membership checks make the + # same assumption. committed = LockFile.read(_frozen_lockfile_path(request)) if committed is None: # pragma: no cover -- enforce_frozen ran first return - committed_paths = set(_collect_tracked_files(committed)) + # DeploymentLedgerCodec owns the deployed-files vocabulary (see + # `scripts/lint-architecture-boundaries.sh`); ask it directly rather + # than borrowing drift.py's private wrapper around the same call. + committed_paths = set(DeploymentLedgerCodec.legacy_deployed_file_claims(committed)) unclaimed: set[str] = set() for payload in withheld: candidate = LockFile.from_yaml(payload) if committed.is_semantically_equivalent(candidate): continue - unclaimed |= set(_collect_tracked_files(candidate)) - committed_paths + claimed = set(DeploymentLedgerCodec.legacy_deployed_file_claims(candidate)) + unclaimed |= claimed - committed_paths if not unclaimed: return @@ -425,7 +434,10 @@ def _enforce_frozen_no_rewrite(request: InstallRequest, withheld: list[str]) -> f" - {path} is deployed by this install but not recorded in apm.lock.yaml" for path in _outermost(unclaimed) ], - tip="Tip: run 'apm install' without --frozen, then commit apm.lock.yaml.", + tip=( + "Tip: the files were still deployed; only the lockfile write was " + "withheld. Run 'apm install' without --frozen, then commit apm.lock.yaml." + ), ) @@ -436,6 +448,10 @@ def _outermost(paths: set[str]) -> list[str]: beneath it, so an unrecorded skill otherwise reports twice. Sorted order puts an ancestor before its descendants, so a single pass over the accumulated output is enough. + + The prefix scan is O(n^2) in the number of unclaimed paths. It runs + only on the error path of a failing frozen install, where *n* is what + one install left unrecorded and the process is about to exit. """ outermost: list[str] = [] for path in sorted(paths): diff --git a/tests/integration/test_update_e2e.py b/tests/integration/test_update_e2e.py index a6b8e801ce..d4fd1665af 100644 --- a/tests/integration/test_update_e2e.py +++ b/tests/integration/test_update_e2e.py @@ -6,7 +6,8 @@ * `apm update --dry-run` resolves, renders a plan, and writes nothing. * `apm update --yes` after install with no manifest changes is a no-op. -* `apm install --frozen` succeeds against an in-sync lockfile. +* `apm install --frozen` succeeds against an in-sync lockfile and leaves + `apm.lock.yaml` byte-identical (req-lk-006's no-rewrite clause). * `apm install --frozen` exits non-zero when lockfile is missing. * `apm install --frozen` exits non-zero when manifest declares a dep not present in the lockfile. @@ -97,10 +98,19 @@ def test_update_after_install_no_changes_short_circuits(self, temp_project, apm_ class TestFrozenE2E: def test_frozen_succeeds_against_in_sync_lockfile(self, temp_project, apm_binary_path): + """Exit 0 *and* `apm.lock.yaml` untouched -- req-lk-006's no-rewrite clause. + + The unit suite proves the write is withheld at the chokepoint, but + it stops at the pipeline boundary. Byte-identity is asserted here, + through the real binary against a real package, because a rewrite + that happens anywhere else in the pipeline would still exit 0. + """ _write_apm_yml(temp_project, ["microsoft/apm-sample-package"]) first = _run_apm(apm_binary_path, ["install"], temp_project) assert first.returncode == 0, first.stderr + lockfile = temp_project / "apm.lock.yaml" + before = lockfile.read_bytes() # Re-run with --frozen on the same manifest+lockfile. result = _run_apm(apm_binary_path, ["install", "--frozen"], temp_project) @@ -108,6 +118,9 @@ def test_frozen_succeeds_against_in_sync_lockfile(self, temp_project, apm_binary assert result.returncode == 0, ( f"Frozen install failed on in-sync project:\n{result.stdout}\n{result.stderr}" ) + assert lockfile.read_bytes() == before, ( + "--frozen rewrote apm.lock.yaml; it must never write it (req-lk-006)" + ) def test_frozen_fails_when_lockfile_missing(self, temp_project, apm_binary_path): _write_apm_yml(temp_project, ["microsoft/apm-sample-package"])