fix: keep plugin hooks live during reinstall (closes #2717) - #2723
Conversation
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: None
What changed in this PR
This PR fixes a reinstall/refresh failure mode where plugin hook scripts could be unlinked while their host settings entries remain registered, by staging replacement package trees and only publishing them after the replacement is fully downloaded and validated. It also codifies “resolution replacement activation” as a single-owner architecture boundary (ResolutionStagingSession) with both behavioral regression coverage and a static lint guard.
Changes:
- Add
ResolutionStagingSession.prepare_replacement()/publish_replacement()to stage replacement downloads without disturbing the live installed tree, and journal prior contents for rollback. - Update the resolve-phase download callback to download into a staged replacement path and publish atomically only after the download completes.
- Tighten resolver behavior to fail closed on failed refresh/replacement attempts (do not silently reuse existing materializations), with new unit + integration architecture guard tests and a doc update.
| File | Description |
|---|---|
src/apm_cli/install/resolution_staging.py |
Adds explicit staging/publish APIs for replacement materializations while preserving rollback semantics. |
src/apm_cli/install/phases/resolve.py |
Routes dependency downloads through the staging replacement flow and publishes only after successful materialization. |
src/apm_cli/deps/apm_resolver.py |
Ensures failed refresh/replacement callbacks don’t fall back to stale on-disk content (fail closed). |
scripts/lint-architecture-boundaries.sh |
Adds an architecture boundary lint rule enforcing a single “resolution replacement activation” owner and expected call-site shapes. |
tests/unit/install/test_resolution_staging_relocate.py |
Adds a unit regression covering “hook stays live until publish” semantics via staging session publish/rollback. |
tests/unit/install/test_resolve_resolution_paths.py |
Adds a resolve-phase regression ensuring refresh downloads stage replacements without unlinking the live hook during download. |
tests/unit/deps/test_apm_resolver_edge_cases.py |
Adds a unit regression ensuring failed refresh does not silently validate/reuse existing materialization. |
tests/integration/test_architecture_authorities.py |
Adds an integration “mutation trap” asserting the new architecture boundary is enforced by the lint script. |
docs/src/content/docs/enterprise/security.md |
Documents the replacement staging behavior as part of integrity/installation hardening. |
.github/instructions/architecture.instructions.md |
Registers “Resolution replacement activation” in the canonical owner table. |
.apm/instructions/architecture.instructions.md |
Mirrors the canonical owner table update for agent instructions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 3 | 1 | 0 | Semver refresh, pre-validation activation, and interrupt windows violate the live-hook and rollback contracts. |
| CLI Logging Expert | 0 | 1 | 0 | Report when a failed reinstall preserves the active package and how to retry. |
| DevX UX Expert | 2 | 1 | 0 | Staging is directionally sound, but semver refresh and callback exceptions still violate fail-closed behavior. |
| Supply Chain Security Expert | 4 | 1 | 0 | Activation can expose unapproved content and stale reuse before validation. |
| OSS Growth Hacker | 0 | 1 | 0 | Expose the fail-closed upgrade value to plugin users. |
| Doc Writer | 1 | 1 | 0 | Correct the security overclaim and document hook continuity on the lifecycle page. |
| Test Coverage Expert | 0 | 3 | 0 | Add in-flight interruption and publish-failure regression traps. |
| Performance Expert | 1 | 1 | 0 | Semver purge remains destructive; the architecture guard is expensive. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Python Architect] (blocking-severity) Remove the
prepare_path/prepare_replacementconflict in direct semver refresh and add a regression test. - [Supply Chain Security Expert] (blocking-severity) Keep replacement content and hooks inactive until resolver validation, integrity checks, and audit gates succeed.
- [DevX UX Expert] (blocking-severity) Make callback exceptions fail closed without stale fallback and cover activation failure injection.
- [Python Architect] (blocking-severity) Durably record rename transitions and recover interruption while preserving the prior package.
- [Doc Writer] (blocking-severity) Correct the security validation-order claim, document lifecycle continuity, and add an Unreleased entry.
Architecture
classDiagram
class InstallTransaction {
+resolution ResolutionStagingSession
+commit(result)
+rollback()
}
class ResolutionStagingSession {
+prepare_replacement(path)
+publish_replacement(path, replacement)
+commit()
+rollback()
}
class ResolvePhase {
+run(ctx)
+download_callback(...)
}
class APMDependencyResolver {
+resolve_dependencies(anchor)
+validate package
}
InstallTransaction *-- ResolutionStagingSession : owns
ResolvePhase ..> ResolutionStagingSession : stages and publishes
ResolvePhase ..> APMDependencyResolver : injects callback
APMDependencyResolver ..> ResolvePhase : receives live path too early
flowchart TD
A["Install refresh"] --> B["Keep current package live"]
B --> C["Download replacement to staging"]
C --> D["Validate package, integrity, and policy"]
D -->|success| E["Publish through ResolutionStagingSession"]
D -->|failure| F["Discard replacement; current package stays live"]
E --> G["Commit transaction"]
Recommendation
Revise the transaction before shipping: restore direct-semver refresh, enforce validate-before-activate ordering, close callback failure semantics, and make interruption recovery safe. Update documentation after those guarantees have focused tests.
Full per-persona findings
Python Architect
- [blocking] Direct semver refresh removes the live package and then rejects replacement staging at
src/apm_cli/install/phases/resolve.py:495. Remove the eager semver purge and route re-resolution through the replacement owner. - [blocking] Replacement content becomes live before package validation at
src/apm_cli/install/phases/resolve.py:696. Validate the staged candidate before publication. - [blocking] Interruption during publication can discard the previous package at
src/apm_cli/install/resolution_staging.py:71. Journal intent before mutation and handle each intermediate state. - [recommended] Callback exceptions can fall through to stale installed content at
src/apm_cli/deps/apm_resolver.py:1106. Return failure after logging and add a regression trap.
CLI Logging Expert
- [recommended] Explain in install diagnostics that the existing installation remains active after replacement failure and how to retry.
DevX UX Expert
- [blocking] Direct semver updates fail before downloading replacement content because eager backup conflicts with replacement staging.
- [blocking] Preparation exceptions can silently reuse stale installed content.
- [recommended] Reconcile abandoned replacement journals on the next install.
Supply Chain Security Expert
- [blocking] Replacement hooks activate before package and security gates complete.
- [blocking] Git-semver updates fail against existing installs; the existing E2E semver update test failed on this head.
- [blocking] Callback exceptions can bypass containment and reuse stale content.
- [blocking] Distinct dependency identities can collide on one physical destination during parallel resolution; reject collisions or reserve destinations.
- [recommended] Make publication recoverable after process interruption.
OSS Growth Hacker
- [recommended] Add a concise Unreleased entry after the behavior is corrected.
Auth Expert -- inactive
The changed files do not alter authentication, token, credential, or host-resolution behavior.
Doc Writer
- [blocking]
docs/src/content/docs/enterprise/security.md:259overstates validation-before-publication. Describe only guarantees actually enforced. - [recommended] Document refresh continuity in
docs/src/content/docs/consumer/copilot-agent-plugins.md.
Test Coverage Expert
- [recommended] Add fixture-backed coverage that invokes the old hook while refresh is paused, interrupts, retries, and then observes the new hook.
- [recommended] Inject failure in staged-to-live activation and assert restoration plus cleanup.
- [recommended] Add Scenario Evidence rows to the PR description.
Performance Expert
- [blocking] Direct semver purge still unlinks live hooks before staged replacement.
- [recommended] Narrow the architecture mutation guard to avoid another full repository copy and monolithic lint run.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Keep semver refreshes staged through package validation, fail closed on callback errors, restore interrupted publication, and tighten the canonical activation guard. Addresses the review-panel follow-ups for PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 1 | 0 | 0 | Cached local dependencies are misclassified as staged candidates. |
| CLI Logging Expert | 0 | 1 | 0 | Validation and publication failures need the same retry guidance. |
| DevX UX Expert | 1 | 0 | 0 | Malformed refresh candidates can appear successful against stale bytes. |
| Supply Chain Security Expert | 1 | 0 | 0 | Candidate metadata can be recorded before validation and activation. |
| OSS Growth Hacker | 0 | 0 | 0 | Changelog and lifecycle docs communicate the reliability value. |
| Doc Writer | 1 | 2 | 0 | Runtime ordering and two docs details remain inconsistent. |
| Test Coverage Expert | 0 | 1 | 0 | Live-hook continuity needs fixture-backed coverage. |
| Performance Expert | 0 | 0 | 0 | No performance concern remains. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Fail malformed-candidate resolution and defer publication and metadata recording until acceptance succeeds.
- [Python Architect] (blocking-severity) Keep cached local paths distinct from staged candidates and add a transitive-dependency regression.
- [Doc Writer] (blocking-severity) Preserve the README security promise by keeping replacement content unexposed until applicable gates finish.
- [Test Coverage Expert] Add fixture-backed integration coverage for live-hook rollback.
- [CLI Logging Expert] Show preserved-install retry guidance for validation and publication failures.
Architecture
flowchart TD
A["Resolve dependency"] --> B{"Live cache or staged candidate?"}
B -->|live cache| C["Validate and consume live package"]
B -->|candidate| D["Validate staged package"]
D -->|invalid| E["Fail resolution; preserve live package"]
D -->|valid| F["Run remaining acceptance gates"]
F -->|success| G["Publish through ResolutionStagingSession"]
Recommendation
Fold the fail-closed candidate and local-cache fixes with focused regressions before shipping. Defer result-object typing and broader resolver restructuring.
Full per-persona findings
Python Architect
- [blocking] Existing local package paths returned by the callback are treated as staged candidates and rejected by activation. Distinguish cache hits from prepared replacements and cover a local package with transitives.
CLI Logging Expert
- [recommended] Apply preserved-install retry guidance to candidate validation and publication failures.
DevX UX Expert
- [blocking] Candidate validation errors can be swallowed while stale live bytes remain. Return nonzero and keep lock state unchanged.
Supply Chain Security Expert
- [blocking]
callback_downloadedmetadata is recorded before candidate validation and activation. Record only after acceptance.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
No authentication, token, credential, or host-resolution surface is touched.
Doc Writer
- [blocking] Agent-visible publication before later gates conflicts with the README security promise; preserve the promise in implementation.
- [recommended] Split
--forcefromapm updatein the lifecycle table. - [recommended] Correct the security page's policy timing statement.
Test Coverage Expert
- [recommended] Add fixture-backed live-hook continuity and failed-refresh rollback coverage.
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.
Fail malformed refreshes before metadata commit, distinguish live local cache paths from staged candidates, and add integration evidence for hook continuity and rollback. Addresses the second review-panel pass on PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | No architecture change requested. |
| CLI Logging Expert | 0 | 2 | 0 | Remove duplicate errors and make guidance state-aware. |
| DevX UX Expert | 0 | 2 | 0 | Complete cache-hit progress and correct first-install guidance. |
| Supply Chain Security Expert | 1 | 1 | 0 | Overlapping destinations can corrupt rollback. |
| OSS Growth Hacker | 0 | 0 | 0 | Reliability story is clear. |
| Doc Writer | 0 | 0 | 0 | No PR-caused drift remains. |
| Test Coverage Expert | 0 | 1 | 0 | Add install-level activation failure coverage. |
| Performance Expert | 0 | 0 | 0 | No performance regression. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Reject ancestor/descendant destination overlap and preserve parent-package files exactly on rollback.
- [Supply Chain Security Expert] Canonicalize replacement reservation keys.
- [Test Coverage Expert] Add install-level activation OSError and KeyboardInterrupt regressions.
- [DevX UX Expert] Complete cache-hit TUI tasks and make first-install guidance accurate.
- [CLI Logging Expert] Remove duplicate resolver warning emission.
Recommendation
Fix destination overlap and prove exact rollback, then fold the bounded CLI follow-ups before the terminal panel pass.
Full per-persona findings
Python Architect
No architecture changes requested; staged validation narrows exposure and preserves the canonical transaction boundary.
CLI Logging Expert
- [recommended] Render fatal downloaded-package errors only through the command logger.
- [recommended] Do not claim an existing installation survives a fresh-install failure.
DevX UX Expert
- [recommended] Complete TUI tasks when the callback returns a cache hit.
- [recommended] Make first-install failure guidance accurate.
Supply Chain Security Expert
- [blocking] Ancestor and descendant reservations overlap candidate and backup trees, so rollback can restore only a fragment of the parent package.
- [recommended] Use canonical replacement paths as reservation keys.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
No authentication-relevant surface changed.
Doc Writer
No findings.
Test Coverage Expert
- [recommended] Add install-level activation failure coverage for exact hook and lock restoration.
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.
Use opaque transaction slots for overlapping package paths, keep failure guidance state-aware, and prove activation rollback through the install pipeline. Addresses the third review-panel pass on PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Exercise the registered PreToolUse command during failed replacement activation and document the transactional refresh contract on every canonical CLI surface. Addresses the final review-panel follow-ups for PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Architecture is sound; the evidence SHA was corrected. |
| CLI Logging Expert | 0 | 0 | 0 | Progress and recovery guidance are state-aware. |
| DevX UX Expert | 0 | 1 | 0 | Canonical CLI and shipped usage docs were updated. |
| Supply Chain Security Expert | 0 | 0 | 0 | Validation, containment, activation, and rollback fail closed. |
| OSS Growth Hacker | 0 | 0 | 0 | The reliability value is clear in docs and changelog. |
| Doc Writer | 0 | 0 | 0 | Documentation matches the implemented boundary. |
| Test Coverage Expert | 0 | 2 | 0 | Registered-hook E2E and exact evidence SHAs were folded. |
| Performance Expert | 0 | 0 | 0 | Opaque slots and O(1) maps add no material regression. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
flowchart TD
A["apm update or install --refresh"] --> B["Download to opaque staging slot"]
B --> C{"Package validation"}
C -->|invalid| D["Fail; keep current package and lockfile"]
C -->|valid| E["Publish through ResolutionStagingSession"]
E --> F{"Later install result"}
F -->|success| G["Commit and remove journal"]
F -->|failure or interruption| H["Restore exact prior package"]
Recommendation
The final follow-ups were folded and CI is green. Ready for maintainer review.
Folded in this run
- (panel) Added a registered
PreToolUsehook invocation during replacement download and after activation interruption -- resolved ina5ef135efd. - (panel) Corrected every Scenario Evidence row to the actual reviewed head -- resolved in
a5ef135efd. - (panel) Documented transactional refresh and retry behavior in
apm install,apm update, and the shippedapm-usagecommand reference -- resolved ina5ef135efd. - (panel) Isolated nested parent/child replacement journals and covered both publication orders -- resolved in
12f02d6ee7. - (panel) Rejected malformed candidates, preserved lock state, and distinguished live local cache paths -- resolved in
ed81bcb3a0. - (panel) Restored direct semver refresh, validate-before-activate, fail-closed callbacks, and interruption rollback -- resolved in
d906bc7faa.
Deferred (out-of-scope follow-ups)
- (panel) Move every install integration output behind integrity and audit acceptance -- scope boundary: this PR covers package replacement hook liveness; a pipeline-wide transactional deployment redesign spans every target integrator and is a separate architecture theme.
Regression-trap evidence (mutation-break gate)
- Registered-hook rollback E2E failed when immediate and transaction restoration were removed; both guards were restored.
- Nested destination rollback tests failed when opaque staging slots were replaced with mirrored paths; the guard was restored.
- Malformed candidate, local-cache distinction, activation ordering, callback failure, destination reservation, and AC36 owner tests each failed when their production/static guard was removed; all guards were restored.
Lint contract
uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ passed with no diagnostics. Pylint R0801, auth-signals, and architecture-boundary checks also passed.
CI
All checks passed on https://github.com/microsoft/apm/actions/runs/33314259137 after 0 CI fix iterations.
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2723 | a5ef135 |
ship_with_followups | 4 | 10 | 1 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
4 outer iterations; 2 Copilot rounds with no inline findings. Final panel stance: ship_with_followups; all in-scope follow-ups were folded.
Full per-persona findings
- Python Architect: architecture sound; requested SHA discrepancy was clerical and is corrected.
- CLI Logging Expert: no findings.
- DevX UX Expert: canonical update/install and shipped usage docs were requested and folded.
- Supply Chain Security Expert: no findings.
- OSS Growth Hacker: no findings.
- Auth Expert: inactive; no auth surface changed.
- Doc Writer: no findings.
- Test Coverage Expert: registered-hook interruption E2E and evidence SHA corrections were requested and folded.
- 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.
Keep semver refreshes staged through package validation, fail closed on callback errors, restore interrupted publication, and tighten the canonical activation guard. Addresses the review-panel follow-ups for PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fail malformed refreshes before metadata commit, distinguish live local cache paths from staged candidates, and add integration evidence for hook continuity and rollback. Addresses the second review-panel pass on PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use opaque transaction slots for overlapping package paths, keep failure guidance state-aware, and prove activation rollback through the install pipeline. Addresses the third review-panel pass on PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Exercise the registered PreToolUse command during failed replacement activation and document the transactional refresh contract on every canonical CLI surface. Addresses the final review-panel follow-ups for PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a5ef135 to
fd3d7fb
Compare
|
Rebased onto current main at b68fcad -> fd3d7fb. Conflicting paths resolved (faithful merge of both intents):
Regression-trap tests re-verified post-rebase (mutation-break gate):
Lint contract: Post-push mergeability: Ready for maintainer review. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep semver refreshes staged through package validation, fail closed on callback errors, restore interrupted publication, and tighten the canonical activation guard. Addresses the review-panel follow-ups for PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fail malformed refreshes before metadata commit, distinguish live local cache paths from staged candidates, and add integration evidence for hook continuity and rollback. Addresses the second review-panel pass on PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use opaque transaction slots for overlapping package paths, keep failure guidance state-aware, and prove activation rollback through the install pipeline. Addresses the third review-panel pass on PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Exercise the registered PreToolUse command during failed replacement activation and document the transactional refresh contract on every canonical CLI surface. Addresses the final review-panel follow-ups for PR #2723. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fd3d7fb to
90d1061
Compare
Stages replacement package trees before publishing them so a refresh or reinstall no longer unlinks the currently registered hook while network and package validation work is in flight. Failed replacement callbacks now fail closed without reusing stale content, and
ResolutionStagingSessionremains the canonical activation owner with behavioral and static mutation guards. Closes #2717.The follow-up pass also preserves direct git-semver refreshes, validates candidates before activation, reserves each physical destination against concurrent writers, restores the prior package across activation errors and
KeyboardInterrupt, and reports that the existing installation remains active after a failed refresh.Scenario Evidence
tests/unit/install/test_resolve_resolution_paths.py::test_refresh_keeps_registered_hook_live_while_replacement_downloadsa5ef135efd837027eb0caece262c7ca6ccb8b977tests/integration/test_git_semver_install_e2e.py::TestUpdateReResolvesGitSemver::test_update_flag_re_resolves_when_install_path_exists_and_new_tag_publisheda5ef135efd837027eb0caece262c7ca6ccb8b977tests/unit/install/test_resolution_staging_relocate.py::test_publish_replacement_restores_old_package_when_activation_failsa5ef135efd837027eb0caece262c7ca6ccb8b977tests/integration/test_git_semver_install_e2e.py::TestUpdateReResolvesGitSemver::test_invalid_update_preserves_live_hook_and_lockfilea5ef135efd837027eb0caece262c7ca6ccb8b977tests/unit/deps/test_apm_resolver_edge_cases.py::TestTryLoadDependencyPackageForceRecheck::test_cached_local_package_is_not_activated_as_candidatea5ef135efd837027eb0caece262c7ca6ccb8b977tests/unit/install/test_resolution_staging_relocate.py::test_nested_replacements_rollback_without_overlapping_staging_pathsa5ef135efd837027eb0caece262c7ca6ccb8b977tests/integration/test_git_semver_install_e2e.py::TestUpdateReResolvesGitSemver::test_activation_error_preserves_live_hook_and_lockfilea5ef135efd837027eb0caece262c7ca6ccb8b977tests/integration/test_architecture_authorities.py::test_resolution_replacement_activation_has_one_ownera5ef135efd837027eb0caece262c7ca6ccb8b977Mutation checks removed the fail-closed callback return, validated publish call, activation rollback, destination reservation, and static owner selector in turn; each corresponding regression test failed before the guard was restored.
Validation: 135 targeted tests passed,
apm audit --cipassed all 10 checks, and the full canonical lint mirror passed at the reviewed head.apm-spec-waiver: Internal transaction repair; no OpenAPM manifest or registry contract changes.