fix(compile): address Copilot review feedback on #1742

- Decide empty-shell suppression before generating content, skipping
  content generation + link resolution for placements that will be
  suppressed (perf: avoids wasted work).
- Cache constitution-existence per directory across placements in
  compile_distributed() to avoid repeated disk reads; _is_placement_empty_shell
  consults the cache.
- Promote the generated-file marker to public AGENTS_MD_GENERATED_MARKER
  (was _AGENTS_MD_GENERATED_MARKER) since it gates deletion and is a
  contract surface depended on by tests; mirrors public CLAUDE_HEADER.
- Fix _cleanup_orphaned_files docstring to match debug-only/no-warning
  behavior for hand-authored skips.
- Make the INFO-message test hermetic by writing the source instruction
  file on disk.

Author: tillig

src/apm_cli/compilation/distributed_compiler.py

@@ -29,7 +29,11 @@
 # Marker present in every APM-generated distributed AGENTS.md.
 # Used to gate --clean deletion: files without this marker are
 # hand-authored and must never be removed automatically.
-_AGENTS_MD_GENERATED_MARKER = "<!-- Generated by APM CLI from distributed .apm/ primitives -->"
+#
+# Public (no leading underscore) because it is a stable contract surface:
+# it gates file deletion and is depended on by tests. Mirrors the public
+# CLAUDE_HEADER marker on the Claude path.
+AGENTS_MD_GENERATED_MARKER = "<!-- Generated by APM CLI from distributed .apm/ primitives -->"
 
 # CRITICAL: Shadow Click commands to prevent namespace collision
 set = builtins.set
@@ -221,22 +225,30 @@ def compile_distributed(
             # Multi-target builds where skip_instructions=False are unaffected.
             content_map: builtins.dict[Path, str] = {}
             suppressed_paths: builtins.list[Path] = []
+            # Cache constitution-existence per directory for the duration of this
+            # compile so repeated placements under the same tree do not re-read the
+            # same constitution file from disk (O(placements) reads -> O(dirs)).
+            constitution_cache: builtins.dict[Path, bool] = {}
 
             for p in placements:
-                content = self._generate_agents_content(
-                    p, primitives, skip_instructions=skip_instructions
-                )
+                # Decide suppression BEFORE generating content: a suppressed
+                # placement is never written, so generating + link-resolving its
+                # content would be wasted work.
                 if skip_instructions and self._is_placement_empty_shell(
-                    p, with_constitution=with_constitution
+                    p,
+                    with_constitution=with_constitution,
+                    constitution_cache=constitution_cache,
                 ):
                     suppressed_paths.append(p.agents_path)
                     _logger.debug(
                         "AGENTS.md suppressed (would-be-empty shell, instructions already"
                         " in .github/instructions/): %s",
                         p.agents_path,
                     )
-                else:
-                    content_map[p.agents_path] = content
+                    continue
+                content_map[p.agents_path] = self._generate_agents_content(
+                    p, primitives, skip_instructions=skip_instructions
+                )
 
             # Phase 4: Handle orphaned file cleanup.
             # generated_paths = ALL placement paths (written + suppressed this run).
@@ -621,7 +633,7 @@ def _generate_agents_content(
 
         # Header with source attribution
         sections.append("# AGENTS.md")
-        sections.append(_AGENTS_MD_GENERATED_MARKER)
+        sections.append(AGENTS_MD_GENERATED_MARKER)
         sections.append(BUILD_ID_PLACEHOLDER)
         sections.append(f"<!-- APM Version: {get_version()} -->")
 
@@ -665,7 +677,11 @@ def _generate_agents_content(
         return content
 
     def _is_placement_empty_shell(
-        self, placement: PlacementResult, *, with_constitution: bool = True
+        self,
+        placement: PlacementResult,
+        *,
+        with_constitution: bool = True,
+        constitution_cache: builtins.dict[Path, bool] | None = None,
     ) -> bool:
         """Return True when a placement would produce a content-free AGENTS.md shell.
 
@@ -683,25 +699,39 @@ def _is_placement_empty_shell(
         a constitution file exists, so the predicate must agree and report "empty".
 
         Target logic:
-            empty = not (with_constitution and read_constitution(placement.agents_path.parent) is not None)
+            empty = not (with_constitution and constitution_exists(placement.agents_path.parent))
 
         Args:
             placement: The placement to evaluate.
             with_constitution: Whether the caller will inject a constitution at
                 write time (mirrors ``CompilationConfig.with_constitution``).
                 Defaults to True for back-compat.
+            constitution_cache: Optional ``dict[Path, bool]`` keyed by placement
+                directory, used to memoize constitution-existence checks across
+                placements within a single ``compile_distributed()`` call so the
+                same directory is not read from disk repeatedly. When omitted,
+                the check reads from disk directly (back-compat for direct callers).
 
         Returns:
             bool: True if writing this placement would produce a content-free file.
         """
-        # With constitution injection enabled, check whether a constitution file
-        # actually exists in the placement directory.  ConstitutionInjector uses
-        # agents_path.parent as its base_dir, so we mirror that here.
-        # If with_constitution is False the writer will skip injection regardless
-        # of what is on disk, so we treat it as "no constitution".
-        return not (
-            with_constitution and read_constitution(placement.agents_path.parent) is not None
-        )
+        # When with_constitution is False the writer skips injection regardless of
+        # what is on disk, so short-circuit without touching the filesystem.
+        if not with_constitution:
+            return True
+
+        # ConstitutionInjector uses agents_path.parent as its base_dir, so mirror
+        # that here. Memoize per directory to avoid repeated disk I/O in large
+        # repos with many placements sharing a tree.
+        directory = placement.agents_path.parent
+        if constitution_cache is not None and directory in constitution_cache:
+            has_constitution = constitution_cache[directory]
+        else:
+            has_constitution = read_constitution(directory) is not None
+            if constitution_cache is not None:
+                constitution_cache[directory] = has_constitution
+
+        return not has_constitution
 
     def _validate_coverage(
         self,
@@ -791,7 +821,7 @@ def _find_orphaned_agents_files(
                     file_content = agents_file.read_text(encoding="utf-8")
                 except OSError:
                     continue
-                if _AGENTS_MD_GENERATED_MARKER not in file_content:
+                if AGENTS_MD_GENERATED_MARKER not in file_content:
                     continue  # Hand-authored -- skip silently
 
                 orphaned_files.append(agents_file)
@@ -844,10 +874,12 @@ def _cleanup_orphaned_files(
     ) -> builtins.list[str]:
         """Actually remove orphaned AGENTS.md files.
 
-        Only APM-generated files (those bearing ``_AGENTS_MD_GENERATED_MARKER``)
-        are removed.  Hand-authored files are warned about but left untouched.
-        (The marker gate is also applied in ``_find_orphaned_agents_files``; this
-        check is defense-in-depth for direct callers.)
+        Only APM-generated files (those bearing ``AGENTS_MD_GENERATED_MARKER``)
+        are removed.  Hand-authored files are skipped silently and recorded at
+        DEBUG level only -- no user-facing message is appended to the returned
+        list.  (The marker gate is also applied in ``_find_orphaned_agents_files``,
+        so reaching the skip branch here is a defense-in-depth path for direct
+        callers and is not expected in normal operation.)
 
         Args:
             orphaned_files: Orphaned files to remove (should be pre-screened by
@@ -882,7 +914,7 @@ def _cleanup_orphaned_files(
                     except OSError:
                         cleanup_messages.append(f"  x Failed to read {rel_path} -- skipping")
                         continue
-                    if _AGENTS_MD_GENERATED_MARKER not in existing_content:
+                    if AGENTS_MD_GENERATED_MARKER not in existing_content:
                         # Defense-in-depth: _find_orphaned_agents_files already
                         # marker-gates, so this branch is unreachable in normal
                         # operation.  Emit at debug level only to avoid a stray

tests/unit/compilation/test_distributed_compiler_hermetic.py

@@ -24,7 +24,7 @@
 from unittest.mock import MagicMock, patch
 
 from apm_cli.compilation.distributed_compiler import (
-    _AGENTS_MD_GENERATED_MARKER,
+    AGENTS_MD_GENERATED_MARKER,
     CompilationResult,
     DirectoryMap,
     DistributedAgentsCompiler,
@@ -33,7 +33,7 @@
 from apm_cli.primitives.models import Instruction, PrimitiveCollection
 
 # Content used by tests that need an APM-generated AGENTS.md
-_GENERATED_CONTENT = f"{_AGENTS_MD_GENERATED_MARKER}\n# Generated content\n"
+_GENERATED_CONTENT = f"{AGENTS_MD_GENERATED_MARKER}\n# Generated content\n"
 # Content used by tests that need a hand-authored AGENTS.md (no APM marker)
 _HAND_AUTHORED_CONTENT = "# My custom AGENTS.md\nThis file was written by hand.\n"
 

tests/unit/compilation/test_distributed_compiler_phase3.py

@@ -24,7 +24,7 @@
 from unittest.mock import MagicMock, patch
 
 from apm_cli.compilation.distributed_compiler import (
-    _AGENTS_MD_GENERATED_MARKER,
+    AGENTS_MD_GENERATED_MARKER,
     CompilationResult,
     DirectoryMap,
     DistributedAgentsCompiler,
@@ -33,7 +33,7 @@
 from apm_cli.primitives.models import Instruction, PrimitiveCollection
 
 # APM-generated AGENTS.md content (marker present) for orphan tests
-_GENERATED_CONTENT = f"{_AGENTS_MD_GENERATED_MARKER}\n# Generated content\n"
+_GENERATED_CONTENT = f"{AGENTS_MD_GENERATED_MARKER}\n# Generated content\n"
 
 # ---------------------------------------------------------------------------
 # Helpers

tests/unit/compilation/test_empty_agents_shells_1730.py

@@ -23,7 +23,7 @@
 
 from apm_cli.compilation.agents_compiler import AgentsCompiler, CompilationConfig
 from apm_cli.compilation.distributed_compiler import (
-    _AGENTS_MD_GENERATED_MARKER,
+    AGENTS_MD_GENERATED_MARKER,
     DistributedAgentsCompiler,
     PlacementResult,
 )
@@ -64,7 +64,7 @@ def _apm_generated_marker_content(extra: str = "") -> str:
     """Produce minimal APM-generated AGENTS.md content (marker present)."""
     return (
         f"# AGENTS.md\n"
-        f"{_AGENTS_MD_GENERATED_MARKER}\n"
+        f"{AGENTS_MD_GENERATED_MARKER}\n"
         f"<!-- Build ID: abc123def456 -->\n"
         f"<!-- APM Version: 0.0.0 -->\n"
         f"\n"
@@ -285,11 +285,21 @@ def test_info_message_logged_when_suppression_fires(self, tmp_path: Path):
             encoding="utf-8",
         )
 
+        # Write the source instruction on disk so the fixture mirrors real
+        # project state and stays hermetic if filesystem-based validation tightens.
+        apm_instr_dir = tmp_path / ".apm" / "instructions"
+        apm_instr_dir.mkdir(parents=True)
+        apm_instr_file = apm_instr_dir / "global.instructions.md"
+        apm_instr_file.write_text(
+            "---\ndescription: Global\n---\nGlobal guidance.\n",
+            encoding="utf-8",
+        )
+
         instruction = _make_instruction(
             name="global",
             content="Global guidance.",
             apply_to=None,
-            file_path=tmp_path / ".apm" / "instructions" / "global.instructions.md",
+            file_path=apm_instr_file,
         )
         primitives = _make_primitives(instruction)