fix(install): drop inactive-target ghost entries from lockfile deployed_files (#2059)#2114
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a lockfile reconciliation edge case where deployed_files can accumulate “ghost” entries for targets the consumer never declares (e.g. .windsurf/...), making apm audit --ci fail permanently even though apm install will never write those files.
Changes:
- Add a “declared target universe” concept and pass it into lockfile/local manifest reconciliation to drop undeclared-target ghost entries while preserving legitimate multi-target history.
- Extend
union_preserving()to filter preserved prior entries against the declared target universe (when known), retaining legacy preserve-all semantics when the consumer declares no targets. - Add unit tests covering ghost-drop,
--targetnarrowing preservation, gated/dynamic URI preservation, and end-to-end_attach_deployed_fileswiring.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/install/phases/test_lockfile_union.py | Adds regression tests for inactive-target ghost dropping and declared-universe behavior. |
| src/apm_cli/install/phases/targets.py | Introduces declared_target_profiles() to compute the consumer’s legitimate target universe. |
| src/apm_cli/install/phases/post_deps_local.py | Passes declared target universe into local deployed-files union logic. |
| src/apm_cli/install/phases/lockfile.py | Passes declared target universe into per-dependency deployed-files union logic. |
| src/apm_cli/install/manifest_reconcile.py | Extends union_preserving() to drop undeclared-target ghost entries when the declared universe is known. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | The target-governance extension fits the existing reconciliation architecture. |
| CLI Logging Expert | 0 | 0 | 1 | Add a verbose breadcrumb for ghost removal; folded in ec453aa. |
| DevX UX Expert | 0 | 0 | 1 | The unfixable audit loop is repaired and verbose diagnostics are now available. |
| Supply Chain Security Expert | 0 | 0 | 1 | Narrow broad fallback handling; folded in ec453aa. |
| OSS Growth Hacker | 0 | 0 | 2 | Changelog and contributor credit are now present. |
| Doc Writer | 0 | 1 | 0 | Baseline-check and lockfile-spec guidance are now synchronized. |
| Test Coverage Expert | 0 | 0 | 0 | Hermetic integration coverage and mutation proof defend the install/audit promise. |
| Performance Expert | 0 | 0 | 2 | Added governance work is bounded and outside the dependency loop. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Ship after CI completes on ec453aa. All real in-scope panel follow-ups are folded into this PR.
Full per-persona findings
- Python Architect: No architectural fault found; caching and fallback diagnostics were polish only.
- CLI Logging / DevX UX: Requested a
--verbosebreadcrumb for ghost removals; implemented. - Supply Chain Security: Requested narrower fallback exception handling; implemented. Other returned items were outside this diff.
- OSS Growth Hacker: Requested a changelog entry and contributor attribution; implemented.
- Auth Expert: Inactive; no authentication surface changed.
- Doc Writer: Requested lockfile and baseline-check documentation; implemented.
- Test Coverage Expert: The former integration-tier gap is closed by
tests/integration/test_inactive_target_ghost_e2e.py. - Performance Expert: No measurable hot-path concern; the reported per-dependency parse does not occur.
This panel is advisory. It does not gate merge. Re-apply the panel-review label after addressing feedback to re-run.
ec453aa to
09481e4
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python architect | 0 | 0 | 0 | Schema failure; no architecture findings accepted. |
| CLI logging expert | 0 | 1 | 2 | Per-path verbose diagnostics are useful; add a default aggregate repair summary. |
| DevX UX expert | 0 | 0 | 2 | Install auto-repair is intuitive; polish verbose wording and remediation guidance. |
| Supply-chain security expert | 0 | 0 | 1 | Fail-safe target-universe handling; no security bypass found. |
| OSS growth hacker | 0 | 0 | 2 | Strong contributor attribution and release-story potential. |
| Doc writer | 0 | 1 | 0 | Qualify docs for fallback and dynamic-target exceptions. |
| Test coverage expert | 0 | 0 | 1 | Critical promise is integration-proven; assert the local diagnostic too. |
| Performance expert | 0 | 0 | 1 | Added set lookups are negligible on the install path. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Doc writer] Qualify lockfile and baseline docs for preserve-all fallback and gated/dynamic-target exceptions.
- [CLI logging expert] Add a default-mode aggregate repair summary while keeping per-path detail verbose.
- [Test coverage expert] Assert the local-state verbose diagnostic in the e2e regression.
Recommendation
The behavioral fix is sound, mutation-guarded, and integration-proven. Fold the small documentation and observability improvements, then rerun the panel.
Full per-persona findings
Python architect
No accepted findings; the return failed schema validation.
CLI logging expert
- [recommended] Add a default-mode summary when ghost entries are pruned.
- [nit] Document eager
dep_keybinding in the loop closure. - [nit] Keep dependency and local-state verbose messages structurally consistent.
DevX UX expert
- [nit] Replace internal ghost-drop jargon with user-actionable wording.
- [nit] Make the lockfile commit remediation copy-pasteable.
Supply-chain security expert
- [nit] Surface an aggregate repair signal outside verbose mode for operator visibility.
OSS growth hacker
- [nit] Lead the changelog entry with the user pain rather than implementation detail.
- [nit] Preserve the contributor-attribution pattern.
Auth expert -- inactive
Install reconciliation, docs, and tests do not touch authentication or credential resolution.
Doc writer
- [recommended] Qualify cleanup claims and document the same lifecycle for
local_deployed_files.
Test coverage expert
- [nit] Explicitly assert the local-state verbose diagnostic; the underlying state repair is already proven.
Performance expert
- [nit] Consider hoisting declared governance-set computation only if target cardinality grows; current cost is noise.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python architect | 0 | 0 | 2 | Clean Strategy and Callback composition; no architectural faults. |
| CLI logging expert | 0 | 0 | 1 | Progressive disclosure and exact repair counts are correct. |
| DevX UX expert | 0 | 0 | 2 | Self-heal flow and remediation guidance are clear. |
| Supply-chain security expert | 0 | 0 | 0 | Fail-closed reconciliation; no security bypass. |
| OSS growth hacker | 0 | 0 | 2 | User-pain framing and contributor attribution are strong. |
| Doc writer | 0 | 0 | 0 | Target lifecycle and fallback behavior are documented accurately. |
| Test coverage expert | 0 | 0 | 1 | Critical promise is mutation-proven at integration tier. |
| Performance expert | 0 | 0 | 1 | Added bookkeeping is negligible on the install path. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
flowchart TD
A[Fresh checkout audit] --> B{Ghost path present?}
B -- Yes --> C[Plain apm install]
C --> D[Derive declared plus gated target universe]
D --> E{Prior path governed?}
E -- Yes --> F[Preserve legitimate sibling entry]
E -- No --> G[Drop ghost and report repair]
G --> H[Write repaired lockfile]
H --> I[Audit passes]
Recommendation
Merge as-is. There are zero blocking-severity or recommended findings, and no follow-ups to track.
Full per-persona findings
Python architect
- [nit] Consider sharing the two small ghost-count callbacks only if a third call site appears.
- [nit] Cache declared profiles only if phase cardinality grows; current cost is negligible.
CLI logging expert
- [nit] Direct logger binding would be a style-only consistency change.
DevX UX expert
- [nit] Dependency and local repair summaries can be two lines when both states need repair; pipeline aggregation would add disproportionate complexity.
- [nit] The test-only audit flags correctly remain absent from user remediation docs.
Supply-chain security expert
No findings.
OSS growth hacker
- [nit] Reorganizing the already-released 0.24.1 bullet list is outside this PR.
- [nit] Positive-promise changelog phrasing is optional polish.
Auth expert -- inactive
Install reconciliation, docs, and tests do not change authentication or credential resolution.
Doc writer
No findings.
Test coverage expert
- [nit] A formal Scenario Evidence table in the PR body is optional; the automated proof itself is complete.
Performance expert
- [nit] Context caching has no measurable value at current scale.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
APM Spec Guardian:
|
| Panel | Panel view | Shocked | New B | New R | New N |
|---|---|---|---|---|---|
| Swagger / OpenAPI Editor | ship_with_followups | 8/10 | 0 | 3 | 1 |
| OCI Distribution Editor | ship_with_followups | 8/10 | 0 | 4 | 2 |
| Package-Manager Registry-Contract Editor | ship_with_followups | 7/10 | 0 | 4 | 2 |
| Web-Platform Architect | ship_with_followups | 8/10 | 0 | 3 | 1 |
B = new blocking-severity findings, R = new recommended, N = new nits. Counts are signal strength, not gates. The maintainer ships.
Convergent themes (flagged by 2+ panels)
- T1 -- Define the legitimate implementation-recognized target boundary.
- T2 -- Define deterministic target attribution.
- T3 -- Add req-lk-020 to the Section 11.3.2 consumer enumeration.
- T4 -- Keep deployed-file lists and hash maps symmetric.
Fold now (4 item(s))
- [F1 / T3] Section 11.3.2 -- Added
req-lk-020afterreq-lk-019.
Success criterion: exactly onereq-lk-020citation appears in the Consumer enumeration. - [F2 / T1] req-lk-020 -- Replaced the undefined registration phrase with an implementation-recognized target whose activation is outside the manifest field.
Success criterion: the target boundary is explicit without inventing manifest syntax. - [F3 / T2] req-lk-020 -- Defined attribution using deploy roots, Registry filename patterns, URI schemes, and the shared-root partition in
req-tg-002.
Success criterion: independent consumers have deterministic attribution inputs. - [F4 / T4] req-lk-020 and Section 10.11 -- Applied retain/drop decisions to corresponding hash entries and cited the requirement in cleanup mitigations.
Success criterion: conformance proves ghost hashes drop and indeterminate hashes remain.
Linter notes (2 check(s) require context)
- [7] The checklist's simplified heading-slug expression reports pre-existing false positives for headings containing numbering and inline Markdown. All explicit requirement links resolve, the docs build gate remains authoritative, and this fold adds no heading.
- [11] Python files are modified because this is a code PR with a spec-conformance test. The general review panel is running separately as required.
Linter handoff: ASCII, forbidden-token, schema/fixture parse, anchor uniqueness, 98-statement count, fixture citation, changelog, orphan alignment, generator clean-diff, and conformance checks pass. The conformance suite reports 131 passed and 2 skipped.
Full per-panel findings
Swagger / OpenAPI Editor -- shocked_meter 8/10, confidence high
Summary: Counts, anchor uniqueness, conformance class, and cross-references align. Recommended defining target registration and attribution and cross-referencing frozen semantics; those items were folded. The multi-MUST indexing note was declined because Appendix C consistently indexes one anchor as one statement.
OCI Distribution Editor -- shocked_meter 8/10, confidence high
Summary: The fail-safe reconciliation is sound. Recommended deterministic attribution, a bounded non-manifest target category, a cleanup security citation, and hash integrity symmetry; all in-scope items were folded.
Package-Manager Registry-Contract Editor -- shocked_meter 7/10, confidence high
Summary: The preserve-by-default fallback is sound. Recommended deterministic attribution, bounded target recognition, and hash-map symmetry; all were folded. Producer-side guidance was deferred as outside this consumer requirement.
Web-Platform Architect -- shocked_meter 8/10, confidence high
Summary: The machine-readable conformance surface is sound. Recommended closing the Section 11.3.2 binding and defining the extension boundary; both were folded. A future garbage-collection policy was deferred as implementation policy.
This panel is advisory. It does not prevent merging. Re-apply the spec-review label after addressing feedback to re-run.
…ed_files (microsoft#2059) A dependency package whose apm.yml declares targets the consumer has not activated (e.g. windsurf) could leave inactive-target paths such as .windsurf/skills/... in the consumer's apm.lock.yaml deployed_files. The target-scoped union in LockfileBuilder._attach_deployed_files preserved any prior entry not governed by the active install, so once such a path entered the lockfile it was re-preserved on every install/update yet never written to disk. This fails the deployed-files-present audit check permanently on fresh checkouts, with no exit: apm install cannot restore a file for an inactive target. Scope the union to the consumer's legitimate target universe -- apm.yml-declared canonical targets plus the always-legitimate gated/dynamic targets (copilot-app, copilot-cowork, openclaw, hermes, which are activated by flag/detection and can never appear in apm.yml). A prior entry governed by none of them is an inactive-target ghost and is dropped; entries for a declared-but-not-installed- this-run target (e.g. a --target-narrowed sibling) are still preserved, keeping the microsoft#1716 multi-target contract intact. When the manifest declares no targets (auto-detect or --target-only), the legacy preserve-all behaviour is retained. Applies symmetrically to per-dependency deployed_files and project-root local_deployed_files.
Add a hermetic fresh-checkout install/audit regression and fold panel follow-ups for docs, changelog, verbose diagnostics, and narrower fallback handling. The mutation gate proves the test fails without the production ghost-drop guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fold panel feedback by qualifying target-universe docs, reporting aggregate repairs in default output, and extending the e2e regression through local lockfile state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Define req-lk-020, bind its machine-readable conformance coverage, and prove path/hash reconciliation against the production implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve declared sibling files under shared deploy roots by reconciling against primitive-level path prefixes, with unit and conformance regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep exact generated-file ownership from consuming lookalike paths while retaining directory-prefix governance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop only paths attributable to excluded registered targets while retaining unknown patterns, with declared-universe unit and conformance coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Prove an exact shared-root filename is preserved for a declared sibling and dropped when that target is excluded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ac95c83 to
d17400a
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Pure prefix-governance transform is simple and bounded; optional caching is unnecessary at current scale. |
| CLI Logging Expert | 0 | 0 | 2 | Aggregate repair counts and verbose path reasons follow progressive disclosure. |
| DevX UX Expert | 0 | 0 | 2 | Install repairs the lockfile with no new flags or migration burden. |
| Supply Chain Security | 0 | 0 | 1 | Known excluded targets drop; unknown patterns preserve; exact filenames do not consume lookalikes. |
| OSS Growth Hacker | 0 | 0 | 0 | User-outcome changelog and contributor credit are strong. |
| Documentation Writer | 0 | 0 | 0 | req-lk-020 and the implementation now match exactly. |
| Test Coverage Expert | 0 | 0 | 0 | All requested shared-root, exact-file, indeterminate, hash, and CLI e2e cases are pinned. |
| Performance Expert | 0 | 0 | 0 | Tiny in-memory prefix sets add no measurable install cost. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
flowchart TD
A[Prior lockfile path] --> B{Owned by current target?}
B -->|Yes| C[Current install is authoritative]
B -->|No| D{Owned by declared target?}
D -->|Yes| E[Preserve path and hash]
D -->|No| F{Owned by any registered target?}
F -->|Yes| G[Remove excluded-target ghost]
F -->|No| H[Preserve indeterminate path]
Recommendation
Ship now. The final branch is rebased, the reported contract gaps are closed, and every required local gate passes on d17400ab3.
Full per-persona findings
Python Architect
No in-scope findings.
CLI Logging Expert
No in-scope findings.
DevX UX Expert
No in-scope findings.
Supply Chain Security
No in-scope findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
No authentication, credential, token, or host-classification surface changed.
Documentation Writer
No findings after code/spec parity closure.
Test Coverage Expert
No findings after the exact shared-root ownership case was 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.
Closes #2059.
Problem
When a dependency package's
apm.ymldeclarestargets:the consumer has not activated (e.g.windsurf), inactive-target paths such as.windsurf/skills/...can end up in the consumer'sapm.lock.yamldeployed_files.apm install/updatecorrectly never writes those files, soapm audit --cifailsdeployed-files-presentpermanently on a fresh checkout — andapm installcannot fix it, since the target is inactive and nothing writes the file. The loop has no exit.Root cause
The failure is not in how
deployed_filesis recorded (that set is already scoped to the active install-target intersection). It is in how prior entries are carried forward.LockfileBuilder._attach_deployed_filesreconciles the current install's manifest with the prior lockfile viaunion_preserving, which preserves any prior entry not governed by the active install — the intended #1716 behaviour so a multi-target deploy keeps every target's files. But that union cannot tell a target the consumer legitimately uses (installed in a separate--targetrun) from a target the consumer never declares. So a.windsurf/...entry, once present, is re-preserved on every install yet never lands on disk.Fix
Scope the union to the consumer's legitimate target universe:
--targetnarrowing), pluscopilot-app,copilot-cowork,openclaw,hermes), which are activated by flag/detection and can never appear inapm.yml, so their entries — e.g.copilot-app-db://rows — must never be treated as ghosts.A prior entry governed by none of these is an inactive-target ghost and is dropped. Entries for a declared-but-not-installed-this-run target (a
--target-narrowed sibling) are still preserved, so the #1716 multi-target contract is unchanged. When the manifest declares no targets (auto-detect or--target-only consumers), the legacy preserve-all behaviour is retained.Because the recorded (
current) set is always clean, a singleapm install/updatenow purges an existing ghost and cannot introduce a new one. Applied symmetrically to per-dependencydeployed_filesand project-rootlocal_deployed_files.Verification
tests/unit/install/phases/test_lockfile_union.py: ghost drop,--target-narrowed sibling preserved, gatedcopilot-app-db://never dropped,declared=Nonefallback preserve-all, and an end-to-end_attach_deployed_fileswiring test reading targets fromapm.yml..agents/skills/deployed content escapes drift check + lockfile integrity (silent drift) #1716 tests unchanged and green..windsurf/...entry into a lockfile —apm audit --cifailsdeployed-files-present; afterapm installthe entry is gone and audit passes.Notes
driftsymptom mentioned in the issue (cache-replay bytes differing from the install write path for cross-target skill paths) looks like a separate concern and is not addressed here; happy to open a follow-up.apm.ymlafter deploying to it, that target's on-disk files drop out of the lockfile (target-scoped cleanup does not remove them). This matches the "no longer a declared target" intent and is called out in a code comment.