Repository navigation
fix(install): preserve refused skill files when deployment receipts are missing - #3440
Tyce Herrman (TyceHerrman) wants to merge 4 commits into
Conversation
Independent reproduction and combined-candidate validationI tested the exact #3440 head On unmodified v0.35.0, all 12 missing-receipt preservation cases reproduced the problem: three package forms × project/user scope × edited/unedited files. Install refused the overwrites, stale cleanup removed deployed files, and install still returned success. The 12-case source-CLI regression group passed in the combined candidate. Its assertions cover repeated refusal attempts, nonzero install outcome, preservation of the deployed files, and preservation of the previous lockfile. With the companion migration experiment creating both v1 and v2 receipts, the missing-evidence fixture must remove both namespaces; retaining a valid v1 is a supported migration case, not absence of trustworthy evidence. These results support keeping the cleanup repair separate and using the existing error/precommit gate before cleanup/lock replacement. They are combined-candidate source-CLI results, not an independent full-suite pass for this PR or a packaged-binary run of those 12 cases. One companion adoption issue surfaced in #3439: fresh install with the newer candidate → source update → install with the actual unpatched 0.35.0 executable can still delete files when the new install never created legacy evidence. This PR cannot change the older executable, so that downgrade boundary needs its own migration coverage; it should not expand this cleanup fix into an identity rewrite. No actual reboot/remount or other-OS runtime validation was performed. The intended downstream path is to validate and consume an official release containing the necessary fixes, rather than ship a separately maintained production fork. |
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
🟡 Changes recommended
Secondary-target refusal errors remain suppressed, allowing cleanup to delete files without receipts that still match their old-lock hashes.
1 open issue
What changed in this PR
This PR addresses #3422 by treating skill-file overwrite refusals caused by missing deployment receipts as errors, allowing the existing install gate to stop before stale cleanup and lockfile replacement.
Changes:
- Escalates missing-baseline diagnostics to errors while retaining warnings for edited files with valid receipts.
- Adds 12 regression cases covering three package formats, project and user scopes, and edited or unchanged files across repeated attempts.
| File | Description |
|---|---|
tests/integration/test_missing_receipt_cleanup.py |
Adds isolated CLI checks for file preservation, unchanged lockfiles, and nonzero exit results. |
src/apm_cli/integration/base_integrator.py |
Records missing-baseline refusals as errors when a diagnostics collector is supplied. |
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The cleanup tests can miss secondary-target diagnostic regressions because CSV ordering makes the retained Claude deployment primary in user scope, allowing primary-target errors to mask the issue.
1 open issue
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The regression test’s APM_FIXED_BINARY override bypasses the canonical executable fixture, potentially testing an unrelated or frozen binary rather than the checkout under review and losing startup instrumentation.
2 open issues
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
A critical mismatch between refusal behavior and lifecycle test expectations causes both scope variants to fail before preservation checks, requiring human assessment of the resulting coverage gap.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
Description
When a skill file has no trustworthy local deployment receipt, installation refuses to overwrite it but previously continued into stale cleanup, which could delete the refused file and report success. This change records that refusal as an error so the existing precommit outcome gate stops before cleanup and lockfile replacement. Edited files with a valid prior receipt retain their existing warning behavior.
Regression coverage exercises APM packages, native skills, and skill bundles in project and user scope, with both unchanged installed files and local edits. Repeated attempts preserve existing files and the old lockfile and return a nonzero result.
Issue and approved scope
Issue: #3422
Does this PR complete the issue, or what remains?
Fixes #3422
Related: #3391
Type of change
Testing
Tested locally with dependencies from
uv.lockon macOS arm64.Added regression tests: 12 preservation cases covering three package formats, two scopes, and edited/unedited files.
Spec conformance (OpenAPM v0.1)
If this PR changes behaviour that an OpenAPM v0.1
req-XXXcovers,confirm the three-step ritual in the
development guide:
docs/src/content/docs/specs/openapm-v0.1.mdupdated(new/changed
<a id="req-XXX"></a>anchor + prose + Appendix Crow).
docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.ymlupdated.
@pytest.mark.req("req-XXX")test undertests/spec_conformance/added or extended.CONFORMANCE.{md,json}regenerated viauv run --extra dev python -m tests.spec_conformance.gen_statementand committed.
This restores file-preservation behavior without changing the lockfile schema or adding a normative requirement