fix(compile): discover local-bundle instructions in apm_modules/#1388
Merged
Conversation
Local bundle installs (apm install <bundle> -t opencode|codex|gemini) stage their primitives at apm_modules/<slug>/.apm/instructions/ but intentionally do not register the bundle in apm.yml dependencies. scan_dependency_primitives only iterates the paths returned by get_dependency_declaration_order, which previously read from apm.yml deps + apm.lock.yaml dependencies[]. With dependencies: [], the staged slug was invisible to compile and produced no AGENTS.md / GEMINI.md. Fix: extend get_dependency_declaration_order to also derive bundle slugs from the existing local_deployed_files lockfile field (written by local_bundle_handler). For each deployed path matching apm_modules/<slug>/.apm/..., append <slug> to the declaration order when not already present. apm uninstall already removes those entries, so cleanup self-heals; slug discovery requires a lockfile attestation, preventing phantom-injection via raw apm_modules/ writes. Tests: - TestLocalBundleStagedSlugs (6 unit tests) covers: staged slug appended, no-lockfile -> no slug, declared dep wins on collision, deterministic ordering across multiple bundles, non-apm_modules paths ignored, non-instructions subdirs (context/) also recognized. - TestLocalBundleCompileRoundTrip (3 E2E params: opencode/codex/gemini) performs the full install -> compile flow and asserts the target's output file contains the staged instruction marker. Closes the narrowed sub-issue of #1363. The other two #1363 sub-issues were already fixed by #1336 and #1207. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes apm compile failing to discover primitives staged by apm install <local-bundle> ... under apm_modules/<slug>/.apm/... by deriving bundle slugs from lockfile provenance (local_deployed_files).
Changes:
- Extend dependency declaration order discovery to append local-bundle slugs sourced from
apm.lock.yamllocal_deployed_files. - Add unit tests covering slug derivation behavior (including provenance gating and deterministic ordering).
- Add integration tests validating install→compile round-trip for compile-only targets (opencode/codex/gemini).
Show a summary per file
| File | Description |
|---|---|
| src/apm_cli/primitives/discovery.py | Appends local-bundle slugs from local_deployed_files to the discovery scan list. |
| tests/unit/primitives/test_discovery_parser.py | Adds focused unit/regression tests for local-bundle slug derivation and provenance rules. |
| tests/integration/test_install_local_bundle_e2e.py | Adds E2E regression tests verifying local-bundle staged instructions produce compile outputs across targets. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 4
Comment on lines
+310
to
+327
| local_slugs: set[str] = set() | ||
| lockfile_path = Path(base_dir) / "apm.lock.yaml" | ||
| if lockfile_path.exists(): | ||
| lock = LockFile.read(lockfile_path) | ||
| if lock is not None: | ||
| for deployed in lock.local_deployed_files: | ||
| # Match ``apm_modules/<slug>/.apm/...`` only. Other | ||
| # deployed files (``.github/instructions/...``, | ||
| # ``.agents/skills/...``) are not bundle staging | ||
| # markers and must not produce phantom slugs. | ||
| parts = Path(deployed).parts | ||
| if len(parts) >= 3 and parts[0] == "apm_modules" and parts[2] == ".apm": | ||
| local_slugs.add(parts[1]) | ||
| seen = set(dependency_names) | ||
| for slug in sorted(local_slugs): | ||
| if slug not in seen: | ||
| dependency_names.append(slug) | ||
| seen.add(slug) |
Collaborator
Author
There was a problem hiding this comment.
Addressed in e084a6a — discovery now reads the lockfile once (LockFile.read on the resolved path with legacy fallback) and reuses the parsed LockFile for both the transitive-paths walk (lock.get_installed_paths) and the local_deployed_files slug derivation. Eliminates the duplicate YAML parse.
Comment on lines
+1156
to
+1159
| install_result = _invoke_install(project, str(bundle), monkeypatch=monkeypatch) | ||
| assert install_result.exit_code == 0, ( | ||
| f"install failed for target={target}: {install_result.output!r}" | ||
| ) |
Collaborator
Author
There was a problem hiding this comment.
Addressed in e084a6a — cwd is now pinned explicitly via monkeypatch.chdir(project) immediately before runner.invoke(["compile", ...]), so the E2E no longer depends on _invoke_install's side-effect.
Comment on lines
+1173
to
+1179
| # Now run `apm compile --target <target>` in the project dir. | ||
| runner = CliRunner() | ||
| compile_result = runner.invoke( | ||
| cli, | ||
| ["compile", "--target", target], | ||
| catch_exceptions=False, | ||
| ) |
Collaborator
Author
There was a problem hiding this comment.
Addressed in e084a6a — same fix as above (monkeypatch.chdir(project) before runner.invoke(["compile", ...])).
Comment on lines
+751
to
+752
| self.assertEqual([r for r in result if r != "real-bundle"], []) | ||
| self.assertIn("real-bundle", result) |
Collaborator
Author
There was a problem hiding this comment.
Addressed in e084a6a — assertion tightened to (a) assert real-bundle is present and (b) assert specific phantom slugs derived from the non-apm_modules entries (style, .github, coding, .agents) are absent, without forbidding unrelated dependency entries from coexisting.
- discovery.py: read apm.lock.yaml once and reuse the parsed LockFile for both transitive-paths walk and local_deployed_files slug derivation. Removes duplicate YAML parse on every compile. - test_discovery_parser.py: refit test_transitive_deps_appended_deduped to write a real lockfile (consistent with the single-read pattern) instead of patching the removed installed_paths_for_project call. - test_discovery_parser.py: tighten test_non_apm_modules_paths_ignored to assert real-bundle is present AND specific phantom slugs are absent, without forbidding unrelated dependency entries. - test_install_local_bundle_e2e.py: pin cwd explicitly via monkeypatch.chdir(project) before invoking compile rather than relying on _invoke_install's side-effect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1363
WHY
Closes the narrowed sub-issue of #1363. The other two sub-issues are already fixed by #1336 and #1207; this PR fixes the third.
apm install <local-bundle> -t opencode|codex|geministages bundle instructions underapm_modules/<slug>/.apm/instructions/(src/apm_cli/install/services.py:660) butapm compilenever discovers them. Root cause:scan_dependency_primitives(src/apm_cli/primitives/discovery.py:196-220) only visits paths returned byget_dependency_declaration_order(), which reads fromapm.ymldependencies:+apm.lock.yamldependencies[]. Local bundles intentionally do NOT mutateapm.yml(documented contract atservices.py:489-490), so a local-bundle install leavesapm.lock.yamldependencies: []and the staged file is invisible to compile.Result before fix:
Compilation completed but produced no output files, noAGENTS.md/GEMINI.md.WHAT
Extend
get_dependency_declaration_orderto also derive bundle slugs from the existinglocal_deployed_filestop-level lockfile field (written bylocal_bundle_handler.py:194-199). For every entry matchingapm_modules/<slug>/.apm/..., append<slug>to the dependency declaration order if it isn't already present. Slugs are deduplicated and sorted alphabetically for deterministic output. Registry/declared dependencies still win on name collision (they are appended first).Design + alternatives considered
Three options were evaluated (full design synthesis in the session plan):
apm.lock.yamldependencies[]repo_url/get_install_pathplumbing and the lockfile schema (source: local, integrity model). Breaks the "does NOT mutateapm.yml" contract atservices.py:489-490.apm_modules/apm_modules/becomes a compile input with no lockfile attestation).local_deployed_fileslockfile fieldapm uninstall. No schema change.apm uninstallalready removes those entries, so discovery self-heals. Cannot be spoofed by rawapm_modules/writes — a lockfile record is required.Validation evidence
Tests (all green)
TestLocalBundleStagedSlugscover: staged slug appended, no-lockfile -> no slug (phantom-injection defense), declared dep wins on collision, deterministic ordering across multiple bundles, non-apm_modulespaths ignored, non-instructions/subdirs (e.g.context/) also recognized.TestLocalBundleCompileRoundTrip[opencode|codex|gemini]exercise the full install -> compile flow and assert the target's output file contains the staged instruction marker.uv run --extra dev pytest tests/unit -x).uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/.Empirical repro (issue scenario, now passes)
Built a real plugin bundle with one
pep8.instructions.md, installed viaapm install <bundle.tar.gz> -t opencode, thenapm compile:Before the fix the same flow ended at
Compilation completed but produced no output fileswith noAGENTS.mdwritten.Out of scope
*.instructions.mdfilename (matches existing convention indiscovery.py:33-35). A bundle that ships raw.mdfiles in aninstructions/directory is staged byte-for-byte but is invisible to compile; that's a separate validation gap from this fix's scope.