Skip to content

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

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

fix(compile): deduplicate Claude Code instructions (#1138)#1146
tillig wants to merge 17 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

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
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
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.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

.

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.8M ·

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

APM Review Panel: needs_rework

PR #1146 delivers APM-native Claude Code coordination: compile skips redundant Project Standards when .claude/rules/ is populated, but three correctness gaps (stale doc labels, OSError path bug, missing escape hatch) need resolution before merge.

cc @danielmeppiel -- a fresh advisory pass is ready for your review.

The feature is strategically correct and well-motivated. APM is the first package manager to detect Claude Code's native rules directory and adjust its compile output accordingly -- that coordination story is defensible and worth amplifying. Unit test coverage across 14 cases is solid, and the CHANGELOG and doc additions are present. The panel agrees unanimously on feature value; the disagreement is entirely about completeness.

Three convergent findings rise to resolve-before-merge: (1) The OSError fallback in claude_formatter.py line 120 compares an unresolved path against a resolved self.base_dir, silently mislabeling non-root placements and potentially dropping the root CLAUDE.md -- flagged independently by the Python Architect, Supply Chain Security, and Test Coverage personas; this is a latent correctness bug in the same PR that introduced it. (2) The doc-writer found three stale "instructions only" labels for CLAUDE.md in the same files this PR touches -- these contradict the new behavior on shipping day. (3) Supply-chain-security confirmed Path.is_dir() follows symlinks, so a symlinked .claude/rules/ outside the project root silently triggers skip_instructions without apm install ever having run; path_security.ensure_path_within is the existing chokepoint and should be applied here.

The remaining recommended findings (escape hatch flag, integration fixture test, missing positive log, CHANGELOG framing, doc deduplication) are post-merge follow-ups. The escape hatch is a genuine UX gap but not a correctness regression. The integration test gap is real but the unit tier already exercises the decision logic.

Dissent. No panelist disagreed on whether the feature should ship. The only weighting call is whether the OSError path bug and symlink trust gap rise above "recommended" severity. They are upgraded to resolve-before-merge because they are introduced by this PR, affect the same files being changed, and have concrete failure modes (silent root CLAUDE.md suppression; ambient symlink triggering skip without apm install). The escape hatch and integration test remain post-merge follow-ups.

Aligned with: Portable by Manifest (compile remains manifest-driven; symlink fix keeps it bounded to the project root), Secure by Default (symlink boundary check and trust-boundary note are pre-merge; lockfile cross-reference is a follow-up), Governed by Policy (deduplication is policy-invisible today; escape hatch flag closes this gap post-merge), Multi-Harness/Multi-Host (feature is Claude Code-specific by design; integration fixture test will confirm this boundary), Pragmatic as npm (automatic deduplication is npm-idiomatic, but npm provides override flags; escape hatch follow-up closes that gap).

Growth signal. The oss-growth-hacker framing upgrade is worth acting on: "APM coordinates with .claude/rules/ so Claude Code always gets clean, non-redundant context" is a shareable hook; "deduplication fix" is not. A short README callout or quickstart addition featuring the apm install -> apm compile coordination loop as a two-command flow for Claude Code users would convert this PR into a concrete acquisition moment for the Claude Code developer segment. Update CHANGELOG framing in this PR; add a README callout in a fast-follow.

Panel summary

Persona B R N Takeaway
Python Architect 0 2 1 Solid, proportionate change; one recommended fix for the OSError fallback path comparison; two nits on double read_constitution I/O and instance-state reset ordering.
CLI Logging Expert 0 1 2 Two new [i] skip messages are ASCII-clean and use correct symbol; one coverage gap: when CLAUDE.md IS generated despite skip_instructions, no message tells user what was included.
DevX UX Expert 0 3 1 Implicit filesystem-triggered behavior change silently drops CLAUDE.md; cli-commands.md not updated; no user escape hatch.
Supply Chain Security 0 3 1 Two trust-boundary gaps: unvalidated ambient files trigger silent instruction suppression; symlink-followed rules dir not bounded to project root. Both recommended fixes before ship.
OSS Growth Hacker 0 1 1 Strong Claude Code integration story; CHANGELOG and docs are solid. One recommended framing upgrade to convert a technical fact into a shareable hook.
Doc Writer 0 3 2 Three stale 'instructions only' references remain after the CLAUDE.md behavior change; deduplication note is duplicated verbatim across two files violating PROSE single-source rule; missing edge case for rules deletion.
Test Coverage Expert 0 1 1 Unit coverage is solid across 14 tests; one OSError fallback branch is untested (nit); integration-tier compile+skip pipeline has no fixture test (recommended).

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

Top 5 follow-ups

  1. [Python Architect] Fix OSError fallback path comparison: use .absolute() not bare == against resolved self.base_dir -- Independently confirmed by python-architect, supply-chain-security, and test-coverage. Latent correctness bug introduced in this PR; silent failure mode (root CLAUDE.md suppressed).
  2. [Supply Chain Security] Bound .claude/rules/ symlink resolution to project root using path_security.ensure_path_within before setting skip_instructions=True -- Path.is_dir() follows symlinks; an out-of-project symlink silently triggers instruction suppression without apm install having run. path_security is the existing chokepoint.
  3. [Doc Writer] Fix three stale "instructions only" labels for CLAUDE.md in compilation.md and ide-tool-integration.md -- Labels directly contradict the new behavior in the same PR. Ships as a documentation regression on day one.
  4. [Test Coverage Expert] Add integration fixture test S15b: compile with .claude/rules/ populated, assert Project Standards absent and dependencies/constitution present -- Integration tier is missing for the cross-module pipeline (agents_compiler -> claude_formatter -> file I/O). Missing test on a behavior-change surface is a regression-trap gap.
  5. [DevX UX Expert] Add --no-deduplicate-rules (or --include-instructions) flag to apm compile; update cli-commands.md in the same PR -- Deduplication is unconditional. Users who want CLAUDE.md Project Standards alongside .claude/rules/ have no escape hatch. npm/cargo/pip precedent is explicit override flags for automatic behavior.

Architecture

classDiagram
    direction LR

    class ClaudeFormatter {
      <<Formatter>>
      +base_dir Path
      +warnings list
      +errors list
      -_skip_instructions bool
      +format_distributed(primitives, placement_map, config) ClaudeCompilationResult
      -_generate_placements(...) list
      -_generate_claude_content(placement, primitives) str
      -_compile_stats(placements, primitives) dict
    }

    class AgentsCompiler {
      <<Orchestrator>>
      +base_dir Path
      +_compile_claude_md(primitives, placement_map, config) dict
      -_log(level, msg, symbol) void
    }

    class ClaudePlacement {
      <<ValueObject>>
      +claude_path Path
      +instructions list
      +dependencies list
      +coverage_patterns set
      +source_attribution dict
    }

    class ClaudeCompilationResult {
      <<ValueObject>>
      +success bool
      +placements list
      +content_map dict
      +warnings list
      +errors list
      +stats dict
    }

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

    AgentsCompiler *-- ClaudeFormatter : creates and calls
    ClaudeFormatter ..> ClaudePlacement : consumes
    ClaudeFormatter ..> ClaudeCompilationResult : produces
    ClaudeCompilationResult *-- ClaudePlacement : holds emitted_placements

    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A(["apm compile --target claude"]) --> B["AgentsCompiler._compile_claude_md"]
    B --> C{"[FS] .claude/rules/ exists\nand contains *.md?"}
    C -- No --> D["skip_instructions = False"]
    C -- Yes --> E["skip_instructions = True"]
    D --> H["ClaudeFormatter.format_distributed\nconfig={skip_instructions: False}"]
    E --> G["[i] log: skipping from CLAUDE.md"]
    G --> H
    H --> I["_generate_placements -> all placements"]
    I --> J{"skip_instructions?"}
    J -- No --> K["emit all placements"]
    J -- Yes --> L{"is_root placement?"}
    L -- No --> M["skip subdirectory"]
    L -- Yes --> N{"constitution or\ndependencies present?"}
    N -- No --> O["skip root placement"]
    N -- Yes --> P["_generate_claude_content\n(instructions block skipped)"]
    K --> Q["_generate_claude_content\n(instructions block included)"]
    P --> R["content_map[claude_path] = content"]
    Q --> R
    R --> S["emitted_placements filter"]
    S --> T["_compile_stats(emitted_placements)"]
    T --> U["ClaudeCompilationResult"]
    U --> V{"files_written==0\nand skip_instructions?"}
    V -- Yes --> W["[i] log: CLAUDE.md not generated"]
    V -- No --> X["write CLAUDE.md files"]
    W --> Y([done])
    X --> Y
Loading

Recommendation

Resolve three in-PR defects before merge: (1) fix the OSError fallback path comparison in claude_formatter.py line 120 (absolute() not bare ==), (2) apply path_security.ensure_path_within to the .claude/rules/ symlink resolution before setting skip_instructions=True, and (3) correct the three stale "instructions only" doc labels in compilation.md and ide-tool-integration.md. All remaining panel findings -- integration fixture test, escape hatch flag, logging gap, CHANGELOG reframe, doc deduplication, instance-state refactor -- are post-merge follow-ups and should be tracked as issues. The feature direction is sound; these are correctness and trust-boundary defects introduced in the same changeset.


Full per-persona findings

Python Architect

  • [recommended] OSError fallback in is_root check compares unresolved path against resolved self.base_dir -- always False on relative paths at src/apm_cli/compilation/claude_formatter.py:120
    In format_distributed, when placement.claude_path.parent.resolve() raises OSError, the fallback is placement.claude_path.parent == self.base_dir. self.base_dir is always resolved (set in init via Path.resolve()), but placement.claude_path.parent may be an unresolved path with relative or symlinked segments. The equality check will be False whenever the caller passes a non-absolute ClaudePlacement path, silently skipping the root CLAUDE.md that should be emitted.
    Suggested: except OSError:\n is_root = placement.claude_path.parent.absolute() == self.base_dir
  • [recommended] _skip_instructions as instance state mutated inside format_distributed violates single-responsibility and is not reset before early-exit paths at src/apm_cli/compilation/claude_formatter.py:102
    ClaudeFormatter accumulates instance state (warnings, errors, _skip_instructions) across calls. If _generate_placements raises and the except block fires, _skip_instructions is left at its previous value for the next call.
    Suggested: Remove self._skip_instructions; thread the flag as a parameter to _generate_claude_content.
  • [nit] read_constitution(self.base_dir) is called up to N+1 times per format_distributed call -- hoist the result at src/apm_cli/compilation/claude_formatter.py:122

CLI Logging Expert

  • [recommended] No log message when skip_instructions=True but CLAUDE.md is still generated (constitution/deps present) at src/apm_cli/compilation/agents_compiler.py:646
    When skip_instructions=True and files_written > 0, the user sees '[i] Instructions already in .claude/rules/ -- skipping from CLAUDE.md' but receives a CLAUDE.md with no log explaining what WAS included.
    Suggested: if files_written > 0 and skip_instructions: self._log('progress', 'CLAUDE.md generated (constitution/dependencies only -- instructions deferred to .claude/rules/)', symbol='info')
  • [nit] 'CLAUDE.md not generated' message passes no actionable hint for users who expected output at src/apm_cli/compilation/agents_compiler.py:647
  • [nit] Replacement comment in claude_formatter.py drops the 'Project Standards' domain label at src/apm_cli/compilation/claude_formatter.py

DevX UX Expert

  • [recommended] cli-commands.md not updated with deduplication behavior at docs/src/content/docs/reference/cli-commands.md
    The canonical CLI reference has no mention of the new skip-instructions behavior. Per the DevX UX persona contract: if a CLI change is not reflected in cli-commands.md in the same PR, that change is incomplete.
  • [recommended] CLAUDE.md silently disappears with no user-visible warning when fully suppressed at src/apm_cli/compilation/agents_compiler.py
    When skip_instructions=True and there is no constitution or dependencies, CLAUDE.md is not written at all. A user who previously had CLAUDE.md will see it vanish with no actionable explanation.
  • [recommended] No escape hatch for users who want CLAUDE.md Project Standards alongside .claude/rules/
    The deduplication is unconditional. There is no --force or --include-instructions flag.
    Suggested: Add --no-deduplicate-rules (or --include-instructions) flag to apm compile.
  • [nit] Dry-run 'Would generate 0 files' gives no hint why zero files are expected at tests/unit/compilation/test_agents_compiler_coverage.py

Supply Chain Security

  • [recommended] Any .md file in .claude/rules/ suppresses Project Standards -- no proof of APM provenance required at src/apm_cli/compilation/agents_compiler.py
    agents_compiler.py checks only that .claude/rules/*.md exists. A malicious or confused dependency whose install writes any .md to .claude/rules/ will silently suppress the Project Standards section. This is a trust-boundary gap.
    Suggested: Cross-reference discovered .md files against the lockfile (apm.lock.yaml). At minimum, log a warning citing which files caused suppression. Stronger fix: write a sentinel marker .apm-managed during apm install and check for that instead.
  • [recommended] claude_rules_dir.is_dir() follows symlinks; a symlinked .claude/rules/ outside the project root can trigger suppression at src/apm_cli/compilation/agents_compiler.py
    Path.is_dir() follows symlinks. If .claude/rules is a symlink pointing outside the project, skip_instructions becomes True without the user having run apm install. path_security.ensure_path_within is the existing chokepoint and is bypassed here.
    Suggested: Apply path_security.ensure_path_within(self.base_dir, claude_rules_dir.resolve()) before using the directory.
  • [recommended] resolve() in OSError fallback path compares unresolved path to resolved base_dir -- may silently mismatch at src/apm_cli/compilation/claude_formatter.py
    Inconsistent path comparison is the precursor to path-confusion bugs.
  • [nit] Silent skip_instructions=True produces no user-visible audit trail at src/apm_cli/compilation/agents_compiler.py

OSS Growth Hacker

  • [recommended] CHANGELOG and docs frame this as a deduplication fix rather than a coordination feature -- the stickier growth hook is buried at CHANGELOG.md:16
    The user-visible promise is 'APM and Claude Code share a single source of truth for instructions -- no more bloated context windows'. 'Deduplication' reads as a bug fix; 'APM coordinates with .claude/rules/ so Claude Code always gets clean, non-redundant context' reads as a feature.
  • [nit] Neither doc callout surfaces a one-liner example that a new user can immediately run to verify the behavior at docs/src/content/docs/integrations/ide-tool-integration.md:182

Auth Expert -- inactive

No auth-related files touched; changed files are limited to CHANGELOG.md, docs/compilation.md, docs/ide-tool-integration.md, agents_compiler.py, claude_formatter.py, and their unit tests -- none of which intersect auth.py, token_manager.py, or any credential/host-resolution surface.

Doc Writer

  • [recommended] Deduplication note duplicated verbatim in two files, violating PROSE single-source rule at docs/src/content/docs/guides/compilation.md
    The deduplication behavior block appears word-for-word in both compilation.md and ide-tool-integration.md. PROSE requires a concept to live in exactly one place with a cross-reference from the other.
    Suggested: Keep canonical note in compilation.md; in ide-tool-integration.md replace with a one-sentence cross-reference.
  • [recommended] Three 'instructions only' labels for CLAUDE.md are now stale after this PR at docs/src/content/docs/guides/compilation.md
    Three locations still say 'instructions only': (1) auto-detection table row, (2) Example Output comment, (3) ide-tool-integration.md code comment. These contradict the new behavior.
    Suggested: Change 'CLAUDE.md (instructions only)' to 'CLAUDE.md'; update comments to 'Instructions for Claude (may include constitution and imports)'.
  • [recommended] Missing edge case: re-running compile after .claude/rules/ is deleted at docs/src/content/docs/guides/compilation.md
    If a user deletes .claude/rules/ and recompiles, instructions section reappears in CLAUDE.md. This roundtrip is undocumented.
    Suggested: Add: 'If .claude/rules/ is later removed, re-running apm compile --target claude will restore the instructions section to CLAUDE.md.'
  • [nit] Deduplication callout uses blockquote format instead of idiomatic Starlight callout syntax at docs/src/content/docs/guides/compilation.md
  • [nit] CHANGELOG PR reference ([FEATURE] Skip instructions in CLAUDE.md when already deployed to .claude/rules/ #1138) does not match the PR number (fix(compile): deduplicate Claude Code instructions (#1138) #1146) at CHANGELOG.md
    Verify whether [FEATURE] Skip instructions in CLAUDE.md when already deployed to .claude/rules/ #1138 is the originating issue or a typo. If a typo, correct to (fix(compile): deduplicate Claude Code instructions (#1138) #1146).

Test Coverage Expert

  • [recommended] No integration-with-fixtures test exercises apm compile --target claude skipping Project Standards when .claude/rules/ is populated at tests/integration/test_target_resolution_e2e.py
    The tier-floor matrix requires integration-with-fixtures for CLI command surface and cross-module integration. The compile pipeline spans agents_compiler.py -> claude_formatter.py -> file I/O; unit tests certify each module in isolation but not their composition. Grepped tests/integration/ for skip_instructions, Project Standards skip, and compile+rules patterns -- zero hits.
    Suggested: Add scenario S15b: set up .claude/rules/existing-rule.md, run apm compile --target claude, assert CLAUDE.md does NOT contain Project Standards section and DOES contain dependencies and constitution sections.
    Proof (missing at integration-with-fixtures): tests/integration/test_target_resolution_e2e.py::test_s15b_compile_skips_project_standards_when_rules_populated -- proves: When .claude/rules/ has .md files, apm compile --target claude omits the Project Standards section from CLAUDE.md without dropping dependencies or constitution. [devx,multi-harness-support]
    assert 'Project Standards' not in claude_md_content\nassert 'dependencies' in claude_md_content
  • [nit] OSError fallback path in ClaudeFormatter is_root resolution (line 118-119) has no test at tests/unit/compilation/test_claude_formatter.py
    Grepped both test files for 'resolve', 'OSError', and 'PermissionError' -- zero hits.
    Suggested: Add a test that patches Path.resolve to raise OSError and confirms the fallback correctly identifies root vs non-root placement.
    Proof (missing at unit): tests/unit/compilation/test_claude_formatter.py::TestSkipInstructions::test_skip_instructions_is_root_oserror_fallback -- proves: When Path.resolve() raises OSError, the is_root fallback still correctly identifies root placements. [devx]

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.8M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 7, 2026
@tillig tillig requested a review from Copilot May 8, 2026 15:57
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 3 comments.

Comment thread src/apm_cli/compilation/agents_compiler.py
Comment thread tests/unit/compilation/test_agents_compiler_coverage.py Outdated
Comment thread tests/unit/compilation/test_claude_formatter.py Outdated
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 2 comments.

Comment thread tests/unit/compilation/test_claude_formatter.py Outdated
Comment thread tests/unit/compilation/test_agents_compiler_coverage.py
@tillig
Copy link
Copy Markdown
Contributor Author

tillig commented May 21, 2026

@danielmeppiel Sorry to be a bother, but with additional challenges coming up around this like #1445 and #1447 I'm curious if there's something extra I need to do with this deduplication PR. I am trying to keep it up to date with main but I'm not positive if you're waiting on me for something else here. I think all the issues raised have been resolved. No rush or anything, just checking to make sure I'm not missing anything.

@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator

APM Review Panel: ship_with_followups

Community-contributed context-window dedup for Claude Code: focused fix, strong tests, clean architecture -- ship with minor follow-ups.

cc @tillig -- a fresh advisory pass is ready for your review.

All eight active panelists converge on the same signal: this is a well-scoped, well-tested community contribution that solves a real user pain point. The python-architect confirms correct separation of concerns (detection in AgentsCompiler, formatting in ClaudeFormatter). The supply-chain-security-expert validates the symlink guard and correctly notes the TOCTOU window is not practically exploitable. The test-coverage-expert confirms all 15 unit tests pass and the primary dedup contract is defended at integration tier. No panelist raised a blocking finding.

The three recommended-severity findings are genuine polish items but none threaten correctness or security. The cli-logging-expert's symlink warning routing through _rich_info instead of _rich_warning is the most actionable: a security-relevant message rendered in blue instead of yellow is a minor UX misclassification, not a vulnerability. The devx-ux-expert's request for a --force/--no-dedup escape hatch is a reasonable future enhancement but would expand the scope of this already-focused PR; it belongs in a follow-up issue. The doc-writer's table density concern is editorial polish. The test-coverage-expert's marker finding is a CI hygiene item that does not affect test correctness.

One factual correction: the supply-chain-security-expert noted 'no unit test for the symlink-outside-project branch,' but the PR's test suite does include test_skip_instructions_ignores_symlink_outside_project. The finding is moot and I have excluded it from follow-ups.

Dissent. Supply-chain-security-expert claimed symlink branch was untested; the test suite includes test_skip_instructions_ignores_symlink_outside_project, making this a factual miss rather than a genuine gap. No other panelist disagreements.

Aligned with: Portable by manifest -- dedup is harness-scoped (Claude Code only), preserving multi-harness portability. Secure by default -- symlink traversal guard uses canonical ensure_path_within. Multi-harness/multi-host -- ClaudePlacement.is_root cleanly separates harness-specific dedup. OSS community-driven -- exemplary community PR from a repeat contributor. Pragmatic as npm -- dedup is automatic and invisible.

Growth signal. This PR is a textbook community contribution story: a repeat external contributor (tillig) identified a real friction point in the most popular harness workflow, shipped a focused fix with 15+ tests and a docs update, and followed every project convention. The narrative -- 'community contributor optimizes Claude Code context window efficiency' -- is worth amplifying in release notes and social channels.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 2 Clean, well-scoped change. Dedup detection lives in AgentsCompiler, formatting in ClaudeFormatter -- correct separation of concerns.
CLI Logging Expert 0 1 3 New log messages are well-crafted. One routing bug (warning rendered as blue info), one verbose-mode gap, and minor wording nits.
DevX UX Expert 0 1 2 Solid UX. Dedup behavior is intuitive, well-logged, and reversible. No opt-out flag and heuristic limitation are minor gaps.
Supply Chain Security Expert 0 0 1 Symlink check correctly uses canonical ensure_path_within guard. TOCTOU window is not practically exploitable. Minimal security surface.
OSS Growth Hacker 0 0 3 Excellent community contribution from a repeat contributor. Fixes a real pain point with exemplary structure.
Doc Writer 0 1 3 Docs changes are well-scoped and accurate. Table cell is overloaded; admonition placement could be tighter.
Test Coverage Expert 0 1 1 Strong coverage across unit and integration tiers; 15 unit tests pass, integration regression trap passes. Ship.

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

Top 4 follow-ups

  1. [CLI Logging Expert] Route symlink warning through warning()/_rich_warning (yellow) instead of progress()/_rich_info (blue). -- Security-relevant message is visually indistinguishable from ordinary info output. One-line fix, high signal-to-noise improvement.
  2. [Test Coverage Expert] Add pytest.mark.integration marker to the new integration test file. -- CI convention compliance ensures the test runs in the correct split.
  3. [DevX UX Expert] Open a follow-up issue for a --no-dedup / --force-instructions escape hatch on apm compile. -- Power users who intentionally duplicate instructions have no override. Low urgency but completes the UX contract.
  4. [Doc Writer] Trim the 'Compile required?' table cell and let the admonition carry the detailed explanation. -- Dense markdown table cells render poorly on narrow viewports.

Architecture

classDiagram
    direction LR
    class AgentsCompiler {
        +base_dir Path
        +compile(config, primitives, logger) CompilationResult
        -_compile_claude_md(config, primitives) CompilationResult
        -_log(method, message, **kwargs)
    }
    class CompilationConfig {
        &lt;&lt;ValueObject&gt;&gt;
        +target CompileTargetType
        +dry_run bool
        +source_attribution bool
        +debug bool
    }
    class ClaudeFormatter {
        +base_dir Path
        +format_distributed(primitives, placement_map, config) ClaudeCompilationResult
        -_generate_placements(placement_map, primitives) list~ClaudePlacement~
        -_generate_claude_content(placement, primitives, skip_instructions) str
        -_collect_dependencies() list~str~
        -_compile_stats(placements, primitives) dict
    }
    class ClaudePlacement {
        &lt;&lt;ValueObject&gt;&gt;
        +claude_path Path
        +instructions list~Instruction~
        +agents list~Chatmode~
        +dependencies list~str~
        +is_root bool
    }
    class ClaudeCompilationResult {
        &lt;&lt;ValueObject&gt;&gt;
        +success bool
        +placements list~ClaudePlacement~
        +content_map dict~Path str~
        +warnings list~str~
        +stats dict~str float~
    }
    class DistributedAgentsCompiler {
        +analyze_directory_structure(instructions) dict
        +determine_agents_placement(instructions, dir_map) dict
    }
    class PathTraversalError {
        &lt;&lt;Exception&gt;&gt;
    }
    AgentsCompiler *-- CompilationConfig : reads
    AgentsCompiler ..> ClaudeFormatter : delegates formatting
    AgentsCompiler ..> DistributedAgentsCompiler : delegates placement
    AgentsCompiler ..> PathTraversalError : catches
    ClaudeFormatter ..> ClaudePlacement : creates
    ClaudeFormatter ..> ClaudeCompilationResult : returns
    class ClaudePlacement:::touched
    class ClaudeFormatter:::touched
    class AgentsCompiler:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A["apm compile --target claude"] --> B["AgentsCompiler._compile_claude_md()"]
    B --> C{"[FS] .claude/rules/ exists?"}
    C -->|No| E["skip_instructions = False"]
    C -->|Yes| D{"[FS] ensure_path_within()"}
    D -->|PathTraversalError| F["Log warning, skip = False"]
    D -->|OK| G{"[FS] glob '*.md' any files?"}
    G -->|No| E
    G -->|Yes| H["skip_instructions = True"]
    E --> I["DistributedAgentsCompiler"]
    H --> I
    I --> J["ClaudeFormatter.format_distributed"]
    J --> L{"skip_instructions?"}
    L -->|False| M["Generate ALL placements"]
    L -->|True| N{"is_root AND has constitution/deps?"}
    N -->|Yes| O["Generate root only, skip Project Standards"]
    N -->|No| P["Placement skipped"]
    M --> Q{"dry_run?"}
    O --> Q
    P --> Q
    Q -->|Yes| R["Preview summary"]
    Q -->|No| S["Write CLAUDE.md files"]
Loading

Recommendation

Merge this PR as-is. The four follow-ups above are all post-merge polish -- none affect correctness, security, or user-facing contracts. The symlink warning color and pytest marker are the most actionable and could land in the same release cycle as small companion PRs. The contributor deserves a fast, clean merge experience; dragging this through another review round for nits would be a poor signal to the community.


Full per-persona findings

Python Architect

  • [nit] read_constitution called up to 3 times per compile: once in _generate_placements (empty-map branch), once as has_constitution in format_distributed, and once inside _generate_claude_content
    Each call reads the same file from disk. At the current scale this has zero user-visible cost. A future cleanup could cache the result.
  • [nit] skip_instructions plumbed via untyped config: dict -- consider a keyword arg or typed config for discoverability
    Matches the existing pattern but a typed config would give type-checker coverage. Not worth changing in this PR.

CLI Logging Expert

  • [recommended] Symlink warning routes through progress() -> _rich_info (blue) instead of warning() -> _rich_warning (yellow)
    At agents_compiler.py ~L567, self._log("progress", "...symlink outside the project root -- ignoring", symbol="warning") passes the warning symbol but dispatches through progress(), rendering in blue. Fix: change first argument from "progress" to "warning".
  • [nit] Dedup detection and reasoning should surface under --verbose
    The explanatory message is always emitted; consider gating behind verbose_detail.
  • [nit] Dry-run parenthetical is long; consider tightening
    19 words in a parenthetical; a 7-word version says the same thing.
  • [nit] Singular/plural fix on preview line is good but may be inconsistent with AGENTS.md path
    Verify the AGENTS.md preview path uses the same pattern.

DevX UX Expert

  • [recommended] No --force or --no-dedup escape hatch to override the auto-detection
    A user who intentionally wants instructions in both locations has no override. The only workaround is deleting .claude/rules/ before compile.
  • [nit] Heuristic treats any .md file in .claude/rules/ as APM-managed instructions
    Cannot distinguish APM-placed files from user-created ones. Acceptable for now since .claude/rules/ is APM-managed.
  • [nit] Dry-run parenthetical explanation indented with two spaces, inconsistent with typical CLI dry-run output

Supply Chain Security Expert

  • [nit] No unit test for the symlink-outside-project branch of the skip logic
    Note: test_skip_instructions_ignores_symlink_outside_project does exist in the PR's test suite. This finding is a factual miss.

OSS Growth Hacker

  • [nit] CHANGELOG entry slightly too dense for release-note reuse
    Consider condensing for social/release announcements.
  • [nit] Story angle worth surfacing: community-contributed fix for context-window efficiency
    Strong narrative for release notes and contributor recognition.
  • [nit] Docs deduplication note could link back to the quickstart or install guide

Auth Expert -- inactive

PR touches only compilation logic, no auth files.

Doc Writer

  • [recommended] Table cell for claude "Compile required?" is too dense for a markdown table column
    ~25 words in a cell where every other row uses 5-10 words. Recommend trimming to 'Conditional (see deduplication note)'.
  • [nit] Raw HTML anchor tag inside Starlight admonition
    Starlight generates heading anchors automatically; the raw <a> tag is a fragile workaround.
  • [nit] Admonition placement between two unrelated sections is slightly orphaned
    Consider placing directly under the table it annotates.
  • [nit] CHANGELOG entry may use smart quotes instead of straight quotes

Test Coverage Expert

  • [nit] Compile-twice integration test self-skips, leaving second dedup trigger path untested e2e
    The primary install-then-compile path IS tested and passes. The gap is narrow.
    Proof (unknown at): tests/integration/test_install_compile_claude_dedup_e2e.py::test_compile_alone_then_compile_again_skips_on_second_run -- proves: Running compile twice deduplicates instructions on the second pass even without install [DevX]
  • [recommended] Integration test file missing pytest.mark.integration marker per codebase convention
    Missing marker means the test may run in the wrong CI split.
    Proof (missing at): tests/integration/test_install_compile_claude_dedup_e2e.py::test_install_then_compile_skips_duplicated_instructions -- proves: CI marker convention compliance so test runs in correct split [DevX]

Performance Expert -- inactive

No cache, dep resolution, fetch, materialization, lockfile, or install pipeline code touched.

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

___BEGIN___COMMAND_DONE_MARKER___0

@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator

Follow-ups from the apm-review-panel pass have landed. Summary:

  • cli-symlink-warning-color: Route symlink warning through warning() instead of progress() -- resolved in 6d7c424.
  • integration-test-marker: Add pytest.mark.integration markers to dedup e2e tests -- resolved in 6d7c424.
  • doc-table-cell-density: Trim compile table cell for claude row; let the admonition carry detail -- resolved in 6d7c424.
  • no-dedup-escape-hatch: Open follow-up issue for --no-dedup / --force-instructions flag -- deferred to feat(compile): add --no-dedup / --force-instructions flag for claude target #1463.

Lint contract: uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both silent.

CI: All checks successful (1/1 -- license/cla).

Ready for maintainer review.

@sergio-sisternes-epam sergio-sisternes-epam force-pushed the feature/double-claude-instructions branch from 6d7c424 to 0d6aab7 Compare May 23, 2026 12:41
sergio-sisternes-epam pushed a commit to tillig/apm that referenced this pull request May 23, 2026
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>
@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator

Conflict resolution complete

Pre-rebase base: 5fd0678f (origin/main at rebase time)
Pre-rebase head: 6d7c4248
Post-rebase head: 0d6aab70da59e538c8181b28d3f9a00be01ed6e1

Conflicting paths resolved

File Resolution
CHANGELOG.md (x4 commits) Kept all main entries; placed PR's #1138 dedup entry under [Unreleased] / Fixed.
docs/src/content/docs/guides/compilation.md (x2 commits) Deleted on main (restructured); PR's dedup admonition ported to docs/src/content/docs/producer/compile.md.
docs/src/content/docs/integrations/ide-tool-integration.md (x2 commits) Accepted main's rewrite; PR's dedup admonition preserved in the primitive-flow section.

Validation

  • uv run --extra dev ruff check src/ tests/ -- silent (exit 0)
  • uv run --extra dev ruff format --check src/ tests/ -- silent (exit 0)
  • Post-push mergeStateStatus: BLOCKED (branch-protection; mergeable=MERGEABLE, no longer DIRTY)

Push command

git push --force-with-lease=feature/double-claude-instructions:6d7c4248ddeee51e6723838789e8a138cd221a43 tillig HEAD:feature/double-claude-instructions

All review-panel follow-ups from the previous completion comment remain intact (cli-symlink-warning-color, integration-test-marker, doc-table-cell-density resolved in 6d7c424; no-dedup-escape-hatch deferred to #1463).

@sergio-sisternes-epam sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label May 23, 2026
tillig and others added 17 commits May 24, 2026 02:11
…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>
- 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
- 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
…y, stale docs)

- Use .absolute() in OSError fallback path comparison so unresolved paths
  still compare correctly against resolved self.base_dir
- Add ensure_path_within check for .claude/rules/ symlinks that escape the
  project root, preventing ambient symlinks from triggering skip_instructions
- Fix three stale "instructions only" labels for CLAUDE.md in docs
- Add tests for symlink-outside-project and OSError fallback scenarios
- Move ensure_path_within check before glob() to avoid traversing
  external directories via symlink
- Skip symlink test gracefully on platforms that don't support symlinks
- Fix OSError fallback test to exercise the .absolute() comparison
  path without fragile global Path.resolve patching
- OSError fallback test now exercises real format_distributed() via mock
  placement path that raises on resolve(), triggering the actual fallback
- Symlink test uses unique tempfile.mkdtemp() for external dir to avoid
  collisions in parallel test runs
The docs were restructured upstream (compilation.md removed,
ide-tool-integration.md rewritten as a hub). Add the Claude Code
deduplication note to the new canonical location for compile docs.
- 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."
- 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.
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>
- Route symlink warning through warning() instead of progress()
- Add pytest.mark.integration markers to dedup e2e tests
- Trim compile table cell for claude row

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tter.__init__

The __init__ method catches OSError and FileNotFoundError from Path.resolve()
and falls back to Path.absolute().  Two new tests exercise each exception path
by patching pathlib.Path.resolve to raise, then asserting the resulting
base_dir is still absolute and no errors are recorded.

This closes the gap flagged in Copilot review comments 3209850236 and 3209969897,
which noted that the original fallback-path test did not actually trigger the
right code path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sergio-sisternes-epam sergio-sisternes-epam force-pushed the feature/double-claude-instructions branch from 886f624 to d6ccf1b Compare May 24, 2026 01:11
@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator

@danielmeppiel this requires your approval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

panel-review Trigger the apm-review-panel gh-aw workflow 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