Feature/windsurf target#1066
Conversation
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Adds Windsurf/Cascade as a first-class APM target so apm install/compile --target windsurf can deploy primitives into .windsurf/ (and user-scope config under ~/.codeium/windsurf/ where applicable).
Changes:
- Registers a new
windsurfTargetProfileand target auto-detection/CLI parsing support. - Adds instruction and agent integration behavior for Windsurf (rules frontmatter conversion; agents -> skills).
- Introduces a Windsurf MCP client adapter and extends MCP runtime install/cleanup logic to include Windsurf.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/integration/test_targets.py | Extends active target detection tests to include .windsurf/. |
| tests/unit/integration/test_scope_integration.py | Adds Windsurf scope resolution tests and user-scope primitive filtering assertions. |
| tests/unit/integration/test_instruction_integrator.py | Adds unit + integration tests for Windsurf rules conversion/deploy. |
| tests/unit/integration/test_agent_integrator.py | Adds unit + integration tests for agents -> Windsurf skills conversion/deploy. |
| tests/unit/core/test_target_detection.py | Adds detection + compile-behavior tests for the windsurf target. |
| tests/unit/core/test_scope.py | Updates KNOWN_TARGETS expectations and user-scope support assertions for Windsurf. |
| src/apm_cli/integration/targets.py | Registers windsurf target mapping and scope metadata. |
| src/apm_cli/integration/mcp_integrator.py | Adds Windsurf to MCP runtime detection and stale-config cleanup. |
| src/apm_cli/integration/instruction_integrator.py | Adds windsurf_rules instruction conversion + copy path. |
| src/apm_cli/integration/agent_integrator.py | Adds windsurf_agent_skill transformer and dispatch. |
| src/apm_cli/factory.py | Registers WindsurfClientAdapter in ClientFactory. |
| src/apm_cli/core/target_detection.py | Adds windsurf to target types, auto-detect, and descriptions. |
| src/apm_cli/compilation/agents_compiler.py | Allows compiling for the windsurf target. |
| src/apm_cli/adapters/client/windsurf.py | New MCP client adapter writing ~/.codeium/windsurf/mcp_config.json. |
| build/apm.spec | Adds the Windsurf client module to PyInstaller hidden imports. |
APM Review Panel Verdict: REJECT
Required before merge (13 items)
Nits (8 items, skip if you want)
CEO arbitrationThe panel reached broad consensus on the critical shape of this PR: it is structurally incomplete. Five of five active panelists raised required findings, and three overlapping clusters crystallize the verdict. First, the hooks gap is the most architecturally dangerous: python-architect and devx-ux-expert independently identified that windsurf TargetProfile declares a hooks primitive but 'windsurf' is absent from _MERGE_HOOK_TARGETS, meaning hooks silently deploy zero files. This is not a nit -- silent feature failures are harder for users to diagnose than hard errors, and shipping a hooks surface that does nothing corrodes trust in the entire APM integration model. Second, the configure_mcp_server copy-paste is flagged as required by both python-architect (maintenance split) and cli-logging-expert (raw print() bypassing the rich output system). These are distinct but compounding harms on the same code block; both reviewers are correct, and both must be resolved before merge. The cli-logging-expert finding is not a subset of the python-architect finding -- fixing the duplication alone would still leave the print() regression in whichever copy survives. Third, supply-chain-security-expert raised three required findings (frontmatter injection via apply_to, YAML key injection via name/description, and missing ensure_path_within on instruction target_path). None of these have compensating controls elsewhere in the PR. Security findings at this severity are never overridden without a written trade-off and a follow-up issue; no such statement exists here. The documentation and discovery gaps (cli-commands.md, README, CHANGELOG, manifest-schema.md, apm-guide skill) raised by devx-ux-expert and oss-growth-hacker are required, not advisory, because APM's contract with contributors is that a feature does not exist until it is discoverable. A target absent from cli-commands.md and manifest-schema.md is effectively a hidden flag -- worse than no feature at all because it will generate confused support requests. The get_target_description mismatch (.windsurf/rules/ vs .windsurf/skills/ for agents) is a user-visible lie and must be corrected in the same PR that introduced it. Dissent resolved: No panelist disagreed on required vs nit classification for any shared finding. The configure_mcp_server finding is raised independently by python-architect (maintenance split) and cli-logging-expert (print() regression). These are additive, not overlapping: the fix must both eliminate the duplication via super() delegation or a protected label property AND replace the surviving print() calls with the rich output helpers. Growth/positioning note: Windsurf / Codeium is the fastest-growing AI IDE segment outside VS Code. Adding Windsurf as a compile target is a top-tier acquisition surface -- Windsurf users searching for AI package management will land on APM's README and see themselves reflected for the first time. When this PR is re-submitted with all required findings resolved, the CHANGELOG entry should be written as a headline feature ('APM now compiles and integrates to Windsurf'), not buried as an adapter addition -- this is positioning language, not just release hygiene. Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class BaseMCPClientAdapter {
<<Abstract>>
+get_config_path() str
+update_config(config_updates) None
+get_current_config() dict
+configure_mcp_server(...) bool
}
class CopilotClientAdapter {
<<ConcreteAdapter>>
+registry_client SimpleRegistryClient
+get_config_path() str
+update_config(config_updates) None
+get_current_config() dict
+configure_mcp_server(...) bool
+_format_server_config(...) dict
}
class WindsurfClientAdapter {
<<ConcreteAdapter>>
+supports_user_scope bool
+get_config_path() str
+update_config(config_updates) None
+get_current_config() dict
+configure_mcp_server(...) bool
}
class HookIntegrator {
<<BaseIntegrator>>
+integrate_hooks_for_target(target) HookIntegrationResult
+_integrate_merged_hooks(config) HookIntegrationResult
}
class _MergeHookConfig {
<<ValueObject>>
+config_filename str
+target_key str
+require_dir bool
}
class InstructionIntegrator {
<<BaseIntegrator>>
+copy_instruction_windsurf(...) int
+_convert_to_windsurf_rules(content) str
}
class AgentIntegrator {
<<BaseIntegrator>>
+_write_windsurf_agent_skill(src, dest) None
}
BaseMCPClientAdapter <|-- CopilotClientAdapter
CopilotClientAdapter <|-- WindsurfClientAdapter
HookIntegrator ..> _MergeHookConfig : dispatches via _MERGE_HOOK_TARGETS
note for WindsurfClientAdapter "configure_mcp_server body duplicates parent verbatim. Docstring says delegates to parent but does not call super(). windsurf absent from _MERGE_HOOK_TARGETS -> silent 0-files result."
note for HookIntegrator "_MERGE_HOOK_TARGETS keys: claude | cursor | codex | gemini. windsurf MISSING -> silent no-op"
class WindsurfClientAdapter:::touched
class HookIntegrator:::touched
class InstructionIntegrator:::touched
class AgentIntegrator:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A([apm compile --target windsurf]) --> B[factory.py: resolve WindsurfTarget]
B --> C[agents_compiler.py: compile agents loop]
B --> D[instruction_integrator.py: integrate instructions]
B --> E[hook_integrator.py: integrate_hooks_for_target windsurf]
C --> C1["[FS] AgentIntegrator._write_windsurf_agent_skill()\nf-string name/description -- injection risk\n-> skills/name/SKILL.md"]
D --> D1{format_id?}
D1 -->|windsurf_rules| D2["[FS] _convert_to_windsurf_rules()\napply_to inserted verbatim -- injection risk"]
D1 -->|windsurf_workflow commands| D3["[FS] copy_instruction()\nplain copy -- falls through, no explicit branch"]
D1 -->|windsurf_agent_skill| C1
E --> E1{target.name in _MERGE_HOOK_TARGETS?}
E1 -->|NO windsurf absent| E2["returns files_integrated=0\nno warning, no error -- SILENT FAILURE"]
E1 -->|YES claude/cursor/codex/gemini| E3["[FS] _integrate_merged_hooks()"]
Design patterns
Required:
Nits:
CLI Logging ExpertRequired:
Nits:
DevX UX ExpertRequired:
Nits:
Supply Chain Security ExpertRequired:
Nits:
Auth ExpertInactive -- No auth files changed; PR adds Windsurf IDE integration target via file-system paths only, inheriting auth from CopilotClientAdapter without modifying AuthResolver, token precedence, or host classification. OSS Growth HackerRequired:
Nits:
Verdict computed deterministically: 13 required findings across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
274379a to
5bc180d
Compare
5bc180d to
87f6c12
Compare
66f1bef to
5bd22b7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/apm_cli/integration/mcp_integrator.py:1033
- MCP install-time runtime selection filters installed runtimes by those referenced in
apm.ymlscripts (via_detect_runtimes). Windsurf was added to the installed-runtime list, but_detect_runtimes()currently cannot detectwindsurfcommands, so a repo that references Windsurf in scripts could incorrectly skip configuring it. Addwindsurfdetection (and ensure any fallback allowlists include it) so script-based targeting works consistently.
for runtime_name in ["copilot", "codex", "vscode", "cursor", "opencode", "gemini", "windsurf"]:
try:
if runtime_name == "vscode":
if _is_vscode_available(project_root=project_root_path):
ClientFactory.create_client(runtime_name)
installed_runtimes.append(runtime_name)
elif runtime_name == "cursor":
# Cursor is opt-in: only target when .cursor/ exists
if (project_root_path / ".cursor").is_dir():
ClientFactory.create_client(runtime_name)
installed_runtimes.append(runtime_name)
elif runtime_name == "opencode":
# OpenCode is opt-in: only target when .opencode/ exists
if (project_root_path / ".opencode").is_dir():
ClientFactory.create_client(runtime_name)
installed_runtimes.append(runtime_name)
elif runtime_name == "gemini":
# Gemini CLI is opt-in: only target when .gemini/ exists
if (Path.cwd() / ".gemini").is_dir():
ClientFactory.create_client(runtime_name)
installed_runtimes.append(runtime_name)
elif runtime_name == "windsurf":
# Windsurf is opt-in: only target when .windsurf/ exists
if (project_root_path / ".windsurf").is_dir():
ClientFactory.create_client(runtime_name)
installed_runtimes.append(runtime_name)
else: # noqa: PLR5501
if manager.is_runtime_available(runtime_name):
ClientFactory.create_client(runtime_name)
installed_runtimes.append(runtime_name)
except (ValueError, ImportError):
continue
except ImportError:
installed_runtimes = [
rt for rt in ["copilot", "codex"] if shutil.which(rt) is not None
]
# VS Code: check binary on PATH or .vscode/ directory presence
if _is_vscode_available(project_root=project_root_path):
installed_runtimes.append("vscode")
# Cursor is directory-presence based, not binary-based
if (project_root_path / ".cursor").is_dir():
installed_runtimes.append("cursor")
# OpenCode is directory-presence based
if (project_root_path / ".opencode").is_dir():
installed_runtimes.append("opencode")
# Gemini CLI is directory-presence based
if (Path.cwd() / ".gemini").is_dir():
installed_runtimes.append("gemini")
# Windsurf is directory-presence based
if (project_root_path / ".windsurf").is_dir():
installed_runtimes.append("windsurf")
# Step 2: Get runtimes referenced in apm.yml scripts
script_runtimes = MCPIntegrator._detect_runtimes(
apm_config.get("scripts", {}) if apm_config else {}
)
5bd22b7 to
5904c15
Compare
Add Windsurf/Cascade support to APM's multi-target architecture. Primitive mapping: - instructions -> .windsurf/rules/*.md (trigger: glob) - agents -> .windsurf/skills/<name>/SKILL.md (no native agent primitive) - skills -> .windsurf/skills/<name>/SKILL.md (native format) - prompts/commands -> .windsurf/workflows/*.md - hooks -> .windsurf/hooks.json - compiled instructions -> AGENTS.md (always-on, native support) Implementation: - Add TargetProfile to KNOWN_TARGETS (rules, skills, workflows, hooks) - Create WindsurfClientAdapter for MCP config (~/.codeium/windsurf/mcp_config.json) - Register in ClientFactory, target detection, agents compiler - Add runtime detection and stale MCP cleanup in mcp_integrator - Add windsurf_rules format handler (applyTo -> trigger/globs frontmatter) - Add windsurf_agent_skill transformer (agent.md -> SKILL.md) - AGENTS.md compilation enabled (Windsurf reads it natively) - Tests: detection, scope, instruction conversion, agent skill conversionfeat: add windsurf/cascade as compilation and integration target - Add TargetProfile to KNOWN_TARGETS (rules, skills, workflows, hooks) - Create WindsurfClientAdapter for MCP config (~/.codeium/windsurf/mcp_config.json) - Register in ClientFactory, target detection, agents compiler - Add runtime detection and stale MCP cleanup in mcp_integrator - Add windsurf_rules format handler (applyTo -> trigger/globs frontmatter) - AGENTS.md compilation enabled (Windsurf reads it natively) - Add tests: detection, scope, instruction conversion, integration (all pass)
5904c15 to
7f49689
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | Clean, well-structured target addition; two focused issues: inconsistent frontmatter parsing in _convert_to_windsurf_rules vs _write_windsurf_agent_skill. |
| CLI Logging Expert | 0 | 1 | 1 | Windsurf adapter is clean and consistent; inherits a pre-existing print() anti-pattern but introduces no new output regressions. |
| DevX UX Expert | 0 | 2 | 2 | Windsurf integration is solid; two gaps break the mental-model contract -- lossy agent->skill conversion is invisible, and apm-guide commands.md is stale. |
| Supply Chain Security Expert | 0 | 1 | 2 | Path-traversal change is architecturally correct and safe; hook script copy destinations lack ensure_path_within guard. |
| OSS Growth Hacker | 0 | 2 | 2 | Windsurf is technically wired up but the growth story is undertold -- README has no runnable example, CHANGELOG is engineer-written. |
| Auth Expert | 0 | 0 | 2 | WindsurfClientAdapter safely inherits Copilot auth logic; path relaxation introduces no auth bypass. |
| Doc Writer | 2 | 5 | 0 | Windsurf docs are partially complete -- integration guide missing Windsurf section, agent->skill lossy-mapping undocumented, auto_create=False unmentioned. |
| Test Coverage Expert | 0 | 2 | 0 | Hook-integrator windsurf routing and WindsurfClientAdapter.get_config_path() have no tests; all other key surfaces are covered. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc Writer] (blocking-severity) Add a Windsurf section to
ide-tool-integration.mdcovering setup,auto_create=Falsebehavior, user-scope limitations, and the lossy agent->skill mapping. -- Blocking doc gap: every other first-class target has an integration guide section. Without it, users see silent no-ops onapm installwith no path to resolution. - [Supply Chain Security Expert] Add
ensure_path_withinguard on hook script copy destinations inhook_integrator.pybeforeshutil.copy2. -- For Windsurf user-scope, unguardedtarget_scriptresolves project-relative instead of user-home-relative -- both a functional mismatch and a missing path safety net. Test-coverage-expert confirms zero test coverage for this branch (outcome: missing), elevating risk. - [Test Coverage Expert] Add unit tests for
WindsurfClientAdapter.get_config_path()and the Windsurf hook routing branch intest_hook_integrator.py. -- Both areoutcome: missingon surfaces that touch path resolution and file deployment -- the two highest regression-trap areas in this PR. - [DevX UX Expert] Emit a per-file warning in
_write_windsurf_agent_skillwhentoolsormodelfrontmatter keys are dropped during agent->skill conversion. -- Silent data loss at install time breaks the 'install adds, never silently mutates' mental model. - [Python Architect] Replace the hand-rolled
applyTo:line-scan in_convert_to_windsurf_ruleswithyaml.safe_loadon the frontmatter block. -- Every other frontmatter parser in the codebase usesyaml.safe_load; the inconsistency silently misparses quoted values and is a maintenance trap.
Architecture
classDiagram
direction LR
class MCPClientAdapter {
<<ABC>>
+get_config_path() str
+update_config(updates) None
+supports_user_scope bool
}
class CopilotClientAdapter {
<<ConcreteAdapter>>
+_client_label str
+get_config_path() str
+update_config(updates) None
}
class WindsurfClientAdapter {
<<ConcreteAdapter>>
+_client_label str
+supports_user_scope bool
+get_config_path() str
}
class BaseIntegrator {
<<BaseClass>>
+check_collision() bool
+resolve_links() tuple
+ensure_path_within() Path
}
class InstructionIntegrator {
<<ConcreteIntegrator>>
+integrate_instructions_for_target(target, ...) IntegrationResult
+copy_instruction_windsurf(src, tgt) int
+_convert_to_windsurf_rules(content) str
}
class AgentIntegrator {
<<ConcreteIntegrator>>
+integrate_agents_for_target(target, ...) IntegrationResult
+_write_windsurf_agent_skill(src, tgt) int
}
class HookIntegrator {
<<ConcreteIntegrator>>
+integrate_hooks_for_target(target, ...) IntegrationResult
}
class TargetProfile {
<<ValueObject>>
+name str
+root_dir str
+primitives dict
+user_supported str
}
MCPClientAdapter <|-- CopilotClientAdapter : extends
CopilotClientAdapter <|-- WindsurfClientAdapter : extends
BaseIntegrator <|-- InstructionIntegrator : extends
BaseIntegrator <|-- AgentIntegrator : extends
BaseIntegrator <|-- HookIntegrator : extends
TargetProfile *-- PrimitiveMapping : contains
InstructionIntegrator ..> TargetProfile : reads format_id
AgentIntegrator ..> TargetProfile : reads format_id
HookIntegrator ..> TargetProfile : reads format_id
note for WindsurfClientAdapter "Template Method: inherits mcpServers JSON logic, overrides get_config_path()"
note for InstructionIntegrator "Strategy dispatch on format_id: cursor_rules / claude_rules / windsurf_rules"
class WindsurfClientAdapter:::touched
class InstructionIntegrator:::touched
class AgentIntegrator:::touched
class HookIntegrator:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm install --target windsurf"]) --> B["dispatch.py\nintegrate_for_target(windsurf)"]
B --> C["InstructionIntegrator\nintegrate_instructions_for_target()"]
B --> D["AgentIntegrator\nintegrate_agents_for_target()"]
B --> E["HookIntegrator\nintegrate_hooks_for_target()"]
B --> F["MCPIntegrator\nWindsurfClientAdapter.update_config()"]
C --> C1["fmt == windsurf_rules?"]
C1 -- yes --> C2["ensure_path_within(target_path, deploy_dir)"]
C2 --> C3["_convert_to_windsurf_rules(content)"]
C3 --> C4["applyTo present?"]
C4 -- yes --> C5["trigger: glob + globs"]
C4 -- no --> C6["trigger: always_on"]
C5 --> C7[".windsurf/rules/name.md"]
C6 --> C7
D --> D1["_write_windsurf_agent_skill(src, tgt)"]
D1 --> D2["yaml.safe_load frontmatter\nstrip model/tools keys"]
D2 --> D3[".windsurf/skills/name/SKILL.md"]
E --> E1["windsurf branch: base_root=.windsurf"]
E1 --> E2[".windsurf/hooks/pkg/ [NO path guard]"]
F --> F1["get_config_path()\n~/.codeium/windsurf/mcp_config.json"]
F1 --> F2["merge mcpServers\nwrite mcp_config.json"]
Recommendation
The integration is structurally sound and the first-class Windsurf target is a genuine APM milestone worth shipping. The two doc-writer blocking findings (ide-tool-integration.md Windsurf section + auto_create=False explanation) should be resolved in-PR or in an immediate follow-on before the release is announced -- users hitting a silent no-op with no docs is a trust-eroding first impression. The hook path guard (supply-chain-security) and missing tests (test-coverage-expert) should be tracked as follow-up issues and closed before the next minor release. The growth story -- README callout, CHANGELOG first-mover framing, commands.md skill update -- is low-effort and high-leverage; fold it into the same doc pass.
Full per-persona findings
Python Architect
-
[recommended]
_convert_to_windsurf_rulesparsesapplyTo:with a line-scan instead ofyaml.safe_loadatsrc/apm_cli/integration/instruction_integrator.py:357
Every other frontmatter parser in this codebase usesyaml.safe_load. The hand-rolled loop silently misparses quoted values, multi-line block scalars, and YAML anchors. The inconsistency creates a maintenance trap: ifapplyTosemantics expand, one path is updated and the other is forgotten.
Suggested: Replace the for-loop withyaml.safe_loadon thefm_block, then readapply_to = fm.get('applyTo', ''), mirroring the pattern in_write_windsurf_agent_skill. -
[nit]
_write_windsurf_agent_skillshould be@staticmethodlike_write_codex_agentor have a comment explaining why it needsselfatsrc/apm_cli/integration/agent_integrator.py:280
The sibling_write_codex_agentis@staticmethod. Add a brief comment:# instance method: requires self.resolve_links(). -
[nit] Lazy import of
ensure_path_withininside method body is inconsistent with module-level import style atsrc/apm_cli/integration/instruction_integrator.py:106
Same PR movesimport yamlto module level. Movefrom apm_cli.utils.path_security import ensure_path_withinto module-level import block too.
CLI Logging Expert
-
[recommended] Success/error messages in inherited
configure()path use rawprint()instead of_rich_success/_rich_erroratsrc/apm_cli/adapters/client/copilot.py
PR expands the surface of a pre-existing anti-pattern. Adding a Windsurf adapter that inherits rawprint()calls makes both targets inconsistent with the rest of the codebase's rich output.
Suggested: Replaceprint(f"Successfully configured...")with_rich_success(...)inCopilotClientAdapter.configure(). Windsurf inherits the fix for free. -
[nit] Stale-removal log hardcodes
'Windsurf config'rather than using_client_labelatsrc/apm_cli/integration/mcp_integrator.py
Low priority; tracked for when the stale-removal block is refactored to drive strings from the adapter.
DevX UX Expert
-
[recommended] Agent->skill conversion silently drops
toolsandmodelfrontmatter with no user-visible warning atsrc/apm_cli/integration/agent_integrator.py
Violates the 'install adds, never silently mutates' mental model. Users debugging why Windsurf skill tool boundaries don't work have no signal from APM.
Suggested: In_write_windsurf_agent_skill, detect whethertoolsormodelkeys were present and emit a per-file note in the install summary. -
[recommended]
packages/apm-guide/.apm/skills/apm-usage/commands.mdnot updated -- windsurf missing fromapm runtime setuptarget list atpackages/apm-guide/.apm/skills/apm-usage/commands.md
AI assistants using this skill to answer Windsurf setup questions will give incomplete guidance. -
[nit]
get_target_description('windsurf')omits.windsurf/hooks.jsonfrom dry-run output atsrc/apm_cli/core/target_detection.py
Add+ .windsurf/hooks.jsonto the description string to match other targets with hooks. -
[nit]
compilation.mdauto-detection table labelAGENTS.md (instructions only)is misleading for Windsurf atdocs/src/content/docs/guides/compilation.md
Change toAGENTS.md (optional roll-up; install deploys to .windsurf/rules/)or simplyAGENTS.md.
Supply Chain Security Expert
-
[recommended] Hook script copy destinations (
project_root / target_rel) are not passed throughensure_path_withinbeforeshutil.copy2atsrc/apm_cli/integration/hook_integrator.py
Verified:target_scriptat lines 599 and 779 ofhook_integrator.pyis never checked viaensure_path_within. For Windsurf user-scope,base_rootset to.codeium/windsurfwould maketarget_scriptresolve project-relative instead of user-home-relative -- a functional mismatch and missing path safety net.
Suggested: Wrap eachshutil.copy2site withensure_path_within(target_script, project_root). -
[nit]
ensure_path_withinchange fromproject_roottodeploy_diris correct but warrants an inline proof comment atsrc/apm_cli/integration/instruction_integrator.py
Add:# target_name is Path.name (no separators), so traversal via deploy_dir is impossible. -
[nit] MCP
envblock written verbatim to global~/.codeium/windsurf/mcp_config.json-- inherited risk, no new exposure atsrc/apm_cli/adapters/client/windsurf.py
Pre-existing shared behavior across all adapters. Tracked for future allowlist consideration.
OSS Growth Hacker
-
[recommended] README has zero runnable Windsurf example -- Windsurf users have no 'aha' moment at
README.md
Copilot has a zero-config callout with a single command. Windsurf appears only as a name in the IDE list. Top-of-funnel conversion miss.
Suggested: Addapm install # deploys to .windsurf/rules/, .windsurf/skills/, .windsurf/workflows/ automaticallyafter the Copilot example. -
[recommended]
compilation.mdauto-detection table saysAGENTS.md (instructions only)-- actively undersells the full native deploy atdocs/src/content/docs/guides/compilation.md
A Windsurf user reading this table may conclude APM support is thin and bounce. -
[nit] CHANGELOG entry is written for maintainers, not users -- the hook is absent at
CHANGELOG.md
Prepend user hook: 'Windsurf/Cascade users can nowapm installand get a fully configured IDE automatically.' -
[nit] First-mover claim is never stated in any shipped doc despite a narrow window
Add(the first package manager with native Windsurf support)to the CHANGELOG entry or README callout.
Auth Expert
-
[nit] GitHub token injection inherited by
WindsurfClientAdapter-- same threat model as Copilot, no new exposure atsrc/apm_cli/adapters/client/windsurf.py
Fires only for GitHub MCP servers (URL/name heuristic). File permissions are OS-level user-owned. No new credential exposure surface. -
[nit] Path boundary relaxation from
project_roottodeploy_diris correct -- cannot escape to~/.sshor~/.gitconfigatsrc/apm_cli/integration/instruction_integrator.py:118
deploy_diris target_root/mapping.subdir; cannot traverse above the deploy target. Semantics are correct.
Doc Writer
-
[blocking] No Windsurf section in
ide-tool-integration.md-- every other first-class target has one atdocs/src/content/docs/integrations/ide-tool-integration.md
Add a### Windsurfsubsection mirroring the Cursor section: primitive-mapping table,auto_create=Falsesetup note, agent->skill conversion note, and partial user-scope caveat. -
[blocking]
auto_create=Falseundocumented -- users will runapm installand see no Windsurf output with no explanation atdocs/src/content/docs/integrations/ide-tool-integration.md
Add: 'Create a.windsurf/directory in your project root (or open the project in Windsurf once), then runapm install.' -
[recommended] Agent->skill conversion (lossy:
tools/modeldropped) not documented atdocs/src/content/docs/integrations/ide-tool-integration.md
Add a note row in the Windsurf primitive-mapping table: 'Agents deploy as Windsurf Skills;toolsandmodelfrontmatter are not preserved.' -
[recommended] Partial user-scope limitation (instructions excluded) not disclosed in any doc at
docs/src/content/docs/integrations/ide-tool-integration.md
Note: 'User-scope deployment is partial -- instructions are not deployed globally (Windsurf global rules use a single-file format incompatible with APM).' -
[recommended]
compilation.mdauto-detection table misleading for Windsurf (AGENTS.md (instructions only)) atdocs/src/content/docs/guides/compilation.md
Clarify or add a footnote that the table coversapm compileoutput, notapm install. -
[recommended] CHANGELOG Windsurf entry omits that user-scope is partial at
CHANGELOG.md
Amend: 'user-scope deployment (partial: instructions excluded at user scope -- global rules use a different Windsurf format).' -
[recommended]
compilation.mdintro says 'all primitives natively' -- masks agent->skill conversion atdocs/src/content/docs/guides/compilation.md
Qualify: 'agents as Windsurf Skills to.windsurf/skills/' (or link to integration guide note).
Test Coverage Expert
-
[recommended]
WindsurfClientAdapter.get_config_path()has no unit test atsrc/apm_cli/adapters/client/windsurf.py
Proof (missing at):tests/unit/test_windsurf_adapter.py-- proves:apm mcp add --target windsurfwrites to the correct global config file [multi-harness-support, devx] -
[recommended]
hook_integrator.pywindsurf routing branch has no test atsrc/apm_cli/integration/hook_integrator.py
Proof (missing at):tests/unit/integration/test_hook_integrator.py-- proves:apm installdeploys hook scripts to.windsurf/hooks/<pkg>/when windsurf is the target [multi-harness-support, devx]
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #1066
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - Feature/windsurf target #1066
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by PR Review Panel for issue #1066 · ● 7M · ◷
b863385 to
01b3245
Compare
01b3245 to
f7cba3f
Compare
Per devx-ux panel review on PR microsoft#1066: _write_windsurf_agent_skill silently strips 'tools' and 'model' fields when converting an .agent.md to a Windsurf SKILL.md, because Windsurf Skills do not support agent-only metadata. Silent loss is a footgun. Thread an optional diagnostics parameter through the method and emit a DiagnosticCollector.warn() entry per source file when either field is present. Message names the dropped field(s) and the source filename so the install summary surfaces the loss after all packages process. The DiagnosticCollector is the canonical collect-then-render pattern in this repo (utils/diagnostics.py); the renderer prepends [!] via _rich_warning, so we do not echo inline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per supply-chain panel review on PR microsoft#1066: the two shutil.copy2 sites in hook_integrator.py constructed target_script from package-controlled JSON (the rewritten hook command), without asserting the resolved path stays inside project_root. A malicious package could craft a target_rel that escapes via .. traversal, and mkdir(parents=True) would happily materialize attacker-controlled directories outside the workspace before the copy ran. Wrap both copy loops with ensure_path_within(target_script, project_root) BEFORE mkdir, matching the canonical pattern in instruction_integrator and skill_integrator. PathTraversalError propagates so the install aborts on the first malicious entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per doc-writer panel review on PR microsoft#1066: ide-tool-integration.md had zero coverage of the Windsurf target. Add a parallel section after Claude Integration covering: - Opt-in auto-detection (auto_create=False; .windsurf/ must exist) - Native Windsurf primitives table (rules / skills / workflows / hooks) - Lossy agent-to-skill conversion (tools and model dropped, with the runtime warning emitted by agent_integrator) - MCP user-scope config at ~/.codeium/windsurf/mcp_config.json - User-scope install limitations (instructions stay workspace-local per unsupported_user_primitives) Every behavior claim cross-checked against src/apm_cli/integration/targets.py, agent_integrator.py, and adapters/client/windsurf.py. ASCII-only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per test-coverage panel review on PR microsoft#1066: zero windsurf-specific unit tests. Add 9 new test functions across 3 files. tests/unit/test_windsurf_adapter.py (new): - get_config_path() resolves to ~/.codeium/windsurf/mcp_config.json - supports_user_scope is True; client label is Windsurf tests/unit/integration/test_hook_integrator.py: - Windsurf branch of _rewrite_hook_command rewrites CLAUDE_PLUGIN_ROOT references to .windsurf/hooks/<pkg>/... - Relative path rewriting and bare system commands - PathTraversalError raised when target_rel escapes project_root (regression test for the new ensure_path_within guard) tests/unit/integration/test_agent_integrator.py: - _write_windsurf_agent_skill records a DiagnosticCollector warning when frontmatter has tools or model - No warning recorded when neither field is present Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Panel follow-ups addressedPushed 4 commits (
Verification
Implementation notes
Authored via the apm-review-panel specialist agents ( |
…tration Adds 3 fields to TargetProfile (pack_prefixes, compile_family, hooks_config_display) and 1 attribute to MCPClientAdapter (mcp_servers_key) so adding a new target becomes a single registry edit instead of touching 4 module-local dicts/if-elif chains. Adds tests/unit/integration/test_targets_registry_completeness.py (52 parametrized tests) which turns "forgot to wire up new target" from a silent runtime bug into a CI failure. Zero behavior change in this commit: consumers are migrated in follow-ups (conflict_detector, lockfile_enrichment, compile/cli, install/services). Also reformats two pre-existing files (target_detection.py, mcp_integrator.py) so the lint contract is silent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… silent breaks
Replaces classname-substring sniffing in get_existing_server_configs
with a direct lookup of self.adapter.mcp_servers_key. This repairs
silent-no-conflict behavior for cursor, gemini, opencode, and windsurf
adapters (whose class names contained none of the hard-coded
substrings copilot/codex/vscode/claude, so the dispatch fell through
and returned {} -- conflict detection was a silent no-op).
The codex TOML flat-key fallback (mcp_servers."name") is preserved
under the same branch.
Updates 3 existing tests that patched __class__.__name__ to instead
set adapter.target_name and adapter.mcp_servers_key. Adds 7 new
regression tests covering windsurf/cursor/gemini/opencode/vscode
parity plus codex flat-key merging and the empty-key defensive case.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…+gemini drops Replaces the static _TARGET_PREFIXES dict in bundle/lockfile_enrichment with a _get_target_prefixes() helper that reads TargetProfile.effective_pack_prefixes off the registry, plus an _all_target_prefixes() helper that unions every deployable target. Behavior fixes (incidental but desired): - apm pack --target windsurf: .windsurf/ files were silently dropped (windsurf was missing from _TARGET_PREFIXES); now included. - apm pack --target all: .windsurf/ AND .gemini/ files were silently dropped (both missing from the hard-coded "all" list); now included via deterministic union of detect_by_dir/auto_create profiles. Adds windsurf entry to _CROSS_TARGET_MAPS so .github/skills/ remap to .windsurf/skills/ and .github/agents/ collapse onto .windsurf/skills/ (windsurf has no native agent surface). Adds 6 regression tests pinning windsurf/all/cross-map behavior plus a "every legacy target still filters correctly" guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ompile_family Replaces hard-coded copilot_family / agents_md_family sets in _resolve_compile_target with a registry-driven derivation that reads TargetProfile.compile_family from KNOWN_TARGETS. Adding a new agents-family target (e.g. windsurf, zed, cline) becomes a single field on the registry entry instead of touching this function. Behavior fixes: - -t windsurf and -t [windsurf]: previously fell through to the hard-coded 'cursor/opencode/codex' bare-fallback list; now routes through compile_family='agents' the same way cursor/codex/opencode do. - -t [windsurf, claude] / -t [windsurf, gemini] / -t [windsurf, copilot]: previously collapsed to wrong family; now produces the expected frozenset / 'vscode' tokens. Bare-target priority is preserved by iterating KNOWN_TARGETS in insertion order, so -t [opencode, codex] still resolves to 'opencode'. Adds a windsurf-specific test plus extends the existing bare-target test to cover windsurf. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces the if/elif chain on _target.name with a single profile-driven lookup. Fixes the silent log-message regression where windsurf hook installs printed the deploy path as ".windsurf/" instead of ".windsurf/hooks.json". Adding any future target with hooks costs zero edits to this code path -- just populate hooks_config_display on the TargetProfile entry. The existing registry-completeness guard already enforces that hooks_config_display lives under the target's root_dir, so any typo lands at test time rather than runtime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Locally-run apm-review-panel (python-architect, cli-logging-expert, test-coverage-expert) flagged four residual gaps after the structural refactor. Address them in one cleanup pass: 1. Gemini hooks_config_display Gemini's TargetProfile omitted hooks_config_display, so install logs would print ".gemini/hooks/" instead of ".gemini/settings.json" (where Gemini hooks actually merge). Same class of silent UX regression the PR already fixed for windsurf. Add the field; the existing registry-completeness guard (test_hooks_display_matches_root) now covers it. 2. mcp_integrator runtime lists mcp_integrator.py:461 (cleanup loop) and :850 (availability fallback) each maintained a hard-coded list of MCP-capable runtimes that had to be updated per-target. Extract _MCP_CLIENT_REGISTRY to a module-level constant in factory.py and expose ClientFactory.supported_clients(); both call sites now derive from it. Adding a new MCP target is one edit (the registry dict) instead of three. 3. CLI --target help string drift pack.py:68, compile/cli.py:267, install.py:841 all enumerated valid target values without "windsurf". Users discovering the flag via --help would not know windsurf is valid. Adds windsurf to all three. 4. Structural guard for --target all The existing canary (test_target_all_includes_windsurf_files) pins today's behavior but a future deployable target could still be silently dropped. Add test_target_all_includes_every_deployable_ target_prefix in TestWindsurfTargetParity that iterates KNOWN_TARGETS and asserts every detect_by_dir/auto_create profile's prefixes appear in _all_target_prefixes(). Also add test_client_factory_ supported_clients_matches_adapter_set in the registry-completeness guard so the factory dict cannot drift from the adapter subclass set. Plus a stale-docstring fix in lockfile_enrichment.py:92 (referenced the removed _TARGET_PREFIXES constant). Validated: ruff lint+format silent, 7322 unit tests pass (+2 new structural guards), integration sanity green (test_pack_unified, test_pack_unpack_e2e, test_multi_runtime). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Structural refactor + advisory panel pass (6 commits)Per the python-architect critique upthread, the windsurf parity gaps were symptoms of the same root cause: per-target metadata scattered across module-local dicts and Commits
Behavior fixes (silent regressions discovered + repaired)
Structural guards (new)
Advisory panel results (run locally)
Validation
Adding a new target (e.g. |
Architecture diagrams (python-architect)Per the apm-review-panel python-architect output, here are the structural diagrams for the post-refactor topology. Class diagram -- single source of truth + adapter strategyclassDiagram
direction LR
class TargetProfile {
<<Registry ValueObject>>
+name str
+root_dir str
+pack_prefixes tuple
+compile_family str or None
+hooks_config_display str or None
+effective_pack_prefixes tuple
}
class MCPClientAdapter {
<<Strategy>>
+target_name str
+mcp_servers_key str
+get_config_path()*
+get_current_config()*
}
class CopilotClientAdapter {
<<ConcreteStrategy>>
+target_name = "copilot"
+mcp_servers_key = "mcpServers"
}
class WindsurfClientAdapter {
<<ConcreteStrategy>>
+target_name = "windsurf"
+mcp_servers_key = "mcpServers"
}
class VSCodeClientAdapter {
<<ConcreteStrategy>>
+target_name = "vscode"
+mcp_servers_key = "servers"
}
class CodexClientAdapter {
<<ConcreteStrategy>>
+target_name = "codex"
+mcp_servers_key = "mcp_servers"
}
class MCPConflictDetector {
<<Consumer>>
+get_existing_server_configs() dict
}
class KNOWN_TARGETS {
<<Module-level Registry>>
+dict~str, TargetProfile~
}
MCPClientAdapter <|-- CopilotClientAdapter
CopilotClientAdapter <|-- WindsurfClientAdapter
MCPClientAdapter <|-- VSCodeClientAdapter
MCPClientAdapter <|-- CodexClientAdapter
MCPConflictDetector o-- MCPClientAdapter : reads mcp_servers_key
MCPConflictDetector ..> KNOWN_TARGETS : joins via target_name
KNOWN_TARGETS *-- TargetProfile : contains
note for KNOWN_TARGETS "Single source of truth for per-target metadata: pack_prefixes, compile_family, hooks_config_display"
note for MCPClientAdapter "mcp_servers_key lives here (not on TargetProfile) because it describes config schema owned by the adapter, and applies to MCP-only targets (vscode) with no profile"
class TargetProfile:::touched
class MCPClientAdapter:::touched
class CopilotClientAdapter:::touched
class WindsurfClientAdapter:::touched
class VSCodeClientAdapter:::touched
class CodexClientAdapter:::touched
class MCPConflictDetector:::touched
classDef touched fill:#fff3b0,stroke:#d47600
Component diagram -- consumers reading from the registryflowchart TD
subgraph CLI_Entry["CLI entry points"]
PACK["apm pack --target T"]
COMPILE["apm compile --target T"]
INSTALL["apm install"]
end
subgraph Registry["targets.py -- single source of truth"]
KT["KNOWN_TARGETS dict<br/>9 TargetProfile entries"]
end
subgraph Pack["bundle/lockfile_enrichment.py"]
GTP["_get_target_prefixes(target)"]
ATP["_all_target_prefixes()"]
FFT["_filter_files_by_target()"]
end
subgraph Compile["commands/compile/cli.py"]
RCT["_resolve_compile_target(target)<br/>reads profile.compile_family"]
end
subgraph Conflict["core/conflict_detector.py"]
GESC["get_existing_server_configs()<br/>reads adapter.mcp_servers_key"]
end
subgraph Install["install/services.py"]
IPP["integrate_package_primitives()<br/>reads profile.hooks_config_display"]
end
subgraph Adapters["adapters/client/*.py"]
ADP["MCPClientAdapter subclasses<br/>target_name + mcp_servers_key"]
end
PACK --> GTP
GTP -->|"reads effective_pack_prefixes"| KT
GTP --> ATP
ATP -->|"iterates deployable profiles"| KT
GTP --> FFT
COMPILE --> RCT
RCT -->|"reads compile_family"| KT
INSTALL --> IPP
IPP -->|"reads hooks_config_display"| KT
INSTALL --> GESC
GESC -->|"reads mcp_servers_key"| ADP
ADP -->|"target_name joins to"| KT
style KT fill:#fff3b0,stroke:#d47600
style ADP fill:#fff3b0,stroke:#d47600
style GTP fill:#fff3b0,stroke:#d47600
style RCT fill:#fff3b0,stroke:#d47600
style GESC fill:#fff3b0,stroke:#d47600
style IPP fill:#fff3b0,stroke:#d47600
Sequence diagram -- adding a new target ("zed") in the post-refactor worldIllustrates the cost of onboarding a new agent harness now that metadata is centralized. Three edits + automatic structural-guard validation -- no scattered if/elif chains to discover. sequenceDiagram
autonumber
actor Dev as Contributor
participant Reg as integration/targets.py<br/>(KNOWN_TARGETS)
participant Adp as adapters/client/zed.py<br/>(ZedClientAdapter)
participant Fac as factory.py<br/>(_MCP_CLIENT_REGISTRY)
participant Guard as test_targets_registry_completeness.py
participant Pack as lockfile_enrichment._all_target_prefixes()
participant Compile as compile/cli._resolve_compile_target()
participant Install as install/services.integrate_package_primitives()
participant Conflict as conflict_detector.get_existing_server_configs()
Dev->>Reg: add TargetProfile("zed", root_dir=".zed",<br/>pack_prefixes=(".zed/",), compile_family="agents",<br/>hooks_config_display=".zed/hooks.json")
Dev->>Adp: subclass MCPClientAdapter,<br/>target_name="zed", mcp_servers_key="mcpServers"
Dev->>Fac: register "zed" -> ZedClientAdapter
Note over Pack,Conflict: All consumers below pick up the new target<br/>without code changes -- they read from the registry.
Pack-->>Reg: iterate KNOWN_TARGETS where<br/>detect_by_dir or auto_create
Pack-->>Pack: --target all now includes ".zed/"
Compile-->>Reg: read profile.compile_family for zed
Compile-->>Compile: --target windsurf,zed routes to "agents" family
Install-->>Reg: read profile.hooks_config_display
Install-->>Install: log path = ".zed/hooks.json"
Conflict-->>Adp: read adapter.mcp_servers_key
Conflict-->>Conflict: extract servers for zed correctly
Dev->>Guard: pytest tests/unit/integration/test_targets_registry_completeness.py
alt Field missing on ZedClientAdapter or TargetProfile
Guard-->>Dev: FAIL: <persona> drift detected, fix at registration time
else All registry fields populated
Guard-->>Dev: 53 passed -- new target wired up correctly
end
|
The frontmatter serializer uses yaml.safe_dump to produce a string (.rstrip on the return value), not to write to a file handle. The CI yaml-io guard regex matches any kwarg after the first arg and flagged this as a false positive. Wrap the call across lines and add the # yaml-io-exempt marker to make the intent explicit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Add Windsurf/Cascade as a first-class APM compilation and integration target.
Windsurf is a VS Code-based AI IDE with its own configuration layout (
.windsurf/). This PR adds full support so thatapm install --target windsurfandapm compile --target windsurfdeploy primitives into the correct locations.Primitive mapping:
.windsurf/rules/*.mdapplyTo→trigger: globfrontmatter conversion.windsurf/skills/<name>/SKILL.md.windsurf/skills/<name>/SKILL.md.windsurf/workflows/*.md.windsurf/hooks.jsonAGENTS.md(root)Why agents → skills? Windsurf has no agent primitive. Skills are the closest match: invocable with
@skill-name, auto-activated by Cascade based on description, and support supplementary resource files. Tool boundaries (tools:) and model selection (model:) are not preserved as Windsurf does not support them at the skill level.Type of change
Testing
New tests added:
TestWindsurfAgentSkillConversion— 5 unit tests for the_write_windsurf_agent_skilltransformerTestWindsurfAgentSkillIntegration— 4 integration tests for end-to-end deploymentNote: 2 pre-existing test failures on Windows (TestClaudeScopeResolution::test_user_scope_outside_home_keeps_absolute and test_user_scope_collapses_dotdot_segments) are unrelated to this PR — they fail on upstream
maindue toPath.home()not respecting monkeypatchedHOMEon Windows.