fix(install): skip lockfile rewrite when local instructions content is unchanged (closes #1702)#1710
Conversation
Carry forward existing local .apm lockfile fields during the dependency lockfile pass so unchanged local instruction hashes do not trigger generated_at churn. post_deps_local remains responsible for recomputing hashes and writing only real content changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the install lockfile idempotency pattern to projects that have both remote APM dependencies and local .apm/instructions content, preventing apm.lock.yaml from being rewritten (generated_at churn) when the local instruction content hashes are unchanged.
Changes:
- Preserve existing local lockfile state during the lockfile phase so semantic comparison does not see a temporary “local fields dropped” delta.
- Add a regression test that runs the lockfile phase +
post_deps_localtwice with different timestamps and asserts the lockfile stays byte-stable.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/install/phases/lockfile.py |
Carries forward local lockfile fields (and self-entry) before semantic comparison to avoid unnecessary lockfile writes. |
tests/unit/install/test_mcp_lockfile_determinism.py |
Adds a determinism regression test for unchanged local instructions across repeated installs. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
| def _preserve_existing_local_state(self, lockfile: LockFile) -> None: | ||
| """Keep local fields until post_deps_local reconciles content hashes.""" | ||
| if self.ctx.existing_lockfile: | ||
| lockfile.local_deployed_files = list(self.ctx.existing_lockfile.local_deployed_files) | ||
| lockfile.local_deployed_file_hashes = copy.deepcopy( | ||
| self.ctx.existing_lockfile.local_deployed_file_hashes | ||
| ) | ||
| if "." in self.ctx.existing_lockfile.dependencies: | ||
| lockfile.dependencies["."] = copy.deepcopy( | ||
| self.ctx.existing_lockfile.dependencies["."] | ||
| ) | ||
| if self.ctx.logger: | ||
| self.ctx.logger.verbose_detail( | ||
| "Local .apm state unchanged -- carrying forward " | ||
| f"{len(lockfile.local_deployed_files)} file(s)" | ||
| ) | ||
|
|
Address shepherd-panel follow-ups by clarifying the verbose diagnostic for local state carry-forward and adding the missing changelog entry for the lockfile idempotency fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Carry-forward before post_deps_local mirrors the existing MCP idempotency pattern; wording nit was folded. |
| CLI Logging Expert | 0 | 0 | 0 | Verbose diagnostic now accurately describes carry-forward pending reconciliation. |
| DevX UX Expert | 0 | 0 | 0 | Install idempotency behavior is defended with a direct byte-stability regression trap. |
| Supply Chain Security Expert | 0 | 0 | 0 | Lockfile integrity remains anchored in post_deps_local hash recomputation. |
| OSS Growth Hacker | 0 | 0 | 0 | CHANGELOG now captures the deterministic-lockfile trust signal. |
| Test Coverage Expert | 0 | 0 | 0 | Regression trap covers remote dependency plus local instructions, repeated install, stable hashes, and byte stability. |
B = highest-signal findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Merge at maintainer convenience. The regression test plus mutation-break gate provide strong evidence the fix is correct and bounded. No follow-ups remain; all panel recommendations were folded in this PR.
Folded in this run
- (panel) Clarified the verbose diagnostic so it says local
.apmstate is carried forward pending hash reconciliation -- resolved in bda5451. - (panel) Added a CHANGELOG entry for the
apm installlocal-instructions lockfile idempotency fix -- resolved in bda5451.
Copilot signals reviewed
- No
copilot-pull-request-reviewer[bot]inline comments were present after two fetch rounds.
Regression-trap evidence (mutation-break gate)
tests/unit/install/test_mcp_lockfile_determinism.py::test_unchanged_local_instructions_do_not_rewrite_lockfile-- deleted_preserve_existing_local_statecall; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both silent.
Additional local guard checks also passed: pylint R0801 and scripts/lint-auth-signals.sh.
CI
All PR checks are green on bda5451, including Lint, CodeQL, Build & Test Shard 1/2, PR Binary Smoke, APM Self-Check, Coverage Combine, spec conformance, NOTICE drift, and license/cla (after 0 CI fix iterations).
Mergeability status
Captured from gh pr view 1710 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #1710 | bda5451 |
ship_now | 1 | 2 | 0 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
1 outer iteration; 2 Copilot rounds. Final panel stance: ship_now.
Ready for maintainer review.
Full per-persona findings
No remaining per-persona findings after folds.
Inactive panelists: auth-expert (no auth surface touched) and doc-writer (no docs drift beyond the folded CHANGELOG entry).
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
…arency) Sync main commits #1700, #1676, #1694, #1710. The #1700 feature (surface installed hook actions during install) wove display-payload tracking through code my refactor extracted into sibling helpers; resolve two conflicts by keeping the extracted structure and porting #1700 semantics: - services.py: accept the extracted _log_per_kind_results() call; move _log_hook_display_payloads into services_integrate.py (avoids circular import) and re-export it from services.py; emit hook summaries inside _log_per_kind_results. - hook_integrator._integrate_merged_hooks: accept extracted _merge_hook_file_entries / _write_merged_config; thread per-file display data out via a new optional capture_entries kwarg on _merge_hook_file_entries; build display payloads after _write_merged_config finalizes (post _apm_source strip). - Keep hook_integrator.py <=800 by relocating _iter_hook_entries / _summarize_command / _build_display_payload to hook_transforms.py and _parse_hook_json to hook_merge.py as thin delegators. Shadow gate green: ruff, ruff format, pylint R0801 10.00/10 (EXIT=0), auth-signals, import smoke, 3178 tests pass. All files within 800-line guard (hook_integrator.py 799, hook_merge.py 766, hook_transforms.py 601, services.py 690, services_integrate.py 307). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e integrity) Sync with main through v0.19.0 (#1715, #1714, #1718, #1719, #1717). Conflicts resolved: - CHANGELOG.md: main cut the v0.19.0 release, moving prior Unreleased entries into a dated section. Took main's restructure; relocated the #1681 "Tightened Stage 2 code-complexity thresholds" entry to the new ## [Unreleased] section (released sections stay immutable per Keep a Changelog). Took main's #1702/#1710 close-ref for the lockfile entry. - src/apm_cli/install/services.py: #1718 added per-file skill-bundle integrity recording inline at a site my refactor had extracted into _log_skill_result. Kept the new _skill_bundle_file_entries helper in services.py (sibling of _deployed_path_entry); dropped main's duplicate inline _log_hook_display_payloads (already relocated to services_integrate.py and re-exported in my #1700 port); accepted the _log_skill_result delegator at the call site. - src/apm_cli/install/services_integrate.py: ported #1718's deployed.extend(_skill_bundle_file_entries(tp, ...)) into _log_skill_result's deployed-path loop (deployed aliases result["deployed_files"]), with a lazy import of the helper. Shadow gate: no markers, all src <=800 lines, ruff check + format clean, pylint R0801 EXIT=0, auth-signals clean; 11144 tests pass across the install/integration suites plus the #1718, #1700, and #1709 targeted sets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
apm install was rewriting apm.lock.yaml on every run for projects with both a remote APM dependency and local .apm/instructions content because the dependency lockfile pass temporarily dropped local_deployed_files before post_deps_local restored them, refreshing generated_at despite unchanged hashes. This carries forward the existing local lockfile state until post_deps_local performs the real hash reconciliation, making this the third instance of the idempotency pattern after #450/#456 and #1532/#1568.\n\nHow-to-test:\n- uv run --extra dev pytest tests/unit/install/test_mcp_lockfile_determinism.py::test_unchanged_local_instructions_do_not_rewrite_lockfile -q\n- uv run --extra dev pytest tests/unit/install/test_mcp_lockfile_determinism.py -q\n- uv run --extra dev pytest tests/unit/test_lockfile_self_entry.py tests/unit/install/test_mcp_lockfile_determinism.py -q\n- uv run --extra dev pytest tests/unit/install -x -q -k "lock or install or idempot or instruction"\n- uv run --extra dev pytest tests/ -x -q -k "lock or install or idempot or instruction"\n- uv run --extra dev ruff check src/ tests/\n- uv run --extra dev ruff format --check src/ tests/\n- uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/\n- bash scripts/lint-auth-signals.sh\n\nCloses #1702