Skip to content

[fix] include dependency instructions in claude compile (#631)#642

Merged
sergio-sisternes-epam merged 3 commits intomicrosoft:mainfrom
edenfunf:fix/include-dep-instructions-claude-compile
Apr 10, 2026
Merged

[fix] include dependency instructions in claude compile (#631)#642
sergio-sisternes-epam merged 3 commits intomicrosoft:mainfrom
edenfunf:fix/include-dep-instructions-claude-compile

Conversation

@edenfunf
Copy link
Copy Markdown
Contributor

@edenfunf edenfunf commented Apr 9, 2026

What

Fix compile path that silently skipped dependency instructions stored in
.github/instructions/ when running apm compile --target claude without
--local-only.

Why

Issue #631: scan_directory_with_source only scanned {dep}/.apm/ for
primitives. Packages that store instructions in .github/instructions/
(instead of .apm/instructions/) were completely invisible to
discover_primitives_with_dependencies. The --local-only and --validate
paths used LOCAL_PRIMITIVE_PATTERNS (which includes **/.github/instructions/)
so they worked, but the normal distributed compile path did not.

Symptoms:

  • apm compile --target claude --verbose showed Instruction Processing: 0.0ms
  • No CLAUDE.md was generated
  • apm compile --target claude --local-only worked correctly
  • apm compile --validate could find the instructions

How

Add DEPENDENCY_GITHUB_PRIMITIVE_PATTERNS (mirroring DEPENDENCY_PRIMITIVE_PATTERNS
for the .github/ directory layout) and scan {dep}/.github/ in
scan_directory_with_source alongside the existing .apm/ scan.

The original early-return guard if not apm_dir.exists(): return is replaced
by independent if apm_dir.exists(): / if github_dir.exists(): checks so
both directories are always considered regardless of which one is present.

No CLI interface changes. No architecture changes. Existing .apm/-based
packages are unaffected.

Test

  • Reproduced issue apm compile --target claude silently skips dependency instructions without --local-only #631: .github/-only dependency → 0 instructions before fix
  • Verified fix: .github/-only dependency → instruction found and CLAUDE.md generated
  • --local-only still works correctly
  • --target copilot unaffected
  • --validate unaffected
  • Regression tests added to TestScanDirectoryWithSource:
    • test_github_instructions_discovered_when_no_apm_dir
    • test_github_instructions_discovered_alongside_apm_dir
  • All 348 existing unit tests pass

Copy link
Copy Markdown
Collaborator

@sergio-sisternes-epam sergio-sisternes-epam left a comment

Choose a reason for hiding this comment

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

Nice fix — this is a real bug and the regression tests are well-targeted. The PR description is excellent too, clear reproduction and rationale.

Two requests before we can merge:

1. Reduce code duplication in scan_directory_with_source

The glob-scan-parse loop is copy-pasted for .apm/ and .github/. Consider extracting a small helper:

def _scan_patterns(base_dir: Path, patterns: Dict[str, List[str]], collection: PrimitiveCollection, source: str) -> None:
    for primitive_type, type_patterns in patterns.items():
        for pattern in type_patterns:
            for file_path_str in glob.glob(str(base_dir / pattern), recursive=True):
                file_path = Path(file_path_str)
                if file_path.is_file() and _is_readable(file_path):
                    try:
                        primitive = parse_primitive_file(file_path, source=source)
                        collection.add_primitive(primitive)
                    except Exception as e:
                        print(f"Warning: Failed to parse dependency primitive {file_path}: {e}")

Then the main function becomes:

if apm_dir.exists():
    _scan_patterns(apm_dir, DEPENDENCY_PRIMITIVE_PATTERNS, collection, source)
if github_dir.exists():
    _scan_patterns(github_dir, DEPENDENCY_GITHUB_PRIMITIVE_PATTERNS, collection, source)

This keeps the logic in one place for future maintenance.

2. Missing CHANGELOG entry

Please add an entry under ## [Unreleased] in CHANGELOG.md, e.g.:

### Fixed
- Fix `apm compile --target claude` silently skipping dependency instructions stored in `.github/instructions/` (#631)

Thanks for the contribution!

…only

scan_directory_with_source only scanned {dep}/.apm/ for primitives, so
dependency packages that store instructions in .github/instructions/ were
silently skipped during discover_primitives_with_dependencies. The
discover_primitives path (used by --local-only and --validate) uses
LOCAL_PRIMITIVE_PATTERNS which includes **/.github/instructions/, so
those two modes worked while the normal compile path did not.

Add DEPENDENCY_GITHUB_PRIMITIVE_PATTERNS and scan {dep}/.github/ in
scan_directory_with_source alongside the existing .apm/ scan.  The
original early-return when .apm/ was absent is replaced by independent
checks so both .apm/ and .github/ are always considered.

Fixes microsoft#631.
…ft#631)

Extract duplicated glob-scan-parse loop into _scan_patterns() so both
.apm/ and .github/ directory scanning share a single implementation.
Add missing CHANGELOG entry under [Unreleased] for the microsoft#631 fix.
@edenfunf edenfunf force-pushed the fix/include-dep-instructions-claude-compile branch from f67705d to b782402 Compare April 10, 2026 17:38
@edenfunf
Copy link
Copy Markdown
Contributor Author

@sergio-sisternes-epam Thanks for the thorough review! Done — both points addressed in the latest commit (f67705d):

  1. Extracted _scan_patterns() helper — the duplicated glob-scan-parse loop now lives in one place; scan_directory_with_source calls it for both .apm/ and .github/.
  2. Added the ### Fixed entry under [Unreleased] in CHANGELOG.md.

All 63 unit tests still pass.

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

Fixes the dependency-scanning compile path so .github/instructions/ inside installed dependencies is discovered during normal distributed compilation (e.g., apm compile --target claude without --local-only), matching the behavior of --local-only/--validate.

Changes:

  • Extend dependency primitive scanning to also inspect {dep}/.github/ (in addition to {dep}/.apm/) during discover_primitives_with_dependencies.
  • Refactor dependency scanning into a shared _scan_patterns() helper to avoid duplicating the glob/parse loop.
  • Add unit regression tests covering .github/instructions/ discovery for dependencies, and add a changelog entry under Unreleased/Fixed.
Show a summary per file
File Description
src/apm_cli/primitives/discovery.py Adds .github/ dependency scanning and refactors scanning loop into a helper.
tests/unit/primitives/test_discovery_parser.py Adds regression tests ensuring dependency instructions in .github/instructions/ are discovered.
CHANGELOG.md Documents the fix under ## [Unreleased]### Fixed.

Copilot's findings

Comments suppressed due to low confidence (1)

src/apm_cli/primitives/discovery.py:350

  • Non-ASCII em dash () in this comment violates the repo's printable-ASCII encoding rule and can break Windows cp1252 terminals. Replace it with ASCII (e.g., "-" or "--").
    # Also scan .github directory — some packages store primitives there instead of (or
    # in addition to) .apm/.  Without this, dependency instructions in .github/instructions/
    # are silently skipped in the normal compile path (issue #631).
  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Args:
directory (Path): Directory to scan (e.g., apm_modules/package_name).
base_dir (Path): Directory to scan (e.g., dep/.apm or dep/.github).
patterns (Dict[str, List[str]]): Primitive-type → glob-pattern mapping.
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

Non-ASCII arrow character () in the docstring violates the repo's printable-ASCII encoding rule and may cause encoding errors on some Windows terminals. Replace with an ASCII equivalent like "->".

This issue also appears on line 348 of the same file.

Suggested change
patterns (Dict[str, List[str]]): Primitive-type glob-pattern mapping.
patterns (Dict[str, List[str]]): Primitive-type -> glob-pattern mapping.

Copilot uses AI. Check for mistakes.
Comment thread CHANGELOG.md

- Pin codex setup to `rust-v0.118.0` for security and reproducibility; update config to `wire_api = "responses"` (#663)
- Propagate headers and environment variables through OpenCode MCP adapter with defensive copies to prevent mutation (#622)
- Fix `apm compile --target claude` silently skipping dependency instructions stored in `.github/instructions/` (#631)
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

Changelog formatting: add a blank line between the last bullet in ### Fixed and the next section header (### Changed) to match the surrounding Keep a Changelog style used elsewhere in this file.

Suggested change
- Fix `apm compile --target claude` silently skipping dependency instructions stored in `.github/instructions/` (#631)
- Fix `apm compile --target claude` silently skipping dependency instructions stored in `.github/instructions/` (#631)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants