Skip to content

Commit 1288b0a

Browse files
alopezsanchezdanielmeppielCopilot
authored
fix: warn when instruction is missing required fields during apm compile (#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>
1 parent 45fcac9 commit 1288b0a

File tree

6 files changed

+167
-20
lines changed

6 files changed

+167
-20
lines changed

src/apm_cli/commands/compile/cli.py

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,8 @@ def _get_validation_suggestion(error_msg):
154154
"""Get actionable suggestions for validation errors."""
155155
if "Missing 'description'" in error_msg:
156156
return "Add 'description: Your description here' to frontmatter"
157-
elif "Missing 'applyTo'" in error_msg:
158-
return "Add 'applyTo: \"**/*.py\"' to frontmatter"
157+
elif "applyTo" in error_msg and "globally" in error_msg:
158+
return "Add 'applyTo: \"**/*.py\"' to scope the instruction, or leave as-is for global"
159159
elif "Empty content" in error_msg:
160160
return "Add markdown content below the frontmatter"
161161
else:
@@ -529,16 +529,13 @@ def compile(
529529
else:
530530
_display_next_steps(output)
531531

532-
# Common error handling for both compilation modes
533-
# Note: Warnings are handled by professional formatters for distributed mode
534-
if config.strategy != "distributed" or single_agents:
535-
# Only show warnings for single-file mode (backward compatibility)
536-
if result.warnings:
537-
logger.warning(
538-
f"Compilation completed with {len(result.warnings)} warnings:"
539-
)
540-
for warning in result.warnings:
541-
logger.warning(f" {warning}")
532+
# Display warnings for all compilation modes
533+
if result.warnings:
534+
logger.warning(
535+
f"Compilation completed with {len(result.warnings)} warning(s):"
536+
)
537+
for warning in result.warnings:
538+
logger.warning(f" {warning}")
542539

543540
if result.errors:
544541
logger.error(f"Compilation failed with {len(result.errors)} errors:")

src/apm_cli/compilation/agents_compiler.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,9 @@ def _compile_distributed(self, config: CompilationConfig, primitives: PrimitiveC
253253
"""
254254
from .distributed_compiler import DistributedAgentsCompiler
255255

256+
errors = self.validate_primitives(primitives)
257+
self.errors.extend(errors)
258+
256259
# Create distributed compiler with exclude patterns
257260
distributed_compiler = DistributedAgentsCompiler(
258261
str(self.base_dir),
@@ -317,8 +320,8 @@ def _compile_distributed(self, config: CompilationConfig, primitives: PrimitiveC
317320
success=True,
318321
output_path="Preview mode - no files written",
319322
content=self._generate_placement_summary(distributed_result),
320-
warnings=distributed_result.warnings,
321-
errors=distributed_result.errors,
323+
warnings=self.warnings + distributed_result.warnings,
324+
errors=self.errors + distributed_result.errors,
322325
stats=distributed_result.stats
323326
)
324327

@@ -405,6 +408,9 @@ def _compile_claude_md(self, config: CompilationConfig, primitives: PrimitiveCol
405408
Returns:
406409
CompilationResult: Result of the CLAUDE.md compilation.
407410
"""
411+
errors = self.validate_primitives(primitives)
412+
self.errors.extend(errors)
413+
408414
# Create Claude formatter
409415
claude_formatter = ClaudeFormatter(str(self.base_dir))
410416

@@ -435,8 +441,8 @@ def _compile_claude_md(self, config: CompilationConfig, primitives: PrimitiveCol
435441
# not at compile time. This keeps behavior consistent with VSCode prompt integration.
436442

437443
# Merge warnings and errors (no command result anymore)
438-
all_warnings = claude_result.warnings
439-
all_errors = claude_result.errors
444+
all_warnings = self.warnings + claude_result.warnings
445+
all_errors = self.errors + claude_result.errors
440446

441447
# Handle dry-run mode
442448
if config.dry_run:

src/apm_cli/primitives/models.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ def validate(self) -> List[str]:
5353
if not self.description:
5454
errors.append("Missing 'description' in frontmatter")
5555
if not self.apply_to:
56-
errors.append("Missing 'applyTo' in frontmatter (required for instructions)")
56+
errors.append("No 'applyTo' pattern specified -- instruction will apply globally")
5757
if not self.content.strip():
5858
errors.append("Empty content")
5959
return errors

tests/unit/compilation/test_compilation.py

Lines changed: 86 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,29 @@ def test_validate_primitives(self):
156156
errors = compiler.validate_primitives(primitives)
157157
self.assertEqual(len(errors), 0)
158158

159+
def test_validate_primitives_warns_on_missing_apply_to(self):
160+
"""Test that validate_primitives adds a warning when applyTo is missing."""
161+
compiler = AgentsCompiler(str(self.temp_path))
162+
163+
primitives = PrimitiveCollection()
164+
instruction = Instruction(
165+
name="no-scope",
166+
file_path=self.temp_path / "no-scope.instructions.md",
167+
description="Instruction without applyTo",
168+
apply_to="",
169+
content="Some content",
170+
author="test",
171+
)
172+
primitives.add_primitive(instruction)
173+
174+
errors = compiler.validate_primitives(primitives)
175+
self.assertEqual(len(errors), 0)
176+
self.assertTrue(len(compiler.warnings) > 0)
177+
self.assertTrue(
178+
any("applyTo" in w for w in compiler.warnings),
179+
f"Expected a warning mentioning 'applyTo', got: {compiler.warnings}",
180+
)
181+
159182
@patch('apm_cli.primitives.discovery.discover_primitives')
160183
def test_compile_with_mock_primitives(self, mock_discover):
161184
"""Test compilation with mocked primitives."""
@@ -185,6 +208,67 @@ def test_compile_with_mock_primitives(self, mock_discover):
185208
self.assertIn("Files matching `**/*.py`", result.content)
186209
self.assertIn("Use type hints.", result.content)
187210

211+
def test_distributed_compile_includes_validation_warnings(self):
212+
"""Test that distributed compilation surfaces warnings for missing applyTo."""
213+
primitives = PrimitiveCollection()
214+
215+
good_instruction = Instruction(
216+
name="good",
217+
file_path=self.temp_path / "good.instructions.md",
218+
description="Valid instruction",
219+
apply_to="**/*.py",
220+
content="Follow PEP 8.",
221+
author="test",
222+
)
223+
bad_instruction = Instruction(
224+
name="bad",
225+
file_path=self.temp_path / "bad.instructions.md",
226+
description="Missing applyTo",
227+
apply_to="",
228+
content="This has no scope.",
229+
author="test",
230+
)
231+
primitives.add_primitive(good_instruction)
232+
primitives.add_primitive(bad_instruction)
233+
234+
compiler = AgentsCompiler(str(self.temp_path))
235+
config = CompilationConfig(
236+
dry_run=True, resolve_links=False, strategy="distributed", target="agents"
237+
)
238+
239+
result = compiler.compile(config, primitives)
240+
241+
self.assertTrue(
242+
any("applyTo" in w for w in result.warnings),
243+
f"Expected a warning about missing 'applyTo', got: {result.warnings}",
244+
)
245+
246+
def test_claude_md_compile_includes_validation_warnings(self):
247+
"""Test that CLAUDE.md compilation surfaces warnings for missing applyTo."""
248+
primitives = PrimitiveCollection()
249+
250+
bad_instruction = Instruction(
251+
name="no-scope",
252+
file_path=self.temp_path / "no-scope.instructions.md",
253+
description="Missing applyTo",
254+
apply_to="",
255+
content="This has no scope.",
256+
author="test",
257+
)
258+
primitives.add_primitive(bad_instruction)
259+
260+
compiler = AgentsCompiler(str(self.temp_path))
261+
config = CompilationConfig(
262+
dry_run=True, resolve_links=False, target="claude"
263+
)
264+
265+
result = compiler.compile(config, primitives)
266+
267+
self.assertTrue(
268+
any("applyTo" in w for w in result.warnings),
269+
f"Expected a warning about missing 'applyTo', got: {result.warnings}",
270+
)
271+
188272
def test_compile_agents_md_function(self):
189273
"""Test the standalone compile function."""
190274
# Create test primitives
@@ -319,8 +403,8 @@ def test_validate_mode_with_valid_primitives(self):
319403
suggestion = _get_validation_suggestion("Missing 'description' in frontmatter")
320404
self.assertIn("Add 'description:", suggestion)
321405

322-
suggestion = _get_validation_suggestion("Missing 'applyTo' in frontmatter")
323-
self.assertIn("Add 'applyTo:", suggestion)
406+
suggestion = _get_validation_suggestion("No 'applyTo' pattern specified -- instruction will apply globally")
407+
self.assertIn("applyTo", suggestion)
324408

325409
suggestion = _get_validation_suggestion("Empty content")
326410
self.assertIn("Add markdown content", suggestion)

tests/unit/compilation/test_compile_target_flag.py

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -847,3 +847,63 @@ def test_cli_override_takes_precedence(self, temp_project_with_config):
847847
assert config.target == "vscode"
848848
finally:
849849
os.chdir(original_dir)
850+
851+
852+
class TestCompileWarningOnMissingApplyTo:
853+
"""Tests that apm compile warns when an instruction is missing applyTo."""
854+
855+
@pytest.fixture
856+
def runner(self):
857+
return CliRunner()
858+
859+
@pytest.fixture
860+
def project_with_bad_instruction(self):
861+
temp_dir = tempfile.mkdtemp()
862+
temp_path = Path(temp_dir)
863+
864+
(temp_path / "apm.yml").write_text("name: test-project\nversion: 0.1.0\n")
865+
866+
apm_dir = temp_path / ".apm" / "instructions"
867+
apm_dir.mkdir(parents=True)
868+
869+
(apm_dir / "good.instructions.md").write_text(
870+
"---\napplyTo: '**/*.py'\n---\nFollow PEP 8.\n"
871+
)
872+
(apm_dir / "bad.instructions.md").write_text(
873+
"---\ndescription: Missing applyTo\n---\nThis instruction has no scope.\n"
874+
)
875+
876+
yield temp_path
877+
shutil.rmtree(temp_dir, ignore_errors=True)
878+
879+
def test_cli_warns_missing_apply_to_distributed(
880+
self, runner, project_with_bad_instruction
881+
):
882+
"""Test that apm compile --dry-run warns about missing applyTo in distributed mode."""
883+
original_dir = os.getcwd()
884+
try:
885+
os.chdir(project_with_bad_instruction)
886+
result = runner.invoke(
887+
cli, ["compile", "--target", "vscode", "--dry-run"]
888+
)
889+
assert "applyTo" in result.output, (
890+
f"Expected warning about missing 'applyTo' in CLI output, got:\n{result.output}"
891+
)
892+
finally:
893+
os.chdir(original_dir)
894+
895+
def test_cli_warns_missing_apply_to_claude(
896+
self, runner, project_with_bad_instruction
897+
):
898+
"""Test that apm compile --target claude --dry-run warns about missing applyTo."""
899+
original_dir = os.getcwd()
900+
try:
901+
os.chdir(project_with_bad_instruction)
902+
result = runner.invoke(
903+
cli, ["compile", "--target", "claude", "--dry-run"]
904+
)
905+
assert "applyTo" in result.output, (
906+
f"Expected warning about missing 'applyTo' in CLI output, got:\n{result.output}"
907+
)
908+
finally:
909+
os.chdir(original_dir)

tests/unit/primitives/test_primitives.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ def test_instruction_validation(self):
7373
)
7474
self.assertEqual(instruction.validate(), [])
7575

76-
# Missing applyTo (required for instructions)
76+
# Missing applyTo (instruction will apply globally)
7777
instruction_no_apply = Instruction(
7878
name="test",
7979
file_path=Path("test.instructions.md"),

0 commit comments

Comments
 (0)