Fix/windsurf uninstall skills cleanup#1486
Conversation
…tall The windsurf TargetProfile declared both 'agents' and 'skills' primitives mapped to the same deploy path .windsurf/skills/<name>/SKILL.md. This caused BaseIntegrator.partition_managed_files to route uninstall paths to the agents_windsurf bucket, where AgentIntegrator.sync_for_target called unlink() on what was actually a directory. The IsADirectoryError was swallowed silently and skill directories survived 'apm uninstall'. Drop the 'agents' primitive from windsurf entirely. Cascade auto-invokes any SKILL.md under .windsurf/skills/ based on the description frontmatter, so a separate agents primitive was redundant and was the only source of the path collision. Also remove the now-unreachable _write_windsurf_agent_skill transformer in AgentIntegrator and its dedicated unit tests, and update the windsurf scope integration tests to assert the new shape. Closes microsoft#1481
…tall The windsurf TargetProfile declared both 'agents' and 'skills' primitives mapped to the same deploy path .windsurf/skills/<name>/SKILL.md. This caused BaseIntegrator.partition_managed_files to route uninstall paths to the agents_windsurf bucket, where AgentIntegrator.sync_for_target called unlink() on what was actually a directory. The IsADirectoryError was swallowed silently and skill directories survived 'apm uninstall'. Drop the 'agents' primitive from windsurf entirely. Cascade auto-invokes any SKILL.md under .windsurf/skills/ based on the description frontmatter, so a separate agents primitive was redundant and was the only source of the path collision. Also remove the now-unreachable _write_windsurf_agent_skill transformer in AgentIntegrator and its dedicated unit tests, and update the windsurf scope integration tests to assert the new shape. Closes microsoft#1481
…b.com/yoelabril/apm into fix/windsurf-uninstall-skills-cleanup
…b.com/yoelabril/apm into fix/windsurf-uninstall-skills-cleanup
yoelabril
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Windsurf/Cascade integration to remove the agents primitive (to avoid uninstall cleanup path collisions with skills) and adds regression tests + documentation updates to lock in the new behavior.
Changes:
- Removed Windsurf
agentsprimitive mapping and the now-unreachable agent→skill transformer. - Added regression tests ensuring
.windsurf/skills/<name>/directories are correctly bucketed and removed on uninstall. - Updated existing tests and docs to reflect that Windsurf no longer supports an
agentsprimitive.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/integration/test_windsurf_uninstall_skills.py | New regression coverage for Windsurf profile shape, routing, and uninstall directory cleanup. |
| tests/unit/integration/test_scope_integration.py | Updates scope assertions to expect skills and forbid agents for Windsurf. |
| tests/unit/integration/test_data_driven_dispatch.py | Removes agents_windsurf from expected bucket parity list. |
| tests/unit/integration/test_agent_integrator.py | Removes Windsurf agent integration tests; leaves explanatory note. |
| tests/integration/test_integrators_validation_rules.py | Removes validation tests for the deleted Windsurf agent-skill transformer. |
| tests/integration/test_integrators_validation_phase3b.py | Same as above for phase3b validation suite. |
| src/apm_cli/integration/targets.py | Drops Windsurf agents primitive; clarifies intended authoring path in comments. |
| src/apm_cli/integration/instruction_integrator.py | Updates a comment to remove reference to removed Windsurf agent transformer. |
| src/apm_cli/integration/agent_integrator.py | Removes the windsurf_agent_skill conversion path and implementation. |
| docs/src/content/docs/reference/targets-matrix.md | Updates Windsurf capabilities to exclude agents and adds guidance to ship personas as skills. |
| docs/src/content/docs/producer/author-primitives/instructions-and-agents.md | Clarifies that Windsurf/Gemini do not receive .agent.md and recommends skills instead. |
| docs/src/content/docs/integrations/ide-tool-integration.md | Updates Windsurf support summary to reflect rules/skills/workflows/MCP. |
| docs/src/content/docs/concepts/primitives-and-targets.md | Updates Windsurf notes and compatibility table to mark agents unsupported. |
…b.com/yoelabril/apm into fix/windsurf-uninstall-skills-cleanup
APM Review Panel: ship_with_followupsCommunity-contributed fix restores the manifest-to-filesystem cleanup invariant for windsurf skills, removing dead code and pinning the fix with a 132-line regression test. cc @yoelabril @danielmeppiel @sergio-sisternes-epam ArbitrationAll six active panelists converge on the same conclusion: the root cause (two primitives mapping to one deploy path, sending uninstall through a file-only branch that swallowed the directory error) is correctly eliminated by dropping the overlapping agents primitive and letting The panel's recommended findings cluster around two themes: (1) communication debt -- a missing CHANGELOG entry and missing migration callout for authors whose Strategically, this PR is a net positive for defensibility: it tightens the Principle alignment
Growth amplificationPR #1486 is an exemplar community contribution: external author reported the bug, self-fixed with surgical dead-code removal, and pinned the promise with a regression test. This pattern -- file, fix, prove -- is exactly the contributor signal APM should celebrate in release notes and use as onboarding proof that the codebase is approachable enough for outside contributors to ship meaningful fixes. Panel summary
Top follow-ups (curated)
Ship recommendationStance: ship_with_followups. The headline cleanup promise is correctly fixed: directory-aware uninstall is restored and pinned with a regression test that exercises the real Architecture sketch (python-architect)Class shape post-fix: classDiagram
class TargetProfile {
+name: str
+primitive_mappings: dict[str, PrimitiveMapping]
}
class PrimitiveMapping {
+subdir: str
+extension: str
+transformer: callable
}
class BaseIntegrator
class SkillIntegrator {
+sync_integration(stats, project_root)
}
class AgentIntegrator
TargetProfile o-- PrimitiveMapping
BaseIntegrator <|-- SkillIntegrator
BaseIntegrator <|-- AgentIntegrator
Uninstall data flow: flowchart LR
A[apm uninstall pkg] --> B[partition_managed_files]
B --> C{primitive type}
C -->|skill| D[SkillIntegrator.sync_integration]
D --> E[shutil.rmtree on .windsurf/skills/pkg/]
E --> F[stats files_removed and errors]
Per-persona findingspython-architect (0 / 0 / 2)
cli-logging-expert (0 / 0 / 3)
devx-ux-expert (0 / 1 / 1)
supply-chain-security-expert (0 / 0 / 2)
oss-growth-hacker (0 / 2 / 1)
auth-expert (inactive)Inactive -- PR touches only integration/targets.py, integration/agent_integrator.py, integration/instruction_integrator.py, integration tests, and doc pages; no auth-surface files. doc-writer (0 / 2 / 2)
test-coverage-expert (0 / 1 / 0)
This is an advisory recommendation from the APM Review Panel. It is not a merge gate. The maintainer and author weigh these signals. |
danielmeppiel
removed
the
panel-review
Windsurf has no native agents primitive, so supports_at_user_scope("agents")
should return False. Update test assertion and clarify comment.
Earlier APM versions compiled `.apm/agents/*.agent.md` to `.windsurf/skills/<name>/SKILL.md` with stripped frontmatter. That mapping has been removed: agents no longer deploy to Windsurf. Add migration caution block advising users to re-author personas as skills under `.apm/skills/<name>/SKILL.md` if they need Windsurf support. Update CHANGELOG.md to document the uninstall cleanup fix for the shared deploy path that caused empty directories to survive.
|
Panel follow-ups addressed inline:
|
- instruction_integrator.py: reflow the yaml.safe_load comment into a single clear sentence so it no longer reads as an orphan continuation. - test_windsurf_uninstall_skills.py: pivot the regression tests to assert filesystem outcomes primarily (managed dirs removed, user dir preserved). The 'files_removed' stats key is kept as a sanity check because it is the established cross-integrator counter convention (see base_integrator, hook_integrator, command_integrator, prompt_integrator, skill_integrator, copilot_app_workflow_integrator, agent_integrator) and is consumed by uninstall/engine.py counters; renaming would have repo-wide blast radius for a directory-counting edge case that affects only the windsurf skills path. Co-authored-by: yoelabril <yoelabril@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- instruction_integrator.py: reflow the yaml.safe_load comment into a single clear sentence so it no longer reads as an orphan continuation. - test_windsurf_uninstall_skills.py: pivot the regression tests to assert filesystem outcomes primarily (managed dirs removed, user dir preserved). The 'files_removed' stats key is kept as a sanity check because it is the established cross-integrator counter convention (see base_integrator, hook_integrator, command_integrator, prompt_integrator, skill_integrator, copilot_app_workflow_integrator, agent_integrator) and is consumed by uninstall/engine.py counters; renaming would have repo-wide blast radius for a directory-counting edge case that affects only the windsurf skills path. Co-authored-by: yoelabril <abril.yoel@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
force-pushed
the
fix/windsurf-uninstall-skills-cleanup
branch
from
545302d to
17d9ffd
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean data-model fix: removes overlapping primitive from TargetProfile, deletes dead code + tests, adds regression pin. Architecturally sound. |
| CLI Logging Expert | 0 | 0 | 1 | Removed warning was dead code coupled to a dead path; uninstall cleanup output already surfaces skill dir removal. No output regression. |
| DevX UX Expert | 0 | 0 | 1 | Migration story is solid; only ergonomic gap is the silent no-op when targeting windsurf with agents post-fix. |
| Supply Chain Security Expert | 0 | 0 | 1 | PR fix is sound from supply-chain perspective; eliminates overlapping-primitive collision that left stale Cascade-auto-invokable skill content on disk after uninstall. |
| OSS Growth Hacker | 0 | 0 | 3 | Textbook contributor-arc story; @yoelabril shipped windsurf target (#1066) and is back fixing its edge case 4 weeks later -- retention proof-point. |
| Doc Writer | 0 | 0 | 2 | Docs updates are accurate, consistent across the three matrices, and cover the migration; one nit on cross-link discoverability. |
| Test Coverage Expert | 0 | 0 | 1 | 7 regression tests at integration-with-fixtures tier cover the fix; all pass in 1.12s. Ship. |
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] Add symlink regression trap test for windsurf uninstall rmtree path -- secure-by-default surface with outcome:missing evidence. General containment check exists but is not pinned for the windsurf-specific directory removal path; a future refactor could silently bypass it.
- [Doc Writer] Verify manifest-schema.md windsurf row accuracy re: AGENTS.md emission post agents-primitive removal -- if AGENTS.md was coupled to the removed agents primitive on windsurf, the reference doc row is now stale. One-minute verification.
- [DevX UX Expert] Emit diagnostics.info when target lacks agents primitive but agent sources exist -- closes discoverability gap for users migrating from agents-on-windsurf. Low effort, high first-run clarity.
- [Doc Writer] Add relative cross-link from migration callout to skills authoring page -- users reading the migration caution block need one click to reach the SKILL.md frontmatter reference.
- [OSS Growth Hacker] Polish CHANGELOG entry to a punchy one-liner crediting @yoelabril for social reshareability -- dense commit-log-style entries get zero amplification; a one-liner compounds contributor-funnel messaging.
Architecture
classDiagram
direction LR
class TargetProfile {
<<ValueObject>>
+name: str
+root_dir: str
+primitives: dict
+supports(prim) bool
+for_scope(user_scope) TargetProfile
}
class PrimitiveMapping {
<<ValueObject>>
+subdir: str
+extension: str
+format_id: str
+deploy_root: str
}
class BaseIntegrator {
<<Abstract>>
+partition_managed_files(managed) dict
+sync_remove_files()
}
class SkillIntegrator {
+sync_integration(pkg, root, files, targets) dict
}
class AgentIntegrator {
+integrate_agents_for_target(target, sources, root)
+sync_integration(pkg, root, files)
}
TargetProfile *-- PrimitiveMapping
BaseIntegrator ..> TargetProfile
SkillIntegrator --|> BaseIntegrator
AgentIntegrator --|> BaseIntegrator
SkillIntegrator ..> TargetProfile
AgentIntegrator ..> TargetProfile
note for TargetProfile "Each deploy path owned by\nexactly one primitive per target"
flowchart TD
A[apm uninstall pkg] --> B[uninstall/engine.py: build managed_files from lockfile]
B --> C{BaseIntegrator.partition_managed_files}
C -->|prefix .windsurf/skills/| D[skills bucket]
C -->|prefix .github/agents/| E[agents_copilot bucket]
D --> F[SkillIntegrator.sync_integration]
F --> G{target.is_dir}
G -->|Yes| H[shutil.rmtree target]
G -->|No| I[target.unlink]
H --> J[stats files_removed plus 1]
I --> J
E --> K[AgentIntegrator.sync_integration]
K --> L[file-level unlink]
sequenceDiagram
participant User
participant Engine as uninstall/engine.py
participant Base as BaseIntegrator
participant Skill as SkillIntegrator
participant FS as Filesystem
User->>Engine: apm uninstall my-pkg
Engine->>Base: partition_managed_files
Note over Base: windsurf has no agents primitive now -- path routes to skills bucket only
Base-->>Engine: skills .windsurf/skills/my-pkg
Engine->>Skill: sync_integration managed_files
Skill->>FS: resolve .windsurf/skills/my-pkg
FS-->>Skill: is_dir True
Skill->>FS: shutil.rmtree .windsurf/skills/my-pkg/
Skill-->>Engine: files_removed 1 errors 0
Recommendation
Merge as-is. All signals are green: lint passes, 7 regression tests pass at integration-with-fixtures tier proving the fix, docs migration is in place, and no panelist raised a blocking or even a recommended-severity finding. The symlink trap and manifest-schema accuracy check are worth filing as fast follow-up issues but do not gate this PR -- the containment check already exists in production code and the behavior fix is proven by passing test evidence. Reward the contributor arc with a clean merge.
Full per-persona findings
Python Architect
- [nit] Design patterns and architectural assessment for this PR
Used in this PR: Dataclass-as-value-object (TargetProfile frozen dataclass = single source of truth for primitive->path mapping; fix propagates through partition_managed_files and SkillIntegrator without touching them). Used in this PR: Base class + subclass (SkillIntegrator inherits BaseIntegrator and owns directory-aware cleanup via sync_integration). Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope. Declarative TargetProfile already encodes the invariant; regression test pins it directly.
CLI Logging Expert
- [nit] No verbose-only migration breadcrumb when agents stop deploying to windsurf on upgrade
A user who previously had .apm/agents/*.agent.md compiling to .windsurf/skills/ will see no CLI indication that this stopped happening after upgrading. The docs caution block covers it, but the newspaper-test principle says the CLI should surface behavioral changes where the user would notice them -- during apm install --verbose at minimum. This is polish, not blocking: behavioral change is correct, docs explain migration, removed diagnostics.warn was tightly coupled to the now-dead transformer.
Suggested: Consider a one-time verbose_detail line in integrate_agents_for_target when target is windsurf and agents exist in the manifest. Low priority; skip if scope creep.
DevX UX Expert
- [nit] Silent no-op when integrate_agents_for_target returns early because target lacks agents primitive at
src/apm_cli/integration/agent_integrator.py:105
When source agent files exist but the target has no agents primitive support, the user gets zero feedback. An author who previously saw agents deploy to windsurf now sees silence with no explanation. A one-time diagnostics.info() would close the discoverability gap. Consistent with existing gaps for other unsupported primitives; suitable for a follow-up issue rather than blocking this PR.
Suggested: Add diagnostics.info('Target {target_name} does not support the agents primitive; skipping agent integration.') before the early return.
Supply Chain Security Expert
- [nit] Add a symlink regression trap for the windsurf uninstall path at
tests/unit/integration/test_windsurf_uninstall_skills.py
Containment check at skill_integrator.py:1349 catches symlink-outside-project generally, but no test pins this for the windsurf-specific rmtree path. A symlink pointing outside project_root could bypass containment if future refactors move the check. Pinning it closes the loop.
Suggested: Add a test case: create a managed skill dir as a symlink to outside dir, assert uninstall refuses or does not follow the symlink outside project root.
Proof (missing at):tests/unit/integration/test_windsurf_uninstall_skills.py-- proves: Uninstall of a symlinked skill directory does not delete content outside the project root [secure-by-default]
OSS Growth Hacker
- [nit] CHANGELOG entry is dense; distill to a one-liner hook for release-narrative reposting at
CHANGELOG.md
Release notes that read like commit logs get zero social reshares. A punchy one-liner is repostable by the contributor and by watchers, compounding reach.
Suggested: Replace dense entry with: 'apm uninstall now fully cleans .windsurf/skills/ directories (by @yoelabril)' - [nit] Migration caution box in instructions-and-agents.md would compound with a before/after file-tree snippet at
docs/src/content/docs/producer/author-primitives/instructions-and-agents.md
Readers skim admonitions; a visual diff lands in under 2 seconds and reduces support questions.
Suggested: Add a before/after file-tree snippet inside the caution box. - [nit] Release-notes shout-out should reinforce that the same contributor who built the feature returned to fix its edge case.
Naming the pattern signals to prospective contributors that they can own surfaces end-to-end, not just drive-by. Compounds contributor funnel messaging.
Auth Expert -- inactive
PR touches only windsurf TargetProfile cleanup and adds uninstall regression tests; no auth-path files are modified.
Doc Writer
- [nit] Migration callout could link to the skills authoring page so readers land on the re-author path with one click. at
docs/src/content/docs/producer/author-primitives/instructions-and-agents.md
Migration block tells readers to re-author personas as skills but does not link to producer/author-primitives/skills/ where SKILL.md frontmatter is documented. A relative link keeps the block short.
Suggested: Change 're-author it as a skill under .apm/skills//SKILL.md' to 're-author it as a skill under .apm/skills//SKILL.md'. - [nit] reference/manifest-schema.md row for windsurf still lists AGENTS.md in the deploy summary; verify it is still emitted after agents primitive removal. at
docs/src/content/docs/reference/manifest-schema.md:154
Accuracy spot-check: the manifest-schema row is unchanged by the PR. If AGENTS.md is still produced (as the compiled context aggregator, independent of the agents primitive) this is fine; if AGENTS.md was tied to the removed agents primitive on windsurf, the row is now wrong.
Test Coverage Expert
- [nit] No test exercises the full apm uninstall CLI command path for windsurf targets end-to-end. at
tests/unit/integration/test_windsurf_uninstall_skills.py
The new tests exercise SkillIntegrator.sync_integration and partition_managed_files with real filesystem fixtures (integration-with-fixtures tier; covers cross-module contract). A full e2e test invoking apm uninstall with a real lockfile and windsurf target dir would additionally cover the uninstall/engine.py orchestration layer, but the risk is low: engine.py dispatches to sync_integration unchanged, and the bug was entirely in the TargetProfile shape + integrator routing. Not blocking; the regression-trap already lives at the right boundary.
Proof (passed):tests/unit/integration/test_windsurf_uninstall_skills.py::TestWindsurfSkillUninstallCleanup::test_sync_removes_windsurf_skill_directories-- proves: apm uninstall removes deployed .windsurf/skills// directories without touching user-authored skill dirs [multi-harness-support,devx]
assert not managed_a.exists() and stats['files_removed'] == 2
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
…l-skills-cleanup # Conflicts: # docs/src/content/docs/producer/author-primitives/instructions-and-agents.md
…llow-up microsoft#1520 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
left a comment
danielmeppiel
left a comment
There was a problem hiding this comment.
Ship. Fix is contract-correct; conflicts resolved against current main; CHANGELOG threaded with follow-up #1520 (windsurf convergence onto .agents/skills/ per Cascade native discovery).
Description
apm uninstallsilently failed to remove deployed skill directories from.windsurf/skills/, leaving stale files behind after a package was removed.Root cause: the windsurf target declared two overlapping primitives (
agentsandskills) pointing at the same deploy path, which sent uninstall through a file-only cleanup branch that errored out on a directory and swallowed the exception.Fix: drop the redundant
agentsprimitive from windsurf so cleanup goes through the directory-aware path. Cascade already auto-invokes skills by theirdescription:frontmatter, so no functionality is lost.Fixes #1481
Type of change
Testing