Skip to content

fix(compile): deduplicate Claude Code instructions (#1138)#1146

Open
tillig wants to merge 27 commits into
microsoft:mainfrom
tillig:feature/double-claude-instructions
Open

fix(compile): deduplicate Claude Code instructions (#1138)#1146
tillig wants to merge 27 commits into
microsoft:mainfrom
tillig:feature/double-claude-instructions

Conversation

@tillig
Copy link
Copy Markdown
Contributor

@tillig tillig commented May 5, 2026

Summary

  • When both apm install (deploys to .claude/rules/) and apm compile --target claude (generates CLAUDE.md) have been run, Claude Code sees every instruction twice. This PR detects when .claude/rules/ already contains .md files and omits the "Project Standards" section from CLAUDE.md, eliminating the duplication.
  • CLAUDE.md is still generated when it carries a constitution block or @import dependency paths — only the instructions section is suppressed.
  • Adds an informational log message when zero CLAUDE.md files are generated (all content already deployed via rules).

Details

Detection heuristic: claude_rules_dir.is_dir() and any(claude_rules_dir.glob("*.md")). This is intentionally simple — if the directory exists and has markdown files, we assume apm install put them there. A future enhancement could cross-check against apm.lock.yaml deployed-file provenance for more precision.

Files changed:

  • src/apm_cli/compilation/agents_compiler.py — detection logic + log messages
  • src/apm_cli/compilation/claude_formatter.pyskip_instructions config plumbing; is_root field on ClaudePlacement; skips subdirectory placements and the Project Standards section when active
  • tests/unit/compilation/test_agents_compiler_coverage.py — 9 tests (TestClaudeCompileSkipInstructions)
  • tests/unit/compilation/test_claude_formatter.py — 6 tests (TestSkipInstructions)
  • docs/src/content/docs/producer/compile.md — deduplication note + updated "Where instructions land" table
  • CHANGELOG.md — entry under Unreleased/Fixed

Possible follow-up

Lockfile-provenance detection: instead of globbing .claude/rules/*.md, cross-check against deployed_files in apm.lock.yaml to distinguish APM-managed rules from hand-authored ones. Deferred to keep this PR focused.

Test plan

  • All 359 compilation unit tests pass
  • Full unit suite passes (8,310 tests)
  • Linter passes on all modified files
  • Verify apm compile --target claude with .claude/rules/ present → no Project Standards in CLAUDE.md
  • Verify apm compile --target claude without .claude/rules/ → CLAUDE.md includes Project Standards as before
  • Verify constitution + dependencies still appear in CLAUDE.md when instructions are skipped

Closes #1138

tillig and others added 4 commits May 4, 2026 16:23
…opulated

When `apm install` has already deployed instructions to `.claude/rules/`
(Claude Code's native format), `apm compile --target claude` now omits
the "Project Standards" section from CLAUDE.md to avoid duplicating
instructions in Claude Code's context window.

CLAUDE.md is still generated when it carries other non-duplicated content
(constitution block or dependency @import paths). Subdirectory CLAUDE.md
files are suppressed entirely since they would only contain instructions.

Detection: if `.claude/rules/` exists and contains any `.md` files, the
instructions are considered already deployed. An empty `.claude/rules/`
directory does not trigger skipping.

Refs: microsoft#1138

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Log "CLAUDE.md not generated" when skip_instructions results in no
  output, so users understand why no file was produced.
- Strengthen test assertion: assert CLAUDE.md does NOT exist (instead
  of conditional check) when no constitution/deps are present.
- Add test for .claude/rules/ containing only non-.md files (e.g.,
  .gitkeep) — verifies the glob doesn't false-positive.
- Add test for dry-run + skip_instructions combination.
- Add test verifying both log messages are emitted via mock logger.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 5, 2026 15:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prevents Claude Code from seeing duplicated instructions when both apm install (deploys .claude/rules/*.md) and apm compile --target claude (generates CLAUDE.md) are used by detecting a populated .claude/rules/ directory and suppressing the # Project Standards section in CLAUDE.md (while still emitting constitution and dependency imports when present).

Changes:

  • Add .claude/rules/*.md detection in the Claude compile path and plumb a skip_instructions flag into the Claude formatter.
  • Update ClaudeFormatter to omit instruction output (and subdirectory CLAUDE.md placements) when skip_instructions is active, while preserving constitution/dependencies behavior.
  • Add unit tests for the skip behavior and record the fix in CHANGELOG.md.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/apm_cli/compilation/agents_compiler.py Detects populated .claude/rules/, passes skip_instructions, and emits info logs when output is suppressed.
src/apm_cli/compilation/claude_formatter.py Implements conditional suppression of # Project Standards and skips subdirectory/root outputs when only instructions would be emitted.
tests/unit/compilation/test_agents_compiler_coverage.py Adds coverage tests for the .claude/rules/ heuristic and logging behavior.
tests/unit/compilation/test_claude_formatter.py Adds formatter-level tests covering skip behavior across instructions/constitution/dependencies.
CHANGELOG.md Adds an Unreleased/Fixed entry for the deduplication behavior.

Comment thread src/apm_cli/compilation/claude_formatter.py
Comment thread tests/unit/compilation/test_agents_compiler_coverage.py Outdated
Comment thread src/apm_cli/compilation/agents_compiler.py
- Stats now reflect only emitted files (not skipped placements)
- Fix weak test assertion that could pass spuriously
- Add stats accuracy test for skip_instructions path
- Update compilation and IDE integration docs to mention deduplication
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Comment thread src/apm_cli/compilation/claude_formatter.py
Comment thread src/apm_cli/compilation/claude_formatter.py Outdated
Comment thread tests/unit/compilation/test_agents_compiler_coverage.py Outdated
Comment thread docs/src/content/docs/guides/compilation.md Outdated
Comment thread docs/src/content/docs/integrations/ide-tool-integration.md Outdated
tillig and others added 2 commits May 5, 2026 09:04
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Initialize _skip_instructions in __init__ to avoid AttributeError
- Update misleading comment about CLAUDE.md content scope
- Fix dry-run test to assert on stats (preview doesn't include section headers)
- Fix compilation guide "only instructions" note to exclude CLAUDE.md
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@tillig tillig requested a review from Copilot May 5, 2026 16:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@tillig tillig requested a review from Copilot May 5, 2026 18:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Comment thread src/apm_cli/compilation/claude_formatter.py Outdated
Comment thread src/apm_cli/compilation/claude_formatter.py Outdated
Comment thread src/apm_cli/compilation/claude_formatter.py Outdated
Comment thread src/apm_cli/compilation/claude_formatter.py Outdated
Comment thread tests/unit/compilation/test_agents_compiler_coverage.py Outdated
tillig added 2 commits May 11, 2026 16:10
- Add is_root field to ClaudePlacement dataclass; set it in
  _generate_placements so root detection is computed once and
  used consistently everywhere (no resolve/absolute fallback).
- Remove _skip_instructions mutable instance state from
  ClaudeFormatter; pass skip_instructions as a keyword argument
  to _generate_claude_content instead.
- Hoist read_constitution() call before the placement loop to
  avoid redundant reads on every iteration.
- Fix grammar: "Would generate 1 files" -> "Would generate 1 file".
- Skip message now explains "to avoid duplicate context" so users
  understand the optimization is intentional.
- Zero-file message now reassures users: "Claude Code reads
  .claude/rules/ directly, no further action needed."
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comment thread src/apm_cli/compilation/claude_formatter.py Outdated
Comment thread src/apm_cli/compilation/agents_compiler.py
Comment thread docs/src/content/docs/producer/compile.md Outdated
Comment thread tests/unit/compilation/test_agents_compiler_coverage.py
- Create root placement when dependencies exist (not only when
  constitution exists), fixing dependency-only project edge case.
- Suppress CompilationFormatter output when skip_instructions
  filtered all placements to avoid contradicting the "not generated"
  log message.
- Update "Where instructions land" table to note CLAUDE.md may be
  omitted when .claude/rules/ is populated.
- Convert bare assert statements to self.assert* for consistency
  with the rest of the unittest.TestCase file.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comment thread docs/src/content/docs/producer/compile.md Outdated
Comment thread docs/src/content/docs/producer/compile.md Outdated
@github-actions
Copy link
Copy Markdown

APM Review Panel: ship_with_followups

PR #1146 eliminates duplicate Project Standards in CLAUDE.md when .claude/rules/ is already populated, reducing context-window bloat for every Claude Code user who runs both apm install and apm compile.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All active panelists converged on the same reading: the deduplication logic is correct, the path-containment guard is sound, and the 14 new unit tests cover the skip_instructions branches thoroughly. The cross-cutting concern is user-visible communication: two independent panelists (cli-logging-expert and devx-ux-expert) identified the same gap -- the dry-run path emits 'Would generate 0 files' with no explanation when skip_instructions fires, leaving scripted consumers and novice users with no signal about why. That is a real UX debt worth clearing in this PR or immediately after. The doc-writer surfaced two correctness issues: a broken fragment anchor (the table's #claude-code-deduplication link will silently 404 because Starlight does not generate anchors from callout directives) and an attribution error that will mislead producers who have never run apm install. Both are pre-ship quality issues, not opinion.

The supply-chain finding (any .md in .claude/rules/ triggers skip) is a legitimate design note but not a blocker: the current behavior is consistent with how APM manages that directory and a sentinel-file approach would require a coordinated install-side change. It belongs on the backlog. The test-coverage-expert's missing-integration finding (outcome: missing on a devx surface) is the highest-signal gap -- the unit tests cover the compiler in isolation, but the user promise ('run apm install then apm compile and see no duplication') is a cross-module flow with no automated guard. This is a regression-trap risk: the next refactor of apm install's write path could silently break the contract with no failing test.

The oss-growth-hacker's growth_strategy_note is well-timed: Claude Code mindshare is accelerating and first-class deduplication support is a concrete, shareable signal. The CHANGELOG and docs note both bury the user benefit; a one-line reframe leads with 'saves context-window tokens on every compile' rather than the mechanism.

Aligned with: Portable by manifest -- deduplication is driven by filesystem state (.claude/rules/ contents), consistent with manifest-driven behavior, no hardcoded agent assumptions. Pragmatic as npm -- silently doing the right thing when the environment already has what it needs matches the npm-install-is-idempotent expectation users carry in. Multi-harness/multi-host -- change is scoped entirely to the claude compile target; no other harness paths are touched.

Growth signal. Claude Code mindshare is climbing steeply in 2025 and first-class support signals compound. The CHANGELOG and docs note should lead with the user benefit -- 'eliminates duplicate instructions that waste Claude's context window on every request' -- rather than the mechanism. That one-line reframe is the difference between a fix and a story that Claude Code users share.

Panel summary

Persona B R N Takeaway
Python Architect 0 2 1 Solid value-object promotion and clean skip logic; one stat inaccuracy and one double-read worth addressing as a follow-up.
CLI Logging Expert 0 1 2 Three new log messages are clean and ASCII-safe; one recommended fix for the dry-run + skip 'Would generate 0 files' blind spot.
DevX UX Expert 0 1 2 Dry-run 'Would generate 0 files' lacks a why; confusing for users who capture output or script around compile.
Supply Chain Security Expert 0 1 1 Path-containment guard is correct and sufficient; one instruction-suppression vector worth noting but not blocking.
OSS Growth Hacker 0 2 1 Solid friction-reduction fix; one missed opportunity: no user-facing signal quantifying the context-window cost saved.
Doc Writer 0 2 1 Two correctness gaps: broken fragment anchor and note incorrectly attributes .claude/rules/ population solely to apm install. CHANGELOG entry is clean.
Test Coverage Expert 0 1 1 Unit coverage is comprehensive (14 new tests); one recommended gap: no integration-with-fixtures test for the install->compile cross-module flow.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Doc Writer] Broken fragment anchor #claude-code-deduplication -- add an explicit heading or <a id="..."> before the note callout -- Starlight does not generate anchors from :::note[] callouts; the table link silently 404s in rendered docs. Pre-ship correctness issue.
  2. [Doc Writer] Deduplication note attributes .claude/rules/ population only to apm install -- misses that apm compile itself also writes there -- A producer who only runs apm compile will hit deduplication silently on the second run with no docs explanation. Correctness gap.
  3. [Test Coverage Expert] Add an integration test for the install->compile deduplication pipeline (outcome: missing at integration-with-fixtures tier) -- The user-visible promise of this PR has no automated guard; a regression-trap gap that could silently break across install-side refactors.
  4. [CLI Logging Expert] Dry-run + skip_instructions emits 'Would generate 0 files' with no dedup explanation -- Scripted consumers and novice users see 0 files with no reason; two independent panelists flagged this same gap.
  5. [Supply Chain Security Expert] Any .md in .claude/rules/ triggers skip; a sentinel file written by apm install would make the trigger authoritative and harder to spoof by incidental file placement -- Design-level improvement; coordinate with install-side changes.

Architecture

classDiagram
    direction LR

    class ClaudeFormatter {
        <<Formatter>>
        +base_dir: Path
        +format_distributed(primitives, placement_map, config) ClaudeCompilationResult
        +_generate_placements(placement_map, primitives, source_attribution) list
        +_generate_claude_content(placement, primitives, skip_instructions) str
        +_compile_stats(emitted_placements, primitives) dict
        +_collect_dependencies() list
    }

    class ClaudePlacement {
        <<ValueObject>>
        +claude_path: Path
        +instructions: list[Instruction]
        +agents: list[Chatmode]
        +dependencies: list[str]
        +coverage_patterns: set[str]
        +source_attribution: dict
        +is_root: bool
    }

    class ClaudeCompilationResult {
        <<ValueObject>>
        +success: bool
        +placements: list[ClaudePlacement]
        +content_map: dict[Path, str]
        +warnings: list[str]
        +errors: list[str]
        +stats: dict
    }

    class AgentsCompiler {
        <<Orchestrator>>
        +base_dir: Path
        +compile(config) CompilationResult
        +_compile_claude_md(config, claude_formatter, distributed_compiler) CompilationResult
    }

    class ensure_path_within {
        <<IOBoundary>>
        +ensure_path_within(path, root) None
    }

    class PathTraversalError {
        <<Exception>>
    }

    class PrimitiveCollection {
        <<ValueObject>>
        +instructions: list[Instruction]
        +chatmodes: list[Chatmode]
        +count() int
    }

    class read_constitution {
        <<IOBoundary>>
        +read_constitution(base_dir) str
    }

    AgentsCompiler ..> ClaudeFormatter : delegates
    AgentsCompiler ..> ensure_path_within : guards symlink
    AgentsCompiler ..> PathTraversalError : catches
    ClaudeFormatter *-- ClaudePlacement : generates
    ClaudeFormatter ..> ClaudeCompilationResult : returns
    ClaudeFormatter ..> read_constitution : reads
    ClaudeFormatter ..> PrimitiveCollection : reads
    ClaudeCompilationResult *-- ClaudePlacement : contains emitted

    class ClaudePlacement:::touched
    class ClaudeFormatter:::touched
    class AgentsCompiler:::touched
    class ClaudeCompilationResult:::touched

    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A(["apm compile --target claude"]) --> B["_compile_claude_md()\nagents_compiler.py:552"]
    B --> C["[FS] claude_rules_dir = base_dir/.claude/rules\nis_dir() check"]
    C -->|"not a dir"| E["skip_instructions = False"]
    C -->|"is a dir"| D["[FS] ensure_path_within(claude_rules_dir, base_dir)\npath_security.py"]
    D -->|"PathTraversalError"| E2["log warning: symlink outside project\nskip_instructions stays False"]
    D -->|"safe"| F["[FS] claude_rules_dir.glob('*.md')"]
    F -->|"no .md files"| E
    F -->|"has .md files"| G["skip_instructions = True\nlog info: omitting from CLAUDE.md"]
    E --> H
    E2 --> H
    G --> H
    H["claude_config = {source_attribution, debug, skip_instructions}"]
    H --> I["ClaudeFormatter.format_distributed(primitives, placement_map, config)\nclaude_formatter.py:80"]
    I --> J["_generate_placements()\nsets is_root=True on root placement"]
    J --> K{{"skip_instructions?"}}
    K -->|"False"| L["generate content for ALL placements\n_generate_claude_content(skip_instructions=False)"]
    K -->|"True"| M{{"placement.is_root?"}}
    M -->|"not root"| N["skip -- no content for subdir placements"]
    M -->|"is root"| O{{"has deps OR constitution?"}}
    O -->|"no"| N
    O -->|"yes"| P["[FS] read_constitution()\n_generate_claude_content(skip_instructions=True)\nomits Project Standards section"]
    L --> Q
    P --> Q
    N --> Q
    Q["emitted_placements = [p for p in placements if p.claude_path in content_map]"]
    Q --> R["_compile_stats(emitted_placements)"]
    R --> S{{"dry_run?"}}
    S -->|"yes"| T["return preview: Would generate N file(s)"]
    S -->|"no"| U["[FS] write CLAUDE.md files\nfiles_written counter"]
    U --> V{{"files_written == 0 and skip_instructions?"}}
    V -->|"yes"| W["log info: CLAUDE.md not generated\nClaude Code reads .claude/rules/ directly"]
    V -->|"no"| X["CompilationFormatter output"]
    W --> Y(["CompilationResult returned"])
    X --> Y
    T --> Y
Loading
sequenceDiagram
    participant User
    participant CLI as apm compile --target claude
    participant Compiler as AgentsCompiler
    participant FS as Filesystem
    participant Formatter as ClaudeFormatter

    User->>CLI: apm compile --target claude
    CLI->>Compiler: compile(config)
    Compiler->>FS: .claude/rules/ is_dir()?
    alt rules dir exists with .md files
        FS-->>Compiler: True
        Compiler->>FS: ensure_path_within(.claude/rules/, base_dir)
        FS-->>Compiler: safe
        Compiler->>FS: glob(*.md) -> non-empty
        Compiler-->>Compiler: skip_instructions=True
        Compiler->>Formatter: format_distributed(..., skip_instructions=True)
        Formatter-->>Compiler: ClaudeCompilationResult(placements=[], content_map={})
        Compiler-->>User: [info] CLAUDE.md not generated -- Claude Code reads .claude/rules/ directly
    else no rules dir or empty
        FS-->>Compiler: False / empty
        Compiler->>Formatter: format_distributed(..., skip_instructions=False)
        Formatter->>FS: write CLAUDE.md with Project Standards
        Formatter-->>Compiler: ClaudeCompilationResult(placements=[root], content_map={CLAUDE.md: ...})
        Compiler-->>User: CLAUDE.md generated with instructions
    end
Loading

Recommendation

Fix the two doc-writer correctness issues (broken anchor, attribution wording) before or alongside merge -- they are small and the rendered docs will be wrong without them. The dry-run UX gap and integration test are the highest-value post-merge items; open tracking issues for both. Everything else is backlog. The core deduplication logic is sound and the unit coverage is thorough; this PR delivers real value to Claude Code users and should not be held for the follow-ups beyond the two doc corrections.


Full per-persona findings

Python Architect

  • [recommended] total_instructions_placed stat overcounts when skip_instructions=True and root CLAUDE.md is still emitted at src/apm_cli/compilation/claude_formatter.py:347
    _compile_stats sums len(p.instructions) over emitted_placements. When skip_instructions=True and a root placement IS emitted (because it has constitution or deps), its ClaudePlacement.instructions list is fully populated by _generate_placements but none of those instructions are rendered. The stat says 'placed' but the content was suppressed.
    Suggested: In _compile_stats, guard the count: total_instructions = sum(len(p.instructions) for p in placements if not skip_instructions) -- or pass skip_instructions into _compile_stats so it can zero the count.

  • [recommended] read_constitution called twice per compile when skip_instructions=True and root is emitted at src/apm_cli/compilation/claude_formatter.py:113
    format_distributed calls read_constitution(self.base_dir) to decide whether to emit the root placement; _generate_claude_content then calls it again for the same root placement. Two filesystem reads of the same file per invocation.
    Suggested: Cache: self._constitution_cache = read_constitution(self.base_dir) once at the top of format_distributed.

  • [nit] config dict now has 3 keys; a typed ClaudeFormatterConfig dataclass would make the contract IDE-discoverable at src/apm_cli/compilation/claude_formatter.py:84
    format_distributed accepts config: dict | None, now carrying source_attribution, debug, and skip_instructions. A frozen dataclass would surface missing keys at construction time and enable autocomplete.

CLI Logging Expert

  • [recommended] Dry-run + skip_instructions emits 'Would generate 0 files' with no dedup explanation at src/apm_cli/compilation/agents_compiler.py:604
    When .claude/rules/ is populated and --dry-run is passed, the skip_instructions [i] message fires, then 'CLAUDE.md Preview: Would generate 0 files' is emitted with no further context. A user who misses the preceding [i] line sees only '0 files' and may think compilation is broken.
    Suggested: When skip_instructions is True and count is 0: f"CLAUDE.md Preview: Would generate 0 files (instructions already in .claude/rules/ -- dedup active)"

  • [nit] Two consecutive [i] messages describe the same logical event (dedup skip) at src/apm_cli/compilation/agents_compiler.py:664
    Line 574-580 logs 'Instructions already in .claude/rules/...' at decision time; line 664-669 logs 'CLAUDE.md not generated...' after write. Consider collapsing to one outcome message: 'CLAUDE.md not generated -- instructions already deployed to .claude/rules/ (Claude Code reads them directly)'.

  • [nit] Symlink warning message uses passive 'ignoring' without naming the consequence at src/apm_cli/compilation/agents_compiler.py:565
    '.claude/rules/ is a symlink outside the project root -- ignoring' does not tell the user that skip_instructions stays False and CLAUDE.md will include full instructions.
    Suggested: '.claude/rules/ is a symlink outside the project root -- ignoring; CLAUDE.md will include full instructions'

DevX UX Expert

  • [recommended] Dry-run summary 'Would generate 0 files' gives no reason when skip_instructions is active at src/apm_cli/compilation/agents_compiler.py:604
    Any user who pipes or captures the content field sees 0 files with no explanation. A new user who expected a CLAUDE.md artifact and gets '0 files' has no copy-pasteable next action.
    Suggested: f'CLAUDE.md Preview: Would generate {count} {"file" if count == 1 else "files"} (instructions already in .claude/rules/ -- skipping)'

  • [nit] Two consecutive info logs say almost the same thing -- consider collapsing to one post-write message at src/apm_cli/compilation/agents_compiler.py:575

  • [nit] No explicit user guidance on how to force CLAUDE.md generation when skipped at src/apm_cli/compilation/agents_compiler.py:666
    When the skip fires, user is told 'no further action needed' but not how to opt out (e.g. by removing .claude/rules/). npm and cargo always tell you the inverse action.

Supply Chain Security Expert

  • [recommended] Any .md file in .claude/rules/ silently suppresses Project Standards from CLAUDE.md -- supply-chain artifact placement could exploit this at src/apm_cli/compilation/agents_compiler.py:571
    If a malicious or misconfigured dependency writes any .md artifact into .claude/rules/ during apm install, the next apm compile will set skip_instructions=True and omit the Project Standards section. The suppression trigger (any *.md present) is coarser than an APM-managed marker.
    Suggested: Consider writing a sentinel file (e.g. .claude/rules/.apm-managed) during apm install and checking for that file specifically rather than any *.md.

  • [nit] Glob after containment check has a narrow TOCTOU window on the resolved directory at src/apm_cli/compilation/agents_compiler.py:559
    .is_dir() (symlink-following) and ensure_path_within (also symlink-resolving) both independently follow the link, creating a race window where a symlink could be swapped between the two calls.
    Suggested: Resolve and capture the result of ensure_path_within first, then call .is_dir() on the resolved path.

OSS Growth Hacker

  • [recommended] Docs note omits the context-window cost angle -- the most compelling hook for Claude Code users at docs/src/content/docs/producer/compile.md:140
    The note explains the mechanism but never quantifies the pain: 'duplicate instructions waste Claude's context window and cost you tokens on every request.' That framing is what turns a fix into a story users share.
    Suggested: Expand the note opener: 'When apm install has already deployed instructions to .claude/rules/, running apm compile --target claude would otherwise write the same instructions into CLAUDE.md -- consuming context window tokens on every Claude Code session. APM detects this and omits the duplicate section automatically.'

  • [recommended] CHANGELOG entry buries the user benefit inside a mechanism description at CHANGELOG.md:47
    OSS CHANGELOG entries work as micro-announcements; the hook should lead with the benefit, not the mechanism.
    Suggested: 'Claude Code users who run both apm install and apm compile --target claude no longer pay a context-window cost from duplicate instructions. CLAUDE.md is still generated for constitution and @import paths. ([FEATURE] Skip instructions in CLAUDE.md when already deployed to .claude/rules/ #1138)'

  • [nit] Docs note does not link to the 'apm install' reference page -- first mention of apm install in the note is plain code without a cross-link to help new users.

Auth Expert -- inactive

No auth files touched; PR only changes Claude Code compilation deduplication logic with no impact on token management, credential resolution, or AuthResolver.

Doc Writer

  • [recommended] Fragment anchor #claude-code-deduplication is broken -- Starlight does not generate anchors from :::note[Title] callouts at docs/src/content/docs/producer/compile.md:118
    The table cell links to #claude-code-deduplication. Starlight only emits fragment IDs for ATX headings (## ...). The in-page link will silently 404 in the rendered docs.
    Suggested: Add ### Claude Code deduplication immediately before the note, or use <a id="claude-code-deduplication"></a> directly above the callout.

  • [recommended] Deduplication note attributes .claude/rules/ population only to apm install -- but apm compile --target claude also writes there at docs/src/content/docs/producer/compile.md:140
    A producer who only runs apm compile will populate .claude/rules/ on the first run, then on the second run the deduplication logic fires and omits instructions from CLAUDE.md with no docs explanation.
    Suggested: 'When .claude/rules/ already contains deployed instructions -- whether written by apm install or by a previous apm compile --target claude run -- apm compile --target claude automatically omits the instructions section from CLAUDE.md...'

  • [nit] Table cell text for the claude 'Compile required?' column is verbose at docs/src/content/docs/producer/compile.md:118
    Suggested: Trim to: Conditional -- see [Claude Code deduplication](#claude-code-deduplication)

Test Coverage Expert

  • [recommended] No integration test for install->compile deduplication pipeline at tests/integration/test_compile_skip_instructions_e2e.py
    The user-visible promise of this PR is a cross-module flow (install pipeline -> compiler -> formatter) and the unit tests only exercise the compiler in isolation. Grepped tests/integration/ for 'skip_instructions', 'CLAUDE.md not generated', 'omit.*CLAUDE', 'rules.*already' -- zero matches. Floor tier for cross-module CLI surface is integration-with-fixtures.
    Proof (missing): tests/integration/test_compile_skip_instructions_e2e.py::test_compile_claude_skips_project_standards_when_rules_deployed -- proves: After apm install deploys rules to .claude/rules/, apm compile --target claude does not duplicate those instructions in CLAUDE.md [devx, portability-by-manifest]

  • [nit] Unit test suite is well-structured and covers all skip_instructions logic branches -- TestClaudeCompileSkipInstructions has 9 tests (no rules dir, populated rules dir, empty rules dir, non-.md files, dry-run, stats, log messages via mock_logger.progress, symlink-outside-project). mock_logger.progress.call_args_list approach is correct. No structural issue found.

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1146 · ● 3.7M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 13, 2026
@sergio-sisternes-epam sergio-sisternes-epam added the status/triaged Initial agentic triage complete; pending maintainer ratification (silence = approval). label May 13, 2026
Daniel Meppiel and others added 2 commits May 14, 2026 17:37
CI:
- ruff format on test_agents_compiler_coverage.py and
  test_claude_formatter.py (Lint job was failing on format --check)
- Fix broken docs anchor #claude-code-deduplication in
  producer/compile.md (Deploy Docs / build was failing on
  starlight-links-validator)

PR microsoft#1146 review-panel follow-ups:
- Doc Writer microsoft#1: add explicit <a id> anchor before the
  :::note[]:::callout so the table link resolves (Starlight does
  not auto-generate anchors from callout directives)
- Doc Writer microsoft#2: rewrite the dedup note to attribute
  .claude/rules/ population to BOTH apm install AND apm compile
  (a producer who only runs compile hits dedup on the second run
  and the docs previously gave them no explanation)
- CLI Logging microsoft#4 / DevX UX: dry-run preview now appends a
  '(instructions section skipped: .claude/rules/ already
  populated...)' line whenever skip_instructions fires, so
  scripted consumers and novice users no longer see a bare
  'Would generate 0 files' with no why
- Test Coverage microsoft#3: add tests/integration/
  test_install_compile_claude_dedup_e2e.py -- exercises the
  install->compile pipeline through the real CLI to lock in the
  cross-module dedup contract; second test pins the
  compile-alone twice path

Verified locally:
- ruff check + ruff format --check both silent
- Full unit suite: 8311 passed
- npm run build inside docs/: 'All internal links are valid'

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/triaged Initial agentic triage complete; pending maintainer ratification (silence = approval).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Skip instructions in CLAUDE.md when already deployed to .claude/rules/

4 participants