Fix #1019: Improve compile target specification#1020
Fix #1019: Improve compile target specification#1020danielmeppiel merged 4 commits intomicrosoft:mainfrom
Conversation
tillig
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
APM Review Panel VerdictDisposition: APPROVE (one pre-merge doc-sync check; two optional follow-ups) Per-persona findingsPython Architect: Bug is clear: multi-target lists (e.g. classDiagram
direction LR
class CompilationConfig {
<<Dataclass>>
+target: CompileTargetType
+dry_run: bool
+single_agents: bool
+from_apm_yml() CompilationConfig
}
class AgentsCompiler {
+compile(config, primitives) CompilationResult
}
class CompileTargetType {
<<TypeAlias>>
Union~str, frozenset~
}
class TargetDetection {
<<Module>>
+detect_target(explicit_target, config_target) TargetType
+should_compile_agents_md(target) bool
+should_compile_claude_md(target) bool
+should_compile_gemini_md(target) bool
}
class ResolveCompileTarget {
<<Pure>>
+_resolve_compile_target(target) CompileTargetType
}
class CompilationResult {
<<ValueObject>>
+success bool
+output_path str
}
AgentsCompiler ..> CompilationConfig : reads
AgentsCompiler ..> TargetDetection : delegates routing
ResolveCompileTarget ..> CompileTargetType : returns
CompilationConfig *-- CompileTargetType : typed field
AgentsCompiler ..> CompilationResult : returns
class CompilationConfig:::touched
class TargetDetection:::touched
class ResolveCompileTarget:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm compile --target claude,codex"] --> B["_resolve_compile_target\ncli.py:_resolve_compile_target()"]
B --> C{families >= 2?}
C -- "pre-fix: returns 'all'" --> D["detect_target(explicit='all')"]
D --> E["[FS] GEMINI.md written -- WRONG"]
C -- "post-fix: returns frozenset" --> F["frozenset({'agents','claude'})"]
F --> G{isinstance frozenset?\ncli.py:408}
G -- yes --> H["bypass detect_target()\ndetection_reason='explicit --target flag'"]
H --> I["should_compile_agents_md(frozenset) --> True\ntarget_detection.py:143"]
H --> J["should_compile_claude_md(frozenset) --> True\ntarget_detection.py:158"]
H --> K["should_compile_gemini_md(frozenset) --> False\ntarget_detection.py:173"]
I --> L["[FS] AGENTS.md written"]
J --> M["[FS] CLAUDE.md written"]
K --> N["GEMINI.md NOT written -- correct"]
G -- no (string) --> O["detect_target() string path\nunchanged from before"]
Design patterns
Non-blocking observations:
CLI Logging Expert: Output paths are clean. All progress messages route through DevX UX Expert: Clear UX improvement. Before: Supply Chain Security Expert: CLEAN. No new path construction, no lockfile changes, no token or auth touch points. The frozenset is always constructed by Auth Expert: Not activated -- PR modifies OSS Growth Hacker: This is a correctness prerequisite for the "one repo, multiple AI assistants" adoption story. Teams using APM across Claude + GitHub Copilot workflows (the most common multi-assistant setup) will immediately benefit: CEO arbitrationAll specialists agree -- no disagreements to arbitrate. This is a correct, well-scoped bug fix with a proper regression test class ( Required actions before merge
Optional follow-ups
|
…t, frozenset family validation - Parameterize CompileTargetType as Union[str, frozenset[str]] for precision - Extract REASON_NO_TARGET_FOLDER constant in target_detection; replace brittle substring match in compile CLI with equality comparison. User-facing log message is unchanged. - Add explicit family validation in AgentsCompiler when config.target is a frozenset. Previously the frozenset branch silently bypassed _KNOWN_TARGETS validation; an invalid family (e.g. via direct API use) could silently produce partial output. Now fails explicitly with a clear error listing the bad value and accepted families. - Add regression test test_unknown_frozenset_target_family_returns_failure. Docs sync: verified docs/src/content/docs/reference/cli-commands.md and packages/apm-guide/.apm/skills/apm-usage/commands.md already document the correct (post-fix) multi-target behavior; no edits needed. Refs microsoft#1020 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed one follow-up commit (fb314d3) onto this branch addressing the three optional nits from the Review Panel, @tillig — feel free to revert/squash/amend as you see fit:
Notably declined the Doc sync (the required action): verified as a no-op — Tests: |
There was a problem hiding this comment.
Pull request overview
Fixes #1019 by changing how multi-target apm compile --target lists are normalized so cross-family combinations no longer collapse to "all" (which caused unrequested artifacts like GEMINI.md to be generated).
Changes:
- Update compile target resolution to return a
frozensetof requested compiler families for multi-target lists (instead of"all"). - Extend target detection helpers and compiler routing to understand family
frozensettargets. - Add/adjust unit tests and update the changelog entry for the regression.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/commands/compile/cli.py |
Introduces frozenset-based multi-target resolution and adjusts compile logging/target selection flow. |
src/apm_cli/core/target_detection.py |
Adds CompileTargetType, a no-target-folder reason constant, and updates should_compile_* helpers for frozenset targets. |
src/apm_cli/compilation/agents_compiler.py |
Allows CompilationConfig.target to be a frozenset and validates allowed families defensively. |
tests/unit/compilation/test_compile_target_flag.py |
Updates target resolution expectations and adds regression coverage ensuring unrequested families do not compile. |
CHANGELOG.md |
Adds an Unreleased Fixed entry for #1019. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 3
| # Compile target: either a single TargetType string or a frozenset of compiler | ||
| # families ({"agents", "claude", "gemini"}) for multi-target lists. | ||
| CompileTargetType = Union[str, frozenset[str]] | ||
|
|
There was a problem hiding this comment.
CompileTargetType = Union[str, frozenset[str]] is too permissive (any string/frozenset member type). This weakens type checking and makes it easier for invalid targets/families to flow into should_compile_* and callers. Consider tightening this to Union[TargetType, frozenset[Literal['agents','claude','gemini']]] (or define a CompileFamily Literal and use TypeAlias) so mypy/pyright can catch mistakes early.
See below for a potential fix:
from typing import List, Literal, Optional, Tuple, TypeAlias, Union
import click
# Valid target values (internal canonical form)
TargetType = Literal["vscode", "claude", "cursor", "opencode", "codex", "gemini", "all", "minimal"]
# Valid compiler families used when a multi-target list is reduced to the
# corresponding compilation outputs.
CompileFamily = Literal["agents", "claude", "gemini"]
# Compile target: either a single TargetType string or a frozenset of compiler
# families ({"agents", "claude", "gemini"}) for multi-target lists.
CompileTargetType: TypeAlias = Union[TargetType, frozenset[CompileFamily]]
| if isinstance(target, list): | ||
| # Multi-target list: show what the compiler will produce | ||
| _target_label = ",".join(target) | ||
| if effective_target == "all": | ||
| logger.progress( | ||
| f"Compiling for AGENTS.md + CLAUDE.md (--target {_target_label})" | ||
| ) | ||
| elif effective_target == "claude": | ||
| logger.progress( | ||
| f"Compiling for CLAUDE.md (--target {_target_label})" | ||
| ) | ||
| else: | ||
| logger.progress( | ||
| f"Compiling for AGENTS.md (--target {_target_label})" | ||
| ) | ||
| elif detected_target == "minimal": | ||
| from ...core.target_detection import ( | ||
| should_compile_agents_md, | ||
| should_compile_claude_md, | ||
| should_compile_gemini_md, | ||
| ) | ||
| _parts = [] | ||
| if should_compile_agents_md(effective_target): | ||
| _parts.append("AGENTS.md") | ||
| if should_compile_claude_md(effective_target): | ||
| _parts.append("CLAUDE.md") | ||
| if should_compile_gemini_md(effective_target): | ||
| _parts.append("GEMINI.md") | ||
| logger.progress( | ||
| f"Compiling for {' + '.join(_parts)} (--target {_target_label})" | ||
| ) | ||
| elif ( | ||
| isinstance(effective_target, str) | ||
| and effective_target == "vscode" | ||
| and detection_reason == REASON_NO_TARGET_FOLDER | ||
| ): | ||
| logger.progress(f"Compiling for AGENTS.md only ({detection_reason})") | ||
| logger.progress( | ||
| " Create .github/, .claude/, .codex/, .opencode/ or .cursor/ folder for full integration", | ||
| symbol="light_bulb", | ||
| ) | ||
| else: | ||
| description = get_target_description(detected_target) | ||
| description = get_target_description(effective_target) | ||
| logger.progress( | ||
| f"Compiling for {description} - {detection_reason}" | ||
| ) |
There was a problem hiding this comment.
The multi-target progress message is only shown when target is a list, but effective_target can also be a frozenset when the target comes from apm.yml (config list) with no CLI --target. In that case this falls through to get_target_description(effective_target) and logs unknown target. Consider switching the condition to isinstance(effective_target, frozenset) (and deriving a label from config_target) so config-driven multi-targets get the same accurate output summary.
| elif isinstance(compile_config_target, frozenset) and compile_target is None: | ||
| effective_target = compile_config_target | ||
| detection_reason = "apm.yml target" | ||
| else: | ||
| detected_target, detection_reason = detect_target( | ||
| project_root=Path("."), | ||
| explicit_target=compile_target, | ||
| config_target=compile_config_target, | ||
| ) |
There was a problem hiding this comment.
detect_target() is typed to accept Optional[str] for config_target, but here compile_config_target can be a frozenset (from _resolve_compile_target() when apm.yml specifies a multi-target list and the user also passed a single-string --target). It happens to work today, but it's an implicit type contract violation and makes the control flow harder to reason about. Consider passing config_target=compile_config_target only when it's a string (otherwise None).
Co-authored-by: Copilot <copilot@github.com>
Round 1 (Review Panel optional follow-ups): - Tighten CompileTargetType to Union[TargetType, frozenset[CompileFamily]] using a narrow CompileFamily Literal; mypy/pyright can now catch invalid families flowing into should_compile_*. - Extract REASON_NO_TARGET_FOLDER constant; replace brittle 'no target' in detection_reason substring match with equality. User-facing log message unchanged. - Add explicit _KNOWN_COMPILE_FAMILIES validation in AgentsCompiler so direct API callers passing an invalid frozenset family fail with a clear error instead of silently no-op'ing. Round 2 (Copilot reviewer findings on follow-up commit): - Fix latent bug: config-driven multi-target (apm.yml target: [a, b]) fell through to get_target_description() and logged 'unknown target'. Switch the multi-target log branch to isinstance(effective_target, frozenset) so CLI and config paths produce the same accurate output. - Honor detect_target()'s Optional[str] contract by passing config_target only when it is a string (frozenset case is already handled by the branch above). Tests: - test_unknown_frozenset_target_family_returns_failure - TestMultiTargetLogOutput.test_cli_multi_target_log_message - TestMultiTargetLogOutput.test_config_multi_target_log_message_does_not_say_unknown Refs microsoft#1020 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
force-pushed
the
feature/gemini-incorrect-generation
branch
from
fb314d3 to
cea1d17
Compare
|
Rebased on 1. Type tightening ( 2. Latent bug fix ( 3. Type contract honored ( All amended into the existing follow-up commit (force-with-lease used; only your three commits + my one follow-up are on the branch). Tests: 812 passed in 3.55s. Branch should now be mergeable + green for @apm-bot's next pass. |
Description
Enables the ability to be more specific about compile targets so GEMINI.md does not get generated unless it's actually requested.
Fixes #1019
Type of change
Testing