fix: gate .claude/commands/ deployment behind integrate_claude flag#443
fix: gate .claude/commands/ deployment behind integrate_claude flag#443neop26 wants to merge 3 commits intomicrosoft:mainfrom
Conversation
The CommandIntegrator was called unconditionally in _integrate_package_primitives(), ignoring the integrate_claude flag derived from the detected or configured target. This caused .claude/commands/ to be created on every apm install, even when the project is configured with target: copilot (vscode), which sets integrate_claude=False. The docstring of should_integrate_claude() explicitly states that 'commands' should be governed by this flag, matching the pattern already applied to Claude agents and hooks. The OpenCode commands integrator has a similar self-guard (.opencode/ must exist). Fix: wrap integrate_package_commands() in if integrate_claude: so it is consistent with all other Claude-specific integration paths. Reproducer: set target: copilot in apm.yml, add .prompt.md files under .apm/prompts/, run apm install — .claude/commands/ is created despite no Claude target being configured.
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR fixes an install-time integration bug where Claude command deployment (.claude/commands/) ran unconditionally during apm install, ignoring the integrate_claude flag derived from the detected/configured target (e.g., target: copilot / vscode).
Changes:
- Gate
.claude/commands/deployment by wrappingintegrate_package_commands()inif integrate_claude:. - Align Claude command integration behavior with other Claude-specific integrations (agents/hooks) that already respect
integrate_claude.
| _log_integration(f" └─ {command_result.files_integrated} commands integrated -> .claude/commands/") | ||
| if command_result.files_updated > 0: | ||
| _log_integration(f" └─ {command_result.files_updated} commands updated") |
There was a problem hiding this comment.
These log strings include Unicode box-drawing characters ("└" and "─"), which violates the repo's ASCII-only encoding rule for source/CLI output (see .github/instructions/encoding.instructions.md). Please switch this message prefix to plain ASCII (for example, a simple "-" or the bracket status symbols) so Windows cp1252 terminals do not crash with UnicodeEncodeError.
There was a problem hiding this comment.
These log strings match the existing pattern used throughout _integrate_package_primitives() — all ~15 existing _log_integration calls on lines 849, 864, 879, etc. use the same └─ character. Changing only the new lines would create an inconsistency. If there's a repo-wide ASCII encoding requirement, I'm happy to raise a separate PR to update all occurrences consistently.
| if integrate_claude: | ||
| command_result = command_integrator.integrate_package_commands( | ||
| package_info, project_root, | ||
| force=force, managed_files=managed_files, | ||
| diagnostics=diagnostics, |
There was a problem hiding this comment.
Please add/adjust a unit test to cover this regression: when the detected/configured target yields integrate_claude=False (e.g., target=vscode/copilot), apm install should not call integrate_package_commands and should not create/deploy anything under .claude/commands/. This will prevent future accidental reintroduction of unconditional Claude command integration.
There was a problem hiding this comment.
Done — added TestIntegratePackagePrimitivesTargetGating in test_command_integrator.py with two tests: one verifying integrate_package_commands is not called when integrate_claude=False (target=copilot/vscode), and one verifying it is called when integrate_claude=True.
…mands/ Covers the fix in _integrate_package_primitives(): - When integrate_claude=False (target=copilot/vscode), integrate_package_commands must not be called and .claude/commands/ must not be created. - When integrate_claude=True (target=claude/all), integrate_package_commands must be called.
The CommandIntegrator was called unconditionally in _integrate_package_primitives(), ignoring the integrate_claude flag derived from the detected or configured target.
This caused .claude/commands/ to be created on every apm install, even when the project is configured with target: copilot (vscode), which sets integrate_claude=False.
The docstring of should_integrate_claude() explicitly states that 'commands' should be governed by this flag, matching the pattern already applied to Claude agents and hooks. The OpenCode commands integrator has a similar self-guard (.opencode/ must exist).
Fix: wrap integrate_package_commands() in if integrate_claude: so it is consistent with all other Claude-specific integration paths.
Reproducer: set target: copilot in apm.yml, add .prompt.md files under .apm/prompts/, run apm install — .claude/commands/ is created despite no Claude target being configured.
Description
Brief description of changes and motivation.
Fixes # (issue)
Type of change
Testing