fix: warn when instruction is missing required fields during apm compile#449
Conversation
…ompile Instructions without applyTo were silently dropped during 'apm compile' in distributed and claude modes because validate_primitives() was only called in the single-file path. This adds validation to all compilation modes and surfaces warnings through the CLI. - Call validate_primitives() in _compile_distributed() and _compile_claude_md() - Merge self.warnings into dry-run and normal compilation results - Remove distributed-mode warning suppression in CLI - Add unit and CLI integration tests for the warning
|
|
||
| # Common error handling for both compilation modes | ||
| # Note: Warnings are handled by professional formatters for distributed mode | ||
| if config.strategy != "distributed" or single_agents: |
There was a problem hiding this comment.
I'm not sure about this line, but it took me some time to realize that I was missing the applyTo field. The apm compile wasn't adding the instruction to the AGENTS/CLAUDE.md, and I didn't know why.
There was a problem hiding this comment.
Pull request overview
Fixes missing validation coverage in apm compile so instructions missing required fields (notably applyTo) are no longer silently dropped in distributed/claude compilation paths, and ensures warnings are surfaced through the CLI.
Changes:
- Run
validate_primitives()in distributed and CLAUDE.md compilation paths. - Merge compiler warnings/errors into compilation results (including dry-run).
- Add unit + CLI integration tests asserting warnings appear when
applyTois missing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/unit/compilation/test_compile_target_flag.py | Adds CLI-level tests that apm compile emits warnings for missing applyTo in distributed and claude targets. |
| tests/unit/compilation/test_compilation.py | Adds unit tests ensuring validation warnings propagate through distributed + claude compilation results. |
| src/apm_cli/compilation/agents_compiler.py | Invokes validation in more compilation modes and merges self.warnings/self.errors into results. |
| src/apm_cli/commands/compile/cli.py | Always prints warnings regardless of compilation strategy. |
| original_dir = os.getcwd() | ||
| try: | ||
| os.chdir(project_with_bad_instruction) | ||
| result = runner.invoke( | ||
| cli, ["compile", "--target", "vscode", "--dry-run"] | ||
| ) |
There was a problem hiding this comment.
These CLI tests mutate the process working directory, which is easy to get wrong and makes tests harder to compose. Prefer pytest’s monkeypatch.chdir(project_with_bad_instruction) (or CliRunner.isolated_filesystem() plus constructing the project inside it) to keep the cwd change scoped and automatically restored.
| self, runner, project_with_bad_instruction | ||
| ): | ||
| """Test that apm compile --dry-run warns about missing applyTo in distributed mode.""" | ||
| original_dir = os.getcwd() | ||
| try: | ||
| os.chdir(project_with_bad_instruction) | ||
| result = runner.invoke( | ||
| cli, ["compile", "--target", "vscode", "--dry-run"] | ||
| ) | ||
| assert "applyTo" in result.output, ( | ||
| f"Expected warning about missing 'applyTo' in CLI output, got:\n{result.output}" | ||
| ) | ||
| finally: | ||
| os.chdir(original_dir) | ||
|
|
||
| def test_cli_warns_missing_apply_to_claude( | ||
| self, runner, project_with_bad_instruction | ||
| ): | ||
| """Test that apm compile --target claude --dry-run warns about missing applyTo.""" | ||
| original_dir = os.getcwd() | ||
| try: | ||
| os.chdir(project_with_bad_instruction) | ||
| result = runner.invoke( | ||
| cli, ["compile", "--target", "claude", "--dry-run"] | ||
| ) | ||
| assert "applyTo" in result.output, ( | ||
| f"Expected warning about missing 'applyTo' in CLI output, got:\n{result.output}" | ||
| ) | ||
| finally: | ||
| os.chdir(original_dir) |
There was a problem hiding this comment.
These CLI tests mutate the process working directory, which is easy to get wrong and makes tests harder to compose. Prefer pytest’s monkeypatch.chdir(project_with_bad_instruction) (or CliRunner.isolated_filesystem() plus constructing the project inside it) to keep the cwd change scoped and automatically restored.
| self, runner, project_with_bad_instruction | |
| ): | |
| """Test that apm compile --dry-run warns about missing applyTo in distributed mode.""" | |
| original_dir = os.getcwd() | |
| try: | |
| os.chdir(project_with_bad_instruction) | |
| result = runner.invoke( | |
| cli, ["compile", "--target", "vscode", "--dry-run"] | |
| ) | |
| assert "applyTo" in result.output, ( | |
| f"Expected warning about missing 'applyTo' in CLI output, got:\n{result.output}" | |
| ) | |
| finally: | |
| os.chdir(original_dir) | |
| def test_cli_warns_missing_apply_to_claude( | |
| self, runner, project_with_bad_instruction | |
| ): | |
| """Test that apm compile --target claude --dry-run warns about missing applyTo.""" | |
| original_dir = os.getcwd() | |
| try: | |
| os.chdir(project_with_bad_instruction) | |
| result = runner.invoke( | |
| cli, ["compile", "--target", "claude", "--dry-run"] | |
| ) | |
| assert "applyTo" in result.output, ( | |
| f"Expected warning about missing 'applyTo' in CLI output, got:\n{result.output}" | |
| ) | |
| finally: | |
| os.chdir(original_dir) | |
| self, runner, project_with_bad_instruction, monkeypatch | |
| ): | |
| """Test that apm compile --dry-run warns about missing applyTo in distributed mode.""" | |
| monkeypatch.chdir(project_with_bad_instruction) | |
| result = runner.invoke(cli, ["compile", "--target", "vscode", "--dry-run"]) | |
| assert "applyTo" in result.output, ( | |
| f"Expected warning about missing 'applyTo' in CLI output, got:\n{result.output}" | |
| ) | |
| def test_cli_warns_missing_apply_to_claude( | |
| self, runner, project_with_bad_instruction, monkeypatch | |
| ): | |
| """Test that apm compile --target claude --dry-run warns about missing applyTo.""" | |
| monkeypatch.chdir(project_with_bad_instruction) | |
| result = runner.invoke(cli, ["compile", "--target", "claude", "--dry-run"]) | |
| assert "applyTo" in result.output, ( | |
| f"Expected warning about missing 'applyTo' in CLI output, got:\n{result.output}" | |
| ) |
|
@alopezsanchez happy to merge after we address the review comments, thank you very much! Will also need to ensure compatibility with claude's "paths" which is equivalent to "applyTo". |
…pile paths Address Copilot PR review comments: - Capture return value of validate_primitives() and extend self.errors in _compile_distributed() and _compile_claude_md(), matching the pattern already used in _compile_single_file() - Add target='agents' to test config to isolate distributed path testing
…note
Instructions without applyTo are valid -- they apply globally:
- Distributed compile: placed at project root
- Claude deploy: becomes unconditional rule (no paths: key)
- Copilot/Cursor: deployed verbatim
The warning message now reflects this ('will apply globally') instead of
implying applyTo is required. Suggestion updated to explain the choice.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks @alopezsanchez -- the approach of surfacing I pushed one change to your branch: the warning message for missing
to:
Rationale:
The old message implied Also updated the suggestion helper to offer both options: add |
The inline comment on Instruction.apply_to still claimed the field was "required for instructions", contradicting the validator (which has treated missing applyTo as a warning since microsoft#449) and now also contradicting the renderers (which from this PR onward emit globals under a `## Global Instructions` section). Update the comment to match the documented behaviour: empty string means global, applies to every file. Surfaced by the panel review on microsoft#1088.
The inline comment on Instruction.apply_to still claimed the field was "required for instructions", contradicting the validator (which has treated missing applyTo as a warning since microsoft#449) and now also contradicting the renderers (which from this PR onward emit globals under a `## Global Instructions` section). Update the comment to match the documented behaviour: empty string means global, applies to every file. Surfaced by the panel review on microsoft#1088.
…icrosoft#1088) * fix(compile): include global instructions in AGENTS.md and CLAUDE.md Instructions without an applyTo pattern were placed correctly by the context optimizer (which reported them as (global) at rel: 100%), but the three rendering paths --- distributed AGENTS.md, the monolithic AGENTS.md template, and CLAUDE.md --- each filtered them out before emission with the same `if instruction.apply_to: continue` pattern. The result was a silent disagreement between the placement report and the rendered file: the user saw "placed", the file saw nothing. This extracts a shared `render_instructions_block` helper in template_builder so all three renderers go through one path that emits a `## Global Instructions` block (in addition to the existing `## Files matching <pattern>` blocks). Each caller still controls its own per-instruction emission via a callback so source-attribution formats stay byte-identical to before. Out of scope: the dedicated `.github/copilot-instructions.md` path already handled global instructions correctly via microsoft#1067; that file is unchanged. Fixes microsoft#1072 * fix(compile): address review feedback on microsoft#1088 - Replace non-ASCII em dashes with `--` in regression test docstring and inline comment to comply with the printable-ASCII-only encoding rule (.github/instructions/encoding.instructions.md). - Trim CHANGELOG entry to a single concise line ending with the PR number, per .github/instructions/changelog.instructions.md. - Update Starlight docs that previously claimed `applyTo` is required: - introduction/key-concepts.md: mark `applyTo` as optional in the instruction frontmatter section and clarify the validation rule. - reference/cli-commands.md: document the `## Global Instructions` section in the generated AGENTS.md structure overview. * fix(compile): document apply_to as optional in Instruction model The inline comment on Instruction.apply_to still claimed the field was "required for instructions", contradicting the validator (which has treated missing applyTo as a warning since microsoft#449) and now also contradicting the renderers (which from this PR onward emit globals under a `## Global Instructions` section). Update the comment to match the documented behaviour: empty string means global, applies to every file. Surfaced by the panel review on microsoft#1088.
…pile` (#449) * fix: warn when instruction is missing required applyTo field during compile Instructions without applyTo were silently dropped during 'apm compile' in distributed and claude modes because validate_primitives() was only called in the single-file path. This adds validation to all compilation modes and surfaces warnings through the CLI. - Call validate_primitives() in _compile_distributed() and _compile_claude_md() - Merge self.warnings into dry-run and normal compilation results - Remove distributed-mode warning suppression in CLI - Add unit and CLI integration tests for the warning * fix: capture validate_primitives errors in distributed and claude compile paths Address Copilot PR review comments: - Capture return value of validate_primitives() and extend self.errors in _compile_distributed() and _compile_claude_md(), matching the pattern already used in _compile_single_file() - Add target='agents' to test config to isolate distributed path testing * fix: change missing applyTo from error-like warning to informational note Instructions without applyTo are valid -- they apply globally: - Distributed compile: placed at project root - Claude deploy: becomes unconditional rule (no paths: key) - Copilot/Cursor: deployed verbatim The warning message now reflects this ('will apply globally') instead of implying applyTo is required. Suggestion updated to explain the choice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Daniel Meppiel <51440732+danielmeppiel@users.noreply.github.com> Co-authored-by: danielmeppiel <dmeppiel@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…1088) * fix(compile): include global instructions in AGENTS.md and CLAUDE.md Instructions without an applyTo pattern were placed correctly by the context optimizer (which reported them as (global) at rel: 100%), but the three rendering paths --- distributed AGENTS.md, the monolithic AGENTS.md template, and CLAUDE.md --- each filtered them out before emission with the same `if instruction.apply_to: continue` pattern. The result was a silent disagreement between the placement report and the rendered file: the user saw "placed", the file saw nothing. This extracts a shared `render_instructions_block` helper in template_builder so all three renderers go through one path that emits a `## Global Instructions` block (in addition to the existing `## Files matching <pattern>` blocks). Each caller still controls its own per-instruction emission via a callback so source-attribution formats stay byte-identical to before. Out of scope: the dedicated `.github/copilot-instructions.md` path already handled global instructions correctly via #1067; that file is unchanged. Fixes #1072 * fix(compile): address review feedback on #1088 - Replace non-ASCII em dashes with `--` in regression test docstring and inline comment to comply with the printable-ASCII-only encoding rule (.github/instructions/encoding.instructions.md). - Trim CHANGELOG entry to a single concise line ending with the PR number, per .github/instructions/changelog.instructions.md. - Update Starlight docs that previously claimed `applyTo` is required: - introduction/key-concepts.md: mark `applyTo` as optional in the instruction frontmatter section and clarify the validation rule. - reference/cli-commands.md: document the `## Global Instructions` section in the generated AGENTS.md structure overview. * fix(compile): document apply_to as optional in Instruction model The inline comment on Instruction.apply_to still claimed the field was "required for instructions", contradicting the validator (which has treated missing applyTo as a warning since #449) and now also contradicting the renderers (which from this PR onward emit globals under a `## Global Instructions` section). Update the comment to match the documented behaviour: empty string means global, applies to every file. Surfaced by the panel review on #1088.
Description
Instructions without
applyTowere silently dropped duringapm compilein distributed and claude modes becausevalidate_primitives()was only called in the single-file path. This adds validation to all compilation modes and surfaces warnings through the CLI.validate_primitives()in_compile_distributed()and_compile_claude_md()self.warningsinto dry-run and normal compilation resultsType of change
Testing